[libcxx-commits] [PATCH] D97802: [libc++] Fix incorrect typeinfo comparison on ARM64
Shoaib Meenai via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Mar 31 14:47:08 PDT 2021
smeenai added inline comments.
Comment at: libcxx/include/typeinfo:158-162
+// 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
This reads a little confusingly, because the "Otherwise, if at least one of the RTTIs can't be assumed to be unique bit" makes you think that the case where on RTTI is unique and one isn't will do the deep string comparison, but then the next sentence changes the interpretation of that case. Maybe say "if both RTTIs can't be assumed to be unique", since the last sentence already handles the non-unique + unique case?
Comment at: libcxx/include/typeinfo:164
-// 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
Thanks for explaining the motivation of the design and when it kicks in. I'd been casually curious about this before, and it's a neat design :)
Comment at: libcxx/include/typeinfo:259
- if (__is_type_name_unique(__lhs, __rhs))
+ else if (__is_type_name_unique(__lhs) != __is_type_name_unique(__rhs))
I don't understand this. My reading of the two parameter `__is_type_name_unique` function was that it would return true if either typeinfo was unique, and that's also what you're doing in line 266 below. Over here, if both typeinfos are unique, this condition won't kick in, so wouldn't we incorrectly fall through to the strcmp below?
Also a nit: LLVM code style recommends not using an `else` after a return: https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return. I don't know if libc++ normally does this differently.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits