[libc++] Remove HIDE_FROM_ABI from virtual functions

_LIBCPP_HIDE_FROM_ABI (which is what _LIBCPP_INLINE_VISIBILITY is) uses
ABI tags to avoid ODR violations when linking together object files
compiled against different versions of libc++. However, pointer
authentication uses the mangled name of the function to sign the
function pointer in the vtable, which means that the ABI tag effectively
changes how the pointers are signed.

This leads to PAC failures when passing an object that holds one of these
pointers in its vtable across an ABI boundary: one side will sign the
pointer using one function mangling (with one ABI tag), and the other
side will authenticate the pointer expecting it to have a different
mangled name, which won't work.

To make sure this does not regress in the future, this patch also adds
a clang-query test to detect incorrect applications of _LIBCPP_HIDE_FROM_ABI.

Differential Revision: https://reviews.llvm.org/D140453

diff --git a/libcxx/include/__config b/libcxx/include/__config
index 9f7aadd..1a519b9 100644 (file)
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -610,6 +610,15 @@ typedef __char32_t char32_t;
 // Note that we use _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION to ensure that we don't depend
 // on _LIBCPP_HIDE_FROM_ABI methods of classes explicitly instantiated in the dynamic library.
 //
+// Also note that the _LIBCPP_HIDE_FROM_ABI_VIRTUAL macro should be used on virtual functions
+// instead of _LIBCPP_HIDE_FROM_ABI. That macro does not use an ABI tag. Indeed, the mangled
+// name of a virtual function is part of its ABI, since some architectures like arm64e can sign
+// the virtual function pointer in the vtable based on the mangled name of the function. Since
+// we use an ABI tag that changes with each released version, the mangled name of the virtual
+// function would change, which is incorrect. Note that it doesn't make much sense to change
+// the implementation of a virtual function in an ABI-incompatible way in the first place,
+// since that would be an ABI break anyway. Hence, the lack of ABI tag should not be noticeable.
+//
 // TODO: We provide a escape hatch with _LIBCPP_NO_ABI_TAG for folks who want to avoid increasing
 //       the length of symbols with an ABI tag. In practice, we should remove the escape hatch and
 //       use compression mangling instead, see https://github.com/itanium-cxx-abi/cxx-abi/issues/70.
@@ -620,6 +629,7 @@ typedef __char32_t char32_t;
 #  else
 #    define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION
 #  endif
+#    define _LIBCPP_HIDE_FROM_ABI_VIRTUAL _LIBCPP_HIDDEN _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION
 
 #  ifdef _LIBCPP_BUILDING_LIBRARY
 #    if _LIBCPP_ABI_VERSION > 1

diff --git a/libcxx/include/__filesystem/filesystem_error.h b/libcxx/include/__filesystem/filesystem_error.h
index eb5c38a..effe699 100644 (file)
--- a/libcxx/include/__filesystem/filesystem_error.h
+++ b/libcxx/include/__filesystem/filesystem_error.h
@@ -61,7 +61,7 @@ public:
   filesystem_error(const filesystem_error&) = default;
   ~filesystem_error() override; // key function
 
-  _LIBCPP_INLINE_VISIBILITY
+  _LIBCPP_HIDE_FROM_ABI_VIRTUAL
   const char* what() const noexcept override {
     return __storage_->__what_.c_str();
   }

diff --git a/libcxx/include/__functional/function.h b/libcxx/include/__functional/function.h
index 436149e..8f34d01 100644 (file)
--- a/libcxx/include/__functional/function.h
+++ b/libcxx/include/__functional/function.h
@@ -262,7 +262,7 @@ class __base<_Rp(_ArgTypes...)>
     __base& operator=(const __base&);
 public:
     _LIBCPP_INLINE_VISIBILITY __base() {}
-    _LIBCPP_INLINE_VISIBILITY virtual ~__base() {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL virtual ~__base() {}
     virtual __base* __clone() const = 0;
     virtual void __clone(__base*) const = 0;
     virtual void destroy() _NOEXCEPT = 0;

diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index e200a95..e598ad2 100644 (file)
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -991,7 +991,7 @@ struct __unbounded_array_control_block<_Tp[], _Alloc> : __shared_weak_count
         return (__bytes + __align - 1) & ~(__align - 1);
     }
 
