Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue warnings when an undefined header field is used #312

Open
jafingerhut opened this issue Mar 14, 2017 · 5 comments
Open

Issue warnings when an undefined header field is used #312

jafingerhut opened this issue Mar 14, 2017 · 5 comments
Assignees

Comments

@jafingerhut
Copy link
Contributor

There are several places in the P4 v1.0.x and P4_16 draft specification where the behavior is allowed to be undefined. Ideally a compiler could perform static analysis and catch some or all of these, but there are concerns that such static analysis might always produce false positives. Although dynamic checking in the behavioral model is not guaranteed to find all such possible behavior, it could at least warn about cases experienced during actual tests.

In particular, warning when an undefined header field value is used would be a good start. I can create separate issues for other undefined behavior if there is interest.

Perhaps the default behavior should be to issue warnings when this occurs. Options to quiet the warnings, or to make them fatal errors, might be useful in some testing contexts.

@antoninbas
Copy link
Member

Hi Andy,
I think this would be a very useful feature. However I have been leaning towards a compile time flag to enable / disable this feature, to avoid penalizing people using bmv2 in a context where a decent throughput needs to be maintained. Dynamic checks like this one are expensive since many field reads take place for each packet. The only thing I am unsure of if whether this should be enabled by default at compile-time or not.

@cc10512
Copy link

cc10512 commented Mar 14, 2017

@antoninbas can you elaborate on how the compiler option will work? Is the idea that the compiler generates a flag in context.json that enables/disables tracing in bmv2, or that the compiler generates the code to do the tracing?

@antoninbas
Copy link
Member

I'm talking about a bmv2 compile time flag for g++. Something like -DMEMORY_DEBUG which would enable some Valgrind-like capabilities for bmv2. If we start adding too many dynamic checks and we cannot turn them off at compile time (only at runtime by passing a flag to bmv2), it may incur a non-negligible cost which would lower throughput for people using bmv2 to do NOS validation. Then again it's hard to predict the actual runtime cost without profiling this.
Maybe the best option would be to enable this by default but let users turn-off the verification code at compile time when running configure.
If that wasn't clear, I'm talking about bmv2 C++ compilation, not P4 compilation.

@cc10512
Copy link

cc10512 commented Mar 14, 2017

Makes sense to have a separate build configuration that one can enable if debugging such issues and not penalize all runs. I would actually keep the default be the performance build.

@jafingerhut
Copy link
Contributor Author

Agreed that if there are strong performance concerns there that a build-time option at the time of building bmv2 is a good way to go. For such debug builds, an init-time option for warn or fatal error might be nice. For example, if someone used the debug build for automated regression runs, a fatal error might be nice for 'making more noise' about the potential problem than just a warning message and continuing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants