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 wrote:
> > 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.
> My thinking is this: if a vtable has discardable linkage, then it can be
> discarded when optimizing if it's not referenced. So there's no point
> emitting it unless we also emit a reference to it. So we should only emit
> vtables with discardable linkage if they're actually referenced, just like we
> do for other symbols with discardable linkage. This is, I think, stronger
> than what you're suggesting, because it affects internal-linkage explicit
> instantiations too.
Given only the ABI rule, using discardable linkage is a bug. If you take the
"those translation units containing the definition must emit the v-table so
that other translation units can use it" argument seriously, you obviously
can't use discardable linkage, because the other translation units can't use
it. That's why I bothered developing the whole argument above about why it's
okay to ignore the ABI rule here.
Your argument about internal-linkage explicit instantiations abstractly makes a
lot of sense but also sets off a bunch of klaxons in my mind about ignoring
obvious programmer intent. Still, I think it's reasonable to try it out.
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
