-
Notifications
You must be signed in to change notification settings - Fork 744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Question : Why Fast-DDS depending on specific commit of foonathan-memory instead of release? [13682] #2151
Comments
We've been building Fast-DDS against foonathan-memory 0.7 for a while, and we don't have this issue. If memory serves, foonathan-memory has a memory debugger tool "nodesize_dbg" which to be needs to run and generate src/container_node_sizes_impl.hpp. If FOONATHAN_MEMORY_BUILD_TOOLS is not switched on, this header is not generated, and incorrect memory allocations result. You might want to turn it off after the library is built, else will become an unnecessary dependency when using the library. It's also a pain for cross compilation, as running this debug application on a foreign architecture requires some trickery. But that's probably more of a bug report for foonathan instead of eProsima. |
Hi @tparys , We are actually planning on taking it a bit further than 0.7.0 (eProsima/foonathan_memory_vendor#49), specially cause we also considered all the things you listed above. We just haven't got to it yet (vacations and such), but we will in the upcoming weeks! |
Yeah, I don't like the solution either. However, there is no other way to get the container node size information. If it's too annoying, you can completely disable their generation and use conservative constants instead (or maybe you don't need them at all?). |
Thanks for pointing this all out. to be honest i just asking this all because i am huge fan of packages. :) and dependency management. Fast-DDS is the best DDS implementation i found so far. ;) That's why want to bring it to Conan Center. Just one small question into direction of our eProsima engineers ;) with what cmake build options do you build foonathan/memory? Reason : i am pretty sure when you using the colcon build the default options are overwritten like : FOONATHAN_MEMORY_MEMORY_RESOURCE:STRING=foonathan_comp::memory_resource One question also to @foonathan - this means : if you want to cross compile (and i think DDS is a topic especially for embedded devices so yes cross compiling) you always have to disable the sizecheck? I currently analyze from conan perspective what default options etc. would be required to have easy to compile normal build examples and also cross compile examples. |
No, you just need to ensure that the container node sizes are computed correctly (or not use the facility at all). I've originally disabled them completely when cross compiling to prevent exactly those errors, but people have submitted various solutions for different targets that work, which I cannot verify. |
This is not exactly true. If you build Fast DDS on a system that has foonathan-memory already installed, that version will be used. The specific commit id is the one we use on our CI via foonathan_memory_vendor package. This is to comply with this quality declaration @foonathan Doing a v0.7.1 release would be nice. |
I can do a new release at the end of the week. |
@MiguelCompany yes you are totally right. ;) if the system has already this library present it wont be linked against different version. But for embedded areas with cross compilation requirements .... it is not that easy. and i actually think Fast-DDS is "the" best implementation of DDS especially for embedded areas like embedded linux and so. @EduPonz small question - the current master is already using this latest foonathan version? if so i would try out the conanfiles and next version i will bring into artifactory with this new reference on latest foonathan version. @foonathan great work! ;) |
Hi:
And I encountered the same error as above when running the Fast-DDS:
So, what is this emulator? Should I define it myself? If I need to define it myself, how should I define it? |
Not yet. We have eProsima/foonathan_memory_vendor#49 to use the latest master (the new tag foonathan just created), but we still want the PR to be verified by the ROS 2 guys, so it'll most probably take a couple of days until we can merge it in. |
@EduPonz thanks for the update - not an issue! :) |
Did you try it with cmake -DFOONATHAN_MEMORY_CHECK_ALLOCATION_SIZE=OFF ..? |
Some details here |
This is maybe more a question towards @MiguelCompany, but why does it work without node sizes? If node sizes are unavailable it just shouldn't compile, instead of giving wrong sizes. How are you constructing the memory_pool in this instance? @large-cat can you show the allocation call stack and where the
This just disables the check! It still tries to allocate a node with size 56 even though each node only has size 48. |
I use cmake -DFOONATHAN_MEMORY_CHECK_ALLOCATION_SIZE=OFF now, and fast-DDS can work normally. But are there any other risks? |
I reported this wring when compiling fast-DDS(v22.3.4). And I can compile it。 |
Hi |
Hi, |
@large-cat Would you mind checking with the head of branch 2.3.x? There are some bugfixes there that could help solve your issue. |
@foonathan I missed this question! Sorry to answer so late! The code for finding the node size starts here and then goes into the impl folder. You could follow the lead from there. Basically, when We have plans to extend this mechanism, so that C++17 In the mean time, if the user has knowledge on the implementation of the |
Ah, this could explain things. Are you sure that your approximation is not buggy? I'm getting the occasional issue about invalid node sizes, because people set up their cross compiling incorrectly, apparently used your implementation, and crash. E.g. foonathan/memory#103 (comment) It seems like the node size for |
@foonathan Due to alignment issues, the code here is wrong, and the mentioned enumeration should be included. |
@dotChris90 @tparys @large-cat @foonathan |
@MiguelCompany @foonathan thanks you two for investigating. Had some private issues to fix last weeks. But after PR is merged will look if i can patch the two packages in conan center since i think cross compilation is a common thing in embedded area so guess everybody will appreciate. Once again. Thank to you two and have a good week ✌️😊 |
@dotChris90 @tparys @large-cat Both #2229 (and backports), as well as eProsima/foonathan_memory_vendor#49 have been merged. Could you check? |
I am going to proceed and close this issue. It has already been solved. |
Hello together,
it's not an urgent question but just for curiosity. I saw that Fast-DDS depends on foonathan-memory - moreover it depends not on a release of foonathan-memory but on a specific commit id ^^'
Is there a reason for that? i ask that question from Conan Center side because of the following issue - conan-io/conan-center-index#6731. because conan as package manager should reference proper released versions of a lib - we decided to reference foonathan-memory 0.7.0 directly. here we need to add the build option FOONATHAN_MEMORY_CHECK_ALLOCATION_SIZE = OFF then examples .
then it works. :)
Since the conan package currently using 0.7.0 foonathan-memory and official build 0.6.2 we have a gap and i currently searching what are differences and if fast-dds is planning to upgrade from 0.6.2 to 0.7.0? plus - do you guys also have to set FOONATHAN_MEMORY_CHECK_ALLOCATION_SIZE = OFF?
The text was updated successfully, but these errors were encountered: