Check for out/inout bindings aliased with uses

[ChrisDodd]

This is compiler support for p4lang/p4-spec#1370.

Basically, the idea is that we want to forbid "aliasing" between out/inout arguments and things that are accessed directly in the called thing. This simplifies backends in that the don't have to detect such cases and special case them to avoid problematic behavior. The fact that there have been a number of issues in the past related to this aliasing suggests that alllowing it is problematic both from implementation complexity and user understanding. Having a clear compile-time error message (rather than confusing runtime behavior) is much better for all involved.

The implementation here is fairly straight-forward. There are two passes here

• The first pass analyzes callable things that contain code (currently actions, functions, controls, and parsers) and identifies all the symbols that they reference from their enclosing scope, along with the expressions used to access them (important for when the object only accesses a few fields from a larger object).
• The second pass analyzes call sites, to determine if any actual argument passed as an out or inout parameter "overlaps" with any of the things found in the first pass of the callee.

Currently it only looks at things on a per-field basis. Accesses to arrays are assumed to access any element of the array, and slices of fields are not tracked per-bit to see if they are disjoint or overlap. The code could be extended to track constant array indexes (so two different constant indexes would not overlap) and bit slices.

As can be seen there are a number of existing test cases that trigger this (new) error. Two of them have been moved to P4_16_errors, as what they were doing was highly questionable. The others have been modified to not have this error as they seem to be testing unrelated things.

[asl]

@ChrisDodd Is this going to support also aliasing cases at the caller side? E.g.

struct S {
  bit<16> field
}

action foo(inout S, out bit<16>) { ... }

S s;
foo(S, s.field)

[ChrisDodd]

@ChrisDodd Is this going to support also aliasing cases at the caller side? E.g.

struct S {
  bit<16> field
}

action foo(inout S, out bit<16>) { ... }

S s;
foo(S, s.field)

I think that should already be an error -- using the same object for two different out or inout args in the same call.

[asl]

I think that should already be an error -- using the same object for two different out or inout args in the same call.

I might be missing something, but the following code compiles fine for me on main:

struct S {
  bit<16> field;
}

action foo(inout S s, inout bit<16> f) {
  s.field = 42;
  f = f + s.field;
}

action bar() {
 S s = { 10 };
 foo(s, s.field);
}

Is this an error with this PR?

[fruffy]

I think that should already be an error -- using the same object for two different out or inout args in the same call.

Iirc this was allowed because the spec has precise description of the expected behavior. There was a similar issue with #2176

[ChrisDodd]

I really don't like allowing multiple writes to the same object in a single call, even though the spec has been worded to allow it. IMO that's s spec flaw we should discuss in the WG.

I'm of a mind to add the check here (even though the spec says it should be ok), but it could also be "fixed" by inserting copies around the call (which, if desirable, should be done in a later, midend pass). In any case this needs more discussion.

[asl]

I really don't like allowing multiple writes to the same object in a single call, even though the spec has been worded to allow it. IMO that's s spec flaw we should discuss in the WG.

Can this be solved in a way done with -fstrict-aliasing in C/C++ compilers? E.g. have an option that enforces particular aliasing pattern? E.g. in C/C++ we can optionally enforce strict aliasing rules. In P4 we can optionally disallow any object aliasing this way?

[ChrisDodd]

Can this be solved in a way done with -fstrict-aliasing in C/C++ compilers? E.g. have an option that enforces particular aliasing pattern? E.g. in C/C++ we can optionally enforce strict aliasing rules. In P4 we can optionally disallow any object aliasing this way?

-fstrict-aliasing in C/C++ just allows the compiler to assume that lvalues of different types don't alias. Since we don't have pointers or pointer casts in P4, there is no way for lvalues of different types to alias (a value cast gives an rvalue -- not something that can be used as an out or inout argument). A slice lvalue is an explicit alias of (some of) the bits of a larger value. So it does have a "different type" but allowing the compiler to assume it does not alias would be even more confusing, I think.

The main thing we're trying to avoid in P4 is "undefined behavior" -- where the spec doesn't say what happens, but an error diagnostic at compile time is not required. The existing spec currently avoids undefined behavior by specifying a strict behavior (copying argument left-to-right), while IMO it would be better to specify it as a compile time error.

[asl]

-fstrict-aliasing in C/C++ just allows the compiler to assume that lvalues of different types don't alias. Since we don't have pointers or pointer casts in P4, there is no way for lvalues of different types to alias (a value cast gives an rvalue -- not something that can be used as an out or inout argument). A slice lvalue is an explicit alias of (some of) the bits of a larger value. So it does have a "different type" but allowing the compiler to assume it does not alias would be even more confusing, I think.

I know quite well what strict aliasing rules are, thanks :) As I mentioned, my idea is to optionally enforce no-alias rules. Certainly, the name of the option would be different, but essentially this might relax copy-in / copy-out semantics giving stronger guarantees to the compiler (in C terms everything would be restrict-qualified).

[asl]

@ChrisDodd what's left here? I believe this is quite an important thing that simplifies backend handling a lot as we know that actions access control locals in predicable way :)

[ChrisDodd]

@ChrisDodd what's left here? I believe this is quite an important thing that simplifies backend handling a lot as we know that actions access control locals in predicable way :)

There are some p4smith tests that this change breaks that I still need to track down.