[PATCH] D54986: Make CodeGen choose when to emit vtables.

[John McCall via Phabricator via cfe-commits]

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