[libcxx-commits] [libcxx] 5efc811 - [libc++] Remove HIDE_FROM_ABI from virtual functions
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Jan 9 07:29:47 PST 2023
Author: Louis Dionne
Date: 2023-01-09T10:29:42-05:00
New Revision: 5efc81166d869a94318134bbb1878b551503d115
URL: https://github.com/llvm/llvm-project/commit/5efc81166d869a94318134bbb1878b551503d115
DIFF: https://github.com/llvm/llvm-project/commit/5efc81166d869a94318134bbb1878b551503d115.diff
LOG: [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
Added:
libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query
Modified:
libcxx/include/__config
libcxx/include/__filesystem/filesystem_error.h
libcxx/include/__functional/function.h
libcxx/include/__memory/shared_ptr.h
libcxx/include/future
libcxx/include/locale
libcxx/include/regex
libcxx/test/libcxx/clang_query.sh.cpp
Removed:
################################################################################
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 9f7aaddc3ad92..1a519b9f44d1c 100644
--- 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 eb5c38ad12e12..effe6998338cb 100644
--- a/libcxx/include/__filesystem/filesystem_error.h
+++ b/libcxx/include/__filesystem/filesystem_error.h
@@ -61,7 +61,7 @@ class _LIBCPP_AVAILABILITY_FILESYSTEM _LIBCPP_EXCEPTION_ABI filesystem_error : p
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 436149e88ae37..8f34d0128bb62 100644
--- 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 e200a959fa295..e598ad2b8a308 100644
--- 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 a4747cdb33c21..dcacd7ad5374b 100644
--- 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 f322a1183f021..874866f698223 100644
--- 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 365a4c9ac2c8e..06c017fcce384 100644
--- 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 704445960f2c9..d59d6a2dfa2b9 100644
--- 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
index 0000000000000..ffa465aa61bee
--- /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
diff erent 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
diff erent mangled name due to the ABI tag being
diff erent, 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())
+ )
More information about the libcxx-commits
mailing list