Allow TLS vars in dllimport/export functions; only inline dllimport functions when safe (PR24593)

[hans]

Summary

This seems to match the MS behaviour.

[majnemer]

LGTM

We should probably do something about lambdas, they can't be imported or exported but this can be done in a follow up patch.

[hans]

In D12422#235509, @majnemer wrote:

We should probably do something about lambdas, they can't be imported or exported but this can be done in a follow up patch.

Follow-up in r247718.

lib/CodeGen/CodeGenModule.cpp

bool TraverseCallExpr(CallExpr *E) {
       if (BuiltinName.startswith("__builtin_") &&
           Name == BuiltinName.slice(strlen("__builtin_"), StringRef::npos)) {
         Result = true;
         return false;
       }
       return true;
     }
   };
+
+  struct DLLImportFunctionVisitor
+      : public RecursiveASTVisitor<DLLImportFunctionVisitor> {
+    bool SafeToInline = true;
+
+    bool VisitVarDecl(VarDecl *VD) {
+      // A thread-local variable cannot be imported.
+      SafeToInline = !VD->getTLSKind();
+      return SafeToInline;
+    }
+
+    // Make sure we're not referencing non-imported vars or functions.
+    bool VisitDeclRefExpr(DeclRefExpr *E) {
+      ValueDecl *VD = E->getDecl();
+      if (isa<FunctionDecl>(VD))
+        SafeToInline = VD->hasAttr<DLLImportAttr>();
+      else if (VarDecl *V = dyn_cast<VarDecl>(VD))
+        SafeToInline = !V->hasGlobalStorage() || V->hasAttr<DLLImportAttr>();
+      return SafeToInline;
+    }
+    bool VisitCXXDeleteExpr(CXXDeleteExpr *E) {
+      SafeToInline = E->getOperatorDelete()->hasAttr<DLLImportAttr>();
+      return SafeToInline;
+    }
+    bool VisitCXXNewExpr(CXXNewExpr *E) {
+      SafeToInline = E->getOperatorNew()->hasAttr<DLLImportAttr>();
+      return SafeToInline;
+    }
+  };
 }
 
 // isTriviallyRecursive - Check if this function calls another
 // decl that, because of the asm attribute or the other decl being a builtin,
 // ends up pointing to itself.
 bool
 CodeGenModule::isTriviallyRecursive(const FunctionDecl *FD) {
   StringRef Name;

 
 bool
 CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
   if (getFunctionLinkage(GD) != llvm::Function::AvailableExternallyLinkage)
     return true;
   const auto *F = cast<FunctionDecl>(GD.getDecl());
   if (CodeGenOpts.OptimizationLevel == 0 && !F->hasAttr<AlwaysInlineAttr>())
     return false;
+
+  if (F->hasAttr<DLLImportAttr>()) {
+    // Check whether it would be safe to inline this dllimport function.
+    DLLImportFunctionVisitor Visitor;
+    Visitor.TraverseFunctionDecl(const_cast<FunctionDecl*>(F));
+    if (!Visitor.SafeToInline)
+      return false;
+  }
+
   // PR9614. Avoid cases where the source code is lying to us. An available
   // externally function should have an equivalent function somewhere else,
   // but a function that calls itself is clearly not equivalent to the real
   // implementation.
   // This happens in glibc's btowc and in some configure checks.
   return !isTriviallyRecursive(F);
 }
 

lib/Sema/SemaDecl.cpp

if (VD->isStaticDataMember() && VD->isOutOfLine() &&
       if (!IsClassTemplateMember)
         VD->setInvalidDecl();
     }
   }
 
   // dllimport/dllexport variables cannot be thread local, their TLS index
   // isn't exported with the variable.
   if (DLLAttr && VD->getTLSKind()) {
+    FunctionDecl *F = dyn_cast<FunctionDecl>(VD->getDeclContext());
+    if (F && getDLLAttr(F)) {
+      assert(VD->isStaticLocal());
+      // But if this is a static local in a dlimport/dllexport function, the
+      // function will never be inlined, which means the var would never be
+      // imported, so having it marked import/export is safe.
+    } else {
       Diag(VD->getLocation(), diag::err_attribute_dll_thread_local) << VD
                                                                     << DLLAttr;
       VD->setInvalidDecl();
     }
+  }
 
   if (UsedAttr *Attr = VD->getAttr<UsedAttr>()) {
     if (!Attr->isInherited() && !VD->isThisDeclarationADefinition()) {
       Diag(Attr->getLocation(), diag::warn_attribute_ignored) << Attr;
       VD->dropAttr<UsedAttr>();
     }
   }
 

