[libc++] Avoid re-exporting a few specific symbols from libc++abi #109054
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
libc++
|
@llvm/pr-subscribers-libcxxabi @llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesIn 6a884a9, I synchronized the export list of libc++abi to the export list of libc++. From the linker's perspective, this caused these symbols to be taken from libc++.dylib instead of libc++abi.dylib. However, that can be problematic when back-deploying. Indeed, this means that the linker will encode an undefined reference to be fullfilled by libc++.dylib, but when backdeploying against an older system, that symbol might only be available in libc++abi.dylib. Most of the symbols that started being re-exported after 6a884a9 turn out to be implementation details of libc++abi, so nobody really depends on them and this back-deployment issue is inconsequential. However, we ran into issues with a few of these symbols while testing LLVM 19, which led to this patch. This slipped between the cracks and that is why the patch is coming so long after the original patch landed. In the future, a follow-up cleanup would be to stop exporting most of the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++ since they are implementation details that nobody should be relying on. Full diff: https://github.com/llvm/llvm-project/pull/109054.diff 7 Files Affected:
diff --git a/libcxx/lib/abi/CHANGELOG.TXT b/libcxx/lib/abi/CHANGELOG.TXT
index 32526f1786c6d9..68c9d980a016ea 100644
--- a/libcxx/lib/abi/CHANGELOG.TXT
+++ b/libcxx/lib/abi/CHANGELOG.TXT
@@ -16,6 +16,36 @@ New entries should be added directly below the "Version" header.
Version 19.0
------------
+* [libc++] Avoid re-exporting a few specific symbols from libc++abi
+
+ In 6a884a9aef39, I synchronized the export list of libc++abi to the
+ export list of libc++. From the linker's perspective, this caused
+ these symbols to be taken from libc++.dylib instead of libc++abi.dylib.
+
+ However, that can be problematic when back-deploying. Indeed, this means
+ that the linker will encode an undefined reference to be fullfilled by
+ libc++.dylib, but when backdeploying against an older system, that symbol
+ might only be available in libc++abi.dylib.
+
+ Most of the symbols that started being re-exported after 6a884a9aef39
+ turn out to be implementation details of libc++abi, so nobody really
+ depends on them and this back-deployment issue is inconsequential.
+
+ However, we ran into issues with a few of these symbols while testing
+ LLVM 19, which led to this patch.
+
+ In the future, a follow-up cleanup would be to stop exporting most of
+ the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++
+ since they are implementation details that nobody should be relying
+ on.
+
+ <arch>-apple-darwin
+ -------------------
+ Symbol not reexported anymore: ___cxa_rethrow_primary_exception
+ Symbol not reexported anymore: __ZTIN10__cxxabiv117__class_type_infoE
+ Symbol not reexported anymore: __ZTIN10__cxxabiv120__si_class_type_infoE
+ Symbol not reexported anymore: __ZTIN10__cxxabiv121__vmi_class_type_infoE
+
* [libc++] Always keep libc++abi re-exports up-to-date
This patch makes sure that the set of libc++abi symbols re-exported from libc++
diff --git a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index 917388f86811fe..32acae46e292d7 100644
--- a/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -78,12 +78,9 @@
{'is_defined': False, 'name': '__ZTIN10__cxxabiv116__enum_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv116__shim_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv117__array_type_infoE', 'type': 'U'}
-{'is_defined': False, 'name': '__ZTIN10__cxxabiv117__class_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv117__pbase_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv119__pointer_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv120__function_type_infoE', 'type': 'U'}
-{'is_defined': False, 'name': '__ZTIN10__cxxabiv120__si_class_type_infoE', 'type': 'U'}
-{'is_defined': False, 'name': '__ZTIN10__cxxabiv121__vmi_class_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv123__fundamental_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv129__pointer_to_member_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIPDh', 'type': 'U'}
@@ -2002,12 +1999,9 @@
{'is_defined': True, 'name': '__ZTIN10__cxxabiv116__enum_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv116__shim_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv117__array_type_infoE', 'type': 'I'}
-{'is_defined': True, 'name': '__ZTIN10__cxxabiv117__class_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv117__pbase_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv119__pointer_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv120__function_type_infoE', 'type': 'I'}
-{'is_defined': True, 'name': '__ZTIN10__cxxabiv120__si_class_type_infoE', 'type': 'I'}
-{'is_defined': True, 'name': '__ZTIN10__cxxabiv121__vmi_class_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv123__fundamental_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv129__pointer_to_member_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTINSt12experimental15fundamentals_v112bad_any_castE', 'size': 0, 'type': 'OBJECT'}
@@ -2615,7 +2609,6 @@
{'is_defined': True, 'name': '___cxa_new_handler', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_pure_virtual', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_rethrow', 'type': 'I'}
-{'is_defined': True, 'name': '___cxa_rethrow_primary_exception', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_terminate_handler', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_throw', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_throw_bad_array_new_length', 'type': 'I'}
diff --git a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
index defe235a283c21..7b5d77499d55fa 100644
--- a/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
+++ b/libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
@@ -78,12 +78,9 @@
{'is_defined': False, 'name': '__ZTIN10__cxxabiv116__enum_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv116__shim_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv117__array_type_infoE', 'type': 'U'}
-{'is_defined': False, 'name': '__ZTIN10__cxxabiv117__class_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv117__pbase_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv119__pointer_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv120__function_type_infoE', 'type': 'U'}
-{'is_defined': False, 'name': '__ZTIN10__cxxabiv120__si_class_type_infoE', 'type': 'U'}
-{'is_defined': False, 'name': '__ZTIN10__cxxabiv121__vmi_class_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv123__fundamental_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIN10__cxxabiv129__pointer_to_member_type_infoE', 'type': 'U'}
{'is_defined': False, 'name': '__ZTIPDh', 'type': 'U'}
@@ -2002,12 +1999,9 @@
{'is_defined': True, 'name': '__ZTIN10__cxxabiv116__enum_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv116__shim_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv117__array_type_infoE', 'type': 'I'}
-{'is_defined': True, 'name': '__ZTIN10__cxxabiv117__class_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv117__pbase_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv119__pointer_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv120__function_type_infoE', 'type': 'I'}
-{'is_defined': True, 'name': '__ZTIN10__cxxabiv120__si_class_type_infoE', 'type': 'I'}
-{'is_defined': True, 'name': '__ZTIN10__cxxabiv121__vmi_class_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv123__fundamental_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTIN10__cxxabiv129__pointer_to_member_type_infoE', 'type': 'I'}
{'is_defined': True, 'name': '__ZTINSt12experimental15fundamentals_v112bad_any_castE', 'size': 0, 'type': 'OBJECT'}
@@ -2649,7 +2643,6 @@
{'is_defined': True, 'name': '___cxa_new_handler', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_pure_virtual', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_rethrow', 'type': 'I'}
-{'is_defined': True, 'name': '___cxa_rethrow_primary_exception', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_terminate_handler', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_throw', 'type': 'I'}
{'is_defined': True, 'name': '___cxa_throw_bad_array_new_length', 'type': 'I'}
diff --git a/libcxxabi/lib/cxxabiv1.exp b/libcxxabi/lib/cxxabiv1.exp
index b1bab45ef33479..0a22831a63d0bb 100644
--- a/libcxxabi/lib/cxxabiv1.exp
+++ b/libcxxabi/lib/cxxabiv1.exp
@@ -2,12 +2,9 @@
__ZTIN10__cxxabiv116__enum_type_infoE
__ZTIN10__cxxabiv116__shim_type_infoE
__ZTIN10__cxxabiv117__array_type_infoE
-__ZTIN10__cxxabiv117__class_type_infoE
__ZTIN10__cxxabiv117__pbase_type_infoE
__ZTIN10__cxxabiv119__pointer_type_infoE
__ZTIN10__cxxabiv120__function_type_infoE
-__ZTIN10__cxxabiv120__si_class_type_infoE
-__ZTIN10__cxxabiv121__vmi_class_type_infoE
__ZTIN10__cxxabiv123__fundamental_type_infoE
__ZTIN10__cxxabiv129__pointer_to_member_type_infoE
diff --git a/libcxxabi/lib/itanium-base.exp b/libcxxabi/lib/itanium-base.exp
index 002e062df423e7..0c23a19c94c528 100644
--- a/libcxxabi/lib/itanium-base.exp
+++ b/libcxxabi/lib/itanium-base.exp
@@ -12,7 +12,6 @@ ___cxa_guard_acquire
___cxa_guard_release
___cxa_increment_exception_refcount
___cxa_pure_virtual
-___cxa_rethrow_primary_exception
___cxa_throw_bad_array_new_length
___cxa_uncaught_exception
___cxa_uncaught_exceptions
diff --git a/libcxxabi/lib/symbols-not-reexported.exp b/libcxxabi/lib/symbols-not-reexported.exp
new file mode 100644
index 00000000000000..ea5d0b3fc15d78
--- /dev/null
+++ b/libcxxabi/lib/symbols-not-reexported.exp
@@ -0,0 +1,13 @@
+# These symbols are not re-exported from libc++ because providing a definition in libc++ causes
+# issues with some clients when backdeploying.
+
+# These symbols are implementation details of libc++abi, but they are referenced from UBSan
+# (which is a total hack). We'll need to figure out how to decouple UBSan from these details
+# before we can stop exporting them from libc++abi.
+__ZTIN10__cxxabiv117__class_type_infoE
+__ZTIN10__cxxabiv120__si_class_type_infoE
+__ZTIN10__cxxabiv121__vmi_class_type_infoE
+
+# This symbol is not an implementation detail of libc++abi, but it also causes issues when moving
+# to libc++. This needs further investigation.
+___cxa_rethrow_primary_exception
diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index c1a7bcb14eb199..1c46e99a4d9e3d 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -214,14 +214,22 @@ if (LIBCXXABI_ENABLE_SHARED)
endif()
add_library(cxxabi-reexports INTERFACE)
- function(reexport_symbols file)
+ function(export_symbols file)
# -exported_symbols_list is only available on Apple platforms
if (APPLE)
target_link_libraries(cxxabi_shared PRIVATE "-Wl,-exported_symbols_list,${file}")
+ endif()
+ endfunction()
+
+ function(reexport_symbols file)
+ export_symbols("${file}")
+ # -reexported_symbols_list is only available on Apple platforms
+ if (APPLE)
target_link_libraries(cxxabi-reexports INTERFACE "-Wl,-reexported_symbols_list,${file}")
endif()
endfunction()
+ export_symbols("${CMAKE_CURRENT_SOURCE_DIR}/../lib/symbols_not_reexported.exp")
reexport_symbols("${CMAKE_CURRENT_SOURCE_DIR}/../lib/cxxabiv1.exp")
reexport_symbols("${CMAKE_CURRENT_SOURCE_DIR}/../lib/fundamental-types.exp")
reexport_symbols("${CMAKE_CURRENT_SOURCE_DIR}/../lib/itanium-base.exp")
|
var-const
approved these changes
Sep 17, 2024
|
(Interesting issue. Aside: |
|
Hi, since we are wrapping up this next release we are very strict with the fixes we pick at this point. Can you please respond to the following questions to help me understand if this has to be included in the final release or not. Is this PR a fix for a regression or a critical issue? What is the risk of accepting this into the release branch? What is the risk of NOT accepting this into the release branch? |
100% understood.
Yes. I basically introduced an unintended ABI break in 6a884a9, it just happened that the ABI break was subtle enough not to cause any issues outside of our internal release testing.
Code compiled against LLVM 19.1 (released with these symbols in
Code linking against LLVM 19.1's |
ldionne
force-pushed
the
review/unexport-cxxabi-symbols
branch
from
01b10a3
to
78afd0b
Compare
In 6a884a9, I synchronized the export list of libc++abi to the export list of libc++. From the linker's perspective, this caused these symbols to be taken from libc++.dylib instead of libc++abi.dylib. However, that can be problematic when back-deploying. Indeed, this means that the linker will encode an undefined reference to be fullfilled by libc++.dylib, but when backdeploying against an older system, that symbol might only be available in libc++abi.dylib. Most of the symbols that started being re-exported after 6a884a9 turn out to be implementation details of libc++abi, so nobody really depends on them and this back-deployment issue is inconsequential. However, we ran into issues with a few of these symbols while testing LLVM 19, which led to this patch. This slipped between the cracks and that is why the patch is coming so long after the original patch landed. In the future, a follow-up cleanup would be to stop exporting most of the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++ since they are implementation details that nobody should be relying on.
ldionne
force-pushed
the
review/unexport-cxxabi-symbols
branch
from
78afd0b
to
d509a46
Compare
|
I'm going to merge this to |
|
/cherry-pick 677e8cd |
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Oct 1, 2024
…vm#109054) In 6a884a9, I synchronized the export list of libc++abi to the export list of libc++. From the linker's perspective, this caused these symbols to be taken from libc++.dylib instead of libc++abi.dylib. However, that can be problematic when back-deploying. Indeed, this means that the linker will encode an undefined reference to be fullfilled by libc++.dylib, but when backdeploying against an older system, that symbol might only be available in libc++abi.dylib. Most of the symbols that started being re-exported after 6a884a9 turn out to be implementation details of libc++abi, so nobody really depends on them and this back-deployment issue is inconsequential. However, we ran into issues with a few of these symbols while testing LLVM 19, which led to this patch. This slipped between the cracks and that is why the patch is coming so long after the original patch landed. In the future, a follow-up cleanup would be to stop exporting most of the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++ since they are implementation details that nobody should be relying on. rdar://131984512 (cherry picked from commit 677e8cd)
|
/pull-request #110677 |
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Oct 2, 2024
…vm#109054) In 6a884a9, I synchronized the export list of libc++abi to the export list of libc++. From the linker's perspective, this caused these symbols to be taken from libc++.dylib instead of libc++abi.dylib. However, that can be problematic when back-deploying. Indeed, this means that the linker will encode an undefined reference to be fullfilled by libc++.dylib, but when backdeploying against an older system, that symbol might only be available in libc++abi.dylib. Most of the symbols that started being re-exported after 6a884a9 turn out to be implementation details of libc++abi, so nobody really depends on them and this back-deployment issue is inconsequential. However, we ran into issues with a few of these symbols while testing LLVM 19, which led to this patch. This slipped between the cracks and that is why the patch is coming so long after the original patch landed. In the future, a follow-up cleanup would be to stop exporting most of the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++ since they are implementation details that nobody should be relying on. rdar://131984512
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Oct 2, 2024
…vm#109054) In 6a884a9, I synchronized the export list of libc++abi to the export list of libc++. From the linker's perspective, this caused these symbols to be taken from libc++.dylib instead of libc++abi.dylib. However, that can be problematic when back-deploying. Indeed, this means that the linker will encode an undefined reference to be fullfilled by libc++.dylib, but when backdeploying against an older system, that symbol might only be available in libc++abi.dylib. Most of the symbols that started being re-exported after 6a884a9 turn out to be implementation details of libc++abi, so nobody really depends on them and this back-deployment issue is inconsequential. However, we ran into issues with a few of these symbols while testing LLVM 19, which led to this patch. This slipped between the cracks and that is why the patch is coming so long after the original patch landed. In the future, a follow-up cleanup would be to stop exporting most of the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++ since they are implementation details that nobody should be relying on. rdar://131984512
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Oct 3, 2024
…vm#109054) In 6a884a9, I synchronized the export list of libc++abi to the export list of libc++. From the linker's perspective, this caused these symbols to be taken from libc++.dylib instead of libc++abi.dylib. However, that can be problematic when back-deploying. Indeed, this means that the linker will encode an undefined reference to be fullfilled by libc++.dylib, but when backdeploying against an older system, that symbol might only be available in libc++abi.dylib. Most of the symbols that started being re-exported after 6a884a9 turn out to be implementation details of libc++abi, so nobody really depends on them and this back-deployment issue is inconsequential. However, we ran into issues with a few of these symbols while testing LLVM 19, which led to this patch. This slipped between the cracks and that is why the patch is coming so long after the original patch landed. In the future, a follow-up cleanup would be to stop exporting most of the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++ since they are implementation details that nobody should be relying on. rdar://131984512
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 6a884a9, I synchronized the export list of libc++abi to the export list of libc++. From the linker's perspective, this caused these symbols to be taken from libc++.dylib instead of libc++abi.dylib.
However, that can be problematic when back-deploying. Indeed, this means that the linker will encode an undefined reference to be fullfilled by libc++.dylib, but when backdeploying against an older system, that symbol might only be available in libc++abi.dylib.
Most of the symbols that started being re-exported after 6a884a9 turn out to be implementation details of libc++abi, so nobody really depends on them and this back-deployment issue is inconsequential.
However, we ran into issues with a few of these symbols while testing LLVM 19, which led to this patch. This slipped between the cracks and that is why the patch is coming so long after the original patch landed.
In the future, a follow-up cleanup would be to stop exporting most of the _cxxabiv1_foo_type_infoE symbols from both libc++abi and libc++ since they are implementation details that nobody should be relying on.
rdar://131984512