option to warn/return 0 on invalid field read#1377
Conversation
|
Would appreciate feedback in a couple of areas in particular:
|
|
Looks like I've got some fixes to make to the old build system 🙁 |
|
On the performance question, hopefully this weekend some time I will get some time to try out locally building bmv2 with these changes, and running them through the p4c test suite, and measure performance with and without these changes on the same tests. I'll bet $10 that it will be difficult to measure any performance difference. Note to self: Ensure that bmv2 with and without these changes are built with any debug/logging options turned off. |
|
Minor question/suggestion: If this is implementing warning/force-0-value on reading a field in a header that is currently invalid, it might be better to call the options and internal variables tracking the values of the options something like "warn_on_accessing_field_in_invalid_header" or something similar. Reason: I can imagine someone perhaps in the future wanting to go even further than what you are doing here, and tracking whether a field has actually ever been assigned to, and if not, issuing a warning if it is read before being initialized (even when the header the field is in is currently valid). |
|
I have no suggestions for modifying the unit tests you have written. They have the benefit of being clear and straightforward. No need that I can see to make them more compact. |
I also don't expect a huge impact. I'm mostly highlighting it for completeness 🙂 |
I included a FIXME comment as a suggestion for future enhancement here: https://github.com/p4lang/behavioral-model/pull/1377/changes#diff-abec37663881efd42bbc504a119d447e9df2898b11ac6c35b944103cf3a3a34dR156-R157 But I will rename the variables to better reflect their current behavior. |
Make Data::value a private attribute (was protectectd). Add accessor methods to allow reading/writing of the attribute. Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
Switch to using the value accessor (get_value()) in Data instead of directly accessing where appropriate. Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
Add mechansim to report a warning and/or return zero when reading a field in an unitialized header. Default behavior remains unchanged; these new behaviors can be individually/jointly enabled via command-line parameters. Can also be enabled from tests by setting flags in the Field class. Downside of this change is that all data value accesses now go through a virtualized get_value() accessor. Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
Rename variable refering to uninit to invalid header (e.g., warn_on_uninit_read -> warn_on_invalid_hdr_read) Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
Signed-off-by: Glen Gibb <gleng@ai-fabrics.com>
jafingerhut
left a comment
jafingerhut
left a comment
There was a problem hiding this comment.
LGTM. @fruffy let me know if you want to review this before we merge it, and if so, whether you want me to run a comparative before vs. after performance test to see what the effect of this, but personally my best guess is the performance difference should be difficult to measure.
2 participants
Add configurable options when reading an invalid field (currently defined as reading a field in a header that is not valid):
Each of these behaviors can be enabled independently. The default behavior is to maintain the existing behavior (no warning, return the last written value, even if the header is invalid.)
Addresses #1370.