[PATCH] D155713: [clang] Fix interaction between dllimport and exclude_from_explicit_instantiation

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155713/new/

https://reviews.llvm.org/D155713