[libc++][libc++abi] Minor follow-up changes after ptrauth upstreaming #87481

ldionne · 2024-04-03T12:15:57Z

This patch applies the comments provided on #84573. This is done as a separate PR to avoid merge conflicts with downstreams that already had ptrauth support.

libcxx/include/typeinfo

github-actions · 2024-04-03T12:19:16Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · 2024-04-03T16:17:10Z

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This patch applies the comments provided on #84573. This is done as a separate PR to avoid merge conflicts with downstreams that already had ptrauth support.

Full diff: https://github.com/llvm/llvm-project/pull/87481.diff

3 Files Affected:

(modified) libcxx/include/typeinfo (+1-1)
(modified) libcxx/src/include/overridable_function.h (+3-3)
(modified) libcxxabi/src/private_typeinfo.cpp (+8-15)

diff --git a/libcxx/include/typeinfo b/libcxx/include/typeinfo
index d1c0de3c1bfdd3..76320c33c692e7 100644
--- a/libcxx/include/typeinfo
+++ b/libcxx/include/typeinfo
@@ -275,7 +275,7 @@ struct __type_info_implementations {
           __impl;
 };
 
-#    if defined(__arm64__) && __has_cpp_attribute(clang::ptrauth_vtable_pointer)
+#    if __has_cpp_attribute(clang::ptrauth_vtable_pointer)
 #      if __has_feature(ptrauth_type_info_discriminated_vtable_pointer)
 #        define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH                                                                  \
           [[clang::ptrauth_vtable_pointer(process_independent, address_discrimination, type_discrimination)]]
diff --git a/libcxx/src/include/overridable_function.h b/libcxx/src/include/overridable_function.h
index fca66ea6daf7a8..2f590483a9023c 100644
--- a/libcxx/src/include/overridable_function.h
+++ b/libcxx/src/include/overridable_function.h
@@ -13,7 +13,7 @@
 #include <__config>
 #include <cstdint>
 
-#if defined(__arm64e__) && __has_feature(ptrauth_calls)
+#if __has_feature(ptrauth_calls)
 #  include <ptrauth.h>
 #endif
 
@@ -85,13 +85,13 @@ _LIBCPP_HIDE_FROM_ABI bool __is_function_overridden(_Ret (*__fptr)(_Args...)) no
   uintptr_t __end   = reinterpret_cast<uintptr_t>(&__lcxx_override_end);
   uintptr_t __ptr   = reinterpret_cast<uintptr_t>(__fptr);
 
-#if defined(__arm64e__) && __has_feature(ptrauth_calls)
+#  if __has_feature(ptrauth_calls)
   // We must pass a void* to ptrauth_strip since it only accepts a pointer type. Also, in particular,
   // we must NOT pass a function pointer, otherwise we will strip the function pointer, and then attempt
   // to authenticate and re-sign it when casting it to a uintptr_t again, which will fail because we just
   // stripped the function pointer. See rdar://122927845.
   __ptr = reinterpret_cast<uintptr_t>(ptrauth_strip(reinterpret_cast<void*>(__ptr), ptrauth_key_function_pointer));
-#endif
+#  endif
 
   // Finally, the function was overridden if it falls outside of the section's bounds.
   return __ptr < __start || __ptr > __end;
diff --git a/libcxxabi/src/private_typeinfo.cpp b/libcxxabi/src/private_typeinfo.cpp
index 9e58501a559342..2fbfa526afc0ec 100644
--- a/libcxxabi/src/private_typeinfo.cpp
+++ b/libcxxabi/src/private_typeinfo.cpp
@@ -55,15 +55,12 @@
 #include <ptrauth.h>
 #endif
 
-
-template<typename T>
-static inline
-T *
-get_vtable(T *vtable) {
+template <typename T>
+static inline T* strip_vtable(T* vtable) {
 #if __has_feature(ptrauth_calls)
-    vtable = ptrauth_strip(vtable, ptrauth_key_cxx_vtable_pointer);
+  vtable = ptrauth_strip(vtable, ptrauth_key_cxx_vtable_pointer);
 #endif
-    return vtable;
+  return vtable;
 }
 
 static inline
@@ -117,8 +114,7 @@ void dyn_cast_get_derived_info(derived_object_info* info, const void* static_ptr
         reinterpret_cast<const uint8_t*>(vtable) + offset_to_ti_proxy;
     info->dynamic_type = *(reinterpret_cast<const __class_type_info* const*>(ptr_to_ti_proxy));
 #else
-    void **vtable = *static_cast<void ** const *>(static_ptr);
-    vtable = get_vtable(vtable);
+    void **vtable = strip_vtable(*static_cast<void ** const *>(static_ptr));
     info->offset_to_derived = reinterpret_cast<ptrdiff_t>(vtable[-2]);
     info->dynamic_ptr = static_cast<const char*>(static_ptr) + info->offset_to_derived;
     info->dynamic_type = static_cast<const __class_type_info*>(vtable[-1]);
@@ -576,8 +572,7 @@ __base_class_type_info::has_unambiguous_public_base(__dynamic_cast_info* info,
        find the layout.  */
     offset_to_base = __offset_flags >> __offset_shift;
     if (is_virtual) {
-      const char* vtable = *static_cast<const char* const*>(adjustedPtr);
-      vtable = get_vtable(vtable);
+      const char* vtable = strip_vtable(*static_cast<const char* const*>(adjustedPtr));
       offset_to_base = update_offset_to_base(vtable, offset_to_base);
     }
   } else if (!is_virtual) {
@@ -1517,8 +1512,7 @@ __base_class_type_info::search_above_dst(__dynamic_cast_info* info,
     ptrdiff_t offset_to_base = __offset_flags >> __offset_shift;
     if (__offset_flags & __virtual_mask)
     {
-        const char* vtable = *static_cast<const char*const*>(current_ptr);
-        vtable = get_vtable(vtable);
+        const char* vtable = strip_vtable(*static_cast<const char*const*>(current_ptr));
         offset_to_base = update_offset_to_base(vtable, offset_to_base);
     }
     __base_type->search_above_dst(info, dst_ptr,
@@ -1538,8 +1532,7 @@ __base_class_type_info::search_below_dst(__dynamic_cast_info* info,
     ptrdiff_t offset_to_base = __offset_flags >> __offset_shift;
     if (__offset_flags & __virtual_mask)
     {
-        const char* vtable = *static_cast<const char*const*>(current_ptr);
-        vtable = get_vtable(vtable);
+        const char* vtable = strip_vtable(*static_cast<const char*const*>(current_ptr));
         offset_to_base = update_offset_to_base(vtable, offset_to_base);
     }
     __base_type->search_below_dst(info,

philnik777 · 2024-04-03T16:40:18Z

libcxx/include/typeinfo

@@ -275,7 +275,7 @@ struct __type_info_implementations {
          __impl;
 };

-#    if defined(__arm64__) && __has_cpp_attribute(clang::ptrauth_vtable_pointer)
+#    if __has_cpp_attribute(clang::ptrauth_vtable_pointer)


This should be _Uglified. Also, why is this not in __config?

Since this is only used for std::type_info, I feel it makes more sense to keep this macro localized to <typeinfo>. If we used it elsewhere then it would make more sense to lift it to __config. WDYT?

Regarding uglifying, the right spelling would be __has_cpp_attribute(_Clang::__ptrauth_vtable_pointer__) and we can also change e.g. __has_feature(__ptrauth_type_info_discriminated_vtable_pointer__), right?

ldionne · 2024-06-07T18:02:54Z

libcxx/include/typeinfo

+#    if __has_cpp_attribute(_Clang::__ptrauth_vtable_pointer__) && __has_feature(__ptrauth_calls__)
+#      if __has_feature(__ptrauth_vtable_address_discrimination__) ||                                                  \
+          __has_feature(__ptrauth_vtable_type_discrimination__)


@ahmedbougacha @asl @kovdan01

Do these conditions seem right to you? This is important to get right since it's ABI affecting.

A different signing schema is used for type info vtable pointers compared to other vtable pointers: see https://github.com/ahmedbougacha/llvm-project/blob/eng/arm64e-upstream-llvmorg/clang/include/clang/Basic/PointerAuthOptions.h#L246 (also see a comment in code for context). The reason is that in ItaniumRTTIBuilder::BuildVTablePointer, we only have mangled names of classes derived from std::type_info, and do not have full definitions with info about base class. So, we can't use type-dependent extra discrimination, since for vtable pointers, it relies on ability to look up base class definition. As for address discrimination for type_info-related vtable pointers, it could probably be supported on clang side, but it currently isn't.

Given that, in libcxx, previously, we unconditionally disabled type-based and address discrimination for type info vtable pointers. Here is the condition we used in downstream:

#if (defined(__arm64__) || defined(__aarch64__)) && \ __has_feature(ptrauth_calls) && \ __has_cpp_attribute(clang::ptrauth_vtable_pointer) #define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH \ [[clang::ptrauth_vtable_pointer(process_independent, no_address_discrimination, no_extra_discrimination)]] #else #define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH #endif

I see that now type-based and address discrimination are enabled in libcxx under certain conditions: see https://github.com/ahmedbougacha/llvm-project/blob/eng/arm64e-upstream-llvmorg/libcxx/include/typeinfo#L278

# if defined(__arm64__) && __has_cpp_attribute(clang::ptrauth_vtable_pointer) # if __has_feature(ptrauth_type_info_discriminated_vtable_pointer) # define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH \ [[clang::ptrauth_vtable_pointer(process_independent, address_discrimination, type_discrimination)]] # else # define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH \ [[clang::ptrauth_vtable_pointer(process_independent, no_address_discrimination, no_extra_discrimination)]] # endif # else # define _LIBCPP_TYPE_INFO_VTABLE_POINTER_AUTH # endif

@ahmedbougacha I was unable to find any information about ptrauth_type_info_discriminated_vtable_pointer preprocessor feature in your branch (except its usage in include/typeinfo in libcxx). I also do not see any changes related to additional discrimination in initialization of CXXTypeInfoVTablePointer field of class PointerAuthOptions. It looks like there are no related changes in ItaniumRTTIBuilder::BuildVTablePointer as well. Please let me know if non-zero discrimination is actually supported on clang side for type info vtable pointers?

@ahmedbougacha @asl @ldionne I think there are 3 questions we need to answer to construct a condition everyone likes.

Do we want to check target architecture? If so, both __arm64__ and __aarch64__ should be checked. I'd prefer to have this check - even though it's probably redundant since other ptrauth-related checks are true only on aarch64, IMHO for those who don't know all the context it might be useful to see the architecture name and know that "OK, it's aarch64-specific stuff".

Do we want to check ptrauth_calls preprocessor feature enabled? I'd prefer to have this check - it's probably also redundant since [[clang::ptrauth_vtable_pointer]] attribute should have no effect w/o ptrauth_calls preprocessor feature, but, again, more verbosity might be useful for those who reads the code.

Do we want to allow type-based and/or address discrimination for type info vtable pointers? If so, what's the condition for that? Check against __has_feature(ptrauth_type_info_discriminated_vtable_pointer) like in @ahmedbougacha 's branch, or smth else?

@ldionne @ahmedbougacha @asl
Update. My preference is the following:

Do not check arm64/aarch64. If in future we have ptrauth support on non-arm64, for example, via soft pointer auth, the conditions will remain the same. Since we don't have different logic for ptrauth+non-arm64, no need for additional check.

Do not check against ptrauth_calls. The [[clang::ptrauth_vtable_pointer]] attribute has no effect w/o ptrauth_calls enabled.

If we want to introduce __has_feature(ptrauth_type_info_discriminated_vtable_pointer), @ahmedbougacha please update your branch to include changes allowing such discrimination - now there is no support on clang side.

Yes, we shouldn't do this change here, it does the opposite of what we want: the ptrauth (current arm64e, or elf pauthabi) ABI does sign vtable pointers with type/address discrimination. But it specifically doesn't sign the type info vtable pointer, which is why we need this special annotation, to override the default vtable ptr discrimination (that's exposed with the ptrauth_vtable_address_discrimination/ptrauth_vtable_type_discrimination features) to match the type info vtable codegen.

Since then we have indeed added support for signing these, and that's indeed denoted by ptrauth_type_info_discriminated_vtable_pointer. So I think the version that's in tree today is doing the right thing, and honoring clang's directive given its understanding of the ABI. (I don't mind and I'm not worried about removing the arm64 check FWIW).

We'll open a clang PR to add ptrauth_type_info_discriminated_vtable_pointer. But looking at it now I think we'll want to rename it first, so let's settle that this week before it's too late (cc @ojhunt)

@kovdan01 to answer your specific questions: in general I do think these should check something else than ptrauth features, because the specific decisions are made by a specific platform ABI, and isn't set in stone for all ptrauth. Concretely, for the soft ptrauth example, a real soft ptrauth implementation could very well force these to be always discriminated, and use a different key (soft ptrauth not being constrained by HW keys, it uses a different key for every single ABI usage; it could use one for "type info vtable ptrs").
In practice there likely won't ever be a serious and meaningfully different ptrauth ABI, so this doesn't seem like a real problem either way.

@ahmedbougacha As far as I understood, you are OK with current implementation present in this PR. It enables type+address discrimination for type info vtable pointers when we have __has_feature(__ptrauth_vtable_address_discrimination__) || __has_feature(__ptrauth_vtable_type_discrimination__). Is this really the thing we want to? I'm a bit concerned with this since I've never seen support for discriminating type info vtable pointers (although I understand that you have that already), and the condition present in the PR will enable such discrimination while mainline clang does not support that yet.

Previously, the condition was __has_feature(ptrauth_type_info_discriminated_vtable_pointer), and I think it's better since it does not affect type info vtable pointers discrimination unless it's really implemented on clang side.

Previously, the condition was __has_feature(ptrauth_type_info_discriminated_vtable_pointer), and I think it's better since it does not affect type info vtable pointers discrimination unless it's really implemented on clang side.

Yes, to be clear I agree that's what's best

libcxx/src/include/overridable_function.h

libcxx/include/typeinfo

This patch applies the comments provided on llvm#84573. This is done as a separate PR to avoid merge conflicts with downstreams that already had ptrauth support.

ldionne · 2024-07-23T15:24:39Z

I believe I have applied all the comments. I will merge this once CI is green and I will cherry-pick this back to the LLVM 19 release branch.

ldionne · 2024-07-23T18:04:47Z

Only the FreeBSD job hasn't finished yet and it won't bring additional signal for this change, merging now.

ldionne · 2024-07-23T18:05:17Z

/cherry-pick e64e745

llvmbot · 2024-07-23T18:10:05Z

Failed to cherry-pick: e64e745

https://github.com/llvm/llvm-project/actions/runs/10064019269

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

ldionne · 2024-07-23T19:17:32Z

/cherry-pick e64e745

…llvm#87481) This patch applies the comments provided on llvm#84573. This is done as a separate PR to avoid merge conflicts with downstreams that already had ptrauth support. (cherry picked from commit e64e745)

llvmbot · 2024-07-23T19:22:32Z

/pull-request #100183

…#87481) Summary: This patch applies the comments provided on #84573. This is done as a separate PR to avoid merge conflicts with downstreams that already had ptrauth support. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251081

…llvm#87481) This patch applies the comments provided on llvm#84573. This is done as a separate PR to avoid merge conflicts with downstreams that already had ptrauth support. (cherry picked from commit e64e745)

ldionne requested review from a team as code owners April 3, 2024 12:15

ldionne mentioned this pull request Apr 3, 2024

[libc++] Upstream ptrauth support in libc++ and libc++abi #84573

Merged

ldionne commented Apr 3, 2024

View reviewed changes

libcxx/include/typeinfo Outdated Show resolved Hide resolved

ldionne added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 3, 2024

ldionne added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Apr 3, 2024

philnik777 reviewed Apr 3, 2024

View reviewed changes

ldionne force-pushed the review/ptrauth-followups branch from 4bc3292 to fe45a9e Compare June 7, 2024 18:01

ldionne commented Jun 7, 2024

View reviewed changes

ahmedbougacha reviewed Jul 18, 2024

View reviewed changes

libcxx/src/include/overridable_function.h Show resolved Hide resolved

ldionne force-pushed the review/ptrauth-followups branch from fe45a9e to 5bafe12 Compare July 18, 2024 16:32

ldionne commented Jul 18, 2024

View reviewed changes

libcxx/include/typeinfo Show resolved Hide resolved

ldionne added this to the LLVM 19.X Release milestone Jul 18, 2024

ldionne added 5 commits July 23, 2024 11:22

[libc++][libc++abi] Minor follow-up changes after ptrauth upstreaming

e3f9d75

This patch applies the comments provided on llvm#84573. This is done as a separate PR to avoid merge conflicts with downstreams that already had ptrauth support.

Formatting

de9fbd1

Uglify and try to also handle ELF

3e0e1ea

Revert to previous feature check

9eb9186

Use ptrauth_type_info_vtable_pointer_discrimination instead

e86c25d

ldionne force-pushed the review/ptrauth-followups branch from 5bafe12 to e86c25d Compare July 23, 2024 15:23

ldionne merged commit e64e745 into llvm:main Jul 23, 2024
52 of 54 checks passed

ldionne deleted the review/ptrauth-followups branch July 23, 2024 18:04

llvmbot added the release:cherry-pick-failed label Jul 23, 2024

ldionne removed the release:cherry-pick-failed label Jul 23, 2024

nico mentioned this pull request Jul 24, 2024

[libc++] Revert "Make std::pair trivially copyable if its members are (#89652)" #100184

Merged

philnik777 mentioned this pull request Aug 2, 2024

[Clang] Add a release note deprecating __is_nullptr #101638

Merged

mgabka mentioned this pull request Aug 22, 2024

Add release note about ABI mgabka/llvm-project#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++][libc++abi] Minor follow-up changes after ptrauth upstreaming #87481

[libc++][libc++abi] Minor follow-up changes after ptrauth upstreaming #87481

ldionne commented Apr 3, 2024

github-actions bot commented Apr 3, 2024 •

edited

Loading

llvmbot commented Apr 3, 2024 •

edited

Loading

philnik777 Apr 3, 2024

ldionne Apr 3, 2024 •

edited

Loading

ldionne Jun 7, 2024

kovdan01 Jun 21, 2024

kovdan01 Jun 24, 2024

ahmedbougacha Jul 18, 2024

kovdan01 Jul 18, 2024

ahmedbougacha Jul 18, 2024

ldionne commented Jul 23, 2024

ldionne commented Jul 23, 2024

ldionne commented Jul 23, 2024

llvmbot commented Jul 23, 2024

ldionne commented Jul 23, 2024

llvmbot commented Jul 23, 2024

[libc++][libc++abi] Minor follow-up changes after ptrauth upstreaming #87481

[libc++][libc++abi] Minor follow-up changes after ptrauth upstreaming #87481

Conversation

ldionne commented Apr 3, 2024

github-actions bot commented Apr 3, 2024 • edited Loading

llvmbot commented Apr 3, 2024 • edited Loading

philnik777 Apr 3, 2024

Choose a reason for hiding this comment

ldionne Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

ldionne Jun 7, 2024

Choose a reason for hiding this comment

kovdan01 Jun 21, 2024

Choose a reason for hiding this comment

kovdan01 Jun 24, 2024

Choose a reason for hiding this comment

ahmedbougacha Jul 18, 2024

Choose a reason for hiding this comment

kovdan01 Jul 18, 2024

Choose a reason for hiding this comment

ahmedbougacha Jul 18, 2024

Choose a reason for hiding this comment

ldionne commented Jul 23, 2024

ldionne commented Jul 23, 2024

ldionne commented Jul 23, 2024

llvmbot commented Jul 23, 2024

ldionne commented Jul 23, 2024

llvmbot commented Jul 23, 2024

github-actions bot commented Apr 3, 2024 •

edited

Loading

llvmbot commented Apr 3, 2024 •

edited

Loading

ldionne Apr 3, 2024 •

edited

Loading