Skip to content

[libcxx] Use alias for detecting overriden function #120805

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

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

petrhosek
Copy link
Member

@petrhosek petrhosek commented Dec 20, 2024

This mechanism is preferable in environments like embedded since it doesn't require special handling of the custom section.

This is a reland of #114961 which addresses the issue reported by downstream users. Specifically, the two differences from the previous version are:

  • The internal symbol##_impl__ symbol in the Mach-O implementation is annotated with __attribute__((used)) to prevent LTO from deleting it which we've seen in the previous version.
  • __is_function_overridden is marked as inline so these symbols are placed in a COMDAT (or fully inlined) to avoid duplicate symbol errors which we've seen in the previous version.

This mechanism is preferable in environments like embedded since it
doesn't require special handling of the custom section.
@petrhosek petrhosek requested a review from ldionne December 20, 2024 22:57
@petrhosek petrhosek requested review from a team as code owners December 20, 2024 22:57
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc++abi libc++abi C++ Runtime Library. Not libc++. labels Dec 20, 2024
@petrhosek
Copy link
Member Author

This is a reland of #114961 which addresses the issue reported by downstream users.

@llvmbot
Copy link
Member

llvmbot commented Dec 20, 2024

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libcxx

Author: Petr Hosek (petrhosek)

Changes

This mechanism is preferable in environments like embedded since it doesn't require special handling of the custom section.


Full diff: https://github.com/llvm/llvm-project/pull/120805.diff

3 Files Affected:

  • (modified) libcxx/src/include/overridable_function.h (+45-70)
  • (modified) libcxx/src/new.cpp (+10-12)
  • (modified) libcxxabi/src/stdlib_new_delete.cpp (+10-12)
diff --git a/libcxx/src/include/overridable_function.h b/libcxx/src/include/overridable_function.h
index 6c70f6242ddd63..7372e347831bb4 100644
--- a/libcxx/src/include/overridable_function.h
+++ b/libcxx/src/include/overridable_function.h
@@ -29,106 +29,81 @@
 // This is a low-level utility which does not work on all platforms, since it needs
 // to make assumptions about the object file format in use. Furthermore, it requires
 // the "base definition" of the function (the one we want to check whether it has been
-// overridden) to be annotated with the _LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE macro.
+// overridden) to be defined using the _LIBCPP_OVERRIDABLE_FUNCTION macro.
 //
 // This currently works with Mach-O files (used on Darwin) and with ELF files (used on Linux
 // and others). On platforms where we know how to implement this detection, the macro
 // _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION is defined to 1, and it is defined to 0 on
-// other platforms. The _LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE macro is defined to
-// nothing on unsupported platforms so that it can be used to decorate functions regardless
-// of whether detection is actually supported.
+// other platforms. The _LIBCPP_OVERRIDABLE_FUNCTION macro expands to regular function
+// definition on unsupported platforms so that it can be used to decorate functions
+// regardless of whether detection is actually supported.
 //
 // How does this work?
 // -------------------
 //
 // Let's say we want to check whether a weak function `f` has been overridden by the user.
-// The general mechanism works by placing `f`'s definition (in the libc++ built library)
-// inside a special section, which we do using the `__section__` attribute via the
-// _LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE macro.
+// The general mechanism works by defining a symbol `f_impl__` and a weak alias `f` via the
+// _LIBCPP_OVERRIDABLE_FUNCTION macro.
 //
 // Then, when comes the time to check whether the function has been overridden, we take
-// the address of the function and we check whether it falls inside the special function
-// we created. This can be done by finding pointers to the start and the end of the section
-// (which is done differently for ELF and Mach-O), and then checking whether `f` falls
-// within those bounds. If it falls within those bounds, then `f` is still inside the
-// special section and so it is the version we defined in the libc++ built library, i.e.
-// it was not overridden. Otherwise, it was overridden by the user because it falls
-// outside of the section.
+// the address of the function `f` and we check whether it is different from `f_impl__`.
+// If so it means the function was overriden by the user.
 //
 // Important note
 // --------------
 //