test/CodeGenCXX/dllimport.cpp

__declspec(dllimport) extern int GlobalRedecl3;
                       extern int GlobalRedecl3; // dllimport ignored
 USEVAR(GlobalRedecl3)
 
 // MSC-DAG: @"\01?ExternalGlobal@ns@@3HA" = external dllimport global i32
 // GNU-DAG: @_ZN2ns14ExternalGlobalE      = external dllimport global i32
 namespace ns { __declspec(dllimport) int ExternalGlobal; }
 USEVAR(ns::ExternalGlobal)
 
-int f();
+int __declspec(dllimport) f();
 // MO1-DAG: @"\01?x@?1??inlineStaticLocalsFunc@@YAHXZ@4HA" = available_externally dllimport global i32 0
 // MO1-DAG: @"\01??_B?1??inlineStaticLocalsFunc@@YAHXZ@51" = available_externally dllimport global i32 0
 inline int __declspec(dllimport) inlineStaticLocalsFunc() {
   static int x = f();
   return x++;
 };
 USE(inlineStaticLocalsFunc);
 

 __declspec(dllimport) void* operator new(__SIZE_TYPE__ n);
 void UNIQ(use)() { ::operator new(42); }
 
 // MSC-DAG: declare dllimport void @"\01?externalFunc@ns@@YAXXZ"()
 // GNU-DAG: declare dllimport void @_ZN2ns12externalFuncEv()
 namespace ns { __declspec(dllimport) void externalFunc(); }
 USE(ns::externalFunc)
 
+// A dllimport function referencing non-imported vars or functions must not be available_externally.
+__declspec(dllimport) int ImportedVar;
+int NonImportedVar;
+__declspec(dllimport) int ImportedFunc();
+int NonImportedFunc();
+__declspec(dllimport) inline int ReferencingImportedVar() { return ImportedVar; }
+// MO1-DAG: define available_externally dllimport i32 @"\01?ReferencingImportedVar@@YAHXZ"
+__declspec(dllimport) inline int ReferencingNonImportedVar() { return NonImportedVar; }
+// MO1-DAG: declare dllimport i32 @"\01?ReferencingNonImportedVar@@YAHXZ"()
+__declspec(dllimport) inline int ReferencingImportedFunc() { return ImportedFunc(); }
+// MO1-DAG: define available_externally dllimport i32 @"\01?ReferencingImportedFunc@@YAHXZ"
+__declspec(dllimport) inline int ReferencingNonImportedFunc() { return NonImportedFunc(); }
+// MO1-DAG: declare dllimport i32 @"\01?ReferencingNonImportedFunc@@YAHXZ"()
+USE(ReferencingImportedVar)
+USE(ReferencingNonImportedVar)
+USE(ReferencingImportedFunc)
+USE(ReferencingNonImportedFunc)
+// References to operator new and delete count too, despite not being DeclRefExprs.
+__declspec(dllimport) inline int *ReferencingNonImportedNew() { return new int[2]; }
+// MO1-DAG: declare dllimport i32* @"\01?ReferencingNonImportedNew@@YAPAHXZ"
+__declspec(dllimport) inline int *ReferencingNonImportedDelete() { delete (int*)nullptr; }
+// MO1-DAG: declare dllimport i32* @"\01?ReferencingNonImportedDelete@@YAPAHXZ"
+USE(ReferencingNonImportedNew)
+USE(ReferencingNonImportedDelete)
+__declspec(dllimport) void* operator new[](__SIZE_TYPE__);
+__declspec(dllimport) void operator delete(void*);
+__declspec(dllimport) inline int *ReferencingImportedNew() { return new int[2]; }
+// MO1-DAG: define available_externally dllimport i32* @"\01?ReferencingImportedNew@@YAPAHXZ"
+__declspec(dllimport) inline int *ReferencingImportedDelete() { delete (int*)nullptr; }
+// MO1-DAG: define available_externally dllimport i32* @"\01?ReferencingImportedDelete@@YAPAHXZ"
+USE(ReferencingImportedNew)
+USE(ReferencingImportedDelete)
+
+// A dllimport function with a TLS variable must not be available_externally.
+__declspec(dllimport) inline void FunctionWithTLSVar() { static __thread int x = 42; }
+// MO1-DAG: declare dllimport void @"\01?FunctionWithTLSVar@@YAXXZ"
+__declspec(dllimport) inline void FunctionWithNormalVar() { static int x = 42; }
+// MO1-DAG: define available_externally dllimport void @"\01?FunctionWithNormalVar@@YAXXZ"
+USE(FunctionWithTLSVar)
+USE(FunctionWithNormalVar)
 
 
 //===----------------------------------------------------------------------===//
 // Function templates
 //===----------------------------------------------------------------------===//
 
 // Import function template declaration.
 // MSC-DAG: declare dllimport void @"\01??$funcTmplDecl@UImplicitInst_Imported@@@@YAXXZ"()