-    _LIBCPP_HIDE_FROM_ABI
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL
     ~__unbounded_array_control_block() override { } // can't be `= default` because of the sometimes-non-trivial union member __data_
 
 private:
@@ -1058,7 +1058,7 @@ struct __bounded_array_control_block<_Tp[_Count], _Alloc>
         std::__uninitialized_allocator_value_construct_n(__alloc_, std::addressof(__data_[0]), _Count);
     }
 
-    _LIBCPP_HIDE_FROM_ABI
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL
     ~__bounded_array_control_block() override { } // can't be `= default` because of the sometimes-non-trivial union member __data_
 
 private:

diff --git a/libcxx/include/future b/libcxx/include/future
index a4747cd..dcacd7a 100644 (file)
--- a/libcxx/include/future
+++ b/libcxx/include/future
@@ -1626,7 +1626,7 @@ class _LIBCPP_AVAILABILITY_FUTURE __packaged_task_base<_Rp(_ArgTypes...)>
 public:
     _LIBCPP_INLINE_VISIBILITY
     __packaged_task_base() {}
-    _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL
     virtual ~__packaged_task_base() {}
     virtual void __move_to(__packaged_task_base*) _NOEXCEPT = 0;
     virtual void destroy() = 0;

diff --git a/libcxx/include/locale b/libcxx/include/locale
index f322a11..874866f 100644 (file)
--- a/libcxx/include/locale
+++ b/libcxx/include/locale
@@ -680,7 +680,7 @@ public:
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~num_get() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~num_get() override {}
 
     template <class _Fp>
     _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
@@ -1350,7 +1350,7 @@ public:
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~num_put() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~num_put() override {}
 
     virtual iter_type do_put(iter_type __s, ios_base& __iob, char_type __fl,
                              bool __v) const;
@@ -1795,7 +1795,7 @@ public:
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~time_get() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_get() override {}
 
     virtual dateorder do_date_order() const;
     virtual iter_type do_get_time(iter_type __b, iter_type __e, ios_base& __iob,
@@ -2414,25 +2414,17 @@ public:
           __time_get_storage<_CharT>(__nm) {}
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~time_get_byname() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_get_byname() override {}
 
-    _LIBCPP_INLINE_VISIBILITY
-     dateorder do_date_order() const override {return this->__do_date_order();}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL dateorder do_date_order() const override {return this->__do_date_order();}
 private:
-    _LIBCPP_INLINE_VISIBILITY
-     const string_type* __weeks() const override  {return this->__weeks_;}
-    _LIBCPP_INLINE_VISIBILITY
-     const string_type* __months() const override {return this->__months_;}
-    _LIBCPP_INLINE_VISIBILITY
-     const string_type* __am_pm() const override  {return this->__am_pm_;}
-    _LIBCPP_INLINE_VISIBILITY
-     const string_type& __c() const override      {return this->__c_;}
-    _LIBCPP_INLINE_VISIBILITY
-     const string_type& __r() const override      {return this->__r_;}
-    _LIBCPP_INLINE_VISIBILITY
-     const string_type& __x() const override      {return this->__x_;}
-    _LIBCPP_INLINE_VISIBILITY
-     const string_type& __X() const override      {return this->__X_;}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type* __weeks() const override  {return this->__weeks_;}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type* __months() const override {return this->__months_;}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type* __am_pm() const override  {return this->__am_pm_;}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __c() const override      {return this->__c_;}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __r() const override      {return this->__r_;}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __x() const override      {return this->__x_;}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL const string_type& __X() const override      {return this->__X_;}
 };
 
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS time_get_byname<char>;
@@ -2482,7 +2474,7 @@ public:
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~time_put() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_put() override {}
     virtual iter_type do_put(iter_type __s, ios_base&, char_type, const tm* __tm,
                              char __fmt, char __mod) const;
 
@@ -2570,7 +2562,7 @@ public:
         : time_put<_CharT, _OutputIterator>(__nm, __refs) {}
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~time_put_byname() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~time_put_byname() override {}
 };
 
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS time_put_byname<char>;
@@ -2618,7 +2610,7 @@ public:
     static const bool intl = _International;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~moneypunct() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~moneypunct() override {}
 
     virtual char_type   do_decimal_point() const {return numeric_limits<char_type>::max();}
     virtual char_type   do_thousands_sep() const {return numeric_limits<char_type>::max();}