-// This mechanism should never be used outside of the libc++ built library. In particular,
-// attempting to use this within the libc++ headers will not work at all because we don't
-// want to be defining special sections inside user's executables which use our headers.
+// This mechanism should never be used outside of the libc++ built library. Functions defined
+// with this macro must be defined at global scope.
 //
 
 #if defined(_LIBCPP_OBJECT_FORMAT_MACHO)
 
-#  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
-#  define _LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE                                                                 \
-    __attribute__((__section__("__TEXT,__lcxx_override,regular,pure_instructions")))
-
 _LIBCPP_BEGIN_NAMESPACE_STD
-template <class _Ret, class... _Args>
-_LIBCPP_HIDE_FROM_ABI bool __is_function_overridden(_Ret (*__fptr)(_Args...)) noexcept {
-  // Declare two dummy bytes and give them these special `__asm` values. These values are
-  // defined by the linker, which means that referring to `&__lcxx_override_start` will
-  // effectively refer to the address where the section starts (and same for the end).
-  extern char __lcxx_override_start __asm("section$start$__TEXT$__lcxx_override");
-  extern char __lcxx_override_end __asm("section$end$__TEXT$__lcxx_override");
-
-  // Now get a uintptr_t out of these locations, and out of the function pointer.
-  uintptr_t __start = reinterpret_cast<uintptr_t>(&__lcxx_override_start);
-  uintptr_t __end   = reinterpret_cast<uintptr_t>(&__lcxx_override_end);
-  uintptr_t __ptr   = reinterpret_cast<uintptr_t>(__fptr);
-
-#  if __has_feature(ptrauth_calls)
-  // We must pass a void* to ptrauth_strip since it only accepts a pointer type. Also, in particular,
-  // we must NOT pass a function pointer, otherwise we will strip the function pointer, and then attempt
-  // to authenticate and re-sign it when casting it to a uintptr_t again, which will fail because we just
-  // stripped the function pointer. See rdar://122927845.
-  __ptr = reinterpret_cast<uintptr_t>(ptrauth_strip(reinterpret_cast<void*>(__ptr), ptrauth_key_function_pointer));
-#  endif
-
-  // Finally, the function was overridden if it falls outside of the section's bounds.
-  return __ptr < __start || __ptr > __end;
-}
-_LIBCPP_END_NAMESPACE_STD
 
-// The NVPTX linker cannot create '__start/__stop' sections.
-#elif defined(_LIBCPP_OBJECT_FORMAT_ELF) && !defined(__NVPTX__)
+template <auto _Func>
+_LIBCPP_HIDE_FROM_ABI constexpr bool __is_function_overridden();
 
-#  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
-#  define _LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE __attribute__((__section__("__lcxx_override")))
+_LIBCPP_END_NAMESPACE_STD
 
