[clang] Fix loss of dllexport for exported template specialization

[nga888]

When dropping DLL attributes, ensure that the most recent declaration is being checked.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Andrew Ng (nga888)

Changes

When dropping DLL attributes, ensure that the most recent declaration is being checked.

---

Full diff: https://github.com/llvm/llvm-project/pull/94664.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenModule.cpp (+13-4)
• (added) clang/test/CodeGenCXX/windows-instantiate-dllexport-template-specialization.cpp (+18)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index be7bf0b72dc0c..dcd80bc19e25c 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4561,10 +4561,19 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
     }
 
     // Handle dropped DLL attributes.
-    if (D && !D->hasAttr<DLLImportAttr>() && !D->hasAttr<DLLExportAttr>() &&
-        !shouldMapVisibilityToDLLExport(cast_or_null<NamedDecl>(D))) {
-      Entry->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
-      setDSOLocal(Entry);
+    if (D) {
+      auto SC = Entry->getDLLStorageClass();
+      if (SC != llvm::GlobalValue::DefaultStorageClass) {
+        const Decl *MRD = D->getMostRecentDecl();
+        if (((SC == llvm::GlobalValue::DLLImportStorageClass &&
+              !MRD->hasAttr<DLLImportAttr>()) ||
+             (SC == llvm::GlobalValue::DLLExportStorageClass &&
+              !MRD->hasAttr<DLLExportAttr>())) &&
+            !shouldMapVisibilityToDLLExport(cast<NamedDecl>(MRD))) {
+          Entry->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
+          setDSOLocal(Entry);
+        }
+      }
     }
 
     // If there are two attempts to define the same mangled name, issue an
diff --git a/clang/test/CodeGenCXX/windows-instantiate-dllexport-template-specialization.cpp b/clang/test/CodeGenCXX/windows-instantiate-dllexport-template-specialization.cpp
new file mode 100644
index 0000000000000..97f341ba1f909
--- /dev/null
+++ b/clang/test/CodeGenCXX/windows-instantiate-dllexport-template-specialization.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple i686-windows         -fdeclspec -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-MS
+// RUN: %clang_cc1 -triple i686-windows-itanium -fdeclspec -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-scei-ps4      -fdeclspec -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-sie-ps5       -fdeclspec -emit-llvm %s -o - | FileCheck %s
+
+struct s {
+  template <bool b = true> static bool f();
+};
+
+template <typename T> bool template_using_f(T) { return s::f(); }
+
+bool use_template_using_f() { return template_using_f(0); }
+
+template<>
+bool __declspec(dllexport) s::f<true>() { return true; }
+
+// CHECK-MS: dllexport {{.*}} @"??$f@$00@s@@SA_NXZ"
+// CHECK: dllexport {{.*}} @_ZN1s1fILb1EEEbv

[zmodem]

Thanks! This basically looks good to me.

It would be nice to keep GetOrCreateLLVMGlobal and GetOrCreateLLVMFunction in sync. Would it be possible to extract the "Handle dropped DLL attributes" logic into a small utility function that could be called from both?

[nga888]

It would be nice to keep GetOrCreateLLVMGlobal and GetOrCreateLLVMFunction in sync. Would it be possible to extract the "Handle dropped DLL attributes" logic into a small utility function that could be called from both?

I did briefly think about this, but does this change really apply to GetOrCreateLLVMGlobal? Is there any test case to exercise it? Perhaps it would be better to leave GetOrCreateLLVMGlobal as is?

[zmodem]

It would be nice to keep GetOrCreateLLVMGlobal and GetOrCreateLLVMFunction in sync. Would it be possible to extract the "Handle dropped DLL attributes" logic into a small utility function that could be called from both?

I did briefly think about this, but does this change really apply to GetOrCreateLLVMGlobal? Is there any test case to exercise it? Perhaps it would be better to leave GetOrCreateLLVMGlobal as is?

I think we should look at it the other way around: unless there's a reason that the functions should behave differently (and I'm not aware of any), it seems better for them to keep using the same logic.

[nga888]

I did briefly think about this, but does this change really apply to GetOrCreateLLVMGlobal? Is there any test case to exercise it? Perhaps it would be better to leave GetOrCreateLLVMGlobal as is?

I think we should look at it the other way around: unless there's a reason that the functions should behave differently (and I'm not aware of any), it seems better for them to keep using the same logic.

One function handles "functions" and the other "globals", so behaviour is not necessarily the same. However, if you feel it would be better that both use the same logic, then I will go ahead with adding this.

[zmodem]

One function handles "functions" and the other "globals", so behaviour is not necessarily the same. However, if you feel it would be better that both use the same logic, then I will go ahead with adding this.

Yes, I think that since the dll attribute logic has been the same between GetOrCreateLLVMGlobal and GetOrCreateLLVMFunction so far, we should keep it the same until we find an explicit reason to make them different.

[zmodem]

Thank you, I think this is much better.

[zmodem]

lgtm