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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to