Page MenuHomePhabricator

MS ABI: Give thunks linkonce_odr linkage
ClosedPublic

Authored by hans on Jun 2 2014, 11:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hans updated this revision to Diff 10024.Jun 2 2014, 11:53 AM
hans retitled this revision from to MS ABI: Give thunks linkonce_odr linkage.
hans updated this object.
hans edited the test plan for this revision. (Show Details)
hans added reviewers: rnk, timurrrr.
hans added subscribers: Unknown Object (MLST), hansw.
rnk edited edge metadata.Jun 2 2014, 12:47 PM

I think it's handling this corner case:

struct Incomplete;
struct A { int a; virtual A *bar(); };
struct B { int b; virtual B *foo(Incomplete); };
struct C : A, B { int c; virtual C *foo(Incomplete); };
C c;

MSVC gives this error:

t.cpp(5) : error C2664: 'C *C::foo(Incomplete)' : cannot convert argument 1 from 'Incomplete' to 'Incomplete'
      Source or target has incomplete type
      This diagnostic occurred in the compiler generated function 'B *C::foo(Incomplete)'

Clang on the other hand relies on the TU defining the virtual function to emit code for the thunk. Therefore I think the correct linkage is weak_odr. This should also be applicable to the Itanium C++ ABI.

hans updated this revision to Diff 10027.Jun 2 2014, 2:28 PM
hans edited edge metadata.

Switch to weak_odr.

rnk added a comment.Jun 2 2014, 3:00 PM

It occurs to me that the linkage is still wrong for classes with thunks in anonymous namespaces. It'd also be nice to have the thunks be discardable (i.e. linkonce_odr) in the common cases.

I think this is the right linkage computation:

  • class has GVA_Internal linkage -> thunk is internal
  • the thunk has a return adjustment -> thunk is weak_odr, to handle the evil corner case
  • all other normal methods -> thunk is linkonce_odr
hans updated this revision to Diff 10032.Jun 2 2014, 5:27 PM

Calculate the thunk linkage as suggested by Reid.

rnk accepted this revision.Jun 6 2014, 11:18 AM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jun 6 2014, 11:18 AM
hans closed this revision.Jun 6 2014, 1:12 PM
hans updated this revision to Diff 10189.

Closed by commit rL210368 (authored by @hans).