[libcxx-commits] [PATCH] D97802: [libc++] Fix incorrect typeinfo comparison on ARM64

[Louis Dionne via Phabricator via libcxx-commits]

ldionne updated this revision to Diff 334532.
ldionne added a comment.
Herald added a subscriber: smeenai.

Updating according to a discussion with John McCall


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97802/new/

https://reviews.llvm.org/D97802

Files:
  libcxx/include/typeinfo


Index: libcxx/include/typeinfo
===================================================================
--- libcxx/include/typeinfo
+++ libcxx/include/typeinfo
@@ -125,7 +125,7 @@
 //               (_LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION = 1)
 // ------------------------------------------------------------------------- //
 // This implementation of type_info assumes a unique copy of the RTTI for a
-// given type inside a program. This is a valid assumption when abiding to
+// given type inside a program. This is a valid assumption when abiding to the
 // Itanium ABI (http://itanium-cxx-abi.github.io/cxx-abi/abi.html#vtable-components).
 // Under this assumption, we can always compare the addresses of the type names
 // to implement equality-comparison of type_infos instead of having to perform
@@ -144,22 +144,30 @@
 //                          NonUniqueARMRTTIBit
 //               (_LIBCPP_TYPEINFO_COMPARISON_IMPLEMENTATION = 3)
 // -------------------------------------------------------------------------- //
+// This implementation is specific to ARM64 on Apple platforms.
+//
 // This implementation of type_info does not assume always a unique copy of
-// the RTTI for a given type inside a program. It packs the pointer to the
-// type name into a uintptr_t and reserves the high bit of that pointer (which
-// is assumed to be free for use under the ABI in use) to represent whether
-// that specific copy of the RTTI can be assumed unique inside the program.
-// To implement equality-comparison of type_infos, we check whether BOTH
-// type_infos are guaranteed unique, and if so, we simply compare the addresses
-// of their type names instead of doing a deep string comparison, which is
-// faster. If at least one of the type_infos can't guarantee uniqueness, we
-// have no choice but to fall back to a deep string comparison.
+// the RTTI for a given type inside a program. When constructing the type_info,
+// the compilers packs the pointer to the type name into a uintptr_t and reserves
+// the high bit of that pointer, which is assumed to be free for use under that
+// ABI. If that high bit is set, that specific copy of the RTTI can't be assumed
+// to be unique within the program. If the high bit is unset, then the RTTI can
+// be assumed to be unique within the program.
 //
-// This implementation is specific to ARM64 on Apple platforms.
+// When comparing type_infos, if both RTTIs can be assumed to be unique, it
+// suffices to compare their addresses. Otherwise, if at least one of the
+// RTTIs can't be assumed to be unique, we must perform a deep string comparison
+// of the type names. Note that if one of the RTTIs is guaranteed unique and
+// the other one isn't, then both RTTIs are necessarily not to be considered
+// equal.
 //
-// Note that the compiler is the one setting (or unsetting) the high bit of
-// the pointer when it constructs the type_info, depending on whether it can
-// guarantee uniqueness for that specific type_info.
+// The intent of this design is to remove the need for weak symbols. Specifically,
+// if a type would normally have a default-visibility RTTI emitted as a weak
+// symbol, it is given hidden visibility instead and the non-unique bit is set.
+// Otherwise, types declared with hidden visibility are always considered to have
+// a unique RTTI: the RTTI is emitted with linkonce_odr linkage and is assumed
+// to be deduplicated by the linker within the linked image. Across linked image
+// boundaries, such types are thus considered different types.
 
 // This value can be overriden in the __config_site. When it's not overriden,
 // we pick a default implementation based on the platform here.
@@ -248,13 +256,14 @@
     static bool __eq(__type_name_t __lhs, __type_name_t __rhs) _NOEXCEPT {
       if (__lhs == __rhs)
         return true;
-      if (__is_type_name_unique(__lhs, __rhs))
+      else if (__is_type_name_unique(__lhs) != __is_type_name_unique(__rhs))
         return false;
-      return __builtin_strcmp(__type_name_to_string(__lhs), __type_name_to_string(__rhs)) == 0;
+      else
+        return __builtin_strcmp(__type_name_to_string(__lhs), __type_name_to_string(__rhs)) == 0;
     }
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_ALWAYS_INLINE
     static bool __lt(__type_name_t __lhs, __type_name_t __rhs) _NOEXCEPT {
-      if (__is_type_name_unique(__lhs, __rhs))
+      if (__is_type_name_unique(__lhs) || __is_type_name_unique(__rhs))
         return __lhs < __rhs;
       return __builtin_strcmp(__type_name_to_string(__lhs), __type_name_to_string(__rhs)) < 0;
     }
@@ -269,10 +278,6 @@
     static bool __is_type_name_unique(__type_name_t __lhs) _NOEXCEPT {
       return !(__lhs & __non_unique_rtti_bit::value);
     }
-    _LIBCPP_INLINE_VISIBILITY
-    static bool __is_type_name_unique(__type_name_t __lhs, __type_name_t __rhs) _NOEXCEPT {
-      return !((__lhs & __rhs) & __non_unique_rtti_bit::value);
-    }
   };
 
   typedef


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D97802.334532.patch
Type: text/x-patch
Size: 4955 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20210331/b54d36d6/attachment-0001.bin>