Always issue vtables when generating coverage instrumentation

[FlameTop]

Summary

The provided test shows a case where enabling coverage instrumentation causes a link error during building. Normally the all the base class items (vtable, ctors and dtors) would be removed in an optimized build. But when building with coverage instrumentation the ctors and dtors are always issued and access the vtable. However, the vtable remains with external linkage. As nobody ever instantiates the base class the vtable remains undefined and causes a link error.

This patch changes the behavior when deciding the vtable linkage when generating coverage instrumentation. vtables are always generated as local linkonce objects.

This is slightly wasteful of link time and target storage but the profile instrumentation is performed too early (as far as I can see) to not instrument ctors/dtors that will later be discarded.

[ahatanak]

llvm/tools/clang/test/CodeGenCXX/vtable-coverage-gen.cpp

3

I'm not sure I understood the purpose of this test, but It looks like the vtable for Category is generated in the IR with linkonce_odr without your patch.

[vsk]

llvm/tools/clang/test/CodeGenCXX/vtable-coverage-gen.cpp

3

Yes, I'm seeing the same thing and am also confused. I can reproduce the build failure without using any vtables. To me this suggests the problem could be elsewhere. Here is a minimal reproducer:

struct Base {
  static const Base *get();
};

struct Derived : public Base {};

const Base *Base::get() {
  static Derived d;
  return &d;
}

int main() { return 0; }

[vsk]

llvm/tools/clang/test/CodeGenCXX/vtable-coverage-gen.cpp

3

^ Please ignore my last comment, I made a mistake while trying to compile your reproducer. When I used a proper compile command, I could not reproduce the build failure on Darwin/MachO (-fprofile-instr-generate -fcoverage-mapping).

[FlameTop]

I must apologise to you all. The problem was reported in our out-of-tree version of the compiler and I must have hit finger problems when I confirmed it in tree. Repeating the test I can no longer reproduce the fault with the 3.9.0 public build. It must be some effect of a local change.

Again apologies.

Phil

llvm/tools/clang/lib/CodeGen/CGVTables.cpp

CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) {
   // function is fully correct.
   const CXXMethodDecl *keyFunction = Context.getCurrentKeyFunction(RD);
   if (keyFunction && !RD->hasAttr<DLLImportAttr>()) {
     // If this class has a key function, use that to determine the
     // linkage of the vtable.
     const FunctionDecl *def = nullptr;
     if (keyFunction->hasBody(def))
       keyFunction = cast<CXXMethodDecl>(def);
-
     switch (keyFunction->getTemplateSpecializationKind()) {
       case TSK_Undeclared:
+        // if we are generating profile instrumentation ensure
+        // vtables are always issued locally as the constructors
+        // and destructors are always issued.
+        if (CodeGenOpts.hasProfileClangInstr() ||
+            CodeGenOpts.hasProfileIRInstr())
+          return !Context.getLangOpts().AppleKext ?
+                   llvm::GlobalVariable::LinkOnceODRLinkage :
+                   llvm::Function::InternalLinkage;
+        LLVM_FALLTHROUGH;
       case TSK_ExplicitSpecialization:
         assert((def || CodeGenOpts.OptimizationLevel > 0) &&
                "Shouldn't query vtable linkage without key function or "
                "optimizations");
         if (!def && CodeGenOpts.OptimizationLevel > 0)
           return llvm::GlobalVariable::AvailableExternallyLinkage;
 
         if (keyFunction->isInlined())

 ///
 /// The only semantic restriction here is that the object file should
 /// not contain a vtable definition when that vtable is defined
 /// strongly elsewhere.  Otherwise, we'd just like to avoid emitting
 /// vtables when unnecessary.
 bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) {
   assert(RD->isDynamicClass() && "Non-dynamic classes have no VTable.");
 
+  // if we are generating profile instrumentation, issue the vtables locally as
+  // the constructors and destructors will always be issued.
+  if (CGM.getCodeGenOpts().hasProfileClangInstr() ||
+      CGM.getCodeGenOpts().hasProfileIRInstr())
+    return false;
+
   // We always synthesize vtables if they are needed in the MS ABI. MSVC doesn't
   // emit them even if there is an explicit template instantiation.
   if (CGM.getTarget().getCXXABI().isMicrosoft())
     return false;
 
   // If we have an explicit instantiation declaration (and not a
   // definition), the vtable is defined elsewhere.
   TemplateSpecializationKind TSK = RD->getTemplateSpecializationKind();

llvm/tools/clang/test/CodeGenCXX/vtable-coverage-gen.cpp

+// RUN: %clang_cc1 -O2 -fprofile-instrument=clang -fcoverage-mapping %s -emit-llvm -o %t
+// RUN: FileCheck %s < %t 
+// CHECK: @_ZTV8Category = linkonce_odr unnamed_addr constant {{.*}}, comdat,
+
+class Category
+{
+  public:
+ Category() {}
+ virtual ~Category() {}
+
+ static const Category *Get(int source);
+
+
+  protected:
+ const char *const ITEM_KEY_CONSOLE_ID = "consoleId";
+
+};
+
+
+
+class CategoryAboutMe : public Category
+{
+  public:
+ CategoryAboutMe() {}
+ virtual ~CategoryAboutMe() {}
+
+  protected:
+};
+
+
+//static
+const Category *
+Category::Get(int source)
+{
+ static CategoryAboutMe s_categoryAboutMe;
+
+ switch (source) {
+ case 0:
+  return &s_categoryAboutMe;
+ default:
+  return __null;
+ }
+  return 0;
+}
+
+
+int main(int argc, char const *argv[])
+{
+  /* code */
+  return 0;
+}
 No newline at end of file