Fix issue #1712

gen/declarations.cpp

@@ -206,10 +206,15 @@ class CodegenVisitor : public Visitor {
     }
 
     // Define the __initZ symbol.
-    IrAggr *ir = getIrAggr(decl);
-    llvm::GlobalVariable *initZ = ir->getInitSymbol();
-    initZ->setInitializer(ir->getDefaultInit());
-    setLinkage(decl, initZ);
+    // If we reach here during codegen of an available_externally function,
+    // the struct initializer should stay external and therefore must not
+    // have an initializer.
+    if ((decl->getModule() == gIR->dmodule) || decl->isInstantiated()) {
+      IrAggr *ir = getIrAggr(decl);
+      llvm::GlobalVariable *initZ = ir->getInitSymbol();
+      initZ->setInitializer(ir->getDefaultInit());
+      setLinkage(decl, initZ);
+    }
 
     // emit typeinfo
     DtoTypeInfoOf(decl->type);
@@ -341,7 +346,8 @@ class CodegenVisitor : public Visitor {
       // If we reach here during codegen of an available_externally function,
       // new variable declarations should stay external and therefore must not
       // have an initializer.
-      if (!(decl->storage_class & STCextern) && !decl->inNonRoot()) {
+      if (!(decl->storage_class & STCextern) &&
+          ((decl->getModule() == gIR->dmodule) || decl->isInstantiated())) {
         // Build the initializer. Might use this->ir.irGlobal->value!
         LLConstant *initVal =
             DtoConstInitializer(decl->loc, decl->type, decl->_init);

gen/function-inlining.cpp

@@ -143,18 +143,21 @@ bool defineAsExternallyAvailable(FuncDeclaration &fdecl) {
     return false;
   }
 
-  if (fdecl.semanticRun >= PASSsemantic3) {
+  if ((fdecl.inlining != PINLINEalways) && fdecl.semanticRun >= PASSsemantic3) {
     // If semantic analysis has come this far, the function will be defined
     // elsewhere and should not get the available_externally attribute from
-    // here.
+    // here. However, pragma(inline, true) functions should remain
+    // defineAsExternallyAvailable candidates to fix GH #1712.
     // TODO: This check prevents inlining of nested functions.
     IF_LOG Logger::println("Semantic analysis already completed");
     return false;
   }
 
-  if (alreadyOrWillBeDefined(fdecl)) {
+  if ((fdecl.inlining != PINLINEalways) && alreadyOrWillBeDefined(fdecl)) {
     // This check is needed because of ICEs happening because of unclear issues
     // upon changing the codegen order without this check.
+    // pragma(inline, true) functions remain defineAsExternallyAvailable
+    // candidates to fix GH #1712.
     IF_LOG Logger::println("Function will be defined later.");
     return false;
   }
@@ -170,6 +173,7 @@ bool defineAsExternallyAvailable(FuncDeclaration &fdecl) {
 
   IF_LOG Logger::println("Potential inlining candidate");
 
+  if (fdecl.semanticRun < PASSsemantic3)
   {
     IF_LOG Logger::println("Do semantic analysis");
     LOG_SCOPE

gen/functions.cpp

@@ -325,7 +325,9 @@ static llvm::Function *DtoDeclareVaFunction(FuncDeclaration *fdecl) {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-void DtoResolveFunction(FuncDeclaration *fdecl) {
+/// When mayDefine is true, the call to this function may possibly define the
+/// function too (e.g. for cross-module inlining).
+void DtoResolveFunction(FuncDeclaration *fdecl, bool mayDefine) {
   if ((!global.params.useUnitTests || !fdecl->type) &&
       fdecl->isUnitTestDeclaration()) {
     IF_LOG Logger::println("Ignoring unittest %s", fdecl->toPrettyChars());
@@ -404,7 +406,7 @@ void DtoResolveFunction(FuncDeclaration *fdecl) {
 
   // queue declaration unless the function is abstract without body
   if (!fdecl->isAbstract() || fdecl->fbody) {
-    DtoDeclareFunction(fdecl);
+    DtoDeclareFunction(fdecl, mayDefine);
   }
 }
 
@@ -434,8 +436,10 @@ void applyDefaultMathAttributes(IrFunction *irFunc) {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-void DtoDeclareFunction(FuncDeclaration *fdecl) {
-  DtoResolveFunction(fdecl);
+/// When mayDefine is true, the call to this function may possibly define the
+/// function too (e.g. for cross-module inlining).
+void DtoDeclareFunction(FuncDeclaration *fdecl, bool mayDefine) {
+  DtoResolveFunction(fdecl, mayDefine);
 
   if (fdecl->ir->isDeclared()) {
     return;
@@ -462,7 +466,8 @@ void DtoDeclareFunction(FuncDeclaration *fdecl) {
   // Check if fdecl should be defined too for cross-module inlining.
   // If true, semantic is fully done for fdecl which is needed for some code
   // below (e.g. code that uses fdecl->vthis).
-  const bool defineAtEnd = defineAsExternallyAvailable(*fdecl);
+  const bool defineAtEnd =
+      mayDefine && defineAsExternallyAvailable(*fdecl);
   if (defineAtEnd) {
     IF_LOG Logger::println(
         "Function is an externally_available inline candidate.");
@@ -742,14 +747,24 @@ void DtoDefineFunction(FuncDeclaration *fd, bool linkageAvailableExternally) {
                          fd->loc.toChars());
   LOG_SCOPE;
   if (linkageAvailableExternally) {
-    IF_LOG Logger::println("linkageAvailableExternally = true");
+    IF_LOG Logger::println("linkageAvailableExternally == true");
+  }
+
+  if ((fd->getModule() != gIR->dmodule) && !fd->isInstantiated()) {
+    // The function is being emitted cross-module for inlining. We reach here
+    // for nested functions inside cross-module emitted functions. We have to
+    // explicitly set `linkageAvailableExternally` to true.
+    linkageAvailableExternally = true;
+    IF_LOG Logger::println("Cross-module emitted nested function: "
+                           "linkageAvailableExternally = true");
   }
 
   if (fd->ir->isDefined()) {
     if (!linkageAvailableExternally &&
         (getIrFunc(fd)->func->getLinkage() ==
          llvm::GlobalValue::AvailableExternallyLinkage)) {
       // Fix linkage
+      IF_LOG Logger::println("Remove available_externally linkage.");
       const auto lwc = lowerFuncLinkage(fd);
       setLinkage(lwc, getIrFunc(fd)->func);
     }
@@ -777,7 +792,7 @@ void DtoDefineFunction(FuncDeclaration *fd, bool linkageAvailableExternally) {
     fatal();
   }
 
-  DtoResolveFunction(fd);
+  DtoResolveFunction(fd, false);
 
   if (fd->isUnitTestDeclaration() && !global.params.useUnitTests) {
     IF_LOG Logger::println("No code generation for unit test declaration %s",
@@ -801,7 +816,7 @@ void DtoDefineFunction(FuncDeclaration *fd, bool linkageAvailableExternally) {
     return;
   }
 
-  DtoDeclareFunction(fd);
+  DtoDeclareFunction(fd, false);
   assert(fd->ir->isDeclared());
 
   // DtoResolveFunction might also set the defined flag for functions we

gen/functions.h

@@ -34,8 +34,8 @@ llvm::FunctionType *DtoFunctionType(Type *t, IrFuncTy &irFty, Type *thistype,
                                     bool hasSel = false);
 llvm::FunctionType *DtoFunctionType(FuncDeclaration *fdecl);
 
-void DtoResolveFunction(FuncDeclaration *fdecl);
-void DtoDeclareFunction(FuncDeclaration *fdecl);
+void DtoResolveFunction(FuncDeclaration *fdecl, bool mayDefine = true);
+void DtoDeclareFunction(FuncDeclaration *fdecl, bool mayDefine = true);
 void DtoDefineFunction(FuncDeclaration *fd, bool linkageAvailableExternally = false);
 
 void DtoDefineNakedFunction(FuncDeclaration *fd);

tests/codegen/inlining_gh1712.d

@@ -0,0 +1,46 @@
+// Test inlining related to Github issue 1712
+
+// REQUIRES: atleast_llvm307
+
+// RUN: %ldc %s -c -output-ll -O3 -of=%t.ll && FileCheck %s < %t.ll
+// RUN: %ldc -O3 -run %s
+
+module mod;
+
+void test9785_2()
+{
+    int j = 3;
+
+    void loop(scope const void function(int x) dg)
+    {
+        pragma(inline, true);
+        dg(++j);
+    }
+
+    static void func(int x)
+    {
+        pragma(inline, true);
+        assert(x == 4);
+    }
+
+    loop(&func);
+}
+
+void main()
+{
+    test9785_2();
+}
+
+// There was a bug where pragma(inline, true) nested functions were incorrectly emitted with `available_externally` linkage.
+
+// CHECK: define
+// CHECK-NOT: available_externally
+// CHECK-SAME: @{{.*}}D3mod10test9785_2FZv
+
+// CHECK: define
+// CHECK-NOT: available_externally
+// CHECK-SAME: @{{.*}}D3mod10test9785_2FZ4loopMFMxPFiZvZv
+
+// CHECK: define
+// CHECK-NOT: available_externally
+// CHECK-SAME: @{{.*}}D3mod10test9785_2FZ4funcFiZv

tests/codegen/inlining_gh1712_originalbug.d

@@ -0,0 +1,15 @@
+// REQUIRES: atleast_llvm307
+
+// RUN: %ldc -output-ll -od=%T -I%S -O0 -release -enable-cross-module-inlining %s %S/inputs/inlining_gh1712_string.d \
+// RUN:   && FileCheck %s < %T/inlining_gh1712_originalbug.ll
+
+import inputs.inlinables;
+
+void main()
+{
+    call_template_foo(1);
+}
+
+// Make sure that the pragma(inline, true) function `call_template_foo` is defined, and not just declared.
+// When it is correctly defined, optimization (available_externally+alwaysinline) even at -O0 means that it will dissappear from IR.
+// CHECK-NOT: declare {{.*}} @call_template_foo

tests/codegen/inlining_staticvar.d

@@ -4,6 +4,12 @@
 
 // RUN: %ldc %s -I%S -c -output-ll                  -O3 -of=%t.O3.ll && FileCheck %s --check-prefix OPT3 < %t.O3.ll
 // RUN: %ldc %s -I%S -c -output-ll -enable-inlining -O0 -of=%t.O0.ll && FileCheck %s --check-prefix OPT0 < %t.O0.ll
+
+// Test linkage with separate compilation.
+// RUN: %ldc -c %S/inputs/inlinables_staticvar.d -of=%t.import%obj \
+// RUN:   && %ldc -I%S -enable-inlining %t.import%obj -run %s
+
+// Test linkage with non-singleobj build.
 // RUN: %ldc -I%S -enable-inlining %S/inputs/inlinables_staticvar.d -run %s
 // RUN: %ldc -I%S -O3              %S/inputs/inlinables_staticvar.d -run %s
 
@@ -55,6 +61,16 @@ void checkNestedStruct_2() @weak
     assert(addAndCheckNestedStruct(7+101, 9));
 }
 
+void checkTemplatedNestedStruct_1() @weak
+{
+    assert(addAndCheckTemplatedNestedStruct(0, 7));
+}
+void checkTemplatedNestedStruct_2() @weak
+{
+    assert(addAndCheckTemplatedNestedStructIndirect(7, 101));
+    assert(addAndCheckTemplatedNestedStruct(7+101, 9));
+}
+
 // OPT0-LABEL: define{{.*}} @_Dmain(
 // OPT3-LABEL: define{{.*}} @_Dmain(
 extern(D)
@@ -68,4 +84,6 @@ void main()
     checkInsideNestedFunc_2();
     checkNestedStruct_1();
     checkNestedStruct_2();
+    checkTemplatedNestedStruct_1();
+    checkTemplatedNestedStruct_2();
 }

tests/codegen/inputs/inlinables_staticvar.d

@@ -83,3 +83,24 @@ pragma(inline, false) bool addAndCheckNestedStructIndirect(int checkbefore, int
 }
 
 /+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++/
+
+pragma(inline, true) bool addAndCheckTemplatedNestedStruct(T)(T checkbefore, T increment)
+{
+    struct NestedStruct(T)
+    {
+        static T structValue;
+    }
+
+    if (NestedStruct!T.structValue != checkbefore)
+        return false;
+
+    NestedStruct!T.structValue += increment;
+    return true;
+}
+
+pragma(inline, false) bool addAndCheckTemplatedNestedStructIndirect(T)(T checkbefore, T increment)
+{
+    return addAndCheckTemplatedNestedStruct!T(checkbefore, increment);
+}
+
+/+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++/

tests/codegen/inputs/inlining_gh1712_string.d

@@ -0,0 +1,6 @@
+import inputs.inlinables;
+
+void foo()
+{
+    call_template_foo(1);
+}