[clang] Fix interaction between dllimport and exclude_from_explicit_instantiation

[ldionne]

Summary

When an entity is marked with both dllimport and exclude_from_explicit_instantiation,
the compiler should not assume that the entity will be provided in another TU.

Fixes https://github.com/llvm/llvm-project/issues/40363

[ldionne]

clang/lib/CodeGen/CodeGenModule.cpp

1515

To be perfectly honest, I searched for DLLImportAttr until I found places that made some amount of sense to exclude when the entity has ExcludeFromExplicitInstantiationAttr, but there might be much deeper consequences to those changes that I don't understand. I'm very naive when it comes to Clang, especially when it comes to codegen. So I recommend really looking over these changes or we might be making things worse.

[rnk]

Thanks for the patch!

The other similar functionality that exists is /Zc:dllexportInlines-, which we could track down the implementation of to try to share logic, but I'm not sure about that.

I should defer to @hans for a more thorough code review.

clang/lib/AST/ASTContext.cpp

11699

I think this may not be quite right. If it is possible for the EFEI attr to appear on non-template entities with vague linkage, we would still need to import/export them.

11702

I wonder if we have to do something on the other side to prevent dllexport from upgrading linkonce_odr linkage to weak_odr linkage if we later go on to suppress the dllexport storage class.

[hans]

Apologies for the delay, I'm still catching up with my post-vacation backlog.

