Correct issue where getSourceLine() could return -1 mod 2^32 instead … #577
Conversation
|
There are other cases I have noticed where the SourceInfo objects created by the compiler for synthesized code is a bit odd, but I will save enhancing those issues for later. This can cause bmv2 to crash. I have a separate PR open to correct the issue on that side: p4lang/behavioral-model#356 but p4c-bm2-ss really shouldn't generate 2^32-1 as a line number. |
| // except in the special case of all 0s, where 0 is preferable to | ||
| // (-1 mod 2^32) | ||
| if (line == 0 && it->first == 0 && it->second.sourceLine == 0) { | ||
| return SourceFileLine(it->second.fileName, 0); |
There was a problem hiding this comment.
It seems to me that this should never happen, and if it does, it indicates a bug somewhere else. So it might be better to have a BUG_CHECK here rather than silently covering up the problem.
There was a problem hiding this comment.
If you compile this program: https://github.com/jafingerhut/p4-guide/blob/master/demo5/demo5.p4_16.p4#L89
using the command p4c-bm2-ss -o tmp.json demo5.p4_16.p4, the action top_level_action, being defined at the top level, and having an inout struct of all headers, but only modifying one field of one header inside of that struct, is compiled to contain several copy_header calls synthesized by the compiler to implement the copy-in and copy-out semantics. One of those has source info of 2^32-1 in the JSON output, which becomes 0 if you make the diff suggested in this PR.
This might very well be a bug elsewhere in the compiler, but I haven't tried to trace it backwards from the point of this change.
While I have your thoughts on this issue, these lists of copy-in and copy-out copy_header primitives can get very very long if the 'headers' struct contains many headers, and in other similar cases. Are they strictly necessary? If so, why?
If they are there just because it was the quickest and easiest way to guarantee that copy-in copy-out semantics are followed, and a fancier compiler could reduce the list by doing an analysis of fields actually read or written inside the action, then I understand the desire to keep things simple.
There was a problem hiding this comment.
I've added a fix (PR #588) that fixes the problem with the json generation code that was triggering the problem you see (and which isn't really fixed by this -- it will just output 0 as the line number to avoid crashing BMV2, rather than the correct line number)
There was a problem hiding this comment.
Closing this PR in favor of @ChrisDodd better one: #588
…of 0