[PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
thakis added inline comments.
Comment at: cfe/trunk/lib/Sema/SemaDecl.cpp:5650
+ NewImportAttr->getRange(), S.Context,
+ NewImportAttr->getSpellingListIndex()));
+} else {
NewImportAttr can be nullptr here, at least for invalid code. This change
introduced https://bugs.llvm.org/show_bug.cgi?id=34981 , please take a look.
Repository:
rL LLVM
https://reviews.llvm.org/D18953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
This revision was automatically updated to reflect the committed changes.
Closed by commit rL270686: [ms][dll] #26935 Defining a dllimport function
should cause it to be exported (authored by dzobnin).
Changed prior to commit:
http://reviews.llvm.org/D18953?vs=58233&id=58401#toc
Repository:
rL LLVM
http://reviews.llvm.org/D18953
Files:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/test/CodeGen/dllimport.c
cfe/trunk/test/CodeGenCXX/dllimport-members.cpp
cfe/trunk/test/CodeGenCXX/dllimport.cpp
cfe/trunk/test/Sema/dllimport.c
cfe/trunk/test/SemaCXX/dllimport.cpp
Index: cfe/trunk/test/SemaCXX/dllimport.cpp
===
--- cfe/trunk/test/SemaCXX/dllimport.cpp
+++ cfe/trunk/test/SemaCXX/dllimport.cpp
@@ -44,17 +44,49 @@
int __declspec(dllimport) GlobalInit2 = 1; // expected-error{{definition of dllimport data}}
// Declare, then reject definition.
-__declspec(dllimport) extern int ExternGlobalDeclInit; // expected-note{{previous declaration is here}} expected-note{{previous attribute is here}}
-int ExternGlobalDeclInit = 1; // expected-warning{{'ExternGlobalDeclInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#ifdef GNU
+// expected-note@+2{{previous attribute is here}}
+#endif
+__declspec(dllimport) extern int ExternGlobalDeclInit; // expected-note{{previous declaration is here}}
+#ifdef MS
+// expected-warning@+4{{'ExternGlobalDeclInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}}
+#else
+// expected-warning@+2{{'ExternGlobalDeclInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#endif
+int ExternGlobalDeclInit = 1;
-__declspec(dllimport) int GlobalDeclInit; // expected-note{{previous declaration is here}} expected-note{{previous attribute is here}}
-int GlobalDeclInit = 1; // expected-warning{{'GlobalDeclInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#ifdef GNU
+// expected-note@+2{{previous attribute is here}}
+#endif
+__declspec(dllimport) int GlobalDeclInit; // expected-note{{previous declaration is here}}
+#ifdef MS
+// expected-warning@+4{{'GlobalDeclInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}}
+#else
+// expected-warning@+2{{'GlobalDeclInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#endif
+int GlobalDeclInit = 1;
-int *__attribute__((dllimport)) GlobalDeclChunkAttrInit; // expected-note{{previous declaration is here}} expected-note{{previous attribute is here}}
-int *GlobalDeclChunkAttrInit = 0; // expected-warning{{'GlobalDeclChunkAttrInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#ifdef GNU
+// expected-note@+2{{previous attribute is here}}
+#endif
+int *__attribute__((dllimport)) GlobalDeclChunkAttrInit; // expected-note{{previous declaration is here}}
+#ifdef MS
+// expected-warning@+4{{'GlobalDeclChunkAttrInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}}
+#else
+// expected-warning@+2{{'GlobalDeclChunkAttrInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#endif
+int *GlobalDeclChunkAttrInit = 0;
-int GlobalDeclAttrInit __attribute__((dllimport)); // expected-note{{previous declaration is here}} expected-note{{previous attribute is here}}
-int GlobalDeclAttrInit = 1; // expected-warning{{'GlobalDeclAttrInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#ifdef GNU
+// expected-note@+2{{previous attribute is here}}
+#endif
+int GlobalDeclAttrInit __attribute__((dllimport)); // expected-note{{previous declaration is here}}
+#ifdef MS
+// expected-warning@+4{{'GlobalDeclAttrInit' redeclared without 'dllimport' attribute: 'dllexport' attribute added}}
+#else
+// expected-warning@+2{{'GlobalDeclAttrInit' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
+#endif
+int GlobalDeclAttrInit = 1;
// Redeclarations
__declspec(dllimport) extern int GlobalRedecl1;
@@ -69,8 +101,6 @@
int GlobalRedecl2c __attribute__((dllimport));
int GlobalRedecl2c __attribute__((dllimport));
-// NB: MSVC issues a warning and makes GlobalRedecl3 dllexport. We follow GCC
-// and drop the dllimport with a warning.
__declspec(dllimport) extern int GlobalRedecl3; // expected-note{{previous declaration is here}} expected-note{{previous attribute is here}}
extern int GlobalRedecl3; // expected-warning{{'GlobalRedecl3' redeclared without 'dllimport' attribute: previous 'dllimport' ignored}}
@@ -135,11 +165,31 @@
template int __declspec(dllimport) VarTmplInit2 = 1; // expected-error{{definition of dllimport data}}
// Declare, then reject definition.
-template __declspec(dllimport) extern int ExternVarTmplDeclInit; // expected-note{{previous declaration is here}} expected-note{{previous attribut
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
rnk added a comment. lgtm http://reviews.llvm.org/D18953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 updated this revision to Diff 58233.
avt77 added a comment.
I built the project from scratch and found an issue in Clang build system.
There was a problem with undefined DiagGroup in DiagnosticSemaKinds.td: the
error message was generated but all executables were created that's why I was
not aware about this issue (I missed the message at the time of my first build
and all next re-builds worked without any problems). I created a new warning
group (MicrosoftInconsistentDllImport) in DiagnosticGroups.td and made the
corresponding changes in DiagnosticSemaKinds.td (2 lines).
Sorry for inconvenience but review the above two files and confirm your LGTM if
it is.
And I suppose I should create a new record in Bugzilla, right?
http://reviews.llvm.org/D18953
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaDecl.cpp
test/CodeGen/dllimport.c
test/CodeGenCXX/dllimport-members.cpp
test/CodeGenCXX/dllimport.cpp
test/Sema/dllimport.c
test/SemaCXX/dllimport.cpp
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -5523,9 +5523,13 @@
static void checkDLLAttributeRedeclaration(Sema &S, NamedDecl *OldDecl,
NamedDecl *NewDecl,
- bool IsSpecialization) {
- if (TemplateDecl *OldTD = dyn_cast(OldDecl))
+ bool IsSpecialization,
+ bool IsDefinition) {
+ if (TemplateDecl *OldTD = dyn_cast(OldDecl)) {
OldDecl = OldTD->getTemplatedDecl();
+if (!IsSpecialization)
+ IsDefinition = false;
+ }
if (TemplateDecl *NewTD = dyn_cast(NewDecl))
NewDecl = NewTD->getTemplatedDecl();
@@ -5581,30 +5585,43 @@
// A redeclaration is not allowed to drop a dllimport attribute, the only
// 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;
- if (const auto *VD = dyn_cast(NewDecl))
+ bool IsMicrosoft = S.Context.getTargetInfo().getCXXABI().isMicrosoft();
+ if (const auto *VD = dyn_cast(NewDecl)) {
// Ignore static data because out-of-line definitions are diagnosed
// separately.
IsStaticDataMember = VD->isStaticDataMember();
- else if (const auto *FD = dyn_cast(NewDecl)) {
+IsDefinition = VD->isThisDeclarationADefinition(S.Context) !=
+ VarDecl::DeclarationOnly;
+ } else if (const auto *FD = dyn_cast(NewDecl)) {
IsInline = FD->isInlined();
IsQualifiedFriend = FD->getQualifier() &&
FD->getFriendObjectKind() == Decl::FOK_Declared;
}
if (OldImportAttr && !HasNewAttr && !IsInline && !IsStaticDataMember &&
!NewDecl->isLocalExternDecl() && !IsQualifiedFriend) {
-S.Diag(NewDecl->getLocation(),
- 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();
-NewDecl->dropAttr();
- } else if (IsInline && OldImportAttr &&
- !S.Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+if (IsMicrosoft && IsDefinition) {
+ S.Diag(NewDecl->getLocation(),
+ diag::warn_redeclaration_without_import_attribute)
+ << NewDecl;
+ S.Diag(OldDecl->getLocation(), diag::note_previous_declaration);
+ NewDecl->dropAttr();
+ NewDecl->addAttr(::new (S.Context) DLLExportAttr(
+ NewImportAttr->getRange(), S.Context,
+ NewImportAttr->getSpellingListIndex()));
+} else {
+ S.Diag(NewDecl->getLocation(),
+ 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();
+ NewDecl->dropAttr();
+}
+ } else if (IsInline && OldImportAttr && !IsMicrosoft) {
// In MinGW, seeing a function declared inline drops the dllimport attribute.
OldDecl->dropAttr();
NewDecl->dropAttr();
@@ -6388,7 +6405,7 @@
if (D.isRedeclaration() && !Previous.empty()) {
checkDLLAttributeRedeclaration(
*this, dyn_cast(Previous.getRepresentativeDecl()), NewVD,
-IsExplicitSpecialization);
+IsExplicitSpecialization, D.isFunctionDefinition());
}
if (NewTemplate) {
@@ -8419,7 +8436,8 @@
if (D.isRedeclaration()
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm http://reviews.llvm.org/D18953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 added a comment. OK, as I see all issues were resolved, right? Could I commit the patch? http://reviews.llvm.org/D18953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
rnk added inline comments.
Comment at: test/SemaCXX/dllimport.cpp:179
@@ -140,1 +178,3 @@
+template
+int ExternVarTmplDeclInit = 1;
avt77 wrote:
> majnemer wrote:
> > avt77 wrote:
> > > rnk wrote:
> > > > Can you check with MSVC 2015 update 2 actually does with definitions of
> > > > dllimport variable templates? I bet it doesn't export them.
> > > They don't support variable templates at all:
> > >
> > > error C2399: variable templates are not supported in this release
> > Your compiler is too old, they are definitely supported.
> >
> > > Previously a template declaration was only allowed to be a function,
> > > class, or alias. Now, in the MSVC compiler it can be a variable as well.
> >
> > https://blogs.msdn.microsoft.com/vcblog/2016/02/11/compiler-improvements-in-vs-2015-update-2/
> OK, I updated several additional components and now CL supports variable
> templates. I checked the issue again and got the following:
>
> C:\_bugs>cl -c t1.cpp
> Microsoft (R) C/C++ Optimizing Compiler Version 19.00.23918 for x86
> Copyright (C) Microsoft Corporation. All rights reserved.
>
> t1.cpp
> t1.cpp(8): warning C4273: 'ExternVarTmplDeclInit': inconsistent dll linkage
> t1.cpp(2): note: see previous definition of 'ExternVarTmplDeclInit'
>
> C:\_bugs>dumpbin /directives t1.obj
> Microsoft (R) COFF/PE Dumper Version 14.00.23918.0
> Copyright (C) Microsoft Corporation. All rights reserved.
>
>
> Dump of file t1.obj
>
> File Type: COFF OBJECT
>
>Linker Directives
>-
>/DEFAULTLIB:LIBCMT
>/DEFAULTLIB:OLDNAMES
>/EXPORT:??$ExternVarTmplDeclInit@H@@3HA,DATA//
>
> As you see they produce warning and change the export attribute. I suppose
> Clang should do the same, right?
Yep, sounds good for now.
Comment at: test/SemaCXX/dllimport.cpp:1137
@@ -1017,1 +1136,3 @@
+template
+void ImportClassTmplMembers::normalDef() {}
#ifdef GNU
avt77 wrote:
> avt77 wrote:
> > rnk wrote:
> > > 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?
> > They prohibit such a definition in the latest MSVC:
> >
> > error C2491: 'ImportClassTmplMembers::normalDef': definition of
> > dllimport function not allowed
> >
> > The same error I see for other definitions as well
> With the latest compiler they disallow the definition of dllimport functions
> as well. I suppose Clang should do the same, right?
Seems like a weird special case. I wouldn't worry about addressing it right now.
http://reviews.llvm.org/D18953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 added inline comments.
Comment at: test/SemaCXX/dllimport.cpp:179
@@ -140,1 +178,3 @@
+template
+int ExternVarTmplDeclInit = 1;
majnemer wrote:
> avt77 wrote:
> > rnk wrote:
> > > Can you check with MSVC 2015 update 2 actually does with definitions of
> > > dllimport variable templates? I bet it doesn't export them.
> > They don't support variable templates at all:
> >
> > error C2399: variable templates are not supported in this release
> Your compiler is too old, they are definitely supported.
>
> > Previously a template declaration was only allowed to be a function, class,
> > or alias. Now, in the MSVC compiler it can be a variable as well.
>
> https://blogs.msdn.microsoft.com/vcblog/2016/02/11/compiler-improvements-in-vs-2015-update-2/
OK, I updated several additional components and now CL supports variable
templates. I checked the issue again and got the following:
C:\_bugs>cl -c t1.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.23918 for x86
Copyright (C) Microsoft Corporation. All rights reserved.
t1.cpp
t1.cpp(8): warning C4273: 'ExternVarTmplDeclInit': inconsistent dll linkage
t1.cpp(2): note: see previous definition of 'ExternVarTmplDeclInit'
C:\_bugs>dumpbin /directives t1.obj
Microsoft (R) COFF/PE Dumper Version 14.00.23918.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file t1.obj
File Type: COFF OBJECT
Linker Directives
-
/DEFAULTLIB:LIBCMT
/DEFAULTLIB:OLDNAMES
/EXPORT:??$ExternVarTmplDeclInit@H@@3HA,DATA//
As you see they produce warning and change the export attribute. I suppose
Clang should do the same, right?
Comment at: test/SemaCXX/dllimport.cpp:1137
@@ -1017,1 +1136,3 @@
+template
+void ImportClassTmplMembers::normalDef() {}
#ifdef GNU
avt77 wrote:
> rnk wrote:
> > 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?
> They prohibit such a definition in the latest MSVC:
>
> error C2491: 'ImportClassTmplMembers::normalDef': definition of dllimport
> function not allowed
>
> The same error I see for other definitions as well
With the latest compiler they disallow the definition of dllimport functions as
well. I suppose Clang should do the same, right?
http://reviews.llvm.org/D18953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
majnemer added a subscriber: majnemer. Comment at: test/SemaCXX/dllimport.cpp:179 @@ -140,1 +178,3 @@ +template +int ExternVarTmplDeclInit = 1; avt77 wrote: > rnk wrote: > > Can you check with MSVC 2015 update 2 actually does with definitions of > > dllimport variable templates? I bet it doesn't export them. > They don't support variable templates at all: > > error C2399: variable templates are not supported in this release Your compiler is too old, they are definitely supported. > Previously a template declaration was only allowed to be a function, class, > or alias. Now, in the MSVC compiler it can be a variable as well. https://blogs.msdn.microsoft.com/vcblog/2016/02/11/compiler-improvements-in-vs-2015-update-2/ http://reviews.llvm.org/D18953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 added a comment.
It seems the latest MSVC changed the support of several features covering in
this patch: more investigations needed.
Comment at: test/SemaCXX/dllimport.cpp:179
@@ -140,1 +178,3 @@
+template
+int ExternVarTmplDeclInit = 1;
rnk wrote:
> Can you check with MSVC 2015 update 2 actually does with definitions of
> dllimport variable templates? I bet it doesn't export them.
They don't support variable templates at all:
error C2399: variable templates are not supported in this release
Comment at: test/SemaCXX/dllimport.cpp:1137
@@ -1017,1 +1136,3 @@
+template
+void ImportClassTmplMembers::normalDef() {}
#ifdef GNU
rnk wrote:
> 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?
They prohibit such a definition in the latest MSVC:
error C2491: 'ImportClassTmplMembers::normalDef': definition of dllimport
function not allowed
The same error I see for other definitions as well
http://reviews.llvm.org/D18953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 added a comment. In fact you introduced the plan of actions to fix the issue. Tnx. http://reviews.llvm.org/D18953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
rsmith added a comment. In http://reviews.llvm.org/D18953#397279, @rnk wrote: > Richard, do you think we should be handling this by rewriting the AST-level > attribute in Sema or by changing our interpretation of things in CodeGen? > We're already creating a bunch of implicit attributes to implement > class-level import/export. Sorry, I forgot to answer this directly. I'm fine with either approach; they both seem to preserve source fidelity and allow AST clients to figure out what's going on without too much complexity. http://reviews.llvm.org/D18953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
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();
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@+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@+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
+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
+void ImportClassTmplMembers::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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 changed the visibility of this Differential Revision from "All Users" to
"Public (No Login Required)".
avt77 updated this revision to Diff 53870.
avt77 added a comment.
I fixed all issues discovered by Richard and Reid. As result the tests were
updated as well. Please, review again.
http://reviews.llvm.org/D18953
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaDecl.cpp
test/CodeGen/dllimport.c
test/CodeGenCXX/dllimport-members.cpp
test/CodeGenCXX/dllimport.cpp
test/Sema/dllimport.c
test/SemaCXX/dllimport.cpp
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -5505,9 +5505,13 @@
static void checkDLLAttributeRedeclaration(Sema &S, NamedDecl *OldDecl,
NamedDecl *NewDecl,
- bool IsSpecialization) {
- if (TemplateDecl *OldTD = dyn_cast(OldDecl))
+ bool IsSpecialization,
+ bool IsDefinition) {
+ if (TemplateDecl *OldTD = dyn_cast(OldDecl)) {
OldDecl = OldTD->getTemplatedDecl();
+if (!IsSpecialization)
+ IsDefinition = false;
+ }
if (TemplateDecl *NewTD = dyn_cast(NewDecl))
NewDecl = NewTD->getTemplatedDecl();
@@ -5563,30 +5567,43 @@
// A redeclaration is not allowed to drop a dllimport attribute, the only
// 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;
- if (const auto *VD = dyn_cast(NewDecl))
+ bool IsMicrosoft = S.Context.getTargetInfo().getCXXABI().isMicrosoft();
+ if (const auto *VD = dyn_cast(NewDecl)) {
// Ignore static data because out-of-line definitions are diagnosed
// separately.
IsStaticDataMember = VD->isStaticDataMember();
- else if (const auto *FD = dyn_cast(NewDecl)) {
+IsDefinition = !VD->isThisDeclarationADefinition(S.Context) ==
+ VarDecl::DeclarationOnly;
+ } else if (const auto *FD = dyn_cast(NewDecl)) {
IsInline = FD->isInlined();
IsQualifiedFriend = FD->getQualifier() &&
FD->getFriendObjectKind() == Decl::FOK_Declared;
}
if (OldImportAttr && !HasNewAttr && !IsInline && !IsStaticDataMember &&
!NewDecl->isLocalExternDecl() && !IsQualifiedFriend) {
-S.Diag(NewDecl->getLocation(),
- 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();
-NewDecl->dropAttr();
- } else if (IsInline && OldImportAttr &&
- !S.Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+if (IsMicrosoft && IsDefinition) {
+ S.Diag(NewDecl->getLocation(),
+ diag::warn_redeclaration_without_import_attribute)
+ << NewDecl;
+ S.Diag(OldDecl->getLocation(), diag::note_previous_declaration);
+ NewDecl->dropAttr();
+ NewDecl->addAttr(::new (S.Context) DLLExportAttr(
+ NewImportAttr->getRange(), S.Context,
+ NewImportAttr->getSpellingListIndex()));
+} else {
+ S.Diag(NewDecl->getLocation(),
+ 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();
+ NewDecl->dropAttr();
+}
+ } else if (IsInline && OldImportAttr && !IsMicrosoft) {
// In MinGW, seeing a function declared inline drops the dllimport attribute.
OldDecl->dropAttr();
NewDecl->dropAttr();
@@ -6370,7 +6387,7 @@
if (D.isRedeclaration() && !Previous.empty()) {
checkDLLAttributeRedeclaration(
*this, dyn_cast(Previous.getRepresentativeDecl()), NewVD,
-IsExplicitSpecialization);
+IsExplicitSpecialization, D.isFunctionDefinition());
}
if (NewTemplate) {
@@ -8369,7 +8386,8 @@
if (D.isRedeclaration() && !Previous.empty()) {
checkDLLAttributeRedeclaration(
*this, dyn_cast(Previous.getRepresentativeDecl()), NewFD,
-isExplicitSpecialization || isFunctionTemplateSpecialization);
+isExplicitSpecialization || isFunctionTemplateSpecialization,
+D.isFunctionDefinition());
}
if (getLangOpts().CUDA) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/Diagnost
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 marked an inline comment as done.
Comment at: lib/Sema/SemaDecl.cpp:5570
@@ -5565,3 +5569,3 @@
// and qualified friend declarations.
- // NB: MSVC converts such a declaration to dllexport.
+ // NB: MSVC converts such a declaration to dllexport that's why we do it
also.
bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
rsmith wrote:
> Just describe this as the semantics of this situation rather than suggesting
> this is some MSVC oddity we're emulating. "In such a case, the declaration is
> treated as if it were marked dllexport." or similar.
>
> It also seems bizarre for this behavior to apply for local extern
> declarations and qualified friend declarations. Does the "dllimport gets
> turned into dllexport" behavior actually only apply to the definition case?
> And does the definition need to be inline?
Yes, I see "dllimport gets turned into dllexport" for definitions only.
No, the definition should not be inline:
if (OldImportAttr && !HasNewAttr && **!IsInline**
Comment at: lib/Sema/SemaDecl.cpp:5595
@@ +5594,3 @@
+ // Replace DLLImportAttr with DLLExportAttr
+ OldDecl->dropAttr();
+ NewDecl->dropAttr();
rsmith wrote:
> Don't change attributes on a prior declaration; AST nodes should generally be
> immutable once created (this would lose source fidelity, and break under
> PCH/modules). Instead, make sure that anyone who looks at this gets the
> attribute from the appropriate (most recent) declaration and only change the
> attributes there.
I don't understand how I could "make sure that anyone...". Please, clarify.
Comment at: lib/Sema/SemaDecl.cpp:5596
@@ +5595,3 @@
+ OldDecl->dropAttr();
+ NewDecl->dropAttr();
+ OldDecl->addAttr(::new (S.Context) DLLExportAttr(
rsmith wrote:
> Is this really valid and treated as `__dllexport` if the new declaration
> explicitly specifies `__dllimport` (rather than inheriting it)?
The new declaration does not have explicitly specified __dllimport attribute:
if (OldImportAttr && **!HasNewAttr**. It's inherited.
Comment at: lib/Sema/SemaDecl.cpp:5600-5602
@@ +5599,5 @@
+ OldImportAttr->getSpellingListIndex()));
+ NewDecl->addAttr(::new (S.Context) DLLExportAttr(
+ NewImportAttr->getRange(), S.Context,
+ NewImportAttr->getSpellingListIndex()));
+} else {
rsmith wrote:
> The new attribute should be marked implicit.
What do you mean? Please, clarify.
http://reviews.llvm.org/D18953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
rsmith added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:5570
@@ -5565,3 +5569,3 @@
// and qualified friend declarations.
- // NB: MSVC converts such a declaration to dllexport.
+ // NB: MSVC converts such a declaration to dllexport that's why we do it
also.
bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
Just describe this as the semantics of this situation rather than suggesting
this is some MSVC oddity we're emulating. "In such a case, the declaration is
treated as if it were marked dllexport." or similar.
It also seems bizarre for this behavior to apply for local extern declarations
and qualified friend declarations. Does the "dllimport gets turned into
dllexport" behavior actually only apply to the definition case? And does the
definition need to be inline?
Comment at: lib/Sema/SemaDecl.cpp:5595
@@ +5594,3 @@
+ // Replace DLLImportAttr with DLLExportAttr
+ OldDecl->dropAttr();
+ NewDecl->dropAttr();
Don't change attributes on a prior declaration; AST nodes should generally be
immutable once created (this would lose source fidelity, and break under
PCH/modules). Instead, make sure that anyone who looks at this gets the
attribute from the appropriate (most recent) declaration and only change the
attributes there.
Comment at: lib/Sema/SemaDecl.cpp:5596
@@ +5595,3 @@
+ OldDecl->dropAttr();
+ NewDecl->dropAttr();
+ OldDecl->addAttr(::new (S.Context) DLLExportAttr(
Is this really valid and treated as `__dllexport` if the new declaration
explicitly specifies `__dllimport` (rather than inheriting it)?
Comment at: lib/Sema/SemaDecl.cpp:5600-5602
@@ +5599,5 @@
+ OldImportAttr->getSpellingListIndex()));
+ NewDecl->addAttr(::new (S.Context) DLLExportAttr(
+ NewImportAttr->getRange(), S.Context,
+ NewImportAttr->getSpellingListIndex()));
+} else {
The new attribute should be marked implicit.
http://reviews.llvm.org/D18953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
rnk added a comment.
Richard, do you think we should be handling this by rewriting the AST-level
attribute in Sema or by changing our interpretation of things in CodeGen? We're
already creating a bunch of implicit attributes to implement class-level
import/export.
Comment at: lib/Sema/SemaDecl.cpp:5572
@@ -5567,2 +5571,3 @@
bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
- if (const auto *VD = dyn_cast(NewDecl))
+ bool isMicrosoft = S.Context.getTargetInfo().getCXXABI().isMicrosoft();
+ if (const auto *VD = dyn_cast(NewDecl)) {
To be consistent, this should be `IsMicrosoft`.
http://reviews.llvm.org/D18953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported
avt77 created this revision.
avt77 added a reviewer: rnk.
avt77 added a subscriber: cfe-commits.
avt77 changed the visibility of this Differential Revision from "Public (No
Login Required)" to "All Users".
If we have some function with dllimport attribute and then we have the function
definition in the same module but without dllimport attribute we should add
dllexport attribute to this function definition. The same should be done for
variables. Example:
struct __declspec(dllimport) C3 {
~C3();
};
C3::~C3() {;} // we should export this definition
http://reviews.llvm.org/D18953
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaDecl.cpp
test/CodeGen/dllimport.c
test/CodeGenCXX/dllimport-members.cpp
test/CodeGenCXX/dllimport.cpp
test/Sema/dllimport.c
test/SemaCXX/dllimport.cpp
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -5504,9 +5504,13 @@
static void checkDLLAttributeRedeclaration(Sema &S, NamedDecl *OldDecl,
NamedDecl *NewDecl,
- bool IsSpecialization) {
- if (TemplateDecl *OldTD = dyn_cast(OldDecl))
+ bool IsSpecialization,
+ bool IsDefinition = false) {
+ if (TemplateDecl *OldTD = dyn_cast(OldDecl)) {
OldDecl = OldTD->getTemplatedDecl();
+if (!IsSpecialization)
+ IsDefinition = false;
+ }
if (TemplateDecl *NewTD = dyn_cast(NewDecl))
NewDecl = NewTD->getTemplatedDecl();
@@ -5563,29 +5567,49 @@
// A redeclaration is not allowed to drop a dllimport attribute, the only
// exceptions being inline function definitions, local extern declarations,
// and qualified friend declarations.
- // NB: MSVC converts such a declaration to dllexport.
+ // NB: MSVC converts such a declaration to dllexport that's why we do it also.
bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
- if (const auto *VD = dyn_cast(NewDecl))
+ bool isMicrosoft = S.Context.getTargetInfo().getCXXABI().isMicrosoft();
+ if (const auto *VD = dyn_cast(NewDecl)) {
// Ignore static data because out-of-line definitions are diagnosed
// separately.
IsStaticDataMember = VD->isStaticDataMember();
+IsDefinition =
+ !VD->isThisDeclarationADefinition(S.Context) == VarDecl::DeclarationOnly;
+ }
else if (const auto *FD = dyn_cast(NewDecl)) {
IsInline = FD->isInlined();
IsQualifiedFriend = FD->getQualifier() &&
FD->getFriendObjectKind() == Decl::FOK_Declared;
}
if (OldImportAttr && !HasNewAttr && !IsInline && !IsStaticDataMember &&
!NewDecl->isLocalExternDecl() && !IsQualifiedFriend) {
-S.Diag(NewDecl->getLocation(),
- 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();
-NewDecl->dropAttr();
- } else if (IsInline && OldImportAttr &&
- !S.Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+if (isMicrosoft && IsDefinition) {
+ S.Diag(NewDecl->getLocation(),
+ diag::warn_redeclaration_without_import_attribute) << NewDecl;
+ S.Diag(OldDecl->getLocation(), diag::note_previous_declaration);
+ S.Diag(OldImportAttr->getLocation(),
+ diag::note_previous_attribute_replaced);
+ // Replace DLLImportAttr with DLLExportAttr
+ OldDecl->dropAttr();
+ NewDecl->dropAttr();
+ OldDecl->addAttr(::new (S.Context) DLLExportAttr(
+ OldImportAttr->getRange(), S.Context,
+ OldImportAttr->getSpellingListIndex()));
+ NewDecl->addAttr(::new (S.Context) DLLExportAttr(
+ NewImportAttr->getRange(), S.Context,
+ NewImportAttr->getSpellingListIndex()));
+} else {
+ S.Diag(NewDecl->getLocation(),
+ 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();
+ NewDecl->dropAttr();
+}
+ } else if (IsInline && OldImportAttr && !isMicrosoft) {
// In MinGW, seeing a function declared inline drops the dllimport attribute.
OldDecl->dropAttr();
NewDecl->dropAttr();
@@ -6369,7 +6393,8 @@
if (D.isRedeclaration() && !Previous.empty()) {
checkDLLAttributeRedeclaration(
*this, dyn_cast(Previous.getRepresentativeDecl()), NewVD,
-IsExplicitSpecialization);
+IsExplicitSpecialization,
+D.isFunctionDefinition());
}
if (NewTemplate) {
@@ -8368,7 +8393,8 @@
if (D.isRedeclaration() && !Prev
