We wasted a good deal of time trying to figure out whether our implementation was correct. In the end, it was, but it wasn't so easy to determine. This patch dumbs down the implementation and improves the documentation to make it easier to validate.
- Group Reviewers
- rG17095dc86111: [libc++][NFC] Increase readability of typeinfo comparison of ARM64
This is a WIP:
- I'm not sure whether this change is actually correct, or whether we are mis-interpreting the ABI and the previous implementation was actually correct.
- I don't know how to test this yet.
I created a patch because I wanted to avoid forgetting about this message on the mailing list forever. I'll need to do some investigation before I can make progress on this.
You're misinterpreting the condition. The "is non-unique" bit says that the RTTI is non-unique and thus its identity is string-based; otherwise it is pointer-based. A uniquely-emitted type is never the same as a non-uniquely-emitted type.
Usually people who are having problems with this are messing up type visibility so that it's inconsistent between libraries.
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?
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 :)
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.
Actually, that isn't what I meant to write. I meant:
When comparing type_infos, if both RTTIs can be assumed to be unique, it suffices to compare their addresses. If both the RTTIs can't be assumed to be unique, we must perform a deep string comparison of the type names. However, if one of the RTTIs is guaranteed unique and the other one isn't, then both RTTIs are necessarily not to be considered equal.
Thanks for catching that!
This is not an ordering relation. For example, we could have non-unique C < unique B < non-unique A < non-unique C, where the first and second comparisons are address comparisons, and the third comparison is a string comparison.
I think perhaps something like this would work:
bool __lhs_unique = __is_type_name_unique(__lhs); if (__lhs_unique != __is_type_name_unique(__rhs)) return __lhs_unique; if (__lhs_unique) return __lhs < __rhs; return __builtin_strcmp(__type_name_to_string(__lhs), __type_name_to_string(__rhs)) < 0;
(That is: order all unique typeinfos before all non-unique ones, then order unique typeinfos by pointer and non-unique ones by string.)