Inherit dll attributes to static locals

[hans]

Summary

This makes us handle static locals in exported/imported functions correctly.

[abdulras]

It would be nice to have similar tests for the Itanium ABI, but I suspect that you are less interested in that.

lib/CodeGen/MicrosoftCXXABI.cpp

1429 ↗

(On Diff #10392)

clsas -> class.

test/CodeGenCXX/dllexport.cpp

274 ↗

(On Diff #10392)

It really would be nice to split this file up at some point.

[nrieck]

lib/CodeGen/MicrosoftCXXABI.cpp

1429 ↗

(On Diff #10392)

"class"

lib/Sema/SemaDecl.cpp

9112 ↗

(On Diff #10392)

There's already a getDLLAttr() function somewhere, but I guess it's not available here. Maybe this helper could be moved to SemaInternal.h?

test/CodeGenCXX/dllexport.cpp

257 ↗

(On Diff #10392)

These tests should be at the end of the section for globals (like in SemaCXX/dllexport.cpp) IMO. Same goes for the dllimport tests.

262 ↗

(On Diff #10392)

These checks can be MSC-DAG.

267 ↗

(On Diff #10392)

Superfluous ";"

269 ↗

(On Diff #10392)

These checks can be MSC-DAG, and please add a comment that MinGW doesn't do this and is not just forgotten here (at least my tests with GCC show this to be true).

[hans]

Thanks for the review!

It would be nice to have similar tests for the Itanium ABI, but I suspect that you are less interested in that.

As Nico pointed out, they don't seem to export the static locals :/

lib/CodeGen/MicrosoftCXXABI.cpp

1429 ↗

(On Diff #10392)

Done.

lib/Sema/SemaDecl.cpp

9112 ↗

(On Diff #10392)

Sounds good to me.

test/CodeGenCXX/dllexport.cpp

257 ↗

(On Diff #10392)

Done.

262 ↗

(On Diff #10392)

Done.

267 ↗

(On Diff #10392)

Removed.

269 ↗

(On Diff #10392)

Done.

[hans]

Addressing review comments.

Please take another look!

[hans]

Nico: does the new version of the patch look ok to you?

[rnk]

lib/Sema/SemaDecl.cpp

9108 ↗

(On Diff #10407)

Should this go before checkAttributesAfterMerging? Consider this obnoxious test case:

int f();
inline __declspec(dllexport) int g() {
  static __declspec(dllimport) int x = f();
  return x;
}

[hans]

lib/Sema/SemaDecl.cpp

9108 ↗

(On Diff #10407)

Local variables cannot have explicit dll attributes. They can only get them by inheriting from the function they're part of, so calling checkAttributesAfterMerging to check up on the attribute we just added seems redundant.

[rnk]

lgtm

lib/Sema/SemaDecl.cpp

9108 ↗

(On Diff #10407)

OK. I assume this is covered by Nico's extensive test cases. :)

[nrieck]

In D4136#10, @hans wrote:

Nico: does the new version of the patch look ok to you?

LGTM

lib/Sema/SemaDecl.cpp

9108 ↗

(On Diff #10407)

Yup, this is handled by requiring external linkage.

[hans]

Closed by commit rL211173 (authored by @hans).

cfe/trunk/include/clang/Sema/SemaInternal.h

inline void MarkVarDeclODRUsed(VarDecl *Var,
   SemaRef.tryCaptureVariable(Var, Loc, Sema::TryCapture_Implicit, 
     /*EllipsisLoc*/ SourceLocation(),
     /*BuildAndDiagnose*/ true, 
     CaptureType, DeclRefType, 
     FunctionScopeIndexToStopAt);
 
   Var->markUsed(SemaRef.Context);
 }
+
+/// Return a DLL attribute from the declaration.
+inline InheritableAttr *getDLLAttr(Decl *D) {
+  assert(!(D->hasAttr<DLLImportAttr>() && D->hasAttr<DLLExportAttr>()) &&
+         "A declaration cannot be both dllimport and dllexport.");
+  if (auto *Import = D->getAttr<DLLImportAttr>())
+    return Import;
+  if (auto *Export = D->getAttr<DLLExportAttr>())
+    return Export;
+  return nullptr;
+}
+
 }
 
 #endif

cfe/trunk/lib/CodeGen/CGDecl.cpp

new llvm::GlobalVariable(CGM.getModule(), LTy,
                              llvm::GlobalVariable::NotThreadLocal,
                              AddrSpace);
   GV->setAlignment(getContext().getDeclAlign(&D).getQuantity());
   CGM.setGlobalVisibility(GV, &D);
 
   if (D.getTLSKind())
     CGM.setTLSMode(GV, D);
 
+  if (D.isExternallyVisible()) {
+    if (D.hasAttr<DLLImportAttr>())
+      GV->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
+    else if (D.hasAttr<DLLExportAttr>())
+      GV->setDLLStorageClass(llvm::GlobalVariable::DLLExportStorageClass);
+  }
+
   // Make sure the result is of the correct type.
   unsigned ExpectedAddrSpace = CGM.getContext().getTargetAddressSpace(Ty);
   if (AddrSpace != ExpectedAddrSpace) {
     llvm::PointerType *PTy = llvm::PointerType::get(LTy, ExpectedAddrSpace);
     return llvm::ConstantExpr::getAddrSpaceCast(GV, PTy);
   }
 
   return GV;

cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp

if (!GI->Guard) {
     // Mangle the name for the guard.
     SmallString<256> GuardName;
     {
       llvm::raw_svector_ostream Out(GuardName);
       getMangleContext().mangleStaticGuardVariable(&D, Out);
       Out.flush();
     }
 
-    // Create the guard variable with a zero-initializer.  Just absorb linkage
-    // and visibility from the guarded variable.
+    // Create the guard variable with a zero-initializer. Just absorb linkage,
+    // visibility and dll storage class from the guarded variable.
     GI->Guard =
         new llvm::GlobalVariable(CGM.getModule(), GuardTy, false,
                                  GV->getLinkage(), Zero, GuardName.str());
     GI->Guard->setVisibility(GV->getVisibility());
+    GI->Guard->setDLLStorageClass(GV->getDLLStorageClass());
   } else {
     assert(GI->Guard->getLinkage() == GV->getLinkage() &&
            "static local from the same function had different linkage");
   }
 
   // Pseudo code for the test:
   // if (!(GuardVar & MyGuardBit)) {
   //   GuardVar |= MyGuardBit;

cfe/trunk/lib/Sema/SemaDecl.cpp

Sema::FinalizeDeclaration(Decl *ThisDecl) {
   ParsingInitForAutoVars.erase(ThisDecl);
 
   VarDecl *VD = dyn_cast_or_null<VarDecl>(ThisDecl);
   if (!VD)
     return;
 
   checkAttributesAfterMerging(*this, *VD);
 
+  // Static locals inherit dll attributes from their function.
+  if (VD->isStaticLocal()) {
+    if (FunctionDecl *FD =
+            dyn_cast<FunctionDecl>(VD->getParentFunctionOrMethod())) {
+      if (Attr *A = getDLLAttr(FD)) {
+        auto *NewAttr = cast<InheritableAttr>(A->clone(getASTContext()));
+        NewAttr->setInherited(true);
+        VD->addAttr(NewAttr);
+      }
+    }
+  }
+
   // Imported static data members cannot be defined out-of-line.
   if (const DLLImportAttr *IA = VD->getAttr<DLLImportAttr>()) {
     if (VD->isStaticDataMember() && VD->isOutOfLine() &&
         VD->isThisDeclarationADefinition()) {
       // We allow definitions of dllimport class template static data members
       // with a warning.
       CXXRecordDecl *Context =
         cast<CXXRecordDecl>(VD->getFirstDecl()->getDeclContext());

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

// Nested classes and class templates.
       CheckAbstractClassUsage(Info, cast<CXXRecordDecl>(D));
     } else if (isa<ClassTemplateDecl>(D)) {
       CheckAbstractClassUsage(Info,
                              cast<ClassTemplateDecl>(D)->getTemplatedDecl());
     }
   }
 }
 
-/// \brief Return a DLL attribute from the declaration.
-static InheritableAttr *getDLLAttr(Decl *D) {
-  assert(!(D->hasAttr<DLLImportAttr>() && D->hasAttr<DLLExportAttr>()) &&
-         "A declaration cannot be both dllimport and dllexport.");
-  if (auto *Import = D->getAttr<DLLImportAttr>())
-    return Import;
-  if (auto *Export = D->getAttr<DLLExportAttr>())
-    return Export;
-  return nullptr;
-}
-
 /// \brief Check class-level dllimport/dllexport attribute.
 static void checkDLLAttribute(Sema &S, CXXRecordDecl *Class) {
   Attr *ClassAttr = getDLLAttr(Class);
   if (!ClassAttr)
     return;
 
   bool ClassExported = ClassAttr->getKind() == attr::DLLExport;
 

cfe/trunk/test/CodeGenCXX/dllexport.cpp

 // MSC-DAG: @"\01?ExternalGlobal@ns@@3HA" = dllexport global i32 0, align 4
 // GNU-DAG: @_ZN2ns14ExternalGlobalE      = dllexport global i32 0, align 4
 namespace ns { __declspec(dllexport) int ExternalGlobal; }
 
 // MSC-DAG: @"\01?ExternalAutoTypeGlobal@@3UExternal@@A" = dllexport global %struct.External zeroinitializer, align 4
 // GNU-DAG: @ExternalAutoTypeGlobal                      = dllexport global %struct.External zeroinitializer, align 4
 __declspec(dllexport) auto ExternalAutoTypeGlobal = External();
 
+int f();
+// MSC-DAG: @"\01?x@?0??nonInlineStaticLocalsFunc@@YAHXZ@4HA" = internal {{(unnamed_addr )*}}global i32 0
+// MSC-DAG: @"\01?$S1@?0??nonInlineStaticLocalsFunc@@YAHXZ@4IA" = internal {{(unnamed_addr )*}}global i32 0
+int __declspec(dllexport) nonInlineStaticLocalsFunc() {
+  static int x = f();
+  return x++;
+};
+
+// MSC-DAG: @"\01?x@?1??inlineStaticLocalsFunc@@YAHXZ@4HA" = weak_odr dllexport global i32 0
+// MSC-DAG: @"\01??_B?1??inlineStaticLocalsFunc@@YAHXZ@51" = weak_odr dllexport global i32 0
+// Note: MinGW doesn't seem to export the static local here.
+inline int __declspec(dllexport) inlineStaticLocalsFunc() {
+  static int x = f();
+  return x++;
+}
+
 
 
 //===----------------------------------------------------------------------===//
 // Variable templates
 //===----------------------------------------------------------------------===//
 
 // Declarations are not exported.
 

cfe/trunk/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();
+// 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);
+
 
 
 //===----------------------------------------------------------------------===//
 // Variable templates
 //===----------------------------------------------------------------------===//
 
 // Import declaration.
 // MSC-DAG: @"\01??$ExternVarTmplDecl@UImplicitInst_Imported@@@@3HA" = external dllimport global i32