(For my notes, this came up in https://reviews.llvm.org/D153709)

I think the root of this bug is how class-level dll attributes get propagated to class members. Here is an example:

template <typename T>
struct __declspec(dllimport) S {
  void __attribute__((exclude_from_explicit_instantiation)) f() {}
};

extern template struct S<int>;

The problem is that when S<int> is explicitly instantiated, the dllimport attribute gets propagated to all its members, assuming all members are also being instantiated -- which may not be true now that exclude_from_explicit_instantiation exists.

I think the right location for the fix is where that inheritance happens:

diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index a7ab0948d01b..12cb2542641a 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -6592,6 +6592,13 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
     if (!VD && !MD)
       continue;
 
+    if ((TSK == TSK_ExplicitInstantiationDeclaration ||
+         TSK == TSK_ExplicitInstantiationDefinition) &&
+        Member->hasAttr<ExcludeFromExplicitInstantiationAttr>()) {
+      // Skip members excluded from instantiation.
+      continue;
+    }
+
     if (MD) {
       // Don't process deleted methods.
       if (MD->isDeleted())

That's also the approach suggested in https://bugs.llvm.org/show_bug.cgi?id=41018#c0 (and it also handles the dllexport case.)

That doesn't handle the second of your test cases though, where dllimport is put on the member directly:

template <typename T>
struct  S {
  void __attribute__((exclude_from_explicit_instantiation)) __declspec(dllimport) f() {}
};

extern template struct S<int>;

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

Now, S<int>::f *is* technically being excluded from explicit instantiation as the attribute says, but when it gets implicitly instantiated it will be dllimport, because it was declared that way.

It's not clear to me how we'd want to handle that. I don't think it comes up in libc++, and I can't think of any code that would want to do that either, really.

https://bugs.llvm.org/show_bug.cgi?id=41018#c0 suggest emitting an error about incompatible attributes in that case. I suppose it could also be a warning, or we could do nothing for now.

[rnk]

That doesn't handle the second of your test cases though, where dllimport is put on the member directly:
...
It's not clear to me how we'd want to handle that. I don't think it comes up in libc++, and I can't think of any code that would want to do that either, really.

I think this does actually matter for libc++, I think I have seen this pattern:

template <typename T>
struct Foo {
  _LIBCPP_FUNC_VIS _LIBCPP_EXCLUDE_FROM_ABI void method() { }
};

I can't find an instance right now, though. I think it comes up when you want to have a default visibility function, and also keep it out of the libc++ DSO interface.

This is a somewhat weird and contradictory case, though, the attributes directly conflict. I think it would be reasonable to teach SemaDeclAttr to ignore the dllimport attribute if the other attribute is already present.

[hans]

In D155713#4592667, @rnk wrote:

That doesn't handle the second of your test cases though, where dllimport is put on the member directly:
...
It's not clear to me how we'd want to handle that. I don't think it comes up in libc++, and I can't think of any code that would want to do that either, really.

I think this does actually matter for libc++, I think I have seen this pattern:

template <typename T>
struct Foo {
  _LIBCPP_FUNC_VIS _LIBCPP_EXCLUDE_FROM_ABI void method() { }
};

I can't find an instance right now, though. I think it comes up when you want to have a default visibility function, and also keep it out of the libc++ DSO interface.

I guess the libc++ folks can answer if that's something that could occur.

This is a somewhat weird and contradictory case, though, the attributes directly conflict. I think it would be reasonable to teach SemaDeclAttr to ignore the dllimport attribute if the other attribute is already present.

I think what worries me is at that point, the attribute is no longer about just suppressing explicit instantiation, but suppressing the dll attribute in general, which means perhaps it shouldn't just apply to templates, but also

struct __declspec(dllimport)  S {
  void __attribute__((exclude_from_explicit_instantiation)) f() {}
};

(This is what you mentioned in https://bugs.llvm.org/show_bug.cgi?id=41018#c1)

In one way I suppose it could make sense, but it's also pretty different from what the attribute is today, and I worry about growing its scope and the complexity that often comes with tweaking the behavior of the dll attributes.

[ldionne]

In D155713#4592852, @hans wrote:

I think this does actually matter for libc++, I think I have seen this pattern:

template <typename T>
struct Foo {
  _LIBCPP_FUNC_VIS _LIBCPP_EXCLUDE_FROM_ABI void method() { }
};

I can't find an instance right now, though. I think it comes up when you want to have a default visibility function, and also keep it out of the libc++ DSO interface.

I guess the libc++ folks can answer if that's something that could occur.

I can't imagine why we'd want to hide something from the DSO but give it default visibility. It's not impossible that we have some of those, but I can't think of any right now and I'd definitely learn something new if I came across one of those. I believe it might make sense to diagnose when that happens and not try to handle this case.

I came back to this as part of the Github PR transition cleanup. Realistically, I don't believe I will have time to work on this in the near future and I also feel like I am not really the best person to fix this issue considering I am fluent neither with Clang nor with the semantics of DLLs. It'd be really great if someone could pick this up, but until then I'll need to abandon the patch to clear up my review queue, which has gotten completely unrealistic.

[hans]

Sent my diff along with tests based on Louis's patch as a pull request here: https://github.com/llvm/llvm-project/pull/65961

clang/lib/AST/ASTContext.cpp

static GVALinkage basicGVALinkageForFunction(const ASTContext &Context,
 
   return GVA_DiscardableODR;
 }
 
 static GVALinkage adjustGVALinkageForAttributes(const ASTContext &Context,
                                                 const Decl *D, GVALinkage L) {
   // See http://msdn.microsoft.com/en-us/library/xa0d9ste.aspx
   // dllexport/dllimport on inline functions.
-  if (D->hasAttr<DLLImportAttr>()) {
+  if (D->hasAttr<DLLImportAttr>() && !D->hasAttr<ExcludeFromExplicitInstantiationAttr>()) {
     if (L == GVA_DiscardableODR || L == GVA_StrongODR)
       return GVA_AvailableExternally;
   } else if (D->hasAttr<DLLExportAttr>()) {
     if (L == GVA_DiscardableODR)
       return GVA_StrongODR;
   } else if (Context.getLangOpts().CUDA && Context.getLangOpts().CUDAIsDevice) {
     // Device-side functions with __global__ attribute must always be
     // visible externally so they can be launched from host.
     if (D->hasAttr<CUDAGlobalAttr>() &&
         (L == GVA_DiscardableODR || L == GVA_Internal))
       return GVA_StrongODR;

clang/lib/CodeGen/CodeGenModule.cpp

if (const auto *Dtor = dyn_cast_or_null<CXXDestructorDecl>(D)) {
     getCXXABI().setCXXDestructorDLLStorage(GV, Dtor, GD.getDtorType());
     return;
   }
   setDLLImportDLLExport(GV, D);
 }
 
 void CodeGenModule::setDLLImportDLLExport(llvm::GlobalValue *GV,
                                           const NamedDecl *D) const {
-  if (D && D->isExternallyVisible()) {
+  if (D && D->isExternallyVisible() && !D->hasAttr<ExcludeFromExplicitInstantiationAttr>()) {
     if (D->hasAttr<DLLImportAttr>())
       GV->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
     else if ((D->hasAttr<DLLExportAttr>() ||
               shouldMapVisibilityToDLLExport(D)) &&
              !GV->isDeclarationForLinker())
       GV->setDLLStorageClass(llvm::GlobalVariable::DLLExportStorageClass);
   }
 }

clang/lib/Sema/Sema.cpp

for (const auto &UndefinedUse : UndefinedButUsed) {
     if (ND->isInvalidDecl()) continue;
 
     // __attribute__((weakref)) is basically a definition.
     if (ND->hasAttr<WeakRefAttr>()) continue;
 
     if (isa<CXXDeductionGuideDecl>(ND))
       continue;
 
-    if (ND->hasAttr<DLLImportAttr>() || ND->hasAttr<DLLExportAttr>()) {
+    if ((ND->hasAttr<DLLImportAttr>() || ND->hasAttr<DLLExportAttr>()) &&
+        !ND->hasAttr<ExcludeFromExplicitInstantiationAttr>()) {
       // An exported function will always be emitted when defined, so even if
       // the function is inline, it doesn't have to be emitted in this TU. An
-      // imported function implies that it has been exported somewhere else.
+      // imported function implies that it has been exported somewhere else,
+      // except if it is marked with exclude_from_explicit_instantiation, in
+      // which case there might not be a definition elsewhere.
       continue;
     }
 
     if (FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
       if (FD->isDefined())
         continue;
       if (FD->isExternallyVisible() &&
           !isExternalWithNoLinkageType(FD) &&

clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.dllimport.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-windows -fms-extensions -emit-llvm -O0 -o - %s | FileCheck %s
+
+// Test that dllimport and exclude_from_explicit_instantiation work properly
+// together. Specifically, we check that when exclude_from_explicit_instantiation
+// is used on a method, the compiler doesn't expect it to be provided externally
+// even if it is marked with dllimport.
+//
+// https://github.com/llvm/llvm-project/issues/40363
+
+#define DLLIMPORT __declspec(dllimport)
+#define EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((exclude_from_explicit_instantiation))
+
+template <class T>
+struct DLLIMPORT Foo {
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION void x() {}
+};
+
+template <class T>
+struct Bar {
+  DLLIMPORT EXCLUDE_FROM_EXPLICIT_INSTANTIATION void x() {}
+};
+
+extern template struct Foo<int>;
+extern template struct Bar<int>;
+
+void test(Foo<int>& foo, Bar<int>& bar) {
+  // CHECK-DAG: define linkonce_odr dso_local void @"?x@?$Foo@H@@QEAAXXZ"
+  foo.x();
+
+  // CHECK-DAG: define linkonce_odr dso_local void @"?x@?$Bar@H@@QEAAXXZ"
+  bar.x();
+}

clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.dllimport.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-windows -fms-extensions -verify %s
+
+// Test that an entity marked as both dllimport and exclude_from_explicit_instantiation
+// isn't instantiated.
+
+#define DLLIMPORT __declspec(dllimport)
+#define EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((exclude_from_explicit_instantiation))
+
+template <class T>
+struct DLLIMPORT Foo {
+  EXCLUDE_FROM_EXPLICIT_INSTANTIATION void x();
+};
+
+template <class T>
+struct Bar {
+  DLLIMPORT EXCLUDE_FROM_EXPLICIT_INSTANTIATION inline void x();
+};
+
+template <class T>
+void Foo<T>::x() { using Fail = typename T::fail; }
+
+template <class T>
+DLLIMPORT inline void Bar<T>::x() { using Fail = typename T::fail; }
+
+// expected-no-diagnostics
+template struct Foo<int>;
+template struct Bar<int>;