@rsmith What should the visibility of the type name be in cases where the type info is hidden?

@rjmccall Thank you for the quick review!

This is causing ASan's ODR violation detector to fire. I think the problematic case looks like this:

TU 1:

struct X;
/*...*/ typeid(X*); /*...*/

TU 2:

struct X { virtual void f(); };
void X::f() {}

Now, TU 1 will emit a linkonce_odr definition of the _ZTS symbol, and TU 2 will emit a strong definition of it. ASan doesn't like that.

There's no way we can know that there will be a strong definition of the typeinfo name at the point where we emit the tentative copy for the pointer type_info. It seems to me there are two things we could do about this:

1. decide this is not a bug and suppress the ASan ODR checker report, or
2. decide this is a bug, and only emit linkonce_odr typeinfo names for pointers to incomplete classes, not for incomplete classes themselves (this would mean that operator== etc would do the wrong thing for users who use the <cxxabi.h> to inspect the pointee type of a pointer type info; in particular, we'd need to check that ubsan's vptr sanitizer can cope with this -- right now it assumes that it can compare the type name address except on iOS64, just like libc++'s <type_info> does).

I believe static and dynamic linkers — at least on ELF and Mach-O — will always drop weak symbols for strong ones. Now, I think that isn't LLVM's posted semantics for linkonce_odr, but to me that means that LLVM's semantics are inadequate, not that we should decline to take advantage of them.

If we can't rely on that, it probably means that the type name symbol for class types always has to be linkonce_odr, even if it's for a type with a key function.

In D46665#1102290, @rjmccall wrote:

I believe static and dynamic linkers — at least on ELF and Mach-O — will always drop weak symbols for strong ones. Now, I think that isn't LLVM's posted semantics for linkonce_odr, but to me that means that LLVM's semantics are inadequate, not that we should decline to take advantage of them.

If we can't rely on that, it probably means that the type name symbol for class types always has to be linkonce_odr, even if it's for a type with a key function.

That all makes sense to me. (But I think weak_odr would be more formally correct, even though the symbol will never actually be discarded as it's referenced from the type_info and vtable.)

The rule that ASan is using is that if there is an external definition for a global variable, there should not be any other definitions for that symbol, which seems right given LLVM's semantics, but wrong here.

For now, the simplest thing to do seem to be to weaken external linkage to weak_odr for the type info name.

The only difference between weak_odr and linkonce_odr is that the LLVM optimizers can discard linkonce_odr globals. From your description, you want to remove the odr-ness, by changing the linkage to "linkonce", I think?

That said, I don't think the usage here violates LangRef's definition of linkonce_odr; the "odr"-ness refers to the global's initializer, not its linkage.

In D46665#1102361, @efriedma wrote:

The only difference between weak_odr and linkonce_odr is that the LLVM optimizers can discard linkonce_odr globals. From your description, you want to remove the odr-ness, by changing the linkage to "linkonce", I think?

The odr-ness is fine; the definitions are all the same. The symbol emitted with the key function should be non-discardable, which is why I'm suggesting we change the linkage to weak_odr rather than changing it to linkonce_odr.

What exactly is the asan odr checker actually checking for? The distinction between common/linkonce_odr/linkonce/weak_odr/weak only affects IR optimizers, not code generation.

In D46665#1102413, @efriedma wrote:

What exactly is the asan odr checker actually checking for? The distinction between common/linkonce_odr/linkonce/weak_odr/weak only affects IR optimizers, not code generation.

It's checking that there is only one definition of each global variable for which only one definition is expected; it believes that if there is an external definition, then there should not be any other definitions.

In D46665#1102348, @rsmith wrote:

In D46665#1102290, @rjmccall wrote:

I believe static and dynamic linkers — at least on ELF and Mach-O — will always drop weak symbols for strong ones. Now, I think that isn't LLVM's posted semantics for linkonce_odr, but to me that means that LLVM's semantics are inadequate, not that we should decline to take advantage of them.

If we can't rely on that, it probably means that the type name symbol for class types always has to be linkonce_odr, even if it's for a type with a key function.

That all makes sense to me. (But I think weak_odr would be more formally correct, even though the symbol will never actually be discarded as it's referenced from the type_info and vtable.)

Yes, of course.

The rule that ASan is using is that if there is an external definition for a global variable, there should not be any other definitions for that symbol, which seems right given LLVM's semantics, but wrong here.

I agree.

For now, the simplest thing to do seem to be to weaken external linkage to weak_odr for the type info name.

Yes.

Please see https://reviews.llvm.org/D47092 for the external -> weak_odr change.

Note: this commit was re-reverted in

commit bbb2655de0049f8e6cf4482702aa616011c851c1
Author: Richard Smith <richard-llvm@metafoo.co.uk>
Date:   Mon May 21 20:10:54 2018 +0000

    Revert r332028; see PR37545 for details.

    llvm-svn: 332879