iox-#2157 Replace uint32_t with uint64_t for payload size

[kozakusek]

Pre-Review Checklist for the PR Author

1. Add a second reviewer for complex new features or larger refactorings
2. Code follows the coding style of CONTRIBUTING.md
3. Tests follow the best practice for testing
4. Changelog updated in the unreleased section including API breaking changes
5. Branch follows the naming format (iox-123-this-is-a-branch)
6. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
7. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
8. Relevant issues are linked
9. Add sensible notes for the reviewer
10. All checks have passed (except task-list-completed)
11. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
12. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Closes Allow For memory chunks bigger than 4GB #2157

[elBoberido]

@kozakusek commit message is fine. Now the more tedious work starts to make it work on all platforms :)

[elBoberido]

It seems the code is not formatted with clang-format. You can use tools/git-hooks to automate some of the stuff which is checked by the CI, e.g. formatting the code or adding the issue number to the commit messages

[kozakusek]

@elBoberido the biggest issue I see now is that

• *TooLargeUserPayload_Fails tests are no longer relevant and
• I might not fully get the idea behind CHUNK_HEADER_VERSION and the threats in chunk_header.hpp: "m_chunkSize and m_chunkHeaderVersion should therefore neither changed the type, nor the position" which might be the cause for failure for the other three tests

And another thnig, should this be mentioned as an API breaking change?

[elBoberido]

@kozakusek I did a stupid mistake when I put chunkSize at the beginning of the ChunkHeader struct instead of chunkHeaderVersion. I never thought the type of chunkSize would change but only that maybe new members would be added. The reason for CHUNK_HEADER_VERSION and the test is to detect when a subscriber receives a chunk with an incompatible ChunkHeader. This can be the case when someone implements something like record&replay and stores all the data. If the the ChunkHeader would change after storing the data, a replay would result in weird bugs in the iceoryx layer. The issue we need to solve now is to keep the chunkHeaderVersion at an offset of 32 bits in the ChunkHeader struct. I think this can be achieved by swapping userHeaderSize with chunkSize. I need to check whether this would cause other issues but I think we should be safe.

Yes, this should indeed be mentioned as a breaking change. I think it is more of an ABI breaking change since the API should still be source compatible.

[kozakusek]

So one last thing before I open this for review - should I add a unit test for memory chunks > 4GB?
Do the CI machines have enough RAM for this?
@elBoberido could you confirm?

[elBoberido]

So one last thing before I open this for review - should I add a unit test for memory chunks > 4GB? Do the CI machines have enough RAM for this? @elBoberido could you confirm?

Unfortunately the CI seems to not reliably have that much memory. AFAIR we already had to deactivate on test which required > 4GB. I need to check what changed maybe it is possible to mock a few things and test at least the algorithm.

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (38e9b6b) 80.17% compared to head (ba5178c) 80.18%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2158   +/-   ##
=======================================
  Coverage   80.17%   80.18%           
=======================================
  Files         421      421           
  Lines       16346    16364   +18     
  Branches     2265     2268    +3     
=======================================
+ Hits        13105    13121   +16     
- Misses       2431     2432    +1     
- Partials      810      811    +1     

Flag	Coverage Δ
unittests	79.97% <95.08%> (+<0.01%)	⬆️
unittests_timing	15.60% <50.81%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
iceoryx_binding_c/source/c_client.cpp	96.87% <100.00%> (ø)
iceoryx_binding_c/source/c_publisher.cpp	92.50% <100.00%> (ø)
iceoryx_binding_c/source/c_server.cpp	95.14% <ø> (ø)
...ude/iceoryx_posh/internal/mepoo/memory_manager.hpp	100.00% <ø> (ø)
...ude/iceoryx_posh/internal/mepoo/typed_mem_pool.inl	82.85% <100.00%> (ø)
...osh/internal/popo/building_blocks/chunk_sender.hpp	100.00% <ø> (ø)
...osh/internal/popo/building_blocks/chunk_sender.inl	88.49% <100.00%> (ø)
...oryx_posh/internal/popo/ports/client_port_user.hpp	100.00% <ø> (ø)
...x_posh/internal/popo/ports/publisher_port_user.hpp	100.00% <ø> (ø)
...oryx_posh/internal/popo/ports/server_port_user.hpp	100.00% <ø> (ø)
... and 16 more

... and 4 files with indirect coverage changes

[kozakusek]

Cleaned up the mess in examples and tests.
Hopefully fixed the issue with building on Mac.
Let me know if I and how should I mock 'stuff' for testing

[elBoberido]

CI looks good. Thanks. Will have a look at the changes the next days.

[elBoberido]

Will continue tomorrow, it's already quite late. Just had a look at the C binding and the release notes

[elBoberido]

Not yet fully done. I have saved some of the critical files for tomorrow :)

Btw, please don't squash commits from now on since this will make the review harder and there are subtle thinks to look at.

[elfenpiff]

Since this affects a lot of files and from the comments from @elBoberido I saw that there are maybe even more places where we overlooked to change an uint32_t to an uint64_t. This could result in weird unsigned integer overflow bugs.

Therefore, I would add a test that verifies that payloads larger than 4GB are actually supported. Those tests shall be added to:

• iceoryx_posh/test/integrationtests/test_client_server.cpp
• iceoryx_posh/test/integrationtests/test_publish_subscriber_communication.cpp

I would introduce a test struct like:

struct LargeData {
  uint8_t payload[4294967296 + 100]; // + 100 to be 100 bytes above the original limit
};

And use it for the typed communication. Also we need to adjust the roudi config to support those large structs. I think this should work in the test setup

RouDi_GTest(MinimalRouDiConfigBuilder().payloadChunkSize(4294967296 + 100).payloadChunkCount(2).create());

I would test both cases, one with the typed API and one with the untyped API. If this is too complex or time-consuming, I would also accept just a test with one of the APIs.

Addition:

• @elBoberido made me aware that the CI is unable to handle this. I would still welcome those tests so that I can run them locally and for the CI we have to disable them.

[elBoberido]

Can you please also update chunk_header.md regarding the uint32_t->uint64_t change

[elBoberido]

Looks good. Maybe a few performance improvements for the new tests but they are not that important since we don't run them in the CI.

More important though would be to add the new option to the cmake options. This shouldn't take much time and you can just copy&paste from an existing options.

[elBoberido]

@elfenpiff @gpalmer-latai please also have a final look

@kozakusek looks good. Thanks for the contributions. Btw, there is a developer meetup on each first Tuesday of the month. Would be great to hear more about your use case.

[gpalmer-latai]

It seems like it would be a good idea for you to take some credit for your contribution by updating the copyright headers: https://github.com/eclipse-iceoryx/iceoryx/blob/master/iceoryx_posh/source/mepoo/mem_pool.cpp#L3

Basically put your individual name as @elBoberido has if you signed the eclipse agreement as an individual contributor, or if you are doing this for work purposes and your company cares to claim your IP, you can put their name as well.

[gpalmer-latai]

Left a few suggestions about more extension testing/error messaging, but overall this looks great to me.

[elfenpiff]

I have some remarks, but they are purely optional. If you have to touch the PR again, it would be nice if you would incorporate them, otherwise just merge the PR.

[kozakusek]

It seems like it would be a good idea for you to take some credit for your contribution by updating the copyright headers: https://github.com/eclipse-iceoryx/iceoryx/blob/master/iceoryx_posh/source/mepoo/mem_pool.cpp#L3

Basically put your individual name as @elBoberido has if you signed the eclipse agreement as an individual contributor, or if you are doing this for work purposes and your company cares to claim your IP, you can put their name as well.

Well
Why not
I'll add it in the few files that I've done something more than just change 32 to 64 :D

[elBoberido]

Well
Why not
I'll add it in the few files that I've done something more than just change 32 to 64 :D

That' s also how I handle it. It must be something more complex than something a trained ferret could have done 😆

[elBoberido]

Looks good. Every request was implemented. I'm going to merge once the CI is finished. Thanks very much for your contribution and patience. Hope to see you in the developer meetup :)