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

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

Typo "othersie"

[hans]

lib/CodeGen/ModuleBuilder.cpp

92

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

Done.

[rsmith]

LGTM

lib/CodeGen/ModuleBuilder.cpp

92

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

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).

lib/CodeGen/CodeGenModule.h

struct DeferredGlobal {
     llvm::AssertingVH<llvm::GlobalValue> GV;
     GlobalDecl GD;
   };
   std::vector<DeferredGlobal> DeferredDeclsToEmit;
   void addDeferredDeclToEmit(llvm::GlobalValue *GV, GlobalDecl GD) {
     DeferredDeclsToEmit.push_back(DeferredGlobal(GV, GD));
   }
 
+  std::vector<Decl*> DeferredTopLevelDeclsToEmit;
+
   /// List of alias we have emitted. Used to make sure that what they point to
   /// is defined once we get to the end of the of the translation unit.
   std::vector<GlobalDecl> Aliases;
 
   typedef llvm::StringMap<llvm::TrackingVH<llvm::Constant> > ReplacementsTy;
   ReplacementsTy Replacements;
 
   /// A queue of (optional) vtables to consider emitting.
public:
   llvm::Value *getBuiltinLibFunction(const FunctionDecl *FD,
                                      unsigned BuiltinID);
 
   llvm::Function *getIntrinsic(unsigned IID, ArrayRef<llvm::Type*> Tys = None);
 
   /// Emit code for a single top level declaration.
   void EmitTopLevelDecl(Decl *D);
 
+  /// Add a top level decl to be emitted at the end of codegen.
+  void AddDeferredTopLevelDeclToEmit(Decl* D) {
+    DeferredTopLevelDeclsToEmit.push_back(D);
+  }
+
   /// Tell the consumer that this variable has been instantiated.
   void HandleCXXStaticMemberVarInstantiation(VarDecl *VD);
 
   /// \brief If the declaration has internal linkage but is inside an
   /// extern "C" linkage specification, prepare to emit an alias for it
   /// to the expected name.
   template<typename SomeDecl>
   void MaybeHandleStaticInExternC(const SomeDecl *D, llvm::GlobalValue *GV);
public:
   void addDeferredVTable(const CXXRecordDecl *RD) {
     DeferredVTables.push_back(RD);
   }
 
   /// Emit code for a singal global function or var decl. Forward declarations
   /// are emitted lazily.
   void EmitGlobal(GlobalDecl D);
 
+  void AddDeferredFunctionDefinition(FunctionDecl *D);
+
 private:
   llvm::GlobalValue *GetGlobalValue(StringRef Ref);
 
   llvm::Constant *
   GetOrCreateLLVMFunction(StringRef MangledName, llvm::Type *Ty, GlobalDecl D,
                           bool ForVTable, bool DontDefer = false,
                           llvm::AttributeSet ExtraAttrs = llvm::AttributeSet());
 

lib/CodeGen/CodeGenModule.cpp

for (std::vector<GlobalDecl>::iterator I = Aliases.begin(),
     auto *Alias = cast<llvm::GlobalAlias>(Entry);
     Alias->replaceAllUsesWith(llvm::UndefValue::get(Alias->getType()));
     Alias->eraseFromParent();
   }
 }
 
 void CodeGenModule::clear() {
   DeferredDeclsToEmit.clear();
+  DeferredTopLevelDeclsToEmit.clear();
 }
 
 void CodeGenModule::Release() {
   EmitDeferred();
   applyReplacements();
   checkAliases();
   EmitCXXGlobalInitFunc();
   EmitCXXGlobalDtorFunc();
void CodeGenModule::EmitModuleLinkOptions() {
 
   // Add the linker options metadata flag.
   getModule().addModuleFlag(llvm::Module::AppendUnique, "Linker Options",
                             llvm::MDNode::get(getLLVMContext(),
                                               LinkerOptionsMetadata));
 }
 
 void CodeGenModule::EmitDeferred() {
+  for (Decl *D : DeferredTopLevelDeclsToEmit)
+    EmitTopLevelDecl(D);
+
   // Emit code for any potentially referenced deferred decls.  Since a
   // previously unused static decl may become used during the generation of code
   // for a static function, iterate until no changes are made.
-
   while (true) {
     if (!DeferredVTables.empty()) {
       EmitDeferredVTables();
 
       // Emitting a v-table doesn't directly cause more v-tables to
       // become deferred, although it can cause functions to be
       // emitted that then need those v-tables.
       assert(DeferredVTables.empty());

lib/CodeGen/ModuleBuilder.cpp

public:
     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);
+        // Defer emission to make sure that all other members, members of nested
+        // classes, etc. have been parsed before doing codegen for this method,
+        // in case it references any of them.
+        Builder->AddDeferredTopLevelDeclToEmit(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 {

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,
+  // othersie foo() would not be emitted.
+  void __attribute__((used)) bar() { foo(); }
+  void foo() { }
+
+  // CHECK: define linkonce_odr {{.*}} @_ZN2X23barEv
+  // CHECK: define linkonce_odr {{.*}} @_ZN2X23fooEv
+};

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)

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"
+}