Optimization in clang frontend breaks `dynamic_cast` on Apple platforms.

[ahatanak]

This is caused by 9d525bf.

dynamic_cast in the following code fails unless the optimization is disabled by passing -fno-assume-unique-vtables.

% cat a.h

struct Foo {
  virtual ~Foo();
};

struct Bar final : public Foo {
};

Foo* makeBar();

% cat a.cc

#include "a.h"

Foo::~Foo() {}

Foo* makeBar() {
  return new Bar();
}

% cat b.cc

#include "a.h"

int main() {
  Foo* f = makeBar();
  Bar* b = dynamic_cast<Bar*>(f); // b will incorrectly be nullptr
  return !!b;
}

% clang++ -std=c++11 -c a.cc -O1
% clang++ -std=c++11 -c b.cc -O1
% clang++ -shared -o liba.so a.o
% clang++ --std=c++11 -o a.out b.o -Wl,-rpath,$PWD liba.so
% ./a.out
% echo $?
0

[ahatanak]

llvm's emits Bar's vtable, which is marked as unnamed_addr in the IR, as weak external automatically hidden and the static linker turns it into a hidden (non-external) symbol.

[ahatanak]

@zygoloid @rjmccall

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Akira Hatanaka (ahatanak)

This is caused by 9d525bf.

dynamic_cast in the following code fails unless the optimization is disabled by passing -fno-assume-unique-vtables.

% cat a.h

struct Foo {
  virtual ~Foo();
};

struct Bar final : public Foo {
};

Foo* makeBar();

% cat a.cc

#include "a.h"

Foo::~Foo() {}

Foo* makeBar() {
  return new Bar();
}

% cat b.cc

#include "a.h"

int main() {
  Foo* f = makeBar();
  Bar* b = dynamic_cast<Bar*>(f); // b will incorrectly be nullptr
  return !!b;
}

% clang++ -std=c++11 -c a.cc -O1
% clang++ -std=c++11 -c b.cc -O1
% clang++ -shared -o liba.so a.o
% clang++ --std=c++11 -o a.out b.o -Wl,-rpath,$PWD liba.so
% ./a.out
% echo $?
0

[zygoloid]

llvm's emits Bar's vtable, which is marked as unnamed_addr in the IR, as weak external automatically hidden and the static linker turns it into a hidden (non-external) symbol.

Interesting. What does automatically hidden mean? Is that a new LLVM visibility? I don't see it in the LangRef.

Looking at the code, as far as I can see the only time we should be setting the vtable as having hidden visibility is if the class has hidden visibility, and in that case we turn off the optimization because dynamic_cast might treat the types as being the same because they have the same type name despite the visibility annotation, and we respect its decision. Is "hidden" being added due to the unnamed_addr? unnamed_addr is only supposed to allow merging of symbols, not duplication of them, so that sounds like a bug if so.

In any case, assuming the ABI intent on this target is that vtables are not unique, we should ensure that hasUniqueVTablePointer returns false. It already returns false when -fapple-kext is enabled; perhaps we need to make it return false for all MachO targets? (Though it'd be good to understand what's going on here; there may be some broader class of cases for which the optimization needs to be disabled, or some other issue.)

[ahatanak]

llvm's emits Bar's vtable, which is marked as unnamed_addr in the IR, as weak external automatically hidden and the static linker turns it into a hidden (non-external) symbol.

Interesting. What does automatically hidden mean? Is that a new LLVM visibility? I don't see it in the LangRef.

Looking at the code, as far as I can see the only time we should be setting the vtable as having hidden visibility is if the class has hidden visibility, and in that case we turn off the optimization because dynamic_cast might treat the types as being the same because they have the same type name despite the visibility annotation, and we respect its decision. Is "hidden" being added due to the unnamed_addr? unnamed_addr is only supposed to allow merging of symbols, not duplication of them, so that sounds like a bug if so.

In any case, assuming the ABI intent on this target is that vtables are not unique, we should ensure that hasUniqueVTablePointer returns false. It already returns false when -fapple-kext is enabled; perhaps we need to make it return false for all MachO targets? (Though it'd be good to understand what's going on here; there may be some broader class of cases for which the optimization needs to be disabled, or some other issue.)

Yes, we can make hasUniqueVTablePointer return false so that the optimization isn't enabled on MachO targets.

weak external automatically hidden is just what I see when I run nm -mv on the binaries, but weak_def_can_be_hidden is what's causing the symbol to be hidden.

AsmPrinter::emitLinkage marks the symbol as weak_def_can_be_hidden when GlobalValue::canBeOmittedFromSymbolTable returns true, which happens when the symbol is linkonce_odr and unnamed_addr. When the static linker sees the weak_def_can_be_hidden on the symbol and other conditions are met, it hides it by removing it from the symbol table.

Does unnamed_addr disallow duplication of symbols? The langref says unnamed_addr indicates only the content is significant, not the address, so shouldn't duplication be allowed as long as the content doesn't change?

[zygoloid]

Does unnamed_addr disallow duplication of symbols? The langref says unnamed_addr indicates only the content is significant, not the address, so shouldn't duplication be allowed as long as the content doesn't change?

I can't answer that with any authority, but I was previously told (though I don't recall who by) that unnamed_addr only permits merging, not duplication. I think that makes sense -- allowing merging is weaker than allowing merging and duplication, and the purpose of the annotation is to permit merging. For example, if we have an unnamed_addr global array and unnamed_addr means "mergeable", we should be able to get the beginning and end of it, and the difference between them should be the size of the array (even if we ask for the two ends from different DSOs), but if it allows duplication too then that's not valid. Someone who knows more about LLVM IR semantics should figure out what semantics we want / meant here.

If unnamed_addr actually allows both merging and duplication, then it's not correct for us to set it on vtables at all, because the Itanium C++ ABI requires that we use a specific symbol as the value of the vtable pointer.

Perhaps we should have both an LLVM IR annotation meaning "mergeable" and one meaning "duplicatable", and Clang can set only "mergeable" on vtables at least by default. Then if the Apple ABI wants to make them duplicatable, that can be enabled in a way that also turns off this optimization?

[efriedma-quic]

"duplicatable" is problematic because it's not actually possible to mark a global "duplicatable", at least given the way LLVM IR currently works. The address of a global is a Constant, so it needs to have the same value throughout the lifetime of the program. See #32127 for a practical case where trying to duplicate a global caused a miscompile.

#111343 is discussing basically the same issue.

[tkloczko]

Hi, we encountered similar issues in a context of template classes. We tried to factorise that into a single project:
https://gitlab.inria.fr/tkloczko/dynamic-cast-clang-issue
Hope it helps.
Sincerely,
Thibaud.