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

Segmentation fault when using stdlibc++ from gcc 13.1 in combination with clang-16 #6

Closed
saschasc opened this issue Jun 27, 2023 · 14 comments

Comments

@saschasc
Copy link

saschasc commented Jun 27, 2023

GCC has changed the way how the global iostream objects are created since gcc 13.1. This can be found on the official page.

For C++, construction of the global iostream objects std::cout, std::cin, etc. is now done inside the standard library, instead of in every source file that includes the header. This change improves the start-up performance of C++ programs, but it means that code compiled with GCC 13.1 will crash if the correct version of libstdc++.so is not used at runtime. See the documentation about using the right libstdc++.so at runtime. Future GCC releases will mitigate the problem so that the program cannot be run at all with an older libstdc++.so.

More details can also be found here:
https://developers.redhat.com/articles/2023/04/03/leaner-libstdc-gcc-13

On macOS SUPPORTS_INIT_PRIORITY within gcc is set to 0. This means that the global iostream object is not initialized and the fallback will be taken (i.e. static initialization of the iostream object).

The problem is that when the iostream include is used, the expression __has_attribute(__init_priority__) is truesince clang-16 supports __init_priority__ and the static initialization as before is not done. See here:

https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/iostream#L78

This leads to a segmentation fault with a simple sample application when using clang-16 in combination with stdlibc++.

Sample application:

#include <iostream>

int main()
{
  std::cout << "Hello" << std::endl;
}
$HOMEBREW_PREFIX/opt/llvm@16/bin/clang++ \
  -v \
  -stdlib=libstdc++ \
  -stdlib++-isystem $HOMEBREW_PREFIX/opt/gcc@13/include/c++/13 \
  -cxx-isystem $HOMEBREW_PREFIX/opt/gcc@13/include/c++/13/x86_64-apple-darwin22 \
  -L $HOMEBREW_PREFIX/opt/gcc@13/lib/gcc/13/ \
  -L $HOMEBREW_PREFIX/opt/llvm/lib \
  -o test main.cpp

Execute test -> segfault.

➜  ~ ./test
[1]    7965 segmentation fault  ./test

I would suggest to add and document a patch for this. Basically we would need to include another #elif statement oder change this #if statement to include Darwin.

#if !__has_attribute(__init_priority__)
  static ios_base::Init __ioinit;
#elif defined(_GLIBCXX_SYMVER_GNU)
  __extension__ __asm (".globl _ZSt21ios_base_library_initv");
#endif
@saschasc
Copy link
Author

For completeness sake. I also started an issue on the GCC Bugzilla. But this might take some more time to get fixed. So we could already patch GCC 13.1 eventually.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110432

@saschasc
Copy link
Author

An easy and working fix would be to include this as a patch until GCC 13.2 comes out with a fixed version. What do you think?

#if !__has_attribute(__init_priority__) || defined __APPLE__
  static ios_base::Init __ioinit;
#elif defined(_GLIBCXX_SYMVER_GNU)
  __extension__ __asm (".globl _ZSt21ios_base_library_initv");
#endif

iostream.patch

@saschasc
Copy link
Author

saschasc commented Jul 1, 2023

Thanks to @jwakely for the fix.
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=fe2651affa8c15624188bfd062fb894648743431.

If would go for the easy fix as proposed above.

@jwakely Do you think it would make sense to patch the current version in brew? If yes should I prepare a patch for it? I would go for a hotfix as proposed in the Bugzilla issue. Afterwards a new brew package would need to be built.

@jwakely
Copy link
Contributor

jwakely commented Jul 1, 2023

For brew the ! defined __APPLE__ solution is good enough, because brew doesn't need to consider hypothetical other platforms where there might be a problem.

@iains
Copy link
Owner

iains commented Jul 1, 2023

  • I'm fine with using the work-around as band-aid on 13.1 (I'll merge that in due course - right now my test hardware is dedicated to 10.5).

  • As a matter of general policy I am trying to minimise the difference between these branches and upstream - so I'd prefer to use the back ported fix for 13.2 (we still have time to deal with that).

Of course home-brew is free to choose independently ;)

@saschasc
Copy link
Author

saschasc commented Jul 1, 2023

I would suggest that I will create a PR for gcc on brew. At the end we only need to patch a header file which might also be doable as post install step.

Then I would close this issue here. Do you agree?

@iains
Copy link
Owner

iains commented Jul 1, 2023

I would suggest that I will create a PR for gcc on brew. At the end we only need to patch a header file which might also be doable as post install step.

Does HB intend to pull from 13.1 again (or just make a local fix)?
(I can do the fix sometime this weekend - but maybe not today)

I'd assume 13.2 would be a new version pulled from the (to be finalised) arm64 darwin 13.2 branch [this is WIP at the moment - but I will do a preview and let you and FX know where to look]

Then I would close this issue here. Do you agree?

Sure - we have an upstream bug for it (which is fine because it affects an upstream target too) so we won't forget :)

@saschasc
Copy link
Author

saschasc commented Jul 1, 2023

Does HB intend to pull from 13.1 again (or just make a local fix)? (I can do the fix sometime this weekend - but maybe not today)

Both would be possible. I am not sure how many are using this gcc-13-branch directly. A local fix might be an easier path to follow.

@iains
Copy link
Owner

iains commented Jul 1, 2023

For brew the ! defined __APPLE__ solution is good enough, because brew doesn't need to consider hypothetical other platforms where there might be a problem.

.. but people do build cross-compilers from Darwin to other platforms, perhaps not so much on homebrew (but I'm not sure it's 0 there).

So, in general, I expect to be able to build a cross to xxxx-linux-gnu or x86_64-mingw-w64 etc. using these branches (and I usually test a few cases).

However, that's just a general comment - in this specific case I doubt it makes any difference.

@saschasc
Copy link
Author

saschasc commented Jul 1, 2023

I did not a lot of cross-compiles. But would __APPLE__ be set when cross-compiling for another platform?

@iains
Copy link
Owner

iains commented Jul 1, 2023

I did not a lot of cross-compiles. But would __APPLE__ be set when cross-compiling for another platform?

no, it would not - but if the general (non-apple-specific) fix is needed for some other platform then we would want it on the Darwin branch too. [this is hypothetical in this case, we are all agreed that it is not a likely scenario] ..

.. however, in general I will resist Darwin-only hacks (except for short-term band-aid) on the branches because that just makes upstreaming harder.

@saschasc
Copy link
Author

saschasc commented Jul 1, 2023

Fully understand now. Thanks. I will close this and try to create a updated PR for brew.

Thanks for both of you.

@saschasc saschasc closed this as completed Jul 1, 2023
@jwakely
Copy link
Contributor

jwakely commented Jul 1, 2023

Defining the variable in <iostream> when not strictly needed doesn't do any harm, just results in slightly slower application startup time (which is already the case for all past gcc releases).

You could just remove the preprocessor directives and asm statement and leave the variable there unconditionally, that's what gcc 12 had.

@saschasc
Copy link
Author

saschasc commented Jul 1, 2023

@jwakely Was easier to use your first band-aid idea with the inreplace method in Homebrew. I've added a safeguard such that the fix will be removed in GCC 13.2.

Homebrew/homebrew-core#135530

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

3 participants