p4lang / p4c

P4_16 reference compiler
https://p4.org/
Apache License 2.0
683 stars 446 forks source link

Define a compiler flag for "strict" code #3880

Open mihaibudiu opened 1 year ago

mihaibudiu commented 1 year ago

Here is a quote from the current spec, appendix G:

The presence of undefined behavior has caused numerous problems in languages like C and HTML, including bugs and serious security vulnerabilities. There are a few places where evaluating a P4 program can result in undefined behaviors: out parameters, uninitialized variables, accessing header fields of invalid headers, and accessing header stacks with an out of bounds index. We think we should make every attempt to avoid undefined behaviors in P416, and therefore we propose to strengthen the wording in the specification, such that by default, we rule out programs that exhibit the behaviors mentioned above. Given the concern for performance, we propose to define compiler flags and/or pragmas that can override the safe behavior. However, our expectation is that programmers should be guided toward writing safe programs, and encouraged to think harder when excepting from the safe behavior.

This is probably a matter of implementation rather than spec. It would be nice if the compiler had a flag which would cause it to reject code that has undefined behaviors.

vgurevich commented 1 year ago

Shouldn't the existing -Werror do the trick?

mihaibudiu commented 1 year ago

We don't issue warnings about undefined behaviors

vgurevich commented 1 year ago

If we can detect them, we probably should. In fact, that's one of the primary reasons for a warning. And then the user will have a choice whether to disable them (on a per-class basis or via NoWarn() annotation) or convert them into errors.

apinski-cavium commented 1 year ago

Note some undefined behaviors as defined in the spec can only be detected at runtime. E.g. accessing an invalid header might not be reachable normally. There might be other cases too. Now P4C warns about the accessing of an invalid header but I don't think P4C warns about all undefined at runtime though.

vgurevich commented 1 year ago

@apinski-cavium -- agreed.

Some backends also perform somewhat deeper analysis, but it is still not exhaustive.

I do not think that the goal is to create anything foolproof. It is just yet another useful tool that might be helpful to some people and what I am trying to do here is to suggest a mechanism (i.e. a warning), for which we already have several flexible policies to handle it further.

mihaibudiu commented 1 year ago

It would even be possible to insert run-time instrumentation to (sample and) send digest messages when undefined behaviors happen dynamically. Like out of bounds accesses to stacks.

hesingh commented 1 year ago

The Cisco qfp routing asic is programmed in C. The C code is compiled with a Tensilica compiler which initializes every variable and pointer before generating firmware.

jafingerhut commented 1 year ago

And it is easily possible to develop a P4 implementation that also initializes every variable to a deterministic value, e.g. 0, and nothing in the language spec disallows this. In most implementations, that will cost something extra, vs. an implementation that does not do so.

Abe149 commented 1 year ago

Perhaps the first thing to do here is to define the user interface.

Should it be just --strict or do we want to require e.g. --strict=on and/or --strict=yes?

Are we planning on having different levels of strictness, such that e.g. --strictness=1, --strictness=2, and --strictness=3 all make sense at least in the context of one particular back-end?