[PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported

[Reid Kleckner via cfe-commits]

rnk added a comment.

I think this generally seems right, but we should make sure our behavior is more consistent in the case of a template definition.


================
Comment at: lib/Sema/SemaDecl.cpp:5570-5571
@@ -5565,4 +5569,4 @@
   // exceptions being inline function definitions, local extern declarations,
-  // and qualified friend declarations.
-  // NB: MSVC converts such a declaration to dllexport.
+  // qualified friend declarations or special MSVC extension: in the last case,
+  // the declaration is treated as if it were marked dllexport.
   bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
----------------
This comment is a run-on, maybe try this:

If a redeclaration drops the dllimport attribute, we usually ignore the previous attribute with a warning.
If the redeclaration is an inline definition, a local extern declaration, or a qualified friend declaration, we do nothing.
If this declaration is a plain, non-inline definition, then we translate dllimport to dllexport and warn the user.

================
Comment at: lib/Sema/SemaDecl.cpp:5595
@@ +5594,3 @@
+      NewDecl->addAttr(::new (S.Context) DLLExportAttr(
+          NewImportAttr->getRange(), S.Context,
+          NewImportAttr->getSpellingListIndex()));
----------------
In CodeGen, when we make decisions about the actual LLVM IR dll storage class, we should call Decl::getMostRecentDecl, and look at the attributes on that.

I think we can defer fixing this. You can see instances of the same problem below where we drop dllimport from previous declarations. We shouldn't be doing that.

================
Comment at: lib/Sema/SemaDecl.cpp:5600-5602
@@ +5599,5 @@
+             diag::warn_redeclaration_without_attribute_prev_attribute_ignored)
+          << NewDecl << OldImportAttr;
+      S.Diag(OldDecl->getLocation(), diag::note_previous_declaration);
+      S.Diag(OldImportAttr->getLocation(), diag::note_previous_attribute);
+      OldDecl->dropAttr<DLLImportAttr>();
----------------
Use DLLExportAttr::CreateImplicit or Attr->setImplicit(true).

================
Comment at: test/Sema/dllimport.c:52
@@ +51,3 @@
+__declspec(dllimport) int GlobalDeclInit; // expected-note{{previous declaration is here}}
+#ifdef MS
+// expected-warning at +4{{'GlobalDeclInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}}
----------------
We've already got one test for the difference between MSVC and GNU mode. I think the rest of these test cases can skip the ifdefs and look for the common prefix instead, like this:
  // expected-warning {{'GlobalDeclInit' redeclared without 'dllimport' attribute}}

================
Comment at: test/SemaCXX/dllimport.cpp:62
@@ +61,3 @@
+__declspec(dllimport) int GlobalDeclInit; // expected-note{{previous declaration is here}}
+#ifdef MS
+// expected-warning at +4{{'GlobalDeclInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}}
----------------
ditto, after the first MS-specific diagnostic test case we don't need the duplication

================
Comment at: test/SemaCXX/dllimport.cpp:179
@@ -140,1 +178,3 @@
+template <typename T>
+int ExternVarTmplDeclInit = 1;
 
----------------
Can you check with MSVC 2015 update 2 actually does with definitions of dllimport variable templates? I bet it doesn't export them.

================
Comment at: test/SemaCXX/dllimport.cpp:1137
@@ -1017,1 +1136,3 @@
+template <typename T>
+void ImportClassTmplMembers<T>::normalDef() {}
 #ifdef GNU
----------------
I'm pretty sure MSVC considers all free function templates to be 'inline', i.e. they put them in comdats. I doubt MSVC exports these. Can you verify what it does?


http://reviews.llvm.org/D18953