@@ -2668,7 +2660,7 @@ public:
         : moneypunct<_CharT, _International>(__refs) {init(__nm.c_str());}
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~moneypunct_byname() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~moneypunct_byname() override {}
 
      char_type   do_decimal_point() const override {return __decimal_point_;}
      char_type   do_thousands_sep() const override {return __thousands_sep_;}
@@ -2796,7 +2788,7 @@ public:
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~money_get() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~money_get() override {}
 
     virtual iter_type do_get(iter_type __b, iter_type __e, bool __intl,
                              ios_base& __iob, ios_base::iostate& __err,
@@ -3340,7 +3332,7 @@ public:
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~money_put() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~money_put() override {}
 
     virtual iter_type do_put(iter_type __s, bool __intl, ios_base& __iob,
                              char_type __fl, long double __units) const;
@@ -3508,7 +3500,7 @@ public:
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~messages() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~messages() override {}
 
     virtual catalog do_open(const basic_string<char>&, const locale&) const;
     virtual string_type do_get(catalog, int __set, int __msgid,
@@ -3597,7 +3589,7 @@ public:
         : messages<_CharT>(__refs) {}
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~messages_byname() override {}
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~messages_byname() override {}
 };
 
 extern template class _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS messages_byname<char>;

diff --git a/libcxx/include/regex b/libcxx/include/regex
index 365a4c9..06c017f 100644 (file)
--- a/libcxx/include/regex
+++ b/libcxx/include/regex
@@ -1446,12 +1446,12 @@ public:
 
     _LIBCPP_INLINE_VISIBILITY
     __node() {}
-    _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL
     virtual ~__node() {}
 
-    _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL
     virtual void __exec(__state&) const {}
-    _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_HIDE_FROM_ABI_VIRTUAL
     virtual void __exec_split(bool, __state&) const {}
 };
 

diff --git a/libcxx/test/libcxx/clang_query.sh.cpp b/libcxx/test/libcxx/clang_query.sh.cpp
index 7044459..d59d6a2 100644 (file)
--- a/libcxx/test/libcxx/clang_query.sh.cpp
+++ b/libcxx/test/libcxx/clang_query.sh.cpp
@@ -17,6 +17,10 @@
 // RUN: cat %t.output
 // RUN: cat %t.output | wc -l | grep -Fxq 1
 
+// RUN: clang-query -f %S/clang_query/abi_tag_on_virtual.query %s --use-color -- -Wno-unknown-warning-option %{compile_flags} -fno-modules > %t.output
+// RUN: cat %t.output
+// RUN: cat %t.output | wc -l | grep -Fxq 1
+
 // Prevent <ext/hash_map> from generating deprecated warnings for this test.
 #if defined(__DEPRECATED)
 #    undef __DEPRECATED

diff --git a/libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query b/libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query
new file mode 100644 (file)
index 0000000..ffa465a
--- /dev/null
+++ b/libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query
@@ -0,0 +1,29 @@
+#===------------------------------------------------------------------------===#
+#
+# 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
+#
+#===------------------------------------------------------------------------===#
+
+# This clang-query test ensures that we don't place an abi_tag attribute on
+# virtual functions. This can happen by mistakenly applying a macro like
+# _LIBCPP_HIDE_FROM_ABI on a virtual function.
+#
+# The problem is that arm64e pointer authentication extensions use the mangled
+# name of the function to sign the function pointer in the vtable, which means
+# that the ABI tag effectively influences how the pointers are signed.
+#
+# This can lead to PAC failures when passing an object that holds one of these
+# pointers in its vtable across an ABI boundary if the two sides have been compiled
+# with different versions of libc++: one side will sign the pointer using one function
+# mangling (with one ABI tag), and the other side will authenticate the pointer expecting
+# it to have a different mangled name due to the ABI tag being different, which will crash.
+#
+# This test ensures that we don't re-introduce this issue in the code base.
+
+match
+cxxMethodDecl(isVirtual(),
+              hasAttr("attr::AbiTag"),
+              unless(isExpansionInSystemHeader())
+             )