-
Notifications
You must be signed in to change notification settings - Fork 200
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
Mcross/cmake node size #129
Conversation
…ping C++ compiler error messages.
Thank you, this is a great idea! @MiguelCompany Can you confirm that it works for you? I don't have access to cross-compiling tools. |
@matt-cross What a nice idea! But I have checked it with Fast DDS and it doesn't build. It complains that 'foonathan::memory::detail::map_node_size<8>', 'foonathan::memory::detail::set_node_size<8>', and 'foonathan::memory::detail::unordered_map_node_size<8>', are not defined. I took a look at the |
Thanks for testing it @MiguelCompany ! Can you tell me about the environment you are building in - what compiler/version and build OS? Were you cross-compiling or compiling native for the build system? During development I found that I could optimize it by generating all values for a container type in one pass of the compiler and parsing the error messages once instead of spinning up a separate compiler execution for each alignment/container pair. I am guessing that the compiler you are using may be stopping at the first error message. I didn't see this behavior in the compilers I tested on Compiler Explorer, but my testing was not exhaustive. |
@matt-cross My check was building natively on Windows with Visual Studio 2019. I am using this branch of the vendor package to test Build log: streams.log |
… type, as some compilers (notably Microsoft) will not report multiple errors in this code.
@MiguelCompany I was able to reproduce the behavior on the Microsoft compiler with Compiler Explorer. I could not find any way to change my test code so that it would report multiple errors in one compilation with this code. I made a change to run the compiler once per type, it takes longer but should now work on Microsoft compilers. Can you try it out now? |
@matt-cross I confirm that Fast DDS now builds correctly on Windows. To be completely sure, I just started a Fast DDS CI here. |
@foonathan It would be a good idea to let the workflows run here... |
…and update regex to handle "ul" suffixes on numbers.
I believe I have addressed the failing workflows - I was able to test my changes on gcc5 and clang4.0 on Linux and this does fix it. I do not have easy access to a macOS or Windows build environment to test there, but baed on the errors in those workflows I think this will address them as well. |
@foonathan Would you mind kicking off the workflows here so I can see if I've addresses the issues? My change to use the more general "try_compile()" function rather than executing a process manually definitely solves the issues with gcc5 and clang4; I suspect it will solve it on macos and windows as well but I'm not sure. |
Right sorry, I thought it's enough if I approve them once. |
No problem, thanks for looking at this. |
Thank you very much! |
Thanks for merging it! Please feel free to reach out to me if anyone runs into any issues with this. |
Hi Jonathan,
I got bit by an issue in eProsima's FastRTPS that uses this library when it was cross compiled. When your "node size debugger" didn't run, it had its own logic that happened to be broken on 64-bit ARM with a recent C++ library. We fixed the issue, but it occurred to me that it would be great if your library could provide this information directly even when it was cross compiled.
I remembered a technique proposed by Scott Meyers in one of his Effective C++ books when debugging what types were generated in templates - put the type in a context that would generate an error and the compiler will tell you what type it generated. Therefore I could get the C++ compiler to tell me in an error message values it could calculate at compile time: like the sizes and alignments of types. I tried it on every C++ compiler on Compiler Explorer, and saw that every compiler put calculated numbers in the generated type name even though some would put calculated sub-types in separate lines.
In the end, this uses compiler error messages and some logic in CMake to generate the
container_node_sizes_impl.hpp
file even when cross-compiling, without requiring an emulator or manually running the debug tool on target hardware.Let me know what you think.