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 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.
> OK, fair enough. I was only starting from the "these vtables have discardable
> linkage" position because that has been the status quo in Clang ~forever
> (godbolt.org only goes back to Clang 3). I'll give it a go and see what
> shakes out.
Yeah, we've never actually taken that argument seriously, but we never took our
current stance to its logical conclusion of emitting the v-table lazily, either.
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
