Spurious and misleading warning for overwriting extract (OverwritingHeader from ParsersUnroll)

[vlstill]

I'm getting the following warning in a quite simple program that just extracts ethernet and then two vlan tags.

./p4c --target dpdk --arch psa parser-reextract.p4
parser-reextract.p4(44): [--Wwarn=expr] warning: pkt.extract: error OverwritingHeader will be triggered
Parser in_parser state chain: start, parse_vlan2, parse_vlan1
        pkt.extract(hdr.vlan);
        ^^^^^^^^^^^^^^^^^^^^^
parser-reextract.p4(44): [--Wwarn=ignore-prop] warning: Result of pkt.extract is not defined: Exception: OverwritingHeader
        pkt.extract(hdr.vlan);

The warning is coming from ParsersUnroll pass. Therefore, this affects any achitecture using this pass, not just dpdk. It also affects p4testgen.

This is quite confusing for many different reasons:

• I see nothing in the P4 spec saying that the same header cannot be extracted twice or that the result would be undefined. The second extract should just replace the first one.
  • My testing (with a larger example) suggests the program is compiled as I would expect, i.e. the header is extracted again, overwriting the first extract.
• There are two warning sfor the same thing, exposing some internals of the compiler.
• There is nothing to be unrolled in the parser.

The code is here: parser-reextract.p4.txt (I had to change its extension for github to accept it).

[jafingerhut]

I agree that the behavior of extract'ing to the same header twice are well-defined in the P4 language specification.

I also believe that MOST people would never want to write a parser that does so [1], and a warning that they have done so is a useful warning to have in a P4 compiler.

What would you think if the warning message was instead something like this?

parser-reextract.p4(44): [--Wwarn=ignore-prop] warning: Performing pkt.extract more than once to the same header will nearly always cause the first header extracted to be deleted from the packet:
        pkt.extract(hdr.vlan);

[1] Because unless you go to unusual lengths to save the data of the first extracted header into the header variable, that first extracted header will be deleted from the packet. There are other perfectly good ways to conditionally remove a header from a packet, e.g. use setInvalid() on the headers you want to remove, later after parsing is finished.

[vgurevich]

I would add to the message something like: "If you do want that effect, replace the previous extract()s with lookahead()s instead".
Because people do actually write such code :)

[fruffy]

The loops unrolling pass has a long history and has various problems iirc. In this case, it is just calling into the interpreter code, which seems to throw that error. I am not sure how easy it is to fix that.

@VolodymyrPeschanenko might know, he worked last on this particular compiler pass.

[vlstill]

I agree there should probably be a warning for this that suggests this might not be the best idea. @jafingerhut I think your formulation is good, and I think it is a good idea to add what @vgurevich suggested. I agree this is unusual and might be even wrong in most cases when the user writes it, but I have seen some that made some sense. In these cases we should make sure the warning reads as something in the sense of "this is not a great idea" rather than "this will be undefined" which is what the current warning suggests (although most likely not what the compiler does).

Looking at the code a little bit, I am wondering if this could affect correctness of loop unrolling in the case when there is a header overwrite... And I have seen a code which has a loop in parser's control flow that could re-extract header.