Fix def_use/SimplifyDefUse to deal with extern methods properly (#1284) #1652

ChrisDodd · 2019-01-08T20:42:18Z

Track extern method calls, assuming that any call to any
non-abstract method might trigger any abstract method implementation
local_copyprop fixed for the same assumption.
propagate reads/writes from callee to caller properly.
@synchronized annotation for abstract methods

This is revised slightly from the previous change -- the additional change is to make RemoveUnused in simplifyDefUse ignore IR::Function nodes that have not been analyzed by FindUninitialized as there were no (direct or indirect) references to the function. The problem being that visiting such a function would remove all the assignments in the function as none of them were in hasUses. That would be ok if the function was in fact unused, but in the case of a non-@synchronous function, that would be a problem. We don't know how such functions are called (from the control plane?) so we can't really analyze them, so we just want to leave them unchanged.

The problem is if such a function accesses a control local variable, the analysis won't see it, so might remove an (apparently unused) control local. Its not clear if such functions should be allowed to access control locals; we don't really have any example use cases for non-@synchronous functions that would want to.

mihaibudiu · 2019-01-08T20:59:39Z

But Functions are global-scoped, so they cannot refer to any control-locals...

mihaibudiu

Can you please add a test for the problem which caused the PR to be reverted?

mihaibudiu · 2019-01-08T21:05:13Z

frontends/p4/simplifyDefUse.cpp

@@ -545,6 +551,13 @@ class RemoveUnused : public Transform {
        }
        return statement;
    }
