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?

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).

Perhaps RemoveReturns should use namegen->newName("retval") rather than just using the fixed name "retval"?

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.

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?

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;
                }
            }

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.

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.