From 6eaa8323a887a878f54dfcc65d4fff72c0f5c161 Mon Sep 17 00:00:00 2001 From: Hans Wennborg Date: Fri, 28 Aug 2015 21:47:01 +0000 Subject: [PATCH] Allow TLS vars in dllimport/export functions; only inline dllimport functions when safe (PR24593) This patch does two things: 1) Don't error about dllimport/export on thread-local static local variables. We put those attributes on static locals in dllimport/export functions implicitly in case the function gets inlined. Now, for TLS variables this is a problem because we can't import such variables, but it's a benign problem becase: 2) Make sure we never inline a dllimport function TLS static locals. In fact, never inline a dllimport function that references a non-imported function or variable (because these are not defined in the importing library). This seems to match MSVC's behaviour. Differential Revision: http://reviews.llvm.org/D12422 llvm-svn: 246338 --- clang/lib/CodeGen/CodeGenModule.cpp | 38 ++++++++++++++++++++++++++ clang/lib/Sema/SemaDecl.cpp | 14 +++++++--- clang/test/CodeGenCXX/dllimport.cpp | 42 ++++++++++++++++++++++++++++- clang/test/SemaCXX/dllexport.cpp | 9 +++++++ clang/test/SemaCXX/dllimport.cpp | 12 +++++++++ 5 files changed, 111 insertions(+), 4 deletions(-) diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index 05258693f1f44..847cb5715239e 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -1448,6 +1448,35 @@ namespace { return true; } }; + + struct DLLImportFunctionVisitor + : public RecursiveASTVisitor { + 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(VD)) + SafeToInline = VD->hasAttr(); + else if (VarDecl *V = dyn_cast(VD)) + SafeToInline = !V->hasGlobalStorage() || V->hasAttr(); + return SafeToInline; + } + bool VisitCXXDeleteExpr(CXXDeleteExpr *E) { + SafeToInline = E->getOperatorDelete()->hasAttr(); + return SafeToInline; + } + bool VisitCXXNewExpr(CXXNewExpr *E) { + SafeToInline = E->getOperatorNew()->hasAttr(); + return SafeToInline; + } + }; } // isTriviallyRecursive - Check if this function calls another @@ -1478,6 +1507,15 @@ CodeGenModule::shouldEmitFunction(GlobalDecl GD) { const auto *F = cast(GD.getDecl()); if (CodeGenOpts.OptimizationLevel == 0 && !F->hasAttr()) return false; + + if (F->hasAttr()) { + // Check whether it would be safe to inline this dllimport function. + DLLImportFunctionVisitor Visitor; + Visitor.TraverseFunctionDecl(const_cast(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 diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index c94d2dd5a9e6b..1fe6a5c18f1c7 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -9967,9 +9967,17 @@ Sema::FinalizeDeclaration(Decl *ThisDecl) { // dllimport/dllexport variables cannot be thread local, their TLS index // isn't exported with the variable. if (DLLAttr && VD->getTLSKind()) { - Diag(VD->getLocation(), diag::err_attribute_dll_thread_local) << VD - << DLLAttr; - VD->setInvalidDecl(); + FunctionDecl *F = dyn_cast(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()) { diff --git a/clang/test/CodeGenCXX/dllimport.cpp b/clang/test/CodeGenCXX/dllimport.cpp index c87d959c5b357..b9c850b8b87b1 100644 --- a/clang/test/CodeGenCXX/dllimport.cpp +++ b/clang/test/CodeGenCXX/dllimport.cpp @@ -86,7 +86,7 @@ USEVAR(GlobalRedecl3) 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() { @@ -314,6 +314,46 @@ void UNIQ(use)() { ::operator new(42); } 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) //===----------------------------------------------------------------------===// diff --git a/clang/test/SemaCXX/dllexport.cpp b/clang/test/SemaCXX/dllexport.cpp index badb9e259597c..c3f26aa315553 100644 --- a/clang/test/SemaCXX/dllexport.cpp +++ b/clang/test/SemaCXX/dllexport.cpp @@ -71,6 +71,15 @@ __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() { diff --git a/clang/test/SemaCXX/dllimport.cpp b/clang/test/SemaCXX/dllimport.cpp index 2fa10756da48f..f17c2e22414c0 100644 --- a/clang/test/SemaCXX/dllimport.cpp +++ b/clang/test/SemaCXX/dllimport.cpp @@ -93,6 +93,18 @@ __declspec(dllimport) auto InternalAutoTypeGlobal = Internal(); // expected-erro // 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}}