p4-spec icon indicating copy to clipboard operation
p4-spec copied to clipboard

Scope leaking caveat of "synchronous" abstract methods

Open QinshiWang opened this issue 3 years ago • 14 comments

Tofino P4 compiler supports not-yet-standard "synchronous" abstract method, which allows abstract methods to read local variables. For example,

control C(in bit<8> x) {
    Register<bit<32>, bit<32>>(32w32768) reg;
    RegisterAction<bit<32>, bit<32>, bit<32>>(reg) regact = {
        void apply(inout bit<32> val, out bit<32> rv) {
            val = val + x;
        }
    };
    ...
}

Things get more complicated because we can pass regact as a constructor parameter. Then we can call it and use x in another control. Despite of the break of modularity, we can still execute this program because d can only be called in any subcontrol of C. So when it is called, we always have a C in the call stack and there is a corresponding x.

control D()(RegisterAction<bit<32>, bit<32>, bit<32>> regact) {
    apply {
        regact.exceute();
    }
}

control C(in bit<8> x) {
    Register<bit<32>, bit<32>>(32w32768) reg;
    RegisterAction<bit<32>, bit<32>, bit<32>>(reg) regact = {
        void apply(inout bit<32> val, out bit<32> rv) {
            val = val + x;
        }
    };
    D(regact) d;
    ...
}

It gets worse when we consider inout parameters. When we can show that there is no alias, we do not need to allocate for the inout parameter. But reading from a register action does cause aliasing. In the following example, the inout parameter is changed, but x in the abstract method should still have the old value.

control D(inout bit<8> x)(RegisterAction<bit<32>, bit<32>, bit<32>> regact) {
    apply {
        x = x + 1;
        regact.exceute();
    }
}

control C(inout bit<8> x) {
    Register<bit<32>, bit<32>>(32w32768) reg;
    RegisterAction<bit<32>, bit<32>, bit<32>>(reg) regact = {
        void apply(inout bit<32> val, out bit<32> rv) {
            val = val + x;
        }
    };
    D(regact) d;
    ...
}

I regard these as noticeable examples for designing abstract methods.

QinshiWang avatar May 04 '22 18:05 QinshiWang

The spec currently says:

The abstract methods can only use the supplied arguments or refer to
values that are in the top-level scope. 

Are you proposing to change the specifications here? Are you saying the reference compiler currently accepts this and we want to change the specifications to allow it too (I have not tried the reference compiler)?

apinski-cavium avatar May 05 '22 01:05 apinski-cavium

Yes, that is correct, there is need for abstract methods that can capture variables in the environment. But then the compiler needs some promises about when these methods may be called; they must be annotated with @synchronous. That's not part of the spec yet.

mihaibudiu avatar May 05 '22 01:05 mihaibudiu

Maybe a new syntax for captures is needed instead of arbitrary capturing all by default.

    RegisterAction<bit<32>, bit<32>, bit<32>>(reg) regact = {
        [in x] void apply(inout bit<32> val, out bit<32> rv) {
            val = val + x;
        }
    };

Or something similar.

To capture all:

    RegisterAction<bit<32>, bit<32>, bit<32>>(reg) regact = {
        [in =] void apply(inout bit<32> val, out bit<32> rv) {
            val = val + x;
        }
    };

What do you think about that? At least then you say which variables gets captured and such? And then aliasing is not as bad.

apinski-cavium avatar May 05 '22 01:05 apinski-cavium

The syntax can be useful perhaps for humans, but the compiler will have no problems discovering these variables by itself too. The most important part is for the compiler to figure out when such variables can be read or written.

mihaibudiu avatar May 05 '22 02:05 mihaibudiu

Note I partly borrowing the idea from C++11 lambdas really. Where the syntax is:

[v,b] (args) {...}
[=](args){...}
[&](args){...}

The first case captures just v and b, passed by value, next is all are captured and passed by value and the last is passed by reference. In the P4 case we could use in/inout/out for the & stuff and = still for all.

apinski-cavium avatar May 05 '22 02:05 apinski-cavium

The syntax can be useful perhaps for humans, but the compiler will have no problems discovering these variables by itself too. The most important part is for the compiler to figure out when such variables can be read or written.

That is the point of the extra syntax is so the compiler does not need to figure that out. Rather it is up to the user to tell the compiler the right thing. Otherwise you run into issues like the above.

apinski-cavium avatar May 05 '22 02:05 apinski-cavium

Even if we were to adopt the C++-style lambda syntax, wouldn't the "leakage" and aliasing issues from @QinshiWang's example out still occur?

To put it another way: is determining which variables may be captured is the main issue here? Or is it the new forms of state interaction that can arise through externs with abstract methods that (explicitly or implicitly) capture?

Or maybe I'm missing something fundamental...

jnfoster avatar May 05 '22 02:05 jnfoster

@apinski-cavium and @mbudiu-vmw had some helpful discussion about variable capturing annotation. First, I'm doubtful whether abstract method is a nice design. It's intended to encode the hardware and cost limitation on what can be done between a read and a write. But it is very tricky to formalize. Second, as pointed out, variable capturing can be computed from the syntax, so we can clarify it (like a type system refinement) that what extern objects with abstract method can be passed as constructor parameters, or variables captured in abstract methods are considered as aliased and cannot be optimized by passing by reference.

QinshiWang avatar May 05 '22 20:05 QinshiWang

Abstract methods are a very general mechanism, they really have nothing to do with reads/writes. Note that we already have variable capture for actions, and its semantics is pretty clear. What is not clear is when the abstract methods execute.

mihaibudiu avatar May 06 '22 16:05 mihaibudiu

I would like to ask this question: do you think extern objects with synchronous abstract methods can be passed to other control blocks as constructor parameters?

I would explain the theory in this way. In C++, one can write programs like

void f() {
  int a;
  // Define a function object that captures the local variable a
  auto f = [&](){return a; };
  handler(f);
}

In such a program, we need to be able to take the address of a. So a needs to be an addressable variable, in contrast to a temporary variable. Addressable variable is not a standard term in C but in CompCert as far as I know. But the concept is just that addressable variables have addresses, while temporary variables do not need to have addresses (e.g. they may be just stored in registers).

So the question is: should we allow addressable variables in P4? Temporary variables are easier to reason about: they are not accessible out of the scope. Without synchronous abstract methods, temporary variable is enough for P4. If we limit extern objects with abstract methods from being passed as constructor parameters, temporary variable is still enough. But if we allow passing extern objects with abstract methods, we will need to use addressable variables. Addressable variables are more powerful, but need to use their addresses to reason.

Please share your opinion.

QinshiWang avatar Jul 19 '22 14:07 QinshiWang

"address" is really an implementation choice, even if the variable uses a dedicated register it still can be captured. This is indeed the main complication of synchronous extern methods.

mihaibudiu avatar Jul 19 '22 17:07 mihaibudiu

The captured variable must be in scope where the declaration of the method occurs, which puts some constraints on what can be captured.

mihaibudiu avatar Jul 19 '22 17:07 mihaibudiu

The question is, can the method which the abstract method is synchronized with can be called outside of the scope of the captured variables?

QinshiWang avatar Jul 20 '22 16:07 QinshiWang

This is in some sense a duplicate of https://github.com/p4lang/p4-spec/issues/943, but we haven't done the work to resolve it.

mihaibudiu avatar Aug 05 '22 18:08 mihaibudiu

In the interest of tidying up the set of active issues on the P4 specification repository, I'm marking this as "stalled" and closing it. Of course, we can always re-open it in the future if there is interest in resurrecting it.

jnfoster avatar Nov 11 '23 13:11 jnfoster