-// This is very similar to what we do for Mach-O above. The ELF linker will implicitly define
-// variables with those names corresponding to the start and the end of the section.
-//
-// See https://stackoverflow.com/questions/16552710/how-do-you-get-the-start-and-end-addresses-of-a-custom-elf-section
-extern char __start___lcxx_override;
-extern char __stop___lcxx_override;
+#  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
+#  define _LIBCPP_OVERRIDABLE_FUNCTION(symbol, type, name, arglist)                                                    \
+    static __attribute__((used)) type symbol##_impl__ arglist __asm__("_" _LIBCPP_TOSTRING(symbol));                   \
+    __asm__(".globl _" _LIBCPP_TOSTRING(symbol));                                                                      \
+    __asm__(".weak_definition _" _LIBCPP_TOSTRING(symbol));                                                            \
+    extern __typeof(symbol##_impl__) name __attribute__((weak_import));                                                \
+    _LIBCPP_BEGIN_NAMESPACE_STD                                                                                        \
+    template <>                                                                                                        \
+    inline bool __is_function_overridden<static_cast<type(*) arglist>(name)>() {                                       \
+      return static_cast<type(*) arglist>(name) != symbol##_impl__;                                                    \
+    }                                                                                                                  \
+    _LIBCPP_END_NAMESPACE_STD                                                                                          \
+    static type symbol##_impl__ arglist
+
+#elif defined(_LIBCPP_OBJECT_FORMAT_ELF)
 
 _LIBCPP_BEGIN_NAMESPACE_STD
-template <class _Ret, class... _Args>
-_LIBCPP_HIDE_FROM_ABI bool __is_function_overridden(_Ret (*__fptr)(_Args...)) noexcept {
-  uintptr_t __start = reinterpret_cast<uintptr_t>(&__start___lcxx_override);
-  uintptr_t __end   = reinterpret_cast<uintptr_t>(&__stop___lcxx_override);
-  uintptr_t __ptr   = reinterpret_cast<uintptr_t>(__fptr);
-
-#  if __has_feature(ptrauth_calls)
-  // We must pass a void* to ptrauth_strip since it only accepts a pointer type. See full explanation above.
-  __ptr = reinterpret_cast<uintptr_t>(ptrauth_strip(reinterpret_cast<void*>(__ptr), ptrauth_key_function_pointer));
-#  endif
-
-  return __ptr < __start || __ptr > __end;
-}
+
+template <auto _Func>
+_LIBCPP_HIDE_FROM_ABI constexpr bool __is_function_overridden();
+
 _LIBCPP_END_NAMESPACE_STD
 
+#  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
+#  define _LIBCPP_OVERRIDABLE_FUNCTION(symbol, type, name, arglist)                                                    \
+    static type symbol##_impl__ arglist __asm__(_LIBCPP_TOSTRING(symbol##_impl__));                                    \
+    [[gnu::weak, gnu::alias(_LIBCPP_TOSTRING(symbol##_impl__))]] type name arglist;                                    \
+    _LIBCPP_BEGIN_NAMESPACE_STD                                                                                        \
+    template <>                                                                                                        \
+    inline bool __is_function_overridden<static_cast<type(*) arglist>(name)>() {                                       \
+      return static_cast<type(*) arglist>(name) != symbol##_impl__;                                                    \
+    }                                                                                                                  \
+    _LIBCPP_END_NAMESPACE_STD                                                                                          \
+    static type symbol##_impl__ arglist
+
 #else
 
 #  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 0
-#  define _LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE /* nothing */
+#  define _LIBCPP_OVERRIDABLE_FUNCTION(symbol, type, name, arglist) _LIBCPP_WEAK type name arglist
 
 #endif
 
diff --git a/libcxx/src/new.cpp b/libcxx/src/new.cpp
index e010fe4c4f1912..b14b52248df332 100644
--- a/libcxx/src/new.cpp
+++ b/libcxx/src/new.cpp
@@ -43,7 +43,7 @@ static void* operator_new_impl(std::size_t size) {
   return p;
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void* operator new(std::size_t size) _THROW_BAD_ALLOC {
+_LIBCPP_OVERRIDABLE_FUNCTION(_Znwm, void*, operator new, (std::size_t size)) _THROW_BAD_ALLOC {
   void* p = operator_new_impl(size);
   if (p == nullptr)
     __throw_bad_alloc_shim();
@@ -54,7 +54,7 @@ _LIBCPP_WEAK void* operator new(size_t size, const std::nothrow_t&) noexcept {
 #  if !_LIBCPP_HAS_EXCEPTIONS
 #    if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t)>(&operator new)),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t)>(&operator new)>(),
       "libc++ was configured with exceptions disabled and `operator new(size_t)` has been overridden, "
       "but `operator new(size_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new(size_t, nothrow_t)` must call `operator new(size_t)`, which will terminate in case "
@@ -74,7 +74,7 @@ _LIBCPP_WEAK void* operator new(size_t size, const std::nothrow_t&) noexcept {
 #  endif
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void* operator new[](size_t size) _THROW_BAD_ALLOC {
+_LIBCPP_OVERRIDABLE_FUNCTION(_Znam, void*, operator new[], (size_t size)) _THROW_BAD_ALLOC {
   return ::operator new(size);
 }
 
@@ -82,7 +82,7 @@ _LIBCPP_WEAK void* operator new[](size_t size, const std::nothrow_t&) noexcept {
 #  if !_LIBCPP_HAS_EXCEPTIONS
 #    if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t)>(&operator new[])),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t)>(&operator new[])>(),
       "libc++ was configured with exceptions disabled and `operator new[](size_t)` has been overridden, "
       "but `operator new[](size_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new[](size_t, nothrow_t)` must call `operator new[](size_t)`, which will terminate in case "
@@ -136,8 +136,8 @@ static void* operator_new_aligned_impl(std::size_t size, std::align_val_t alignm
   return p;
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void*
-operator new(std::size_t size, std::align_val_t alignment) _THROW_BAD_ALLOC {
+_LIBCPP_OVERRIDABLE_FUNCTION(_ZnwmSt11align_val_t, void*, operator new, (std::size_t size, std::align_val_t alignment))
+_THROW_BAD_ALLOC {
   void* p = operator_new_aligned_impl(size, alignment);
   if (p == nullptr)
     __throw_bad_alloc_shim();
@@ -148,7 +148,7 @@ _LIBCPP_WEAK void* operator new(size_t size, std::align_val_t alignment, const s
 #    if !_LIBCPP_HAS_EXCEPTIONS
 #      if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new)),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new)>(),
       "libc++ was configured with exceptions disabled and `operator new(size_t, align_val_t)` has been overridden, "
       "but `operator new(size_t, align_val_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new(size_t, align_val_t, nothrow_t)` must call `operator new(size_t, align_val_t)`, which will "
@@ -168,16 +168,14 @@ _LIBCPP_WEAK void* operator new(size_t size, std::align_val_t alignment, const s
 #    endif
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void*
-operator new[](size_t size, std::align_val_t alignment) _THROW_BAD_ALLOC {
-  return ::operator new(size, alignment);
-}
+_LIBCPP_OVERRIDABLE_FUNCTION(_ZnamSt11align_val_t, void*, operator new[], (size_t size, std::align_val_t alignment))
+_THROW_BAD_ALLOC { return ::operator new(size, alignment); }
 
 _LIBCPP_WEAK void* operator new[](size_t size, std::align_val_t alignment, const std::nothrow_t&) noexcept {
 #    if !_LIBCPP_HAS_EXCEPTIONS
 #      if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new[])),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new[])>(),
       "libc++ was configured with exceptions disabled and `operator new[](size_t, align_val_t)` has been overridden, "
       "but `operator new[](size_t, align_val_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new[](size_t, align_val_t, nothrow_t)` must call `operator new[](size_t, align_val_t)`, which will "
diff --git a/libcxxabi/src/stdlib_new_delete.cpp b/libcxxabi/src/stdlib_new_delete.cpp
index f386b28f0cfe64..73798e211c3134 100644
--- a/libcxxabi/src/stdlib_new_delete.cpp
+++ b/libcxxabi/src/stdlib_new_delete.cpp
@@ -63,7 +63,7 @@ static void* operator_new_impl(std::size_t size) {
   return p;
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void* operator new(std::size_t size) _THROW_BAD_ALLOC {
+_LIBCPP_OVERRIDABLE_FUNCTION(_Znwm, void*, operator new, (std::size_t size)) _THROW_BAD_ALLOC {
   void* p = operator_new_impl(size);
   if (p == nullptr)
     __throw_bad_alloc_shim();
@@ -74,7 +74,7 @@ _LIBCPP_WEAK void* operator new(size_t size, const std::nothrow_t&) noexcept {
 #if !_LIBCPP_HAS_EXCEPTIONS
 #  if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t)>(&operator new)),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t)>(&operator new)>(),
       "libc++ was configured with exceptions disabled and `operator new(size_t)` has been overridden, "
       "but `operator new(size_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new(size_t, nothrow_t)` must call `operator new(size_t)`, which will terminate in case "
@@ -94,7 +94,7 @@ _LIBCPP_WEAK void* operator new(size_t size, const std::nothrow_t&) noexcept {
 #endif
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void* operator new[](size_t size) _THROW_BAD_ALLOC {
+_LIBCPP_OVERRIDABLE_FUNCTION(_Znam, void*, operator new[], (size_t size)) _THROW_BAD_ALLOC {
   return ::operator new(size);
 }
 
@@ -102,7 +102,7 @@ _LIBCPP_WEAK void* operator new[](size_t size, const std::nothrow_t&) noexcept {
 #if !_LIBCPP_HAS_EXCEPTIONS
 #  if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t)>(&operator new[])),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t)>(&operator new[])>(),
       "libc++ was configured with exceptions disabled and `operator new[](size_t)` has been overridden, "
       "but `operator new[](size_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new[](size_t, nothrow_t)` must call `operator new[](size_t)`, which will terminate in case "
@@ -156,8 +156,8 @@ static void* operator_new_aligned_impl(std::size_t size, std::align_val_t alignm
   return p;
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void*
-operator new(std::size_t size, std::align_val_t alignment) _THROW_BAD_ALLOC {
+_LIBCPP_OVERRIDABLE_FUNCTION(_ZnwmSt11align_val_t, void*, operator new, (std::size_t size, std::align_val_t alignment))
+_THROW_BAD_ALLOC {
   void* p = operator_new_aligned_impl(size, alignment);
   if (p == nullptr)
     __throw_bad_alloc_shim();
@@ -168,7 +168,7 @@ _LIBCPP_WEAK void* operator new(size_t size, std::align_val_t alignment, const s
 #  if !_LIBCPP_HAS_EXCEPTIONS
 #    if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new)),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new)>(),
       "libc++ was configured with exceptions disabled and `operator new(size_t, align_val_t)` has been overridden, "
       "but `operator new(size_t, align_val_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new(size_t, align_val_t, nothrow_t)` must call `operator new(size_t, align_val_t)`, which will "
@@ -188,16 +188,14 @@ _LIBCPP_WEAK void* operator new(size_t size, std::align_val_t alignment, const s
 #  endif
 }
 
-_LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE _LIBCPP_WEAK void*
-operator new[](size_t size, std::align_val_t alignment) _THROW_BAD_ALLOC {
-  return ::operator new(size, alignment);
-}
+_LIBCPP_OVERRIDABLE_FUNCTION(_ZnamSt11align_val_t, void*, operator new[], (size_t size, std::align_val_t alignment))
+_THROW_BAD_ALLOC { return ::operator new(size, alignment); }
 
 _LIBCPP_WEAK void* operator new[](size_t size, std::align_val_t alignment, const std::nothrow_t&) noexcept {
 #  if !_LIBCPP_HAS_EXCEPTIONS
 #    if _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION
   _LIBCPP_ASSERT_SHIM(
-      !std::__is_function_overridden(static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new[])),
+      !std::__is_function_overridden<static_cast<void* (*)(std::size_t, std::align_val_t)>(&operator new[])>(),
       "libc++ was configured with exceptions disabled and `operator new[](size_t, align_val_t)` has been overridden, "
       "but `operator new[](size_t, align_val_t, nothrow_t)` has not been overridden. This is problematic because "
       "`operator new[](size_t, align_val_t, nothrow_t)` must call `operator new[](size_t, align_val_t)`, which will "

@ldionne
Copy link
Member

ldionne commented Jan 7, 2025

Can you please describe how this PR is different from #114961? Looking at the diff, it doesn't jump at me. Preferably explain what's different in the PR description.

@petrhosek
Copy link
Member Author

Can you please describe how this PR is different from #114961? Looking at the diff, it doesn't jump at me. Preferably explain what's different in the PR description.

Done, I included it in the PR description.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

Ah, I see what went wrong with the previous attempt now. Thanks, LGTM.

@ldionne ldionne merged commit 8418955 into llvm:main Jan 7, 2025
8 checks passed
_LIBCPP_END_NAMESPACE_STD

# define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
# define _LIBCPP_OVERRIDABLE_FUNCTION(symbol, type, name, arglist) \
static type symbol##_impl__ arglist __asm__(_LIBCPP_TOSTRING(symbol##_impl__)); \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this string concatenation going to break demangling? This looks like it's going to create the string _Znw..._impl__, which won't demangle correctly. These symbols show up a lot in crash stack traces, so I would prefer a solution that actually doesn't change the symbol used by operator new stack frames at all. That's going to disrupt a lot of profile information, which would be a problem for Google server applications.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for raising this, I haven't haven't considered that scenario. I think we should be able to use the same name for the internal symbol.

Copy link
Member Author

Choose a reason for hiding this comment

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

#122983 should address this issue.

petrhosek added a commit that referenced this pull request Jan 25, 2025
petrhosek added a commit that referenced this pull request Jan 28, 2025
Reverts #120805

This change while desirable has two issues we discovered:

- It is incompatible with `-funique-internal-linkage-names`, see
#120805 (comment)
- It is incompatible with `-fvisibility-global-new-delete=force-hidden`,
see
#123224 (comment)

We were hoping to address both of these issues with
#122983, but that change has
other issues we haven't yet managed to resolve. For now, we have decided
to revert the change to avoid shipping a broken feature in LLVM 20, and
we plan to follow up with a new approach post branch.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 28, 2025
…n" (#124431)

Reverts llvm/llvm-project#120805

This change while desirable has two issues we discovered:

- It is incompatible with `-funique-internal-linkage-names`, see
llvm/llvm-project#120805 (comment)
- It is incompatible with `-fvisibility-global-new-delete=force-hidden`,
see
llvm/llvm-project#123224 (comment)

We were hoping to address both of these issues with
llvm/llvm-project#122983, but that change has
other issues we haven't yet managed to resolve. For now, we have decided
to revert the change to avoid shipping a broken feature in LLVM 20, and
we plan to follow up with a new approach post branch.
oneseer pushed a commit to oneseer/libcxxabi that referenced this pull request Mar 26, 2025
Reverts llvm/llvm-project#120805

This change while desirable has two issues we discovered:

- It is incompatible with `-funique-internal-linkage-names`, see
llvm/llvm-project#120805 (comment)
- It is incompatible with `-fvisibility-global-new-delete=force-hidden`,
see
llvm/llvm-project#123224 (comment)

We were hoping to address both of these issues with
llvm/llvm-project#122983, but that change has
other issues we haven't yet managed to resolve. For now, we have decided
to revert the change to avoid shipping a broken feature in LLVM 20, and
we plan to follow up with a new approach post branch.

NOKEYCHECK=True
GitOrigin-RevId: 4167ea2cb082a2acb00b8b1dc09aa780dc0e3110
petrhosek added a commit to petrhosek/llvm-project that referenced this pull request Apr 1, 2025
We plan to replace the existing mechanism for overriding detection with
one that doesn't require the use of a special section as an alternative
to llvm#120805 which had other downsides.

This change is a pure refactoring that lays the foundation for a
subsequent change that will introduce the new detection mechanism.
petrhosek added a commit that referenced this pull request May 9, 2025
…ion (#133876)

We plan to replace the existing mechanism for overriding detection with
one that doesn't require the use of a special section as an alternative
to #120805 which had other downsides.

This change is a pure refactoring that lays the foundation for a
subsequent change that will introduce the new detection mechanism.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 9, 2025
…ding detection (#133876)

We plan to replace the existing mechanism for overriding detection with
one that doesn't require the use of a special section as an alternative
to llvm/llvm-project#120805 which had other downsides.

This change is a pure refactoring that lays the foundation for a
subsequent change that will introduce the new detection mechanism.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++. libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants