diff --git a/libcxx/include/__config b/libcxx/include/__config --- a/libcxx/include/__config +++ b/libcxx/include/__config @@ -610,6 +610,15 @@ // 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 @@ # 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 --- a/libcxx/include/__filesystem/filesystem_error.h +++ b/libcxx/include/__filesystem/filesystem_error.h @@ -61,7 +61,7 @@ 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 --- a/libcxx/include/__functional/function.h +++ b/libcxx/include/__functional/function.h @@ -262,7 +262,7 @@ __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 --- a/libcxx/include/__memory/shared_ptr.h +++ b/libcxx/include/__memory/shared_ptr.h @@ -991,7 +991,7 @@ 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 @@ 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 --- a/libcxx/include/future +++ b/libcxx/include/future @@ -1626,7 +1626,7 @@ 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 --- a/libcxx/include/locale +++ b/libcxx/include/locale @@ -680,7 +680,7 @@ static locale::id id; protected: - _LIBCPP_HIDE_FROM_ABI ~num_get() override {} + _LIBCPP_HIDE_FROM_ABI_VIRTUAL ~num_get() override {} template _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS @@ -1350,7 +1350,7 @@ 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 @@ 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 @@ __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; @@ -2482,7 +2474,7 @@ 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 @@ : 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; @@ -2618,7 +2610,7 @@ 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::max();} virtual char_type do_thousands_sep() const {return numeric_limits::max();} @@ -2668,7 +2660,7 @@ : 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 @@ 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 @@ 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 @@ 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&, const locale&) const; virtual string_type do_get(catalog, int __set, int __msgid, @@ -3597,7 +3589,7 @@ : 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; diff --git a/libcxx/include/regex b/libcxx/include/regex --- a/libcxx/include/regex +++ b/libcxx/include/regex @@ -1446,12 +1446,12 @@ _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 --- 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 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 --- /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()) + )