I do support the suggested change.

I would like to point to an issue reported on s390x:
https://github.com/google/sanitizers/issues/1390

In D97263#2581226, @vitalybuka wrote:

Can you try to add a test?

In D95864#2573793, @vitalybuka wrote:

There is D96348

Merged to master as b638b63b99d66786cb37336292604a2ae3490cfd.

It fixes multilib build in GCC on ppc64 after libsanitizer merge.

In D70707#1760759, @pcc wrote:

Test case? (Do none of the tests really need to be updated?)

Old report look:

Memory tags around the buggy address (one tag corresponds to 16 bytes):
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
=> d7  00 [08] 00  00  00  00  00  00  00  00  00  00  00  00  00 <=
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
   00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00   
Tags for short granules around the buggy address (one tag corresponds to 16 bytes):
   ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..   
=> ..  .. [be] ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  .. <=
   ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..  ..

Thank you for review. Can you please install the commit?

Updated version of the patch.

Updated patch with a test-case.

May I please ping this..

I've made the error message more verbose.

In D58028#1414723, @marxin wrote:

Update version where I address last comment.

Update version where I address last comment.

Gentle ping.

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

In D58028#1398872, @Bigcheese wrote:

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?

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

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.

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

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.

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.

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

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); }

It's installed.

The right patch, previous one belongs to GCC :)

Updated version of the patch.

Thanks for testing Vitaly. I must confirm that i386 is broken on my openSUSE, thus I can't test that.

Updated patch that introduces 2 new tests.

@vitalybuka: I modified the patch based on your notes.

Updated patch, I also found a test-case that fails for clang before my patch:

In D54030#1286807, @ro wrote:

Overall, this looks good to me, with the nits noted.

However, I wonder how you tested this:

• SPARC isn't among cmake/config-ix.cmake (ALL_ASAN_SUPPORTED_ARCH); in fact there's no compiler-rt support for SPARC at all.

• In gcc, it's similar: there's no definition of TARGET_ASAN_SHADOW_OFFSET in the SPARC backend yet. I've submitted one for Solaris/SPARC in PR sanitizer/80953 and hope to finish and post it soon, but right now there's nothing.

Wondering...