Defer codegen of dllexport/attribute(used) inline method definitions

[hans]

Summary

We would previously fail to emit a definition of bar() for the following code:

struct __declspec(dllexport) S {
  void foo() {
    t->bar();
  }
  struct T {
    void bar() {}
  };
  T *t;
};

Note that foo() is an exported method, but bar() is not. However, foo() refers to bar() so we need to emit its definition. To make this work, we need to delay the codegen of foo() until the bodies of all the other inline methods have been parsed.

The same problem applies to inline methods that are marked used.

[hans]

Simplify things. Let ModuleBuilder defer method definitions, and then consider them at the next top level decl. At that point, it's safe to calculate the GVA linkage, so we can just call into EmitTopLevelDecl and let that figure out if the method needs to be emitted in the usual way - no special cases.

[rsmith]

lib/CodeGen/ModuleBuilder.cpp

92 ↗

(On Diff #10160)

An alternative fix would be to check D->isUsed() instead of checking for UsedAttr here. That'd avoid the locality degradation and memory allocation here.

Or just remove this check and unconditionally call EmitTopLevelDecl? That would better match what we do for out-of-line method definitions, and would avoid duplicating this between here and ASTContext::DeclMustBeEmitted.

test/CodeGenCXX/attr-used.cpp

21 ↗

(On Diff #10160)

Typo "othersie"

[hans]

lib/CodeGen/ModuleBuilder.cpp

92 ↗

(On Diff #10160)

Checking D->isUsed() certainly sounds simpler :-) I'll try that out.

We can't unconditionally call EmitTopLevelDecl, because it will call DeclMustBeEmitted, which might calculate the gva linkage, and then we fail this test from SemaCXX/undefined-internal.cpp :(

namespace test7 {
  typedef struct {
    void bar();
    void foo() { bar(); }
  } A;
}

I suspect that test doesn't work well with the "used" attribute either.

test/CodeGenCXX/attr-used.cpp

21 ↗

(On Diff #10160)

Done.

[rsmith]

LGTM

lib/CodeGen/ModuleBuilder.cpp

92 ↗

(On Diff #10160)

Arrgh! That's horrible, but it does justify delaying the emission to end of TU. The existing code is wrong for this case even without checking 'used'. A comment here mentioning this case would be useful for future archaeologists.

[hans]

lib/CodeGen/ModuleBuilder.cpp

92 ↗

(On Diff #10160)

The current patch delays emission to the end of the current top level declaration. Is that OK, or are you saying we need to delay all the way to the end of the TU?

[hans]

Closed by commit rL210356 (authored by @hans).

cfe/trunk/lib/CodeGen/ModuleBuilder.cpp

public:
 
     bool HandleTopLevelDecl(DeclGroupRef DG) override {
       if (Diags.hasErrorOccurred())
         return true;
 
       // Make sure to emit all elements of a Decl.
       for (DeclGroupRef::iterator I = DG.begin(), E = DG.end(); I != E; ++I)
         Builder->EmitTopLevelDecl(*I);
+
+      // Emit any deferred inline method definitions.
+      for (CXXMethodDecl *MD : DeferredInlineMethodDefinitions)
+        Builder->EmitTopLevelDecl(MD);
+      DeferredInlineMethodDefinitions.clear();
+
       return true;
     }
 
     void HandleInlineMethodDefinition(CXXMethodDecl *D) override {
       if (Diags.hasErrorOccurred())
         return;
 
       assert(D->doesThisDeclarationHaveABody());
 
-      // We may have member functions that need to be emitted at this point.
-      if (!D->isDependentContext() &&
-          (D->hasAttr<UsedAttr>() || D->hasAttr<ConstructorAttr>() ||
-           D->hasAttr<DLLExportAttr>())) {
-        Builder->EmitTopLevelDecl(D);
-      }
+      // We may want to emit this definition. However, that decision might be
+      // based on computing the linkage, and we have to defer that in case we
+      // are inside of something that will change the method's final linkage,
+      // e.g.
+      //   typedef struct {
+      //     void bar();
+      //     void foo() { bar(); }
+      //   } A;
+      DeferredInlineMethodDefinitions.push_back(D);
     }
 
     /// HandleTagDeclDefinition - This callback is invoked each time a TagDecl
     /// to (e.g. struct, union, enum, class) is completed. This allows the
     /// client hack on the type, which can occur at any point in the file
     /// (because these can be defined in declspecs).
     void HandleTagDeclDefinition(TagDecl *D) override {
       if (Diags.hasErrorOccurred())
public:
     void HandleDetectMismatch(llvm::StringRef Name,
                               llvm::StringRef Value) override {
       Builder->AddDetectMismatch(Name, Value);
     }
 
     void HandleDependentLibrary(llvm::StringRef Lib) override {
       Builder->AddDependentLib(Lib);
     }
+
+  private:
+    std::vector<CXXMethodDecl *> DeferredInlineMethodDefinitions;
   };
 }
 
 void CodeGenerator::anchor() { }
 
 CodeGenerator *clang::CreateLLVMCodeGen(DiagnosticsEngine &Diags,
                                         const std::string& ModuleName,
                                         const CodeGenOptions &CGO,
                                         const TargetOptions &/*TO*/,
                                         llvm::LLVMContext& C) {
   return new CodeGeneratorImpl(Diags, ModuleName, CGO, C);
 }

cfe/trunk/test/CodeGenCXX/attr-used.cpp

 
 // PR19743: not emitting __attribute__((used)) inline methods in nested classes.
 struct X1 {
   struct Nested {
     // CHECK: define linkonce_odr {{.*}} @_ZN2X16Nested1fEv
     void __attribute__((used)) f() {}
   };
 };
+
+struct X2 {
+  // We must delay emission of bar() until foo() has had its body parsed,
+  // otherwise foo() would not be emitted.
+  void __attribute__((used)) bar() { foo(); }
+  void foo() { }
+
+  // CHECK: define linkonce_odr {{.*}} @_ZN2X23barEv
+  // CHECK: define linkonce_odr {{.*}} @_ZN2X23fooEv
+};

cfe/trunk/test/CodeGenCXX/dllexport-members.cpp

struct ExportMembers {
   // G32-DAG: define          dllexport x86_thiscallcc void @_ZN13ExportMembers9normalDefEv(%struct.ExportMembers* %this)
   // G64-DAG: define          dllexport                void @_ZN13ExportMembers9normalDefEv(%struct.ExportMembers* %this)
   // G32-DAG: define weak_odr dllexport x86_thiscallcc void @_ZN13ExportMembers13normalInclassEv(%struct.ExportMembers* %this)
   // G64-DAG: define weak_odr dllexport                void @_ZN13ExportMembers13normalInclassEv(%struct.ExportMembers* %this)
   // G32-DAG: define weak_odr dllexport x86_thiscallcc void @_ZN13ExportMembers15normalInlineDefEv(%struct.ExportMembers* %this)
   // G64-DAG: define weak_odr dllexport                void @_ZN13ExportMembers15normalInlineDefEv(%struct.ExportMembers* %this)
   // G32-DAG: define weak_odr dllexport x86_thiscallcc void @_ZN13ExportMembers16normalInlineDeclEv(%struct.ExportMembers* %this)
   // G64-DAG: define weak_odr dllexport                void @_ZN13ExportMembers16normalInlineDeclEv(%struct.ExportMembers* %this)
+  // M32-DAG: define linkonce_odr       x86_thiscallcc void @"\01?referencedNonExportedInClass@ExportMembers@@QAEXXZ"
   __declspec(dllexport)                void normalDef();
-  __declspec(dllexport)                void normalInclass() {}
+  __declspec(dllexport)                void normalInclass() { referencedNonExportedInClass(); }
   __declspec(dllexport)                void normalInlineDef();
   __declspec(dllexport)         inline void normalInlineDecl();
+                                       void referencedNonExportedInClass() {}
 
   // M32-DAG: define          dllexport x86_thiscallcc void @"\01?virtualDef@ExportMembers@@UAEXXZ"(%struct.ExportMembers* %this)
   // M64-DAG: define          dllexport                void @"\01?virtualDef@ExportMembers@@UEAAXXZ"(%struct.ExportMembers* %this)
   // M32-DAG: define weak_odr dllexport x86_thiscallcc void @"\01?virtualInclass@ExportMembers@@UAEXXZ"(%struct.ExportMembers* %this)
   // M64-DAG: define weak_odr dllexport                void @"\01?virtualInclass@ExportMembers@@UEAAXXZ"(%struct.ExportMembers* %this)
   // M32-DAG: define weak_odr dllexport x86_thiscallcc void @"\01?virtualInlineDef@ExportMembers@@UAEXXZ"(%struct.ExportMembers* %this)
   // M64-DAG: define weak_odr dllexport                void @"\01?virtualInlineDef@ExportMembers@@UEAAXXZ"(%struct.ExportMembers* %this)
   // M32-DAG: define weak_odr dllexport x86_thiscallcc void @"\01?virtualInlineDecl@ExportMembers@@UAEXXZ"(%struct.ExportMembers* %this)

cfe/trunk/test/CodeGenCXX/dllexport.cpp

 }
 
 struct __declspec(dllexport) DefaultedCtorsDtors {
   DefaultedCtorsDtors() = default;
   // M32-DAG: define weak_odr dllexport x86_thiscallcc %struct.DefaultedCtorsDtors* @"\01??0DefaultedCtorsDtors@@QAE@XZ"
   ~DefaultedCtorsDtors() = default;
   // M32-DAG: define weak_odr dllexport x86_thiscallcc void @"\01??1DefaultedCtorsDtors@@QAE@XZ"
 };
+
+namespace ReferencedInlineMethodInNestedClass {
+  struct __declspec(dllexport) S {
+    void foo() {
+      t->bar();
+    }
+    struct T {
+      void bar() {}
+    };
+    T *t;
+  };
+  // M32-DAG: define weak_odr dllexport x86_thiscallcc void @"\01?foo@S@ReferencedInlineMethodInNestedClass@@QAEXXZ"
+  // M32-DAG: define linkonce_odr x86_thiscallcc void @"\01?bar@T@S@ReferencedInlineMethodInNestedClass@@QAEXXZ"
+}