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

ODR violation when compiling with LTO/C++17 #465

Closed
biojppm opened this issue Aug 19, 2024 · 5 comments
Closed

ODR violation when compiling with LTO/C++17 #465

biojppm opened this issue Aug 19, 2024 · 5 comments

Comments

@biojppm
Copy link
Owner

biojppm commented Aug 19, 2024

          So I started by trying to imitate Fedora’s build flags as closely as I reasonably could, with the idea that if I could reproduce this with `samples/add_subdirectory`, I would 
$ mock -r fedora-rawhide-ppc64le --clean
$ mock -r fedora-rawhide-ppc64le -i git-core cmake gcc-c++ 'cmake(gtest)'
$ mock -r fedora-rawhide-ppc64le --shell --enable-network
<mock-chroot> sh-5.2# git clone https://github.com/biojppm/rapidyaml.git
<mock-chroot> sh-5.2# cd rapidyaml/
<mock-chroot> sh-5.2# git checkout fix/464_ub_ppc64le
<mock-chroot> sh-5.2# git submodule update --init --recursive
<mock-chroot> sh-5.2# cd samples/add_subdirectory
<mock-chroot> sh-5.2# CXXFLAGS="-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -mcpu=power8 -mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection" cmake -S . -B build -DRYML_CXX_STANDARD=14
<mock-chroot> sh-5.2# cmake --build build -j16 --target run
[ 95%] Linking CXX executable ryml-quickstart
/builddir/rapidyaml/ext/c4core/src/c4/charconv.hpp:197:3: warning: type ‘c4::RealFormat_e’ violates the C++ One Definition Rule [-Wodr]
  197 | } RealFormat_e;
      |   ^
/builddir/rapidyaml/ext/c4core/src/c4/charconv.hpp:209:3: note: an enum with different value name is defined in another translation unit
  209 | } RealFormat_e;
      |   ^
/builddir/rapidyaml/ext/c4core/src/c4/charconv.hpp:190:5: note: name ‘FTOA_FLOAT’ is defined as 32-bit while another translation unit defines it as 8-bit
  190 |     FTOA_FLOAT = static_cast<std::underlying_type<std::chars_format>::type>(std::chars_format::fixed),
      |     ^
/builddir/rapidyaml/ext/c4core/src/c4/charconv.hpp:202:5: note: mismatching definition
  202 |     FTOA_FLOAT = 'f',
      |     ^
[ 95%] Built target ryml-quickstart
[100%] running: /builddir/rapidyaml/samples/add_subdirectory/build/ryml-quickstart
WTF1:
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
gmake[3]: *** [CMakeFiles/run.dir/build.make:71: CMakeFiles/run] Segmentation fault (core dumped)

I haven’t had a chance to try to minimize the compiler flags yet, but that looked interesting enough to go ahead and report it!

Originally posted by @musicinmybrain in #464 (comment)

@biojppm
Copy link
Owner Author

biojppm commented Aug 23, 2024

@musicinmybrain I think this is happening because the sample does not use RYML_CXX_STANDARD in its cmake file. The tell-tale sign is that if you use --std=c++17 or whatever, things work as expected.

That is, with RYML_CXX_STANDARD the library ends up being compiled with a different standard than the application.

So the solution is simple: not to use RYML_CXX_STANDARD.

@biojppm
Copy link
Owner Author

biojppm commented Aug 23, 2024

In other words, this does not seem to be an issue.

@musicinmybrain
Copy link
Contributor

I definitely agree with your assessment of the reason for this, and that the experiments in #464 showed that it’s unrelated to any test failures.

We do have some mixing of C++ versions in Fedora:

  • the c4core package is built as C++11 because that’s its default
  • rapidyaml is C++14 (via RYML_CXX_STANDARD) because it uses the system gtest for testing, and gtest 1.13.0 and later require at least C++14
  • jsonnet is implicitly C++17 because that’s the current GCC default

In almost all cases, we are able to link together shared libraries built with different C++ standards without problems. This isn’t necessarily true across all C++ compilers, but it’s true when everything is built with GCC, because GCC specifically tries to guarantee that. There are a few cases where a design decision can subvert this – like this case, where the preprocessor is used to give a type has a different layout depending on the standard version. Even there, we don’t encounter trouble in practice because c4project and rapidyaml are both compiled as shared libraries, and c4::RealFormat_e doesn’t appear in the signatures of any externally visible functions. (We can verify this with e.g. objdump -C -T usr/lib64/libc4core.so.0.2.1 and objdump -C -T usr/lib64/libryml.so.0.7.1.)

So the use of shared libraries protects us from any ODR issues in practice; they appeared in #464 because I was working in a git checkout and linking statically. I agree that “just don’t do that” is a reasonable thing to say about that.

I could preemptively start build c4core and rapidyaml as C++17, but it doesn’t really seem to matter one way or the other.

@biojppm
Copy link
Owner Author

biojppm commented Aug 23, 2024

Well, the recommendation is of course that if c4core is linked to ryml, then both should use the same standard. In this case, I would recommend bringing up Fedora's c4core to c++14 (although I don't know if that's up to you).

But anyway, this is a user problem. So I'll close this issue for now.

@biojppm biojppm closed this as completed Aug 23, 2024
@musicinmybrain
Copy link
Contributor

Well, the recommendation is of course that if c4core is linked to ryml, then both should use the same standard. In this case, I would recommend bringing up Fedora's c4core to c++14 (although I don't know if that's up to you).

It is, and since we generally defer to upstream defaults when choosing a C++ standard, I’ll cite this comment when I do it.

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

No branches or pull requests

2 participants