Move the network hashes implementation from Bmv2 testgen to lib/ #4526
Conversation
|
I plan to do follow-up PRs with more library functions to allow code sharing with the BMv2 concolic functions implementation. |
|
Seems like the macOS failure may be related to #4525. |
|
Fyi, these are lifted from https://github.com/p4lang/behavioral-model/blob/5f1c590c7bdb32ababb6d6fe18977cf13ae3b043/include/bm/bm_sim/calculations.h but at this point we can freely modify them. The good thing is changes are differentially tested against BMv2's execution of the same hashes. |
lib/nethash.cpp
Outdated
|
|
||
| /* generating from my Python script gen_crc_tables inspired from the C code at: | ||
| http://www.barrgroup.com/Embedded-Systems/How-To/CRC-Calculation-C-Code */ | ||
| /* This code was adapted from: |
There was a problem hiding this comment.
Make this comment part of the header?
There was a problem hiding this comment.
Actually I think that both this, and the license snipped with (c) Barefoot should be in the .cpp. The reason is that is the code that has that source. The declarations are kind of "on top of that". Or we can have the disclaimer at both, but I think it should be at the implementation and so should be the license/copyright.
Sadly it seems at least the |
Good idea, didn't even consider that we need tests. I think it makes sense to add the tests here: https://github.com/p4lang/behavioral-model/blob/main/tests/test_calculations.cpp We can do this in this PR. |
|
@fruffy, do you have any code suggestion? Do you think someone else should review this? I would really like to get this one in reasonably soon. As for the tests in BMv2, I've tried adding them (p4lang/behavioral-model#1235), but I can't compile BMv2 on my work machine and sadly the CI does not show me what failed. I will re-try later. |
|
Oh I mean the other way round. Since this is now a lib class we should copy the BMv2 tests to the compiler. Apologize for the confusion. Other than that I have no further comments. |
Oh! There are not many in BMv2. But I've added them and wrote some more for csum16 & identity. |
|
The validate does not build, but this is unrelated -- some problem with |
vlstill
changed the title
vlstill
added
the
core
|
Changed the PR name to indicate that this is not only about testgen. |
Will fix that. |
* Move the network hashes implementation from Bmv2 testgen to lib/ * Add missing stream operator signature * Apply suggestions from code review Co-authored-by: Fabian Ruffy <[email protected]> * Update namespace to CamelCase * Shuffle the file-level comments and license to make more sense * Add basic CRC unit tests * Generalize CRC calculation, avoid duplicates * Polish the reflection function * Fix format * Fix crcCCITT * Make constructor explicit * Add comments on the checksums (mainly CRC) * Add more tests, some from BMv2 --------- Co-authored-by: Fabian Ruffy <[email protected]>
There are hash functions from various network protocols hidden deep in the BMv2 testgen target implementation. This moves then to the
lib/so they can be used elsewhere (mainly I would presume in other testgen targets, but I don't see anything testgen/p4tools specific about them, therefore I'm moving them to top-levellib/).I'm open to a better name then
nethashif someone has a suggestion :-).