[libc++] Remove HIDE_FROM_ABI from virtual functions

[ldionne]

Summary

_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.

[ldionne]

@philnik Do you think writing a clang-tidy check for this would be hard?

[philnik]

In D140453#4009813, @ldionne wrote:

@philnik Do you think writing a clang-tidy check for this would be hard?

match
cxxMethodDecl(isVirtual(),
              hasAttr("attr::AbiTag"),
              unless(isExpansionInSystemHeader())
             )

should do the trick (using clang-query for now). Looking at the CI failures I'm not sure just removing the _LIBCPP_HIDE_FROM_ABI is the right call here. Maybe we need something like _LIBCPP_HIDE_VIRTUAL_FUNCTION_FROM_ABI that gives the functions internal linkage. That didn't suffer from this problem, right? (And also maybe mention the problem in the _LIBCPP_HIDE_FROM_ABI explanation in <__config>.)

[ldionne]

Add test, comments and _LIBCPP_HIDDEN.

[ldionne]

In D140453#4010549, @philnik wrote:

match
cxxMethodDecl(isVirtual(),
              hasAttr("attr::AbiTag"),
              unless(isExpansionInSystemHeader())
             )

should do the trick (using clang-query for now). Looking at the CI failures I'm not sure just removing the _LIBCPP_HIDE_FROM_ABI is the right call here. Maybe we need something like _LIBCPP_HIDE_VIRTUAL_FUNCTION_FROM_ABI that gives the functions internal linkage. That didn't suffer from this problem, right? (And also maybe mention the problem in the _LIBCPP_HIDE_FROM_ABI explanation in <__config>.)

Thanks a lot, this is magic. It actually found many more instances of the same problem! This tool is really growing on me.

[Mordante]

I also feel clang-query is really useful. This change really convinces me to spend some time with it too.
LGTM!

libcxx/test/libcxx/clang_query/abi_tag_on_virtual.query

23 ↗

(On Diff #484580)

Nice comment!

[ldionne]

Add _LIBCPP_HIDE_FROM_ABI_VIRTUAL

libcxx/include/__functional/function.h

 
 template<class _Rp, class ..._ArgTypes>
 class __base<_Rp(_ArgTypes...)>
 {
     __base(const __base&);
     __base& operator=(const __base&);
 public:
     _LIBCPP_INLINE_VISIBILITY __base() {}
-    _LIBCPP_INLINE_VISIBILITY virtual ~__base() {}
+    virtual ~__base() {}
     virtual __base* __clone() const = 0;
     virtual void __clone(__base*) const = 0;
     virtual void destroy() _NOEXCEPT = 0;
     virtual void destroy_deallocate() _NOEXCEPT = 0;
     virtual _Rp operator()(_ArgTypes&& ...) = 0;
 #ifndef _LIBCPP_NO_RTTI
     virtual const void* target(const type_info&) const _NOEXCEPT = 0;
     virtual const std::type_info& target_type() const _NOEXCEPT = 0;

libcxx/include/future

 template<class _Rp, class ..._ArgTypes>
 class _LIBCPP_AVAILABILITY_FUTURE __packaged_task_base<_Rp(_ArgTypes...)>
 {
     __packaged_task_base(const __packaged_task_base&);
     __packaged_task_base& operator=(const __packaged_task_base&);
 public:
     _LIBCPP_INLINE_VISIBILITY
     __packaged_task_base() {}
-    _LIBCPP_INLINE_VISIBILITY
     virtual ~__packaged_task_base() {}
     virtual void __move_to(__packaged_task_base*) _NOEXCEPT = 0;
     virtual void destroy() = 0;
     virtual void destroy_deallocate() = 0;
     virtual _Rp operator()(_ArgTypes&& ...) = 0;
 };
 
 template<class _FD, class _Alloc, class _FB> class __packaged_task_func;

libcxx/include/locale

iter_type put(iter_type __s, ios_base& __iob, char_type __fl,
                   const tm* __tm, char __fmt, char __mod = 0) const
     {
         return do_put(__s, __iob, __fl, __tm, __fmt, __mod);
     }
 
     static locale::id id;
 
 protected:
-    _LIBCPP_HIDE_FROM_ABI ~time_put() override {}
+    ~time_put() override {}
     virtual iter_type do_put(iter_type __s, ios_base&, char_type, const tm* __tm,
                              char __fmt, char __mod) const;
 
     _LIBCPP_INLINE_VISIBILITY
     explicit time_put(const char* __nm, size_t __refs)
         : locale::facet(__refs),
           __time_put(__nm) {}
     _LIBCPP_INLINE_VISIBILITY

libcxx/include/regex

 {
     __node(const __node&);
     __node& operator=(const __node&);
 public:
     typedef _VSTD::__state<_CharT> __state;
 
     _LIBCPP_INLINE_VISIBILITY
     __node() {}
-    _LIBCPP_INLINE_VISIBILITY
     virtual ~__node() {}
 
-    _LIBCPP_INLINE_VISIBILITY
     virtual void __exec(__state&) const {}
-    _LIBCPP_INLINE_VISIBILITY
     virtual void __exec_split(bool, __state&) const {}
 };
 
 // __end_state
 
 template <class _CharT>
 class __end_state
     : public __node<_CharT>