+    const IR::Node* preorder(IR::Function* func) override {


do you have an example where this is important?

I extended the virtual.p4 test case to include the problematic case -- an abstract method with an inout arg where the implementation assigns to the arg (could also happen with an out arg) -- not skipping the method here would remove the assignment (leaving the method empty).

With the rules I proposed this would be correct, because no one is calling this method.
We need to know who is calling the method to analyze it.
If we don't know the method is either asynchronous or dead.

mihaibudiu · 2019-01-08T21:08:38Z

midend/local_copyprop.cpp

+        } else if (mi->is<P4::ExternFunction>()) {
+            LOG3("extern function call " << mc->method);
+            // assume it has no side-effects on anything not explicitly passed to it?
+            // maybe should have annotations if it does


even if it has side-effects it should be unable to change any named variables (locals). We do not have references, so you cannot change something you cannot name - due to copy-in copy-out.

it cannot call any methods, since it cannot refer to instances of the allocated objects that have these methods.

We don't know for certain that the architecture does not provide a back-door method of referring to other objects -- for example, the packet_in and packet_out objects that are passed to parser and deparsers might be affected by other methods in some way? Perhaps a clone object that can have a method invoked that triggers an additional copy of the packet?

If we want to allow such architectures, we really need to provide langauge that the architecture can be described in (in the arch.p4 file); I'm not sure annotations are good enough longer-term. Something for the language design group.

mihaibudiu · 2019-01-08T21:14:36Z

testdata/p4_16_samples/virtual3.p4

+    // abstract methods must be implemented
+    // by the users
+    void increment();
+    @synchronous(increment) abstract bit<16> f(in bit<16> ix);


So the meaning of this is that increment calls f?
Shouldn't the annotation be on increment instead?

Yes, the annotation means that increment might call f (but is not guarenteed to). At the moment I have the annotations on the abstract methods, listing the other methods that might call them, but it could equivalently be done the opposite way (put @might_call annotations on methods listing the other methods it might call?)

If we want to support the case where the same method may be called both synchronously and asynchronously it may make sense to put the annotation on the caller.

I propose these rules:

a method (virtual or non-virtual) that may be called asynchronously must be annotated with @async. This means that the method may be called at any time, from any thread. Such methods cannot access any P4 locals, only their own parameters. Such methods can also be called synchronously.

a non-virtual method that may call callee is labeled with @calls(callee)

With these rules we have that asynchronous methods are always live, but can never reach into locals. We should always run def-use for them. Synchronous methods may use locals, but we should know all their callers, so if they are unreachable they are actually dead. Also, it is an error for an asynchronous method to call a synchronous one.

Problem is that we can't run def_use on these methods as its currently structured, as it tends to crash with a BUG_CHECK in Definitions::getPoints because it can't figure out where the function is called from. We need some way of constructing a ProgramPoint corresponding to the (non-existant) call site before we can analyze the body.

As far a what the annotations should be and which functions they should be on, the two are equivalent -- you're now back to something more like the original suggestion before it was later suggested they should be on the abstract methods. The only really important point IMO is that the annotation MUST be on/in the extern declaration in the arch file and NOT on the user's code implementing the abstract function.

We should be able to create a ProgramPoints for asynchronous functions (which have no explicit caller). The same problem appears in several other places. There's a "beforeStart" ProgramPoint which may work.

ChrisDodd · 2019-01-10T21:38:07Z

Ok, so I've gone back to (more like) the old code -- def_use now (continues to) analyze all functions with an assumption that they're called from outside the control, and additionally analyzes directly called and @synchronous functions per the direct call site.

Unfortunately it looks like Travis has begun failing the OSX CI build with *** cannot import name RawPcapWriter for all ebpf tests?

mihaibudiu · 2019-01-10T22:17:00Z

How does one even debug a failing Travis build for a different OS?
RawPcapWriter should be part of scapy.

mihaibudiu · 2019-01-10T22:34:47Z

@fruffy : if you have some time can you try to figure out what's wrong with the Python testing script?

mihaibudiu · 2019-01-10T22:48:08Z

@ChrisDodd : I am not sure whether this will make a difference, but maybe you can try to modify the file backends/ebpf/targets/target.py as follows:

from scapy.utils import *
from scapy.layers.all import *

The stars replace more specific names.

fruffy · 2019-01-10T22:54:47Z

Looking at the output log of travis it seems that it fails to install scapy.

https://scapy.readthedocs.io/en/latest/installation.html#mac-os-x
Maybe these steps are missing?

scapy.txt

mihaibudiu · 2019-01-10T23:00:37Z

Actually the p4c-xdp build failed with the same error, so this does not seem to be OSX only:
https://travis-ci.org/vmware/p4c-xdp

fruffy · 2019-01-10T23:18:44Z

I just tried to install scapy on my local machine
pip install --upgrade --user scapy
and it failed with the same error.
pip install --upgrade --user scapy==2.4.0
worked fine. So this might be a quick and dirty workaround.

mihaibudiu · 2019-01-10T23:23:21Z

This sounds like a good idea. @ChrisDodd : can you please try this fix?
We should indeed stick to specific versions of packages that we know are working.
I assume it's enough to replace in the Dockerfile scapy with scapy==2.4.0

ChrisDodd · 2019-01-11T17:01:33Z

Looks like scapy==2.4.0 does not work on Travis's OSX -- see #1656. I'm going to try just removing all the ebpf tests from the OSX build next.

mihaibudiu · 2019-01-11T17:05:08Z

Actually by looking at the travis.yml file it looks to me like the ebpf tests should not be run on OSX.

script:
  - if [[ $TRAVIS_OS_NAME == 'linux' ]] ; then docker run -w /p4c/build -e CTEST_PARALLEL_LEVEL p4c ctest --output-on-failure --schedule-random ; fi
  - if [[ $TRAVIS_OS_NAME == 'osx' ]] ; then ctest --output-on-failure -j 2 --schedule-random -LE "ebpf$" ; fi

But I don't know much about how travis works.

mihaibudiu · 2019-01-11T17:05:45Z

At least that was the intention, I guess.

ChrisDodd · 2019-01-11T17:07:28Z

The tests were working (on OSX) a week or so ago -- something changed (in the Travis setup?) that causes them all to fail now.

mihaibudiu · 2019-01-11T17:09:26Z

I actually think it is a change in the scapy python module that breaks things.
No one touched Travis or Docker lately.

mihaibudiu · 2019-01-11T17:10:38Z

That's why I was hoping that moving to an old version would fix the problem.

mihaibudiu · 2019-01-11T17:21:58Z

Making this change in the Dockerfile of the p4c-xdp project has fixed that build:

scapy==2.4.0

mihaibudiu · 2019-01-11T17:22:18Z

The build there was failing with the same error.

fruffy · 2019-01-11T17:56:57Z

Looking at #1656 it still seems to download scapy-2.4.2:

Collecting scapy
  Downloading https://files.pythonhosted.org/packages/d0/04
/b8512e5126a1816581767bf95cbf525e0681a22c055bcd45f47ecff60170/scapy-2.4.2.tar.gz (795kB)

The file tools/install_os_deps.sh installs scapy before the dockerfile runs I think.
With OSX the dockerfile is skipped?

By the way, I think the scapy devs just pushed a broken version yesterday:
secdev/scapy#1783
Should be fixed soon.

mihaibudiu · 2019-01-11T18:04:51Z

Good catch! Thanks @fruffy!

@synchronized

- Track extern method calls, assuming that any call to any non-abstract method might trigger any abstract method implementation - local_copyprop fixed for the same assumption. - propagate reads/writes from callee to caller properly. - @synchronized annotation for abstract methods - adjust virtual.p4 test to reflect problematic case of non-synchronous function

…itialized

mihaibudiu

I think you should bring this up in the LDWG. We should be prepared to change some of the ways annotations are used if the LDWG reaches a different conclusion. But it is great to have a working proof-of-concept.

ChrisDodd requested a review from mihaibudiu January 8, 2019 20:42

mihaibudiu requested changes Jan 8, 2019

View reviewed changes

ChrisDodd force-pushed the cdodd-virtfn branch 3 times, most recently from a5ef67a to a1a8579 Compare January 10, 2019 21:31

ChrisDodd force-pushed the cdodd-virtfn branch from a1a8579 to 888151d Compare January 11, 2019 18:32

ChrisDodd force-pushed the cdodd-virtfn branch from 888151d to e4f92ba Compare January 11, 2019 18:35

remove no longer used controlLocals from ComputeWriteSet and FindUnin…

4d6d029

…itialized

ChrisDodd force-pushed the cdodd-virtfn branch from e0bbdd8 to 4d6d029 Compare January 11, 2019 19:48

mihaibudiu approved these changes Jan 11, 2019

View reviewed changes

ChrisDodd merged commit 0559c32 into master Jan 12, 2019

ChrisDodd deleted the cdodd-virtfn branch January 12, 2019 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix def_use/SimplifyDefUse to deal with extern methods properly (#1284) #1652

Fix def_use/SimplifyDefUse to deal with extern methods properly (#1284) #1652

ChrisDodd commented Jan 8, 2019

mihaibudiu commented Jan 8, 2019

mihaibudiu left a comment

mihaibudiu Jan 8, 2019

ChrisDodd Jan 8, 2019

mihaibudiu Jan 9, 2019

mihaibudiu Jan 8, 2019

mihaibudiu Jan 8, 2019

ChrisDodd Jan 8, 2019

mihaibudiu Jan 8, 2019

ChrisDodd Jan 8, 2019

mihaibudiu Jan 9, 2019

ChrisDodd Jan 9, 2019 •

edited

mihaibudiu Jan 9, 2019

ChrisDodd commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

fruffy commented Jan 10, 2019 •

edited

mihaibudiu commented Jan 10, 2019

fruffy commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

ChrisDodd commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

ChrisDodd commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

fruffy commented Jan 11, 2019 •

edited

mihaibudiu commented Jan 11, 2019

mihaibudiu left a comment

Fix def_use/SimplifyDefUse to deal with extern methods properly (#1284) #1652

Fix def_use/SimplifyDefUse to deal with extern methods properly (#1284) #1652

Conversation

ChrisDodd commented Jan 8, 2019

mihaibudiu commented Jan 8, 2019

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDodd Jan 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDodd commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

fruffy commented Jan 10, 2019 • edited

mihaibudiu commented Jan 10, 2019

fruffy commented Jan 10, 2019

mihaibudiu commented Jan 10, 2019

ChrisDodd commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

ChrisDodd commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

mihaibudiu commented Jan 11, 2019

fruffy commented Jan 11, 2019 • edited

mihaibudiu commented Jan 11, 2019

mihaibudiu left a comment

Choose a reason for hiding this comment

ChrisDodd Jan 9, 2019 •

edited

fruffy commented Jan 10, 2019 •

edited

fruffy commented Jan 11, 2019 •

edited