p4c compiler driver #378

hanw · 2017-03-22T05:04:56Z

Compiler driver for bmv2, ebpf and third-party backends.

Installing the driver:

cd p4c/tools/driver
python setup.py install

Usage

usage: p4c [-h] [-v] [-debug] [-###] [-Xpreprocessor <arg>] [-Xp4c <arg>]
           [-Xassembler <arg>] [-Xlinker <arg>] [-b BACKEND] [-E] [-e] [-S]
           [-c] [-x LANGUAGE] [-I SEARCH_PATH] [-o PATH]
           source_file

positional arguments:
  source_file           Files to compile

optional arguments:
  -h, --help            show this help message and exit
  -v, --version         show version and exit
  -debug                verbose
  -###                  print (but do not run) the commands
  -Xpreprocessor <arg>  Pass <arg> to the preprocessor
  -Xp4c <arg>           Pass <arg> to the compiler
  -Xassembler <arg>     Pass <arg> to the assembler
  -Xlinker <arg>        Pass <arg> to the linker
  -b BACKEND            specify target backend
  -E                    Only run the preprocessor
  -e                    Skip the preprocessor
  -S                    Only run the preprocess and compilation steps
  -c                    Only run preprocess, compile, and assemble steps
  -x LANGUAGE           Treat subsequent input files as having type language.
  -I SEARCH_PATH        Add directory to include search path
  -o PATH               Write output to the provided path

cc10512

Nice work. Needs some polishing, but this is going in the right direction.

Note that it would be useful to add a readme with the install instructions and adding a new arch-target-platform.

Also, please add copyright notices.

Unfortunately, I couldn't get it to run the compiler:

First, it requires P4C_INCLUDE_PATH env var to be defined:

p4c -b bmv2 testdata/p4_16_samples/arith1-bmv2.p4
error: P4C_INCLUDE_PATH undefined.

Then, couldn't find the backend:

p4c> export P4C_INCLUDE_PATH=`pwd`/p4include
P4C_INCLUDE_PATH=/Users/cascaval/Barefoot/Projects/P4/src/p4c/p4include
p4c> p4c -b bmv2 testdata/p4_16_samples/arith1-bmv2.p4

clang: error: no input files

p4c/build> p4c -c -b p4c-bm2-ss testdata/p4_16_samples/arith1-bmv2.p4
Unknown target: p4c-bm2-ss

p4c/build> p4c -debug -c -b bmv2 testdata/p4_16_samples/arith1-bmv2.p4
running cpp -E -x c -o ./arith1-bmv2.p4i testdata/p4_16_samples/arith1-bmv2.p4

clang: error: no input files

p4c/build> p4c -### -debug -c -b bmv2 testdata/p4_16_samples/arith1-bmv2.p4
preprocessor: cpp -E -x c -o ./arith1-bmv2.p4i testdata/p4_16_samples/arith1-bmv2.p4
p4c-bm2-ss: command not found

oh well, it's not looking in the current dir:

p4c/build> ls -l p4c-bm2-ss 
-rwxr-xr-x  1 cascaval  staff  18376136 Mar 21 18:16 p4c-bm2-ss

p4c/build> PATH=.:$PATH p4c -### -debug -c -b bmv2 testdata/p4_16_samples/arith1-bmv2.p4
preprocessor: cpp -E -x c -o ./arith1-bmv2.p4i testdata/p4_16_samples/arith1-bmv2.p4
compiler: p4c-bm2-ss --p4v=16 -o ./arith1-bmv2.json ./arith1-bmv2.p4i

Now, dry run finds it! but the real run does not:

p4c/build> PATH=.:$PATH p4c  -debug -c -b bmv2 testdata/p4_16_samples/arith1-bmv2.p4
running cpp -E -x c -o ./arith1-bmv2.p4i testdata/p4_16_samples/arith1-bmv2.p4

clang: error: no input files

cc10512 · 2017-03-22T06:09:11Z

tools/driver/p4c/main.py

+
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument("-v", "--version", dest="show_version",


Shall we use -V for version and -v for debug?

cc10512 · 2017-03-22T06:19:01Z

tools/driver/p4c/main.py

+                        metavar="<arg>",
+                        help="Pass <arg> to the linker",
+                        action="append", default=[])
+    parser.add_argument("-b", dest="backend",


backend is not sufficiently specified. Think about bmv2, it may support multiple targets. In fact we have 3 factors determining the backend:

the architecture (PSA, TSA, etc.)

the target (simple switch, FPGA, Tofino, etc.)

and the platform (bmv2, cycle accurate model, hardware)

Similar to one of the gnu triples to specify host-os-arch, we need to have an arch-target-platform triple to specify a configuration for which we want to compile, e.g., psa-ss-bmv2.

Also it would be very useful to have an option that list all the supported configurations (the equivalent of --target-help):

gcc --target-help The following options are target specific: -m128bit-long-double sizeof(long double) is 16 -m16 Generate 16bit i386 code -m32 Generate 32bit i386 code -m3dnow Support 3DNow! built-in functions -m64 Generate 64bit x86-64 code -m80387 Use hardware fp [...]

It's also worth noting that the include path for different architectures may be different, so we may want to let the configurations determine it.

used target-arch-vendor triplet for now.

The code still needs a bit more work to actually use the triplet info. We can probably do that after the psa work is in.

cc10512 · 2017-03-22T06:23:06Z

tools/driver/p4c/main.py

+
+    # default search path
+    for path in os.environ['P4C_INCLUDE_PATH'].split(':'):
+        if (opts.language == 'p4-16' and os.path.isdir(path + '/p4include')):


I think we need to surface the language as a top level option.

language is already a top level options (-x p4-14 or p4-16)

cc10512 · 2017-03-22T06:25:08Z

tools/driver/p4c/p4c.site.cfg

+
+import os
+
+def config_preprocessor():


tofino should live in its backend and the install script should search the extensions folder for backend specific configurations and install them with the rest of the default configurations.

cc10512 · 2017-03-22T06:26:05Z

tools/driver/p4c/util.py

+# top-down find, good for deployment
+def find_bin(exe):
+    found_path = None
+    for pp in os.environ['PATH'].split(':'):


might need to add the current directory to the path (see below).

mihaibudiu · 2017-03-22T15:29:34Z

You should add a link to the documentation in the toplevel README.md, or add documentation in docs/README.md.

sethfowler · 2017-03-22T16:14:29Z

tools/driver/p4c/main.py

+        options = cfg.options[opts.backend][step]
+        cmd = cmd + options
+        # convert ['-a -b', '-c', '-d'] to ['-a', '-b', '-c', '-d']
+        cmd = list(itertools.chain.from_iterable(el.split() for el in cmd))


This doesn't handle quotes right. =) Try this:

https://docs.python.org/2/library/shlex.html

sethfowler · 2017-03-22T16:16:15Z

tools/driver/p4c/main.py

+            sys.exit(p.returncode)
+
+        if opts.debug:
+            print out


Why not print stdout all the time? Seems like a footgun.

sethfowler · 2017-03-22T16:18:02Z

tools/driver/p4c/util.py

+        for root, dirs, files in os.walk(pp):
+            for ff in files:
+                if ff == exe:
+                    found_path = pp + '/' + ff


The idiomatic thing here would be os.path.join(pp, ff).

(There's at least one more occurrence of this.)

sethfowler · 2017-03-22T16:20:02Z

tools/driver/p4c/run.py

@@ -0,0 +1 @@
+


Seems like this is just an empty file?

sethfowler · 2017-03-22T16:23:28Z

tools/driver/p4c/config.py

+
+    def __init__(self, config_prefix):
+        self.config_prefix = config_prefix or 'p4c'
+        self.site_config_name = '%s.site.cfg' % (self.config_prefix)


You never refer to this.

In general, there are a lot of comments that suggest that site.cfg is special, but it seems like the code now just searches for .cfg files. You should update the comments and clarify what's happening.

sethfowler · 2017-03-22T16:25:53Z

tools/driver/p4c/main.py

+
+def save_options(cmd, options):
+    for opt in options:
+        opt_split = opt.split(' ')


See my comment elsewhere about this not handling quotes right.

sethfowler · 2017-03-22T16:28:34Z

tools/driver/p4c/main.py

+    commands['preprocessor'].insert(0, cfg.get_preprocessor(opts.backend))
+    commands['compiler'].insert(0, cfg.get_compiler(opts.backend))
+    commands['assembler'].insert(0, cfg.get_assembler(opts.backend))
+    commands['linker'].insert(0, cfg.get_linker(opts.backend))


This seems closely related to what save_options() does. Since we split() the options again later, when we actually run the commands, maybe you could just use this approach for command line options too?

ChrisDodd

I think it would be highly desirable to have the ste.cfg files set up in such a way that it would be easy to run things out of their individual build directories for debugging, as well as out of the install dir/prefix for post-installation.

ChrisDodd · 2017-03-22T16:17:29Z

tools/driver/p4c.py

+from p4c.main import main
+
+if __name__ == '__main__':
+    main()


The p4c driver program should not have a .py extension (so it can be invoked directly as p4c ...args, rather than p4c.py ...args)

after installation python setup.py install, user is able to call p4c to invoke the driver.

quick question: since the rest of the repo is using autotools, why not use autotools to install the driver as well?

good point, i will add it in.

ChrisDodd · 2017-03-22T16:31:01Z

tools/driver/p4c/main.py

+
+    # default search path
+    for path in os.environ['P4C_INCLUDE_PATH'].split(':'):
+        if (opts.language == 'p4-16' and os.path.isdir(path + '/p4include')):


Given the way it is used, P4C_INCLUDE_PATH should probably be replaced by P4C_INSTALL_PREFIX, and should be specified .cfg file (possibly per target?) rather than in an envvar

ChrisDodd · 2017-03-22T16:37:08Z

tools/driver/p4c/p4c.site.cfg

+    config.add_compiler_option("tofino", "{}/{}.p4i".format(output_dir, source_basename))
+
+def config_assembler():
+    config.add_assembler_option("tofino", "-vvvl")


-vvvvl is really only for debugging the compiler/assembler combo in a way that stores the verbose output in the assembler output dir. For futurre release, we might not want all of the -v options and might want to leave off the -l as well -- with no -l logging output will go to stderr, and with no -v, the only log output will be assembler internal ERROR messages which almost always mean the resulting config is corrupt.

But for now, this is a reasonble way of configuring things.

cc10512 · 2017-03-23T06:58:24Z

tools/driver/setup.py

+    version = p4c.__version__,
+
+    author = p4c.__author__,
+    license = "BSD",


Actually all the software we put on p4.org is Apache 2.0.

cc10512 · 2017-03-23T06:59:55Z

I was talking to @sethfowler and he thinks we're about ready to try to generate APIs from the compiler (even if they are not yet correct). So let's add an option to do the generation as well.

- installing p4c using autotools - use target-arch-vendor triplet to specify backend - generate p4runtime api in bmv2 - clean up

hanw · 2017-03-24T01:00:06Z

All comments should be addressed now. Please take another look.

sethfowler · 2017-03-24T01:03:28Z

tools/driver/p4c_src/p4c.bmv2.cfg

+    config.add_compiler_option("bmv2-*-p4org", "{}/{}.p4i".format(output_dir, source_basename))
+    # generate api has part of the compilation
+    config.add_compiler_option("bmv2-*-p4org", "--p4runtime-file");
+    config.add_compiler_option("bmv2-*-p4org", "{}/{}.p4rt".format(output_dir, source_basename))


cc10512

Minor issues, but overall works.
We should add a blurb in docs/README.md to document its existence.

cc10512 · 2017-03-24T05:21:39Z

tools/driver/p4c_src/main.py

+    parser.add_argument("-v", dest="debug",
+                        help="verbose",
+                        action="store_true", default=False)
+    parser.add_argument("-###", dest="dry_run",


this option was brilliant!

cc10512 · 2017-03-24T05:51:42Z

tools/driver/p4c_src/p4c.bmv2.cfg

+# -*- Python -*-
+
+def config_preprocessor():
+    config.add_preprocessor_option("bmv2-*-p4org", "-E -x c -o")


for some reason, this cpp command does not work on Mac. Works fine on Linux.

changed to cc

cc10512 · 2017-03-24T05:53:46Z

tools/driver/p4c_src/main.py

+        os.makedirs(opts.output_directory)
+
+    commands['preprocessor'].insert(0, cfg.get_preprocessor(backend))
+    commands['compiler'].insert(0, cfg.get_compiler(backend))


if the compiler is just built in the current directory and one does not have the current dir in the path, then it can not find the compiler, unless run with:

PATH=.:$PATH ./p4c -v ../testdata/p4_16_samples/arith-bmv2.p4

I wonder whether we should force the current directory in the path.

p4c compiler driver

50b9e12

hanw requested review from cc10512 and ChrisDodd March 22, 2017 05:04

cc10512 suggested changes Mar 22, 2017

View reviewed changes

sethfowler reviewed Mar 22, 2017

View reviewed changes

tools/driver/p4c/run.py Outdated

@@ -0,0 +1 @@

Copy link

Contributor

sethfowler Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is just an empty file?

sethfowler reviewed Mar 22, 2017

View reviewed changes

ChrisDodd requested changes Mar 22, 2017

View reviewed changes

fix comments

ce68459

cc10512 reviewed Mar 23, 2017

View reviewed changes

address comments

44ee999

- installing p4c using autotools - use target-arch-vendor triplet to specify backend - generate p4runtime api in bmv2 - clean up

sethfowler reviewed Mar 24, 2017

View reviewed changes

cc10512 approved these changes Mar 24, 2017

View reviewed changes

Han Wang and others added 2 commits March 24, 2017 01:35

add installation dir to PATH, preprocess with cc

25e1d97

Merge branch 'master' into driver-v3

409aa8f

ChrisDodd approved these changes Mar 24, 2017

View reviewed changes

hanw merged commit 41910a0 into p4lang:master Mar 24, 2017

hanw deleted the driver-v3 branch April 5, 2017 23:50

cc10512 mentioned this pull request Mar 1, 2018

Support inferring which architecture file to include from --target command line option. #1177

Closed

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed

p4c compiler driver #378

p4c compiler driver #378

Conversation

hanw commented Mar 22, 2017

cc10512 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Mar 22, 2017

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

cc10512 commented Mar 23, 2017

hanw commented Mar 24, 2017

Choose a reason for hiding this comment

cc10512 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