Skip to content
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

Merged
merged 4 commits into from
Feb 13, 2022
Merged

Conversation

matt-cross
Copy link
Contributor

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.

-Matt Cross

@foonathan
Copy link
Owner

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.

@MiguelCompany
Copy link
Contributor

@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 generated container_node_sizes_impl.hpp and it only has constants for alignments of 1.

@matt-cross
Copy link
Contributor Author

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.

@MiguelCompany
Copy link
Contributor

MiguelCompany commented Feb 8, 2022

@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
Generated container_node_sizes_impl.hpp

… type, as some compilers (notably Microsoft) will not report multiple errors in this code.
@matt-cross
Copy link
Contributor Author

@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?

@MiguelCompany
Copy link
Contributor

@matt-cross I confirm that Fast DDS now builds correctly on Windows. To be completely sure, I just started a Fast DDS CI here.

@MiguelCompany
Copy link
Contributor

@foonathan It would be a good idea to let the workflows run here...

…and update regex to handle "ul" suffixes on numbers.
@matt-cross
Copy link
Contributor Author

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.

@matt-cross
Copy link
Contributor Author

@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.

@foonathan
Copy link
Owner

Right sorry, I thought it's enough if I approve them once.

@matt-cross
Copy link
Contributor Author

No problem, thanks for looking at this.

@foonathan foonathan merged commit 354204b into foonathan:main Feb 13, 2022
@foonathan
Copy link
Owner

Thank you very much!

@matt-cross
Copy link
Contributor Author

Thanks for merging it! Please feel free to reach out to me if anyone runs into any issues with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants