This is an archive of the discontinued LLVM Phabricator instance.

[MS] Emit vftable thunks for functions with incomplete prototypes
ClosedPublic

Authored by rnk on Mar 30 2018, 3:19 PM.

Details

Summary

The following class hierarchy requires that we be able to emit a
this-adjusting thunk for B::foo in C's vftable:

struct Incomplete;
struct A {
  virtual A* foo(Incomplete p) = 0;
};
struct B : virtual A {
  void foo(Incomplete p) override;
};
struct C : B { int c; };

This TU is valid, but lacks a definition of 'Incomplete', which makes it
hard to build a thunk for the final overrider, B::foo.

Before this change, Clang gives up attempting to emit the thunk, because
it assumes that if the parameter types are incomplete, it must be
emitting the thunk for optimization purposes. This is untrue for the MS
ABI, where the implementation of B::foo has no idea what thunks C's
vftable may require. Clang needs to emit the thunk without necessarily
having access to the complete prototype of foo.

This change makes Clang emit a musttail variadic call when it needs such
a thunk. I call these "unprototyped" thunks, because they only prototype
the "this" parameter, which must always come first in the MS C++ ABI.

These thunks work, but they create ugly LLVM IR. If the call to the
thunk is devirtualized, it will be a call to a bitcast of a function
pointer. Today, LLVM cannot inline through such a call, but I want to
address that soon, because we also use this pattern for virtual member
pointer thunks.

This change also implements an old FIXME in the code about reusing the
thunk's computed CGFunctionInfo as much as possible. Now we don't end up
computing the thunk's mangled name and arranging it's prototype up to
around three times.

Fixes PR36952

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Mar 30 2018, 3:19 PM

Does this help PR25641?

This revision is now accepted and ready to land.Mar 31 2018, 10:03 AM
rnk added a comment.Mar 31 2018, 10:16 AM

Does this help PR25641?

I think so, these are probably duplicate PRs. I think your analysis on that bug is slightly off, because the TU that provides ImplCanvas::createColor *can't* provide the thunks that ImplBitmapCanvas needs. It has no way to predict how the offset from the virtual base to the final overrider will change in derived classes.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
sberg added a subscriber: sberg.Apr 18 2018, 7:04 AM

see https://bugs.llvm.org/show_bug.cgi?id=37161 "clang-cl triggers ASTContext::getASTRecordLayout Assertion `D && 'Cannot get layout of forward declarations!''" for what appears to be fallout from this change