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

Fix crash when building with _GLIBCXX_USE_CXX11_ABI=0 #686

Merged
merged 4 commits into from
Nov 28, 2024

Conversation

graebm
Copy link
Contributor

@graebm graebm commented Nov 27, 2024

Issue:
A user had been building aws-crt-cpp with CXXFLAGS=-D_GLIBCXX_USE_CXX11_ABI=0, and that worked fine until the recent change where we started hiding symbols by default (PR #666)

With symbols hidden by default, and _GLIBCXX_USE_CXX11_ABI=0, tests would crash in the destructor of Aws::Crt::String (which is std::string with a custom allocator).

Description of changes:
Don't hide symbols when building for the ancient glibcxx ABI.

I don't 100% understand why this fixes the issue, but the situation is esoteric enough that it doesn't seem worth spending more days on this problem.

Background
See: https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html
GCC had to introduce a new ABI for C++11 which removed the copy-on-write optimization for strings, which is forbidden in C++11 and later (new ABI uses a small-string optimization instead). But GCC let users manually set _GLIBCXX_USE_CXX11_ABI=0 so they could opt back into the old ABI and continue working with libraries compiled for C++03.

I don't think GCC intended devs to continue using this 10 years later, but some people are, because the old copy-on-write optimization has less memory usage.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

I don't 100% understand why this fixes the issue, but the situation is esoteric enough that it doesn't seem worth spending any more days on this problem
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@graebm graebm enabled auto-merge (squash) November 27, 2024 22:45
CMakeLists.txt Outdated
# see: https://github.com/awslabs/aws-crt-cpp/pull/675
if(NOT (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "5.0"))
# Except where it causes problems, and the situation is weird enough that it's not worth investigating further.
string(FIND "${CMAKE_CXX_FLAGS}" "-D_GLIBCXX_USE_CXX11_ABI=0" found_ancient_abi_flag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to set CXX flags via CMAKE_CXX_FLAGS_<CONFIG>.

The following command will add -D_GLIBCXX_USE_CXX11_ABI=0 option to compile command, but CMAKE_CXX_FLAGS will be empty:

cmake -S . -B build -DCMAKE_CXX_FLAGS_DEBUG="-D_GLIBCXX_USE_CXX11_ABI=0" -DCMAKE_BUILD_TYPE=Debug

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think developers actually set CXX flags this way, but additional comment might help to debug in the future if someone decides to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice callout. Checking every variant _<CONFIG> variant now

I tried to be clever and use CheckCXXSourceCompiles, but this didn't take the _<CONFIG> variants into account

@graebm graebm disabled auto-merge November 27, 2024 22:48
BTW check_cxx_source_compiles() doesn't  use the _<CONFIG> flags
@graebm graebm enabled auto-merge (squash) November 28, 2024 00:32
@graebm graebm merged commit 5478ce2 into main Nov 28, 2024
61 checks passed
@graebm graebm deleted the glibcxx-ancent-abi branch November 28, 2024 00:39
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.

4 participants