test/SemaCXX/dllexport.cpp

 namespace    { __declspec(dllexport) int InternalGlobal; } // expected-error{{'(anonymous namespace)::InternalGlobal' must have external linkage when declared 'dllexport'}}
 namespace ns { __declspec(dllexport) int ExternalGlobal; }
 
 __declspec(dllexport) auto InternalAutoTypeGlobal = Internal(); // expected-error{{'InternalAutoTypeGlobal' must have external linkage when declared 'dllexport'}}
 __declspec(dllexport) auto ExternalAutoTypeGlobal = External();
 
 // Thread local variables are invalid.
 __declspec(dllexport) __thread int ThreadLocalGlobal; // expected-error{{'ThreadLocalGlobal' cannot be thread local when declared 'dllexport'}}
+inline void InlineWithThreadLocal() {
+  static __declspec(dllexport) __thread int ThreadLocalGlobal; // expected-error{{'ThreadLocalGlobal' cannot be thread local when declared 'dllexport'}}
+}
+
+// But if they're in a dllexport function, it's ok, because they will never get imported.
+inline void __declspec(dllexport) ExportedInlineWithThreadLocal() {
+  static __declspec(dllexport) __thread int OK1; // no-error
+  static __thread int OK2; // no-error
+}
 
 // Export in local scope.
 void functionScope() {
   __declspec(dllexport)        int LocalVarDecl; // expected-error{{'LocalVarDecl' must have external linkage when declared 'dllexport'}}
   __declspec(dllexport)        int LocalVarDef = 1; // expected-error{{'LocalVarDef' must have external linkage when declared 'dllexport'}}
   __declspec(dllexport) extern int ExternLocalVarDecl;
   __declspec(dllexport) static int StaticLocalVar; // expected-error{{'StaticLocalVar' must have external linkage when declared 'dllexport'}}
 }

test/SemaCXX/dllimport.cpp

 namespace    { __declspec(dllimport) int InternalGlobal; } // expected-error{{'(anonymous namespace)::InternalGlobal' must have external linkage when declared 'dllimport'}}
 namespace ns { __declspec(dllimport) int ExternalGlobal; }
 
 __declspec(dllimport) auto InternalAutoTypeGlobal = Internal(); // expected-error{{'InternalAutoTypeGlobal' must have external linkage when declared 'dllimport'}}
                                                                 // expected-error@-1{{definition of dllimport data}}
 
 // Thread local variables are invalid.
 __declspec(dllimport) __thread int ThreadLocalGlobal; // expected-error{{'ThreadLocalGlobal' cannot be thread local when declared 'dllimport'}}
+inline void InlineWithThreadLocal() {
+  static __declspec(dllimport) __thread int ThreadLocalGlobal; // expected-error{{'ThreadLocalGlobal' cannot be thread local when declared 'dllimport'}}
+}
+
+// But if they're in a dllimported function, it's OK because we will not inline the function.
+// This doesn't work on MinGW, because there, dllimport on the inline function is ignored.
+#ifndef GNU
+inline void __declspec(dllimport) ImportedInlineWithThreadLocal() {
+  static __declspec(dllimport) __thread int OK1; // no-error
+  static __thread int OK2; // no-error
+}
+#endif
 
 // Import in local scope.
 __declspec(dllimport) float LocalRedecl1; // expected-note{{previous declaration is here}}
 __declspec(dllimport) float LocalRedecl2; // expected-note{{previous declaration is here}}
 __declspec(dllimport) float LocalRedecl3; // expected-note{{previous declaration is here}}
 void functionScope() {
   __declspec(dllimport) int LocalRedecl1; // expected-error{{redeclaration of 'LocalRedecl1' with a different type: 'int' vs 'float'}}
   int *__attribute__((dllimport)) LocalRedecl2; // expected-error{{redeclaration of 'LocalRedecl2' with a different type: 'int *' vs 'float'}}