Always call packet->reset_exit() at beginning of Pipeline::apply() #1279
Conversation
The only place the code checks for is_marked_for_exit() is inside of Pipeline::apply(), so by resetting exit flag at beginning of Pipeline::apply, no users of Pipeline class need to worry about the exit flag at all. It is handled completely locally within Pipeline::apply(), and could even be a local variable of Pipeline::appply() if the primitive made that straightforward (which it does not). Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
|
I did not confirm whether there was a bug related to #1275, but in thinking about it a bit, the changes proposed in this PR seems to me to simplify the thinking about the question, and make it disappear as a concern. Users of the Pipeline class don't need to know about the exit flag at all, if Pipeline::apply() correctly initializes and uses it locally. I ran a full test suite of p4c with these changes to behavioral-model, and they all passed. |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment, or this will be closed in 180 days |
|
@antoninbas I always hesitate to bug you about things, so feel free to ignore this if it is a bother. If you do have any comments or questions on this, I'd love to hear them. Based on what I know of the code, these look like reasonable safe changes, which would help future code maintainers avoid bugs in handling the behavior of exit statements. If you do not have time or interest, no worries. I will consider merging in these changes after getting review from people working on BMv2 changes for Google Summer of Code 2025. |
|
Only concern for me is that the last part of the behavioral-model/include/bm/bm_sim/packet.h Lines 256 to 259 in c0ff3ab Now I have no idea why I thought that was the desired behavior. But if someone calls |
The only place the code checks for is_marked_for_exit() is inside of Pipeline::apply(), so by resetting exit flag at beginning of Pipeline::apply, no users of Pipeline class need to worry about the exit flag at all. It is handled completely locally within Pipeline::apply(), and could even be a local variable of Pipeline::appply() if the primitive made that straightforward (which it does not).