Page MenuHomePhabricator

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

Authored by marxin on Jan 9 2019, 3:36 AM.

Details

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

Diff Detail

Event Timeline

marxin created this revision.Jan 9 2019, 3:36 AM
marxin added a reviewer: kcc.Jan 9 2019, 3:36 AM

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?

filcab requested changes to this revision.Jan 17 2019, 7:47 AM
This revision now requires changes to proceed.Jan 17 2019, 7:47 AM
marxin marked an inline comment as done.Jan 18 2019, 1:43 AM
marxin added inline comments.
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"

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 added a subscriber: pcc.Jan 18 2019, 5:58 AM

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

filcab added a reviewer: pcc.Jan 18 2019, 5:59 AM

@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 added a comment.Jan 18 2019, 10:27 PM

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.

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

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.

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.

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.

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.

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 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 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 added a comment.Feb 4 2019, 6:12 AM

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

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.

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.

@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 added a comment.Feb 5 2019, 9:40 AM

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.

marxin abandoned this revision.Feb 11 2019, 12:31 AM