rsmith marked 2 inline comments as done.
rsmith 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))
----------------
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.
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
