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

[marxin]

Summary

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

[filcab]

I have a few problems/questions with this patch as it is right now. Please address them before committing.

Thank you,
Filipe

lib/ubsan/ubsan_type_hash_itanium.cc

121

We shouldn't need to always check this. Some platforms guarantee unique typeinfo, that's why we have SANITIZER_NON_UNIQUE_TYPEINFO. Please restore the platform dependent code unless you have good reason for removing it (e.g: If typeinfo is not unique on most platforms that the sanitizers support, or if it's too easy to "cheat" when it should be unique and end up with problems).

The check against '*' looks a bit weird and maybe libstdc++ specific. What does it mean? Can you add a comment, please?

[marxin]

lib/ubsan/ubsan_type_hash_itanium.cc

121

We shouldn't need to always check this. Some platforms guarantee unique typeinfo, that's why we have SANITIZER_NON_UNIQUE_TYPEINFO.

When using libstdc++ there's currently no platforms that is using pointer comparison in GCC.

Please restore the platform dependent code unless you have good reason for removing it (e.g: If typeinfo is not unique on most platforms that the sanitizers support, or if it's too easy to "cheat" when it should be unique and end up with problems).
The check against '*' looks a bit weird and maybe libstdc++ specific. What does it mean? Can you add a comment, please?

libstdc++ adds '*' at the beginning for private types:
https://github.com/gcc-mirror/gcc/blob/master/gcc/cp/rtti.c#L399

Apparently clang can be linked against libstdc++ with:

clang++ typename.cpp -c -S -stdlib=libstdc++

but then I see wrong type names:

grep 'asci.*GLOBAL_' typename.s
	.asciz	"N12_GLOBAL__N_11SE"
	.asciz	"N12_GLOBAL__N_11TE"

GCC uses:

grep 'string.*GLOBAL_' typename.gcc.s
	.string	"*N12_GLOBAL__N_11TE"
	.string	"*N12_GLOBAL__N_11SE"

[marxin]

Sample code I used:

namespace { struct S { S () {} ~S () {} virtual void foo () { asm volatile (""); } }; }
namespace { struct T : public S { T () {} ~T () {} virtual void foo () { asm volatile ("nop"); } }; }
void bar (S &s) { s.foo (); }
void baz () { S s; T t; bar (s); bar (t); }

[filcab]

On the platform I most care about, we have defined SANITIZER_NON_UNIQUE_TYPEINFO, so I am ok with this patch from our point of view. But I want to make sure other platforms using this are ok with the additional slowdown (assuming they have typeinfo equality).

@kcc Can you give your opinion on Linux/Android, or send to someone else?
@rnk: I'm pinging you because of the Windows part, but I think Windows doesn't care about this code path, as they have their own type related stuff
@pcc: Since this impacts CFI, can you say if the perf impact is acceptable on your end?
@mehdi_amini: Same Qs, but about macOS

I'm going from memory, so please forgive me if I pinged the wrong people for a platform and forward please.

Thank you,
Filipe

lib/ubsan/ubsan_type_hash_itanium.cc

121

When using libstdc++ there's currently no platforms that is using pointer comparison in GCC.

Yes, but what about those not using libstdc++? If people are using platforms which guarantee pointer-equality, we shouldn't make them slower just because gcc switched.

121

libstdc++ adds '*' at the beginning for private types:
https://github.com/gcc-mirror/gcc/blob/master/gcc/cp/rtti.c#L399

Isn't that gcc's code for generating the symbol name strings? Which would imply that other libraries that use object code from gcc will need to know about this edge case. Is this documented properly somewhere? Can it be?

As I understand it, this is how it works:

• All code still "usually" follows the itanium C++ ABI and has unique typeinfo.
• GCC tacks on a * at the beginning of the name string for private types which shouldn't have their comparisons against others be true unless they're the same object.
• libstdc++ (and compiler-rt after this patch) checks for pointer equality. If true, all is ok. If false, checks if one (both?) of the name strings start with *. If it does, it knows it's one of the "types likely to end up with non-unique typeinfo" and will compare strings.

If this is incorrect, please let me know what parts are wrong.

If this is ok, I think we'll need to either:

• Be able to figure out if we're running with libstdc++ (or something with a similar ABI) and only do the strcmp if the answer is yes or SANITIZER_NON_UNIQUE_TYPEINFO is set
• Know that the people working on platforms where UBSan's vptr check (*and* the CFI sanitizer) is supported are ok with the slowdown. I'll add a few people to the review, at least for:

Linux/sanitizers in general
CFI
OS X
Windows

[mehdi_amini]

In D56485#1363031, @filcab wrote:

@pcc: Since this impacts CFI, can you say if the perf impact is acceptable on your end?
@mehdi_amini: Same Qs, but about macOS

Let me loop in @dexonsmith and @dcoughlin instead (I'm not at Apple anymore FYI).

[pcc]

This change would only impact the "diagnostic" (non-production) mode of CFI. In production builds we do not link the runtime library at all. So from a CFI perspective this is fine.

[filcab]

In D56485#1364241, @pcc wrote:

This change would only impact the "diagnostic" (non-production) mode of CFI. In production builds we do not link the runtime library at all. So from a CFI perspective this is fine.

Well, that makes sense. CFI was my biggest problem with this, so I'm tempted to accept the patch (The * doesn't seem to be a valid character for Itanium C++ ABI names, so I think having the test there should only trigger on gcc code).
Please wait 1 or 2 days (to see if there are objections), and then you can consider the patch LGTMed. I'll try to poke it on tuesday/wednesday.

Thank you,
Filipe

[marxin]

In D56485#1365063, @filcab wrote:

In D56485#1364241, @pcc wrote:

This change would only impact the "diagnostic" (non-production) mode of CFI. In production builds we do not link the runtime library at all. So from a CFI perspective this is fine.

Well, that makes sense. CFI was my biggest problem with this, so I'm tempted to accept the patch (The * doesn't seem to be a valid character for Itanium C++ ABI names, so I think having the test there should only trigger on gcc code).
Please wait 1 or 2 days (to see if there are objections), and then you can consider the patch LGTMed. I'll try to poke it on tuesday/wednesday.

Thank you,
Filipe

Thank you then, I'll install the patch when there will be no comment about it later this week.

[lebedev.ri]

In D56485#1365063, @filcab wrote:

Please wait 1 or 2 days (to see if there are objections), and then you can consider the patch LGTMed.

Except the lists aren't subscibed, thus no feedback can happen :S
So this wasn't really reviewed.

[marxin]

In D56485#1369010, @lebedev.ri wrote:

In D56485#1365063, @filcab wrote:

Please wait 1 or 2 days (to see if there are objections), and then you can consider the patch LGTMed.

Except the lists aren't subscibed, thus no feedback can happen :S
So this wasn't really reviewed.

Ups. Ok, let make the discussion happen and then I'll suggest follow up patch.

[lebedev.ri]

In D56485#1369020, @marxin wrote:

In D56485#1369010, @lebedev.ri wrote:

In D56485#1365063, @filcab wrote:

Please wait 1 or 2 days (to see if there are objections), and then you can consider the patch LGTMed.

Except the lists aren't subscibed, thus no feedback can happen :S
So this wasn't really reviewed.

Ups. Ok, let make the discussion happen and then I'll suggest follow up patch.

I would suggest to revert the commit, and submit the new differential with properly subscribed lists this time.
At least that is the 'recommended' general practice.

[marxin]

I would suggest to revert the commit, and submit the new differential with properly subscribed lists this time.
At least that is the 'recommended' general practice.

Done.

[dexonsmith]

In D56485#1369023, @marxin wrote:

I would suggest to revert the commit, and submit the new differential with properly subscribed lists this time.
At least that is the 'recommended' general practice.

Done.

I'm having trouble finding the new differential (I don't think I'm CC'ed, and I'm not seeing it in llvm-commits). Can you add me as a subscriber, and @Bigcheese and @dcoughlin as reviewers?

[lebedev.ri]

In D56485#1369569, @dexonsmith wrote:

In D56485#1369023, @marxin wrote:

I would suggest to revert the commit, and submit the new differential with properly subscribed lists this time.
At least that is the 'recommended' general practice.

Done.

I'm having trouble finding the new differential (I don't think I'm CC'ed, and I'm not seeing it in llvm-commits). Can you add me as a subscriber, and @Bigcheese and @dcoughlin as reviewers?

I don't see any new differentials on https://reviews.llvm.org/p/marxin/ so i'm guessing it wasn't submitted yet.

@marxin to be noted, right now i have no other objections to the patch, so i think after that one is up for review, the resolution from https://reviews.llvm.org/D56485#1365063 can carry over to that new review.

[marxin]

Hi, there's no reply from the people who were added to this review.
May I understand that as ready to be installed?

[dexonsmith]

In D56485#1383077, @marxin wrote:

Hi, there's no reply from the people who were added to this review.
May I understand that as ready to be installed?

filcab is still marked as blocking this. Also I thought you were going to file a new differential that had the commits list cc’ed correctly? See above where I asked you to add a couple of relevant reviewers to the new differential (and me as a subscriber) when you do.

[marxin]

In D56485#1383378, @dexonsmith wrote:

In D56485#1383077, @marxin wrote:

Hi, there's no reply from the people who were added to this review.
May I understand that as ready to be installed?

filcab is still marked as blocking this. Also I thought you were going to file a new differential that had the commits list cc’ed correctly? See above where I asked you to add a couple of relevant reviewers to the new differential (and me as a subscriber) when you do.

@filcab : Are you fine with the patch as it is right now?

@dexonsmith: I'm not sure why a new differential should be created? I updated reviewers in this one.

[mehdi_amini]

@dexonsmith: I'm not sure why a new differential should be created? I updated reviewers in this one.

This is the project policy: a patch should be sent to the llvm-commits mailing list for review. If the list is not subscribed when you create the revision, the patch is never sent. This is the usual reason to request closing a revision and re-opening a new one, so that Phabricator sends the patch.

[filcab]

In D56485#1384700, @marxin wrote:

In D56485#1383378, @dexonsmith wrote:

In D56485#1383077, @marxin wrote:

Hi, there's no reply from the people who were added to this review.
May I understand that as ready to be installed?

filcab is still marked as blocking this. Also I thought you were going to file a new differential that had the commits list cc’ed correctly? See above where I asked you to add a couple of relevant reviewers to the new differential (and me as a subscriber) when you do.

@filcab : Are you fine with the patch as it is right now?

@dexonsmith: I'm not sure why a new differential should be created? I updated reviewers in this one.

Yes, I'm ok with the current patch. But please create a new revision CCing llvm-commits. I'll accept in one or two days, to give time for other people to chime in.

lib/sanitizer_common/sanitizer_platform.h

 ///  * 1800: Microsoft Visual Studio 2013 / 12.0
 ///  * 1900: Microsoft Visual Studio 2015 / 14.0
 #ifdef _MSC_VER
 # define MSC_PREREQ(version) (_MSC_VER >= (version))
 #else
 # define MSC_PREREQ(version) 0
 #endif
 
-#if defined(__arm64__) && SANITIZER_IOS
-# define SANITIZER_NON_UNIQUE_TYPEINFO 1
-#else
-# define SANITIZER_NON_UNIQUE_TYPEINFO 0
-#endif
-
 // On linux, some architectures had an ABI transition from 64-bit long double
 // (ie. same as double) to 128-bit long double.  On those, glibc symbols
 // involving long doubles come in two versions, and we need to pass the
 // correct one to dlvsym when intercepting them.
 #if SANITIZER_LINUX && (SANITIZER_S390 || SANITIZER_PPC32 || SANITIZER_PPC64V1)
 #define SANITIZER_NLDBL_VERSION "GLIBC_2.4"
 #endif
 

lib/ubsan/ubsan_type_hash_itanium.cc

 }
 
 /// \brief Determine whether \p Derived has a \p Base base class subobject at
 /// offset \p Offset.
 static bool isDerivedFromAtOffset(const abi::__class_type_info *Derived,
                                   const abi::__class_type_info *Base,
                                   sptr Offset) {
   if (Derived->__type_name == Base->__type_name ||
-      (SANITIZER_NON_UNIQUE_TYPEINFO &&
+      (Derived->__type_name[0] != '*' &&
        !internal_strcmp(Derived->__type_name, Base->__type_name)))
     return Offset == 0;
 
   if (const abi::__si_class_type_info *SI =
         dynamic_cast<const abi::__si_class_type_info*>(Derived))
     return isDerivedFromAtOffset(SI->__base_type, Base, Offset);
 
   const abi::__vmi_class_type_info *VTI =