Fix wrong references to return value when function is inlined multiple times into the same scope #5163
Conversation
|
See #5162 for the complete breakdown. Testcase distilled from very subtly miscompilation of large app :( |
| // { <body2> | ||
| // retval = <something2> | ||
| // a = retval + retval; | ||
| // } |
There was a problem hiding this comment.
These should get renamed to retval_1 and retval_2 -- that seems to happen with the inline-function3-frontend.p4 below, so this extra copy, while not harmful (and pretty much always eliminated by local_copyprop) should not be necessary?
There was a problem hiding this comment.
Perhaps the bug in 5162 was seen with a different midend.cpp than backends/p4test/midend.cpp? There would seem to be a subtle order dependency between inlinining and RemoveReturns and UniqueNames where non-unique names moved into the same scope? In general, inlining assumes names are globall unique (requires running UniqueNames before it).
There was a problem hiding this comment.
Perhaps RemoveReturns should use namegen->newName("retval") rather than just using the fixed name "retval"?
There was a problem hiding this comment.
The function is same. Inlined twice. So, RemoveReturns will not help here as it will process the caller, not the callee.
The problem is at the callee side – we're passing PathExpression for return value which is "out the scope" here (see #5162 for step by step breakdown).
The bug is clearly reproducible with the mainline p4c as test shows.
In general, inlining assumes names are globall unique (requires running UniqueNames before it).
Indeed. And this PR ensures this.
There was a problem hiding this comment.
These should get renamed to
retval_1andretval_2-- that seems to happen with the inline-function3-frontend.p4 below, so this extra copy, while not harmful (and pretty much always eliminated by local_copyprop) should not be necessary?
Yes, it is, after the fix :) Before the fix, it will be just a single value:
y3_0 = tmp;
if (y3_0 == 1w1) {
y0 = 1w0;
} else if (y0 != 1w1) @inlinedFrom("foo") {
a_4 = bt3;
retval_2 = a_4 + 1w1;
y0 = retval_2 | retval_2;
}
bt = y0;
}After first inliner iteration (without the fix) we're having:
} else if (y0_0 != 1w1) @inlinedFrom("foo") {
bit<1> a_1;
a_1 = bt2;
@name("hasReturned") bool hasReturned_0;
@name("retval") bit<1> retval_0;
hasReturned_0 = false;
{
hasReturned_0 = true;
retval_0 = a_1 + 1w1;
}
y0_0 = foo(bt3) | retval_0;
}
bt = y0_0;
}after second (note two different retval_0 here in different scopes, reference to outer one will be lost as the resolution is by the name):
} else if (y0_0 != 1w1) @inlinedFrom("foo") {
bit<1> a_1;
a_1 = bt2;
@name("hasReturned") bool hasReturned_0;
@name("retval") bit<1> retval_0;
hasReturned_0 = false;
{
hasReturned_0 = true;
retval_0 = a_1 + 1w1;
}
@inlinedFrom("foo") {
bit<1> a_2;
a_2 = bt3;
@name("hasReturned") bool hasReturned_0;
@name("retval") bit<1> retval_0;
hasReturned_0 = false;
{
hasReturned_0 = true;
retval_0 = a_2 + 1w1;
}
y0_0 = retval_0 | retval_0;
}
}There was a problem hiding this comment.
Ah, ok -- I see. The problem is that the function is being inlined twice from the same source into the same scope. I'm concerned that if there are other names local to the function being inlined (eg, if it has a user-declared local var that is used somehow), the same thing will happen with that variable.
In this case retval is just a local variable in the function that was introduced by RemoveReturns.
There was a problem hiding this comment.
I checked this case:
- The function is inlined into own block, so all function locals are sealed there
- We create new names for inout / out parameters, so we are good here as well
- What remained is function return value, and this PR essentially handles this case
This is special "function case", actions do not return and therefore are fine.
…e times into the same scope. Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Fixes #5162