[clang-cl] Put dllexport attrs on static locals also in template instantiations (PR39496)

[hans]

Summary

In the course of D51340, @takuto.ikuta discovered that Clang fails to put dllexport/import attributes on static locals during template instantiation.

For regular functions, this happens in Sema::FinalizeDeclaration(), however for template instantiations we need to do something in or around TemplateDeclInstantiator::VisitVarDecl(). This patch does that, and extracts the code to a utility function.

Please take a look!

[rnk]

lgtm

test/CodeGenCXX/dllimport.cpp

1010

I notice that we don't emit foo as an available_externally definition right now. With your change, will we do so? Should we?

[takuto.ikuta]

LGTM, thank you for fix!

[hans]

test/CodeGenCXX/dllimport.cpp

1010

Ah, good point. It's actually the static local that was previously preventing us from emitting it available_externally. We normally would, but DLLImportFunctionVisitor would discover that the function referenced a non-dllimport "global" variable, and determine that it was not safe to emit the definition.

But now that the static local inherits the dll attribute, this works out automatically.

$ bin/clang -cc1 -triple i686-windows-msvc -fms-extensions -emit-llvm -std=c++1y -O1 -disable-llvm-passes -o - ../tools/clang/test/CodeGenCXX/dllimport.cpp -DMSABI -w | grep 'define.*?foo@?$T@H@pr39496'
define available_externally dllimport x86_thiscallcc i32 @"?foo@?$T@H@pr39496@@QAEHXZ"(%"struct.pr39496::T"* %this) #0 align 2 {

(Same for S<int>::foo() one.)

[hans]

Bah, I should have known something was going to break if I touched this :-)
http://lab.llvm.org:8011/builders/sanitizer-windows/builds/37132
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/1017

Here's a repro:

template <typename T> struct __declspec(dllimport) S {
  void foo() {
    static constexpr char src[] = {"hello"};
    T arr[sizeof(src)];
  }
};

void use() {
  S<int> s;
  s.foo();
}

$ bin/clang -target i686-pc-win32 -c /tmp/a.cc
/tmp/a.cc:4:17: error: invalid application of 'sizeof' to an incomplete type 'char const[]'
    T arr[sizeof(src)];
                ^~~~~
/tmp/a.cc:10:5: note: in instantiation of member function 'S<int>::foo' requested here
  s.foo();
    ^
1 error generated.

This is annoying. Looking into it now.

[hans]

r345709 should do it.

include/clang/Sema/Sema.h

public:
   void ActOnPureSpecifier(Decl *D, SourceLocation PureSpecLoc);
   void ActOnCXXForRangeDecl(Decl *D);
   StmtResult ActOnCXXForRangeIdentifier(Scope *S, SourceLocation IdentLoc,
                                         IdentifierInfo *Ident,
                                         ParsedAttributes &Attrs,
                                         SourceLocation AttrEnd);
   void SetDeclDeleted(Decl *dcl, SourceLocation DelLoc);
   void SetDeclDefaulted(Decl *dcl, SourceLocation DefaultLoc);
+  void CheckStaticLocalForDllExport(VarDecl *VD);
   void FinalizeDeclaration(Decl *D);
   DeclGroupPtrTy FinalizeDeclaratorGroup(Scope *S, const DeclSpec &DS,
                                          ArrayRef<Decl *> Group);
   DeclGroupPtrTy BuildDeclaratorGroup(MutableArrayRef<Decl *> Group);
 
   /// Should be called on all declarations that might have attached
   /// documentation comments.
   void ActOnDocumentableDecl(Decl *D);

lib/Sema/SemaDecl.cpp

static bool hasDependentAlignment(VarDecl *VD) {
   if (VD->getType()->isDependentType())
     return true;
   for (auto *I : VD->specific_attrs<AlignedAttr>())
     if (I->isAlignmentDependent())
       return true;
   return false;
 }
 
