Member access to incomplete type from dllimport

[zahiraam]

Summary

Explicit instantiation of a dllimport class should be ignored.

[rnk]

I'm pretty sure we should treat this like an explicit extern template declaration. I'm not sure your code does that.

[hans]

In D40551#937914, @rnk wrote:

I'm pretty sure we should treat this like an explicit extern template declaration. I'm not sure your code does that.

Right, see r270897 for where we added this for classes.

I'm not quite sure where the right place to do this for functions would be.

lib/Sema/SemaType.cpp

7613

Indentation looks off.

[hans]

I've been looking some more, and I'm starting to wonder if treating the explicit instantiation definition as a decl ("extern") instead is enough.

It's enough for the reduced test case, but if we consider the example below, where we then try to use the method:

class __declspec(dllimport) Edge;

template <class T>
class Range
{
public:
  void insert(T *obj) { obj->loc(); }
};

extern template void Range<Edge>::insert(Edge *);

void f(Range<Edge> *r, Edge *e) {
    r->insert(e);
}

MSVC accepts this, but Clang does not:

/tmp/c.cc:7:28: error: member access into incomplete type 'Edge'
  void insert(T *obj) { obj->loc(); }
                           ^
/tmp/c.cc:13:8: note: in instantiation of member function 'Range<Edge>::insert' requested here
    r->insert(e);
       ^
/tmp/c.cc:1:29: note: forward declaration of 'Edge'
class __declspec(dllimport) Edge;
                            ^

This is a little surprising to me, as I'd think the explicit instantiation decl should "block" any later implicit specializations, but GCC also diagnoses this: https://godbolt.org/g/8MPhY9

[hans]

Richard points out that explicit instantiation decls doesn't block instantiation of inline functions, so GCC and Clang are correct to error here.

Zahira: In your original test case, is there anything that tries to use Range<Edge>::insert(Edge *) after the explicit instantiation definition, or is it just that one that's problematic?

[zahiraam]

It's just that one that is problematic.
But then the test case was created artificially to test the access of member function of a dll import type, presumably because some customer code had that in their code.

[zahiraam]

Is this something that you are interested in fixing or are you happy with having clang behave the same way than GCC?

[hans]

In D40551#939319, @zahiraam wrote:

Is this something that you are interested in fixing or are you happy with having clang behave the same way than GCC?

Making explicit instantiation definitions of dllimport functions be treated as instantiation decls is something we should do, and I'll try to get a patch out later today. That will fix the test case you provided.

But I'm not sure we want to follow msvc's behaviour of having explicit instantiation delcs block further instantiation of inline functions, since it goes against the standard. We'd only want to do that if there is code that cannot be fixed which relies on it.

[zahiraam]

In D40551#937947, @hans wrote:

In D40551#937914, @rnk wrote:

I'm pretty sure we should treat this like an explicit extern template declaration. I'm not sure your code does that.

Right, see r270897 for where we added this for classes.

I'm not quite sure where the right place to do this for functions would be.

May be here: Sema::ActOnExplicitInstantiation??

[hans]

In D40551#939443, @zahiraam wrote:

In D40551#937947, @hans wrote:

In D40551#937914, @rnk wrote:

I'm pretty sure we should treat this like an explicit extern template declaration. I'm not sure your code does that.

Right, see r270897 for where we added this for classes.

I'm not quite sure where the right place to do this for functions would be.

May be here: Sema::ActOnExplicitInstantiation??

Yes, I think that's the place. I've uploaded https://reviews.llvm.org/D40621 for this, please take a look.

[zahiraam]

See https://reviews.llvm.org/D40621

lib/Sema/SemaType.cpp

if (ClassTemplateSpecializationDecl *ClassTemplateSpec
         if (MSI->getTemplateSpecializationKind() !=
             TSK_ExplicitSpecialization) {
           Diagnosed = InstantiateClass(Loc, Rec, Pattern,
                                        getTemplateInstantiationArgs(Rec),
                                        TSK_ImplicitInstantiation,
                                        /*Complain=*/Diagnoser);
           Instantiated = true;
         }
+      } else if (const SubstTemplateTypeParmType *SubstTypeParam =
+                     dyn_cast<SubstTemplateTypeParmType>(MaybeTemplate)) {
+  MaybeTemplate = SubstTypeParam->getReplacementType();
+  if (getDLLAttr(MaybeTemplate->getAsCXXRecordDecl())) {
+    // Don't instantiate a dllimport class.
+    Diagnosed = false;
+    Instantiated = false;
+    return true;
+  }
       }
     }
 
     if (Instantiated) {
       // Instantiate* might have already complained that the template is not
       // defined, if we asked it to.
       if (Diagnoser && Diagnosed)
         return true;

test/Sema/member-reference-dll.cpp

+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -fms-extensions -fms-compatibility %s
+// expected-no-diagnostics
+
+namespace test1 {
+class __declspec(dllimport) Edge;
+
+template <class T>
+class __declspec(dllimport) Range {
+  void insert(T *obj) { obj->loc(); }
+};
+
+template void Range<Edge>::insert(Edge *);
+} // namespace test1
+
+namespace test2 {
+class __declspec(dllexport) Edge;
+
+template <class T>
+class __declspec(dllimport) Range {
+  void insert(T *obj) { obj->loc(); }
+};
+
+template void Range<Edge>::insert(Edge *);
+} // namespace test2