[libcxx] Use alias for detecting overriden function #114961

petrhosek · 2024-11-05T09:43:57Z

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

llvmbot · 2024-11-05T09:44:35Z

@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/114961.diff

2 Files Affected:

(modified) libcxx/src/include/overridable_function.h (+48-67)
(modified) libcxx/src/new.cpp (+10-10)

diff --git a/libcxx/src/include/overridable_function.h b/libcxx/src/include/overridable_function.h
index 6c70f6242ddd63..09487d84df8b50 100644
--- a/libcxx/src/include/overridable_function.h
+++ b/libcxx/src/include/overridable_function.h
@@ -29,7 +29,7 @@
 // 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
@@ -42,93 +42,74 @@
 // -------------------
 //
 // 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.
 //
 
 #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 <typename _Func>
+constexpr _Func* __overload_of(_Func* f) { return f; }
 
-#  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
-#  define _LIBCPP_MAKE_OVERRIDABLE_FUNCTION_DETECTABLE __attribute__((__section__("__lcxx_override")))
+template <auto _Func>
+constexpr bool __is_function_overridden();
 
-// 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;
+_LIBCPP_END_NAMESPACE_STD
+
+#  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
+#  define _LIBCPP_OVERRIDABLE_FUNCTION(symbol, type, name, arglist) \
+  extern "C" type symbol##_impl__ arglist; \
+  __asm__(".globl _" _LIBCPP_TOSTRING(symbol)); \
+  __asm__(".set _" _LIBCPP_TOSTRING(symbol) ", _" _LIBCPP_TOSTRING(symbol##_impl__)); \
+  extern __typeof(symbol##_impl__) name __attribute__((weak_import)); \
+  _LIBCPP_BEGIN_NAMESPACE_STD \
+  template <> \
+  constexpr bool __is_function_overridden<__overload_of<type arglist>(name)>() { \
+    return __overload_of<type arglist>(name) != symbol##_impl__; \
+  } \
+  _LIBCPP_END_NAMESPACE_STD \
+  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 <typename _Func>
+constexpr _Func* __overload_of(_Func* f) { return f; }
+
+template <auto _Func>
+constexpr bool __is_function_overridden();
+
 _LIBCPP_END_NAMESPACE_STD
 
+#  define _LIBCPP_CAN_DETECT_OVERRIDDEN_FUNCTION 1
+#  define _LIBCPP_OVERRIDABLE_FUNCTION(symbol, type, name, arglist) \
+  extern "C" type symbol##_impl__ arglist; \
+  [[gnu::weak, gnu::alias(_LIBCPP_TOSTRING(symbol##_impl__))]] type name arglist; \
+  _LIBCPP_BEGIN_NAMESPACE_STD \
+  template <> \
+  constexpr bool __is_function_overridden<__overload_of<type arglist>(name)>() { \
+    return __overload_of<type arglist>(name) != symbol##_impl__; \
+  } \
+  _LIBCPP_END_NAMESPACE_STD \
+  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 b0c731678cac30..3e99aed455c4b5 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(_Znmw, 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 {
 #  ifdef _LIBCPP_HAS_NO_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<std::__overload_of<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 {
 #  ifdef _LIBCPP_HAS_NO_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<std::__overload_of<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,9 @@ 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_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 +149,7 @@ _LIBCPP_WEAK void* operator new(size_t size, std::align_val_t alignment, const s
 #    ifdef _LIBCPP_HAS_NO_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<std::__overload_of<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,8 +169,7 @@ _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 {
+_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);
 }
 
@@ -177,7 +177,7 @@ _LIBCPP_WEAK void* operator new[](size_t size, std::align_val_t alignment, const
 #    ifdef _LIBCPP_HAS_NO_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<std::__overload_of<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 "

github-actions · 2024-11-05T09:48:37Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ldionne

This looks like a reasonable approach to me, and I think I like that better than adding a way to disable the option altogether.

I'm curious to know what @EricWF thinks about this as well.

libcxx/src/include/overridable_function.h

ldionne · 2024-11-05T20:32:12Z

libcxx/src/new.cpp

@@ -54,7 +54,7 @@ _LIBCPP_WEAK void* operator new(size_t size, const std::nothrow_t&) noexcept {
 #  ifdef _LIBCPP_HAS_NO_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<std::__overload_of<void* (std::size_t)>(&operator new)>(),


What's the benefit of using __overload_of here instead of just static_cast?

Clang doesn't like static_cast:

error: address of overloaded function 'operator new' cannot be static_cast to type 'void *(size_t)' (aka 'void *(unsigned long)') ... error: address of overloaded function 'operator new[]' cannot be static_cast to type 'void *(size_t)' (aka 'void *(unsigned long)')

I was able to remove __overload_of this way:

You need to use static_cast<Return (*)(Args...)>(name) instead of static_cast<Return(Args...)>(name). You're casting to a function-pointer type, not to a function type.

You need to make __is_function_overridden non-constexpr, otherwise Clang will like this, which makes a lot of sense since this comparison should always be done at runtime:

[...]/libcxxabi/src/stdlib_new_delete.cpp:66:1: error: constexpr function never produces a constant expression [-Winvalid-constexpr] 66 | _LIBCPP_OVERRIDABLE_FUNCTION(_Znwm, void*, operator new, (std::size_t size)) _THROW_BAD_ALLOC { | ^ [...]/libcxxabi/../libcxx/src/include/overridable_function.h:76:20: note: expanded from macro '_LIBCPP_OVERRIDABLE_FUNCTION' 76 | constexpr bool __is_function_overridden<static_cast<type(*) arglist>(name)>() { \ | ^ [...]/libcxxabi/src/stdlib_new_delete.cpp:66:1: note: comparison against address of weak declaration '&operator new' can only be performed at runtime [...]/libcxxabi/../libcxx/src/include/overridable_function.h:77:49: note: expanded from macro '_LIBCPP_OVERRIDABLE_FUNCTION' 77 | return static_cast<type(*) arglist>(name) != symbol##_impl__; \ | ^

Overall, the changes for Mach-O look like this:

diff --git a/libcxx/src/include/overridable_function.h b/libcxx/src/include/overridable_function.h index 0ce9f9c1cee5..b392a099ab36 100644 --- a/libcxx/src/include/overridable_function.h +++ b/libcxx/src/include/overridable_function.h @@ -60,13 +60,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD -template <class _Func> -_LIBCPP_HIDE_FROM_ABI constexpr _Func* __overload_of(_Func* f) { - return f; -} - template <auto _Func> -_LIBCPP_HIDE_FROM_ABI constexpr bool __is_function_overridden(); +_LIBCPP_HIDE_FROM_ABI bool __is_function_overridden(); _LIBCPP_END_NAMESPACE_STD @@ -78,8 +73,8 @@ _LIBCPP_END_NAMESPACE_STD extern __typeof(symbol##_impl__) name __attribute__((weak_import)); \ _LIBCPP_BEGIN_NAMESPACE_STD \ template <> \ - constexpr bool __is_function_overridden<__overload_of<type arglist>(name)>() { \ - return __overload_of<type arglist>(name) != symbol##_impl__; \ + bool __is_function_overridden<static_cast<type(*) arglist>(name)>() { \ + return static_cast<type(*) arglist>(name) != symbol##_impl__; \ } \ _LIBCPP_END_NAMESPACE_STD \ type symbol##_impl__ arglist

And you can also replace mentions of __overload_of in the .cpp files by appropriate static_casts.

Thanks, I made this change and Clang seems to be happy with it.

libcxx/src/new.cpp

ldionne · 2024-11-06T13:36:17Z

libcxx/src/include/overridable_function.h

+      return __overload_of<type arglist>(name) != symbol##_impl__;                                                     \
+    }                                                                                                                  \
+    _LIBCPP_END_NAMESPACE_STD                                                                                          \
+    type symbol##_impl__ arglist


Can the _impl symbol be made static? After all it doesn't need to be externally visible, right? Same goes for the ELF version.

I had to use __asm__(_LIBCPP_TOSTRING(symbol##_impl__)) instead of extern "C" but other than that it seems to be working.

ldionne · 2024-11-06T13:45:50Z

libcxx/src/new.cpp

@@ -54,7 +54,7 @@ _LIBCPP_WEAK void* operator new(size_t size, const std::nothrow_t&) noexcept {
 #  ifdef _LIBCPP_HAS_NO_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<std::__overload_of<void* (std::size_t)>(&operator new)>(),


I was able to remove __overload_of this way:

You need to use static_cast<Return (*)(Args...)>(name) instead of static_cast<Return(Args...)>(name). You're casting to a function-pointer type, not to a function type.

You need to make __is_function_overridden non-constexpr, otherwise Clang will like this, which makes a lot of sense since this comparison should always be done at runtime:

[...]/libcxxabi/src/stdlib_new_delete.cpp:66:1: error: constexpr function never produces a constant expression [-Winvalid-constexpr] 66 | _LIBCPP_OVERRIDABLE_FUNCTION(_Znwm, void*, operator new, (std::size_t size)) _THROW_BAD_ALLOC { | ^ [...]/libcxxabi/../libcxx/src/include/overridable_function.h:76:20: note: expanded from macro '_LIBCPP_OVERRIDABLE_FUNCTION' 76 | constexpr bool __is_function_overridden<static_cast<type(*) arglist>(name)>() { \ | ^ [...]/libcxxabi/src/stdlib_new_delete.cpp:66:1: note: comparison against address of weak declaration '&operator new' can only be performed at runtime [...]/libcxxabi/../libcxx/src/include/overridable_function.h:77:49: note: expanded from macro '_LIBCPP_OVERRIDABLE_FUNCTION' 77 | return static_cast<type(*) arglist>(name) != symbol##_impl__; \ | ^

Overall, the changes for Mach-O look like this:

diff --git a/libcxx/src/include/overridable_function.h b/libcxx/src/include/overridable_function.h index 0ce9f9c1cee5..b392a099ab36 100644 --- a/libcxx/src/include/overridable_function.h +++ b/libcxx/src/include/overridable_function.h @@ -60,13 +60,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD -template <class _Func> -_LIBCPP_HIDE_FROM_ABI constexpr _Func* __overload_of(_Func* f) { - return f; -} - template <auto _Func> -_LIBCPP_HIDE_FROM_ABI constexpr bool __is_function_overridden(); +_LIBCPP_HIDE_FROM_ABI bool __is_function_overridden(); _LIBCPP_END_NAMESPACE_STD @@ -78,8 +73,8 @@ _LIBCPP_END_NAMESPACE_STD extern __typeof(symbol##_impl__) name __attribute__((weak_import)); \ _LIBCPP_BEGIN_NAMESPACE_STD \ template <> \ - constexpr bool __is_function_overridden<__overload_of<type arglist>(name)>() { \ - return __overload_of<type arglist>(name) != symbol##_impl__; \ + bool __is_function_overridden<static_cast<type(*) arglist>(name)>() { \ + return static_cast<type(*) arglist>(name) != symbol##_impl__; \ } \ _LIBCPP_END_NAMESPACE_STD \ type symbol##_impl__ arglist

And you can also replace mentions of __overload_of in the .cpp files by appropriate static_casts.

ldionne

This LGTM assuming CI passes. I'd still like @EricWF to have a quick pass at this.

petrhosek · 2024-11-25T19:25:14Z

@EricWF Do you have any further comments? I'd like to merge this change soon.

ldionne

LGTM pending CI.

ldionne · 2024-11-28T14:04:57Z

libcxx/src/include/overridable_function.h

-extern char __stop___lcxx_override;
+#  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__));                                    \


I tried locally and I think this should fix your macOS CI issues:

Suggested change

static type symbol##_impl__ arglist __asm__(_LIBCPP_TOSTRING(symbol##_impl__)); \

static type symbol##_impl__ arglist __asm__("_" _LIBCPP_TOSTRING(symbol##_impl__)); \

…tion-detection

nico · 2024-12-19T20:14:17Z

Heads-up: This probably broke building with LTO on macOS: https://crbug.com/385154007

I'll try to make a repro.

nico · 2024-12-19T20:52:53Z

It repros trivially if you build libc++ with -flto=thin.

Here's a script that makes repro pretty easy (zipped because github won't let me attach it otherwise).

standalone-repro.sh.zip

chmod +x it, put it in the root of your llvm checkout, tweak GENFILES_DIR to point at where a generated __config_site and __assertion_handler are, and run it. It should succeed. Then add -flto=thin on line 93, and run it again. You'll get linker errors. After reverting this PR, it works again.

(You'll have to run this on a mac.)

petrhosek · 2024-12-19T20:53:14Z

Looks like this change broke several LTO builds, so I'd be fine reverting it while I'm investigating the issue.

This reverts commit 62bd10f.

This reverts commit 62bd10f. Breaks building with -flto=thin, see #114961 (comment)

nico · 2024-12-19T20:54:18Z

Thanks! Reverted in 8dfae0c for now.

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.

…n (#114961)" This reverts commit 62bd10f. Breaks building with -flto=thin, see llvm/llvm-project#114961 (comment)

This mechanism is preferable in environments like embedded since it doesn't require special handling of the custom section. This is a reland of llvm/llvm-project#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 reverts commit 62bd10f7d18ca6f544286767cae2c9026d493888. Breaks building with -flto=thin, see llvm/llvm-project#114961 (comment) NOKEYCHECK=True GitOrigin-RevId: 8dfae0c462e9558df77c83c97d89b4b83ed1baff

This mechanism is preferable in environments like embedded since it doesn't require special handling of the custom section. This is a reland of llvm/llvm-project#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. NOKEYCHECK=True GitOrigin-RevId: 841895543edcf98bd16027c6b85fe7c6419a4566

[libcxx] Use alias for detecting overriden function

02cffe2

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

petrhosek requested a review from ldionne November 5, 2024 09:43

petrhosek requested a review from a team as a code owner November 5, 2024 09:43

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 5, 2024

petrhosek mentioned this pull request Nov 5, 2024

[libcxx] Option to disable overridden function detection #108734

Closed

ldionne reviewed Nov 5, 2024

View reviewed changes

petrhosek added 2 commits November 5, 2024 22:07

Review feedback

87c8627

Fix formatting

d7add33

ldionne reviewed Nov 6, 2024

View reviewed changes

petrhosek added 2 commits November 6, 2024 11:56

Use static_cast

ab62cfd

Use static symbols

c2f4b04

petrhosek requested a review from a team as a code owner November 6, 2024 21:00

llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Nov 6, 2024

Remove constexpr

dea7e36

ldionne approved these changes Nov 11, 2024

View reviewed changes

Merge branch 'main' into libcxx-overriden-function-detection

b88d67c

ldionne approved these changes Nov 26, 2024

View reviewed changes

Fix formatting

08e55d9

ldionne reviewed Nov 28, 2024

View reviewed changes

petrhosek and others added 5 commits November 28, 2024 23:23

Fix for Darwin

199b73d

Fix weak symbol definition on Darwin

516d75c

Merge remote-tracking branch 'origin/main' into libcxx-overriden-func…

867dd0c

…tion-detection

Merge branch 'main' into libcxx-overriden-function-detection

d8ef6d0

Merge remote-tracking branch 'origin/main' into libcxx-overriden-func…

bf2f287

…tion-detection

petrhosek merged commit 62bd10f into llvm:main Dec 17, 2024
61 checks passed

petrhosek added a commit that referenced this pull request Dec 19, 2024

Revert "[libcxx] Use alias for detecting overriden function (#114961)"

c4d8d4d

This reverts commit 62bd10f.

petrhosek mentioned this pull request Dec 19, 2024

Revert "[libcxx] Use alias for detecting overriden function" #120635

Closed

nico added a commit that referenced this pull request Dec 19, 2024

Revert "[libcxx] Use alias for detecting overriden function (#114961)"

8dfae0c

This reverts commit 62bd10f. Breaks building with -flto=thin, see #114961 (comment)

petrhosek deleted the libcxx-overriden-function-detection branch December 20, 2024 22:57

petrhosek mentioned this pull request Dec 20, 2024

[libcxx] Use alias for detecting overriden function #120805

Merged

	static type symbol##_impl__ arglist __asm__(_LIBCPP_TOSTRING(symbol##_impl__)); \
	static type symbol##_impl__ arglist __asm__("_" _LIBCPP_TOSTRING(symbol##_impl__)); \

[libcxx] Use alias for detecting overriden function #114961

[libcxx] Use alias for detecting overriden function #114961

Uh oh!

Conversation

petrhosek commented Nov 5, 2024

Uh oh!

llvmbot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

petrhosek commented Nov 25, 2024

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nico commented Dec 19, 2024

Uh oh!

nico commented Dec 19, 2024

Uh oh!

petrhosek commented Dec 19, 2024

Uh oh!

nico commented Dec 19, 2024

Uh oh!

Uh oh!

llvmbot commented Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024 •

edited

Loading