+/// Check if VD needs to be dllexport/dllimport due to being in a
+/// dllexport/import function.
+void Sema::CheckStaticLocalForDllExport(VarDecl *VD) {
+  assert(VD->isStaticLocal());
+
+  auto *FD = dyn_cast_or_null<FunctionDecl>(VD->getParentFunctionOrMethod());
+  if (!FD)
+    return;
+
+  // Static locals inherit dll attributes from their function.
+  if (Attr *A = getDLLAttr(FD)) {
+    auto *NewAttr = cast<InheritableAttr>(A->clone(getASTContext()));
+    NewAttr->setInherited(true);
+    VD->addAttr(NewAttr);
+  }
+}
+
 /// FinalizeDeclaration - called by ParseDeclarationAfterDeclarator to perform
 /// any semantic actions necessary after any initializer has been attached.
 void Sema::FinalizeDeclaration(Decl *ThisDecl) {
   // Note that we are no longer parsing the initializer for this declaration.
   ParsingInitForAutoVars.erase(ThisDecl);
 
   VarDecl *VD = dyn_cast_or_null<VarDecl>(ThisDecl);
   if (!VD)
if (VD->getTLSKind() && !hasDependentAlignment(VD) &&
         Diag(VD->getLocation(), diag::err_tls_var_aligned_over_maximum)
           << (unsigned)Context.getDeclAlign(VD).getQuantity() << VD
           << (unsigned)MaxAlignChars.getQuantity();
       }
     }
   }
 
   if (VD->isStaticLocal()) {
-    if (FunctionDecl *FD =
-            dyn_cast_or_null<FunctionDecl>(VD->getParentFunctionOrMethod())) {
-      // Static locals inherit dll attributes from their function.
-      if (Attr *A = getDLLAttr(FD)) {
-        auto *NewAttr = cast<InheritableAttr>(A->clone(getASTContext()));
-        NewAttr->setInherited(true);
-        VD->addAttr(NewAttr);
-      }
+    CheckStaticLocalForDllExport(VD);
+
+    if (dyn_cast_or_null<FunctionDecl>(VD->getParentFunctionOrMethod())) {
       // CUDA 8.0 E.3.9.4: Within the body of a __device__ or __global__
       // function, only __shared__ variables or variables without any device
       // memory qualifiers may be declared with static storage class.
       // Note: It is unclear how a function-scope non-const static variable
       // without device memory qualifier is implemented, therefore only static
       // const variable without device memory qualifier is allowed.
       [&]() {
         if (!getLangOpts().CUDA)

lib/Sema/SemaTemplateInstantiateDecl.cpp

if (Bindings)
     Var = DecompositionDecl::Create(SemaRef.Context, DC, D->getInnerLocStart(),
                                     D->getLocation(), DI->getType(), DI,
                                     D->getStorageClass(), *Bindings);
   else
     Var = VarDecl::Create(SemaRef.Context, DC, D->getInnerLocStart(),
                           D->getLocation(), D->getIdentifier(), DI->getType(),
                           DI, D->getStorageClass());
 
+  if (Var->isStaticLocal())
+    SemaRef.CheckStaticLocalForDllExport(Var);
+
   // In ARC, infer 'retaining' for variables of retainable type.
   if (SemaRef.getLangOpts().ObjCAutoRefCount &&
       SemaRef.inferObjCARCLifetime(Var))
     Var->setInvalidDecl();
 
   // Substitute the nested name specifier, if any.
   if (SubstQualifier(D, Var))
     return nullptr;

test/CodeGenCXX/dllexport.cpp

struct __declspec(dllexport) LayerTreeImpl {
   struct __declspec(dllexport) ElementLayers {
     LayerImpl *main = nullptr;
   };
   LayerSelection foo;
 };
 // M32-DAG: define weak_odr dso_local dllexport x86_thiscallcc %"struct.LayerTreeImpl::ElementLayers"* @"??0ElementLayers@LayerTreeImpl@@QAE@XZ"
 // M64-DAG: define weak_odr dso_local dllexport %"struct.LayerTreeImpl::ElementLayers"* @"??0ElementLayers@LayerTreeImpl@@QEAA@XZ"
 
+namespace pr39496 {
+// Make sure dll attribute are inherited by static locals also in template
+// specializations.
+template <typename> struct __declspec(dllexport) S { int foo() { static int x; return x++; } };
+int foo() { S<int> s; return s.foo(); }
+// MSC-DAG: @"?x@?{{1|2}}??foo@?$S@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
+
+template <typename> struct T { int foo() { static int x; return x++; } };
+template struct __declspec(dllexport) T<int>;
+// MSC-DAG: @"?x@?{{1|2}}??foo@?$T@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
+}
+
 class __declspec(dllexport) ACE_Shared_Object {
 public:
   virtual ~ACE_Shared_Object();
 };
 class __declspec(dllexport) ACE_Service_Object : public ACE_Shared_Object {};
 // Implicit move constructor declaration.
 // MSVC2015-DAG: define weak_odr dso_local dllexport {{.+}}ACE_Service_Object@@Q{{.+}}@$$Q
 // The declarations should not be exported.
 // MSVC2013-NOT: define weak_odr dso_local dllexport {{.+}}ACE_Service_Object@@Q{{.+}}@$$Q

test/CodeGenCXX/dllimport.cpp

 
 template <typename T> struct ExplicitInstantiationDeclTemplateBase2 { void func() {} };
 extern template struct ExplicitInstantiationDeclTemplateBase2<int>;
 struct __declspec(dllimport) DerivedFromExplicitInstantiationDeclTemplateBase2 : public ExplicitInstantiationDeclTemplateBase2<int> {};
 template struct __declspec(dllexport) ExplicitInstantiationDeclTemplateBase2<int>;
 USEMEMFUNC(ExplicitInstantiationDeclTemplateBase2<int>, func)
 // M32-DAG: declare dllimport x86_thiscallcc void @"?func@?$ExplicitInstantiationDeclTemplateBase2@H@@QAEXXZ"
 // G32-DAG: define weak_odr dso_local x86_thiscallcc void @_ZN38ExplicitInstantiationDeclTemplateBase2IiE4funcEv
+
+namespace pr39496 {
+// Make sure dll attribute are inherited by static locals also in template
+// specializations.
+template <typename> struct __declspec(dllimport) S { int foo() { static int x; return x++; } };
+int foo() { S<int> s; return s.foo(); }
+// MO1-DAG: @"?x@?{{1|2}}??foo@?$S@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = available_externally dllimport global i32 0, align 4
+
+template <typename> struct T { int foo() { static int x; return x++; } };
+extern template struct __declspec(dllimport) T<int>;
+int bar() { T<int> t; return t.foo(); }
+// MO1-DAG: @"?x@?{{1|2}}??foo@?$T@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = available_externally dllimport global i32 0, align 4
+}