[libc++] Avoid ODR violations in __exception_guard

[philnik]

Summary

Having an ODR violation with __exception_guard seems to be problematic in LTO builds. To avoid the ODR violation, give the class different names for exception/no-exceptions mode and have an alias to the correct class.

[rupprecht]

libcxx/include/__config

446–447 ↗

(On Diff #493940)

Is there a reason this is conditionally defined? The value of LIBCPP_EXCEPTION_ABI_TAG is only used when _LIBCPP_NO_EXCEPTIONS is enabled, so it would be possible to unconditionally set it to this.

[philnik]

libcxx/include/__config

446–447 ↗

(On Diff #493940)

This makes it possible to use the tag for other cases where we have ODR violations.

[philnik]

Try to fix CI

[alexfh]

libcxx/include/__utility/exception_guard.h

61

Should this be moved inside the definition of the class to make it clear which code is shared between the _LIBCPP_NO_EXCEPTIONS and !_LIBCPP_NO_EXCEPTIONS modes?

[ldionne]

Thanks for looking into this. I agree with fixing this ASAP, however in the longer term we should simply encode the ABI-affecting properties of the library into the ABI tag. For example, whether the debug mode is enabled or not, whether exceptions are enabled, etc. Then that would apply library-wide. In order for that to work, we'd also need to drop always_inline and probably exclude_from_explicit_instantiation from those macros, which is a WIP.

To avoid making it look like this is the final solution, I would avoid introducing a macro in __config and I would simply use two differently-named classes.

LGTM w/ comments, also this needs to be cherry-picked onto release/16.x.

libcxx/include/__utility/exception_guard.h

62–63

Instead, let's define two classes, one is __exception_guard_exceptions and the other one is __exception_guard_noexceptions. Then we can have an alias to the right one.

libcxx/test/libcxx/utilities/exception_guard.odr.sh.cpp

10

12–13

Makes it easier to see what's going on.

[philnik]

Address comments

[dblaikie]

libcxx/include/__utility/exception_guard.h

62–63

If they're going to have separate names with an alias - do they need to be/is it better that they not be preprocessor conditional, and only the definition of the alias would need the preprocessor conditionals? Or is it worth keeping the extra code out of the parsing/worth the extra preprocessor conditionality to do so?

126–130

I guess the other changes in the patch to use __make_exception_guard are because CTAD can't be used through the alias? If that's true, maybe remove the CTAD support macros here - since it won't be used portably between exception/non-exception code anyway?

[philnik]

libcxx/include/__utility/exception_guard.h

62–63

I left them in the conditional to avoid accidentally depending on one of them directly. This should be caught in code review, but letting the compiler complain makes it more obvious.

126–130

I expect us to revert part of this once Clang supports CTAD with aliases, so I'd just keep them since they will be added again later.

[alexfh]

The implementation with two different class names seems much cleaner. Thanks!

[philnik]

Try to fix CI

[philnik]

Next try

[philnik]

Next try

[ldionne]

libcxx/include/__utility/exception_guard.h

62–63

FWIW I would have kept only the alias in #ifdef to keep the amount of conditional code as small as possible.

[smeenai]

I'm still seeing LTO errors after this patch:

fragment covers entire variable
  call void @llvm.dbg.value(metadata i64 poison, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %5, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
fragment covers entire variable
  call void @llvm.dbg.value(metadata i64 poison, metadata !798697, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !798698
!798697 = !DILocalVariable(name: "__last", arg: 3, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %7, metadata !798697, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798697 = !DILocalVariable(name: "__last", arg: 3, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %9, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
fragment is larger than or outside of variable
  call void @llvm.dbg.value(metadata ptr %12, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698
!798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997)
LLVM ERROR: Broken module found, compilation aborted!

I went to the libc++ source state right before D133661 and was able to build fine, and then I checked out that change and applied this one directly on top (versus just checking out this revision, to ensure there were no other changes) and repro'd, so it's definitely caused by these patches. There were a bunch of other errors that this patch did fix, but not these.

Unfortunately I'm not sure how to create a standalone reproducer (this is part of a giant internal build that I can't share as-is). If I use --save-temps during LTO and then use llvm-dis to convert to textual IR to try reducing it, it just ignores the invalid debug info, which defeats the whole purpose. Does anyone have suggestions for that?

I also requested a backport to 16.0.1 in https://github.com/llvm/llvm-project/issues/61276, since it's late in the LLVM 16 release cycle now.

(Also apologies for raising this a month later; I was on an extended leave and am only getting around to evaluating libc++ 16 now.)

[smeenai]

Okay, -print-before-all did the trick, and the local variables it's complaining about are from https://github.com/llvm/llvm-project/blob/08b65c5c9e58a4f9e79a52a0fccd70e4c4ebd43b/libcxx/include/__memory/uninitialized_algorithms.h#L520. I guess we have some ODR issue with the specific types that template is being instantiated with (reverse_iterator<reverse_iterator<internal_type*>>) that's being exposed now?

EDIT: Yup, this was entirely our fault. We were LTOing a TU built with C++14 and a TU built with C++17 together, and we use the unstable ABI configuration, so iterators ended up being different sizes in the TUs. Sorry for the noise.

libcxx/include/__expected/expected.h

if constexpr (is_nothrow_constructible_v<_T1, _Args...>) {
       std::construct_at(std::addressof(__newval), std::move(__tmp));
     } else {
       static_assert(
           is_nothrow_move_constructible_v<_T2>,
           "To provide strong exception guarantee, T2 has to satisfy `is_nothrow_move_constructible_v` so that it can "
           "be reverted to the previous state in case an exception is thrown during the assignment.");
       _T2 __tmp(std::move(__oldval));
       std::destroy_at(std::addressof(__oldval));
-      __exception_guard __trans([&] { std::construct_at(std::addressof(__oldval), std::move(__tmp)); });
+      auto __trans =
+          std::__make_exception_guard([&] { std::construct_at(std::addressof(__oldval), std::move(__tmp)); });
       std::construct_at(std::addressof(__newval), std::forward<_Args>(__args)...);
       __trans.__complete();
     }
   }
 
 public:
   // [expected.object.assign], assignment
   _LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(const expected&) = delete;
requires(is_swappable_v<_Tp> &&
              is_move_constructible_v<_Err> &&
              (is_nothrow_move_constructible_v<_Tp> ||
               is_nothrow_move_constructible_v<_Err>))
   {
     auto __swap_val_unex_impl = [&](expected& __with_val, expected& __with_err) {
       if constexpr (is_nothrow_move_constructible_v<_Err>) {
         _Err __tmp(std::move(__with_err.__union_.__unex_));
         std::destroy_at(std::addressof(__with_err.__union_.__unex_));
-        __exception_guard __trans([&] {
+        auto __trans = std::__make_exception_guard([&] {
           std::construct_at(std::addressof(__with_err.__union_.__unex_), std::move(__tmp));
         });
         std::construct_at(std::addressof(__with_err.__union_.__val_), std::move(__with_val.__union_.__val_));
         __trans.__complete();
         std::destroy_at(std::addressof(__with_val.__union_.__val_));
         std::construct_at(std::addressof(__with_val.__union_.__unex_), std::move(__tmp));
       } else {
         static_assert(is_nothrow_move_constructible_v<_Tp>,
                       "To provide strong exception guarantee, Tp has to satisfy `is_nothrow_move_constructible_v` so "
                       "that it can be reverted to the previous state in case an exception is thrown during swap.");
         _Tp __tmp(std::move(__with_val.__union_.__val_));
         std::destroy_at(std::addressof(__with_val.__union_.__val_));
-        __exception_guard __trans([&] {
+        auto __trans = std::__make_exception_guard([&] {
           std::construct_at(std::addressof(__with_val.__union_.__val_), std::move(__tmp));
         });
         std::construct_at(std::addressof(__with_val.__union_.__unex_), std::move(__with_err.__union_.__unex_));
         __trans.__complete();
         std::destroy_at(std::addressof(__with_err.__union_.__unex_));
         std::construct_at(std::addressof(__with_err.__union_.__val_), std::move(__tmp));
       }
       __with_val.__has_val_ = false;

libcxx/include/__memory/uninitialized_algorithms.h

constexpr void __allocator_construct_at_multidimensional(_Alloc& __alloc, _Tp* __loc) {
 
     if constexpr (is_array_v<_Tp>) {
         using _Element = remove_extent_t<_Tp>;
         __allocator_traits_rebind_t<_Alloc, _Element> __elem_alloc(__alloc);
         size_t __i = 0;
         _Tp& __array = *__loc;
 
         // If an exception is thrown, destroy what we have constructed so far in reverse order.
-        __exception_guard __guard([&]() {
+        auto __guard = std::__make_exception_guard([&]() {
           std::__allocator_destroy_multidimensional(__elem_alloc, __array, __array + __i);
         });
 
         for (; __i != extent_v<_Tp>; ++__i) {
             std::__allocator_construct_at_multidimensional(__elem_alloc, std::addressof(__array[__i]));
         }
         __guard.__complete();
     } else {
if constexpr (is_array_v<_Tp>) {
             "trying to construct an array.");
 
         using _Element = remove_extent_t<_Tp>;
         __allocator_traits_rebind_t<_Alloc, _Element> __elem_alloc(__alloc);
         size_t __i = 0;
         _Tp& __array = *__loc;
 
         // If an exception is thrown, destroy what we have constructed so far in reverse order.
-        __exception_guard __guard([&]() {
+        auto __guard = std::__make_exception_guard([&]() {
           std::__allocator_destroy_multidimensional(__elem_alloc, __array, __array + __i);
         });
         for (; __i != extent_v<_Tp>; ++__i) {
             std::__allocator_construct_at_multidimensional(__elem_alloc, std::addressof(__array[__i]), __arg[__i]);
         }
         __guard.__complete();
     } else {
         allocator_traits<_Alloc>::construct(__alloc, __loc, __arg);

 template<class _Alloc, class _BidirIter, class _Tp, class _Size = typename iterator_traits<_BidirIter>::difference_type>
 _LIBCPP_HIDE_FROM_ABI constexpr void
 __uninitialized_allocator_fill_n_multidimensional(_Alloc& __alloc, _BidirIter __it, _Size __n, _Tp const& __value) {
     using _ValueType = typename iterator_traits<_BidirIter>::value_type;
     __allocator_traits_rebind_t<_Alloc, _ValueType> __value_alloc(__alloc);
     _BidirIter __begin = __it;
 
     // If an exception is thrown, destroy what we have constructed so far in reverse order.
-    __exception_guard __guard([&]() { std::__allocator_destroy_multidimensional(__value_alloc, __begin, __it); });
+    auto __guard = std::__make_exception_guard([&]() { std::__allocator_destroy_multidimensional(__value_alloc, __begin, __it); });
     for (; __n != 0; --__n, ++__it) {
         std::__allocator_construct_at_multidimensional(__value_alloc, std::addressof(*__it), __value);
     }
     __guard.__complete();
 }
 
 // Same as __uninitialized_allocator_fill_n_multidimensional, but doesn't pass any initialization argument
 // to the allocator's construct method, which results in value initialization.
 template <class _Alloc, class _BidirIter, class _Size = typename iterator_traits<_BidirIter>::difference_type>
 _LIBCPP_HIDE_FROM_ABI constexpr void
 __uninitialized_allocator_value_construct_n_multidimensional(_Alloc& __alloc, _BidirIter __it, _Size __n) {
     using _ValueType = typename iterator_traits<_BidirIter>::value_type;
     __allocator_traits_rebind_t<_Alloc, _ValueType> __value_alloc(__alloc);
     _BidirIter __begin = __it;
 
     // If an exception is thrown, destroy what we have constructed so far in reverse order.
-    __exception_guard __guard([&]() { std::__allocator_destroy_multidimensional(__value_alloc, __begin, __it); });
+    auto __guard = std::__make_exception_guard([&]() { std::__allocator_destroy_multidimensional(__value_alloc, __begin, __it); });
     for (; __n != 0; --__n, ++__it) {
         std::__allocator_construct_at_multidimensional(__value_alloc, std::addressof(*__it));
     }
     __guard.__complete();
 }
 
 #endif // _LIBCPP_STD_VER > 14
 

libcxx/include/__memory_resource/polymorphic_allocator.h

#  if _LIBCPP_STD_VER >= 20
   template <class _Type>
   void deallocate_object(_Type* __ptr, size_t __n = 1) {
     deallocate_bytes(__ptr, __n * sizeof(_Type), alignof(_Type));
   }
 
   template <class _Type, class... _CtorArgs>
   [[nodiscard]] _Type* new_object(_CtorArgs&&... __ctor_args) {
     _Type* __ptr = allocate_object<_Type>();
-    __exception_guard __guard([&] { deallocate_object(__ptr); });
+    auto __guard = std::__make_exception_guard([&] { deallocate_object(__ptr); });
     construct(__ptr, std::forward<_CtorArgs>(__ctor_args)...);
     __guard.__complete();
     return __ptr;
   }
 
   template <class _Type>
   void delete_object(_Type* __ptr) {
     destroy(__ptr);

libcxx/include/__utility/exception_guard.h

 //        for (; n > 0; ++iter, ++out, --n) {
 //            ::new ((void*)std::addressof(*out)) value_type(*iter);
 //        }
 //        guard.__complete();
 //        return out;
 //    }
 //
 
 #ifndef _LIBCPP_NO_EXCEPTIONS
 template <class _Rollback>
-struct __exception_guard {
-  __exception_guard() = delete;
+struct __exception_guard_exceptions {
+  __exception_guard_exceptions() = delete;
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit __exception_guard(_Rollback __rollback)
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit __exception_guard_exceptions(_Rollback __rollback)
       : __rollback_(std::move(__rollback)), __completed_(false) {}
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 __exception_guard(__exception_guard&& __other)
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
+  __exception_guard_exceptions(__exception_guard_exceptions&& __other)
       _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value)
       : __rollback_(std::move(__other.__rollback_)), __completed_(__other.__completed_) {
     __other.__completed_ = true;
   }
 
-  __exception_guard(__exception_guard const&)            = delete;
-  __exception_guard& operator=(__exception_guard const&) = delete;
-  __exception_guard& operator=(__exception_guard&&)      = delete;
+  __exception_guard_exceptions(__exception_guard_exceptions const&)            = delete;
+  __exception_guard_exceptions& operator=(__exception_guard_exceptions const&) = delete;
+  __exception_guard_exceptions& operator=(__exception_guard_exceptions&&)      = delete;
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __complete() _NOEXCEPT { __completed_ = true; }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 ~__exception_guard() {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 ~__exception_guard_exceptions() {
     if (!__completed_)
       __rollback_();
   }
 
 private:
   _Rollback __rollback_;
   bool __completed_;
 };
+
+_LIBCPP_CTAD_SUPPORTED_FOR_TYPE(__exception_guard_exceptions);
+
+template <class _Rollback>
+using __exception_guard = __exception_guard_exceptions<_Rollback>;
 #else  // _LIBCPP_NO_EXCEPTIONS
 template <class _Rollback>
-struct __exception_guard {
-  __exception_guard() = delete;
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG explicit __exception_guard(_Rollback) {}
+struct __exception_guard_noexceptions {
+  __exception_guard_noexceptions() = delete;
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
+      _LIBCPP_NODEBUG explicit __exception_guard_noexceptions(_Rollback) {}
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG __exception_guard(__exception_guard&& __other)
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG
+  __exception_guard_noexceptions(__exception_guard_noexceptions&& __other)
       _NOEXCEPT_(is_nothrow_move_constructible<_Rollback>::value)
       : __completed_(__other.__completed_) {
     __other.__completed_ = true;
   }
 
-  __exception_guard(__exception_guard const&)            = delete;
-  __exception_guard& operator=(__exception_guard const&) = delete;
-  __exception_guard& operator=(__exception_guard&&)      = delete;
+  __exception_guard_noexceptions(__exception_guard_noexceptions const&)            = delete;
+  __exception_guard_noexceptions& operator=(__exception_guard_noexceptions const&) = delete;
+  __exception_guard_noexceptions& operator=(__exception_guard_noexceptions&&)      = delete;
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG void __complete() _NOEXCEPT {
     __completed_ = true;
   }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG ~__exception_guard() {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG ~__exception_guard_noexceptions() {
     _LIBCPP_ASSERT(__completed_, "__exception_guard not completed with exceptions disabled");
   }
 
 private:
   bool __completed_ = false;
 };
-#endif // _LIBCPP_NO_EXCEPTIONS
 
-_LIBCPP_CTAD_SUPPORTED_FOR_TYPE(__exception_guard);
+_LIBCPP_CTAD_SUPPORTED_FOR_TYPE(__exception_guard_noexceptions);
+
+template <class _Rollback>
+using __exception_guard = __exception_guard_noexceptions<_Rollback>;
+#endif // _LIBCPP_NO_EXCEPTIONS
 
 template <class _Rollback>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __exception_guard<_Rollback> __make_exception_guard(_Rollback __rollback) {
   return __exception_guard<_Rollback>(std::move(__rollback));
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___UTILITY_TRANSACTION_H

libcxx/test/libcxx/utilities/exception_guard.odr.sh.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Make sure that we don't get ODR violations with __exception_guard when
+// linking together TUs compiled with different values of -f[no-]exceptions.
+
+// RUN: %{cxx} %s %{flags} %{compile_flags} -c -o %t.except.o   -O1 -Wno-private-header -fexceptions
+// RUN: %{cxx} %s %{flags} %{compile_flags} -c -o %t.noexcept.o -O1 -Wno-private-header -fno-exceptions
+// RUN: %{cxx} %{flags} %{link_flags} -o %t.exe %t.except.o %t.noexcept.o
+// RUN: %{run}
+
+#include <__utility/exception_guard.h>
+#include <cassert>
+#include <cstring>
+#include <typeinfo>
+
+struct Rollback {
+  void operator()() {}
+};
+
+#if defined(__cpp_exceptions) && __cpp_exceptions >= 199711L
+
+const char* func();
+
+int main(int, char**) {
+  assert(std::strcmp(typeid(std::__exception_guard<Rollback>).name(), func()) != 0);
+
+  return 0;
+}
+
+#else
+
+const char* func() { return typeid(std::__exception_guard<Rollback>).name(); }
+
+#endif