This is an archive of the discontinued LLVM Phabricator instance.

Always compare C++ typeinfo (based on libstdc++ implementation).
ClosedPublic

Authored by marxin on Feb 11 2019, 12:35 AM.

Details

Summary

Resending https://reviews.llvm.org/D56485

As seen here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88684#c4

GCC libstdc++v3 implementation is __GXX_MERGED_TYPEINFO_NAMES=0 for all targets.
Thus it should be always compared string-wise. And one needs to check for name[0] != '*' as seen in typeinfo
implementation:

libstdc++-v3/libsupc++/typeinfo

Diff Detail

Event Timeline

marxin created this revision.Feb 11 2019, 12:35 AM

There hasn't been any reply yet. May I understand that as approval?

I’d like @kubamracek and/or @Bigcheese to take a look first.

Bigcheese requested changes to this revision.Feb 14 2019, 5:30 PM

I do not believe this change is correct for MacOS or iOS. The original bug does not reproduce on MacOS, and on arm64 iOS you always need to compare strings. Could you preserve the existing behavior for those platforms?

This revision now requires changes to proceed.Feb 14 2019, 5:30 PM

I do not believe this change is correct for MacOS or iOS. The original bug does not reproduce on MacOS, and on arm64 iOS you always need to compare strings. Could you preserve the existing behavior for those platforms?

I can if you want. Have you tested that on MacOS or iOS with:

-stdlib=libstdc++

?

I can if you want. Have you tested that on MacOS or iOS with:

-stdlib=libstdc++

?

I have not, but this behavior depends on how clang emits the symbols and how the platform dynamic linker works, not on the standard library. We do not use libstdc++, so this needs to be correct for other implementations too.

marxin updated this revision to Diff 187740.Feb 21 2019, 1:55 AM

Updated version which preserves behavior on MACOS and IOS. For other targets SANITIZER_NON_UNIQUE_TYPEINFO is set to true.

Gentle ping.

Bigcheese requested changes to this revision.Feb 27 2019, 4:02 PM

Thanks for the update. This still looks incorrect in that SANITIZER_NON_UNIQUE_TYPEINFO should be 1 on defined(__arm64__) && SANITIZER_IOS and 0 otherwise on SANITIZER_MAC.

I believe the following is correct.

#if SANITIZER_MAC && !(defined(__arm64__) && SANITIZER_IOS)
# define SANITIZER_NON_UNIQUE_TYPEINFO 0
#else
# define SANITIZER_NON_UNIQUE_TYPEINFO 1
#endif
This revision now requires changes to proceed.Feb 27 2019, 4:02 PM
marxin updated this revision to Diff 188861.Mar 1 2019, 1:25 AM

Update version where I address last comment.

marxin added a comment.Mar 5 2019, 4:05 AM

Update version where I address last comment.

May I please remind that?

This revision is now accepted and ready to land.Mar 5 2019, 10:11 AM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 6 2019, 12:36 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript