rjmccall added inline comments.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:3991-3992
+ if (auto *MD = dyn_cast<CXXMethodDecl>(D)) {
+ // FIXME: There's no reason to do this if the key function is inline.
+ // Formally, the ABI requires it, but the difference is not observable.
+ if (declaresSameEntity(Context.getCurrentKeyFunction(MD->getParent()), MD))
----------------
rsmith wrote:
> @rjmccall Is there any reason we (from the CodeGen perspective) should treat
> an inline key function as emitting the vtable? I can't think of any reason to
> do so -- it's not in a comdat with the vtable or anything like that, so every
> translation unit that emits a reference to the vtable should emit its own
> copy anyway.
The thinking was as follows: translation units that can't see a definition of
the key function don't know that the definition is actually inline, so they'll
emit a reference to an external v-table definition, which will lead to link
failures if the translation units that do contain the inline definition don't
eagerly emit the v-table. However, ARM pointed out years ago that the ODR
requires inline definitions of virtual functions to be present in every
translation unit which declares the virtual function at all, so there's no
legal situation where a translation unit can't see the definition of an inline
key function. Furthermore, I believe Clang has never made v-tables
undiscardable in translation units with inline key function definitions, so
there's no real guarantee that ODR-violating code will actually link, although
you can certainly imagine ways in which an ODR-violating program might get by
without such a guarantee.
Personally, I think the strongest argument for "deviating" here is that the
language standard takes priority over the ABI, which means we're allowed to
assume the program is overall well-formed, i.e. we're only required to
interoperate with legal code. Now, that's a line of reasoning which leads us
into some grey areas of implementation-definedness, but I feel fairly
comfortable about deviating in this particular instance because I don't know
why someone would really *want* to take advantage of v-tables being emitted
eagerly; in general, eager emission of inline code is a bad thing that
significantly slows down builds.
Now, ARM used this property of inline definitions to change the key function to
the first non-inline function, and unfortunately we can't do that on existing
targets without breaking interoperation. (We did do it on some newer Darwin
targets, though, and we haven't had any problem with it.) But I do think we
could use this property of inline definitions to just treat the class as no
longer having a key function when we see an inline definition of it. That
would rid us of this particular scourge of eager emission of inline code.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54986/new/
https://reviews.llvm.org/D54986
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
