This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by avt77 on Apr 11 2016, 3:59 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

avt77 updated this revision to Diff 53212.Apr 11 2016, 3:59 AM
avt77 retitled this revision from to [ms][dll] #26935 Defining a dllimport function should cause it to be exported.
avt77 updated this object.
avt77 added a reviewer: rnk.
avt77 changed the visibility from "Public (No Login Required)" to "All Users".
avt77 added a subscriber: cfe-commits.
rnk added a reviewer: rsmith.Apr 11 2016, 9:43 AM
rnk edited edge metadata.Apr 11 2016, 10:16 AM

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.

lib/Sema/SemaDecl.cpp
5572 ↗(On Diff #53212)

To be consistent, this should be IsMicrosoft.

rsmith added inline comments.Apr 11 2016, 11:27 AM
lib/Sema/SemaDecl.cpp
5570 ↗(On Diff #53212)

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?

5595 ↗(On Diff #53212)

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.

5596 ↗(On Diff #53212)

Is this really valid and treated as __dllexport if the new declaration explicitly specifies __dllimport (rather than inheriting it)?

5600–5602 ↗(On Diff #53212)

The new attribute should be marked implicit.

avt77 marked an inline comment as done.Apr 12 2016, 3:32 AM
avt77 added inline comments.
lib/Sema/SemaDecl.cpp
5570 ↗(On Diff #53212)

Yes, I see "dllimport gets turned into dllexport" for definitions only.
No, the definition should not be inline:

if (OldImportAttr && !HasNewAttr && **!IsInline**
5595 ↗(On Diff #53212)

I don't understand how I could "make sure that anyone...". Please, clarify.

5596 ↗(On Diff #53212)

The new declaration does not have explicitly specified __dllimport attribute:
if (OldImportAttr && !HasNewAttr. It's inherited.

5600–5602 ↗(On Diff #53212)

What do you mean? Please, clarify.

avt77 updated this revision to Diff 53870.Apr 15 2016, 4:06 AM
avt77 edited edge metadata.
avt77 changed the visibility from "All Users" to "Public (No Login Required)".

I fixed all issues discovered by Richard and Reid. As result the tests were updated as well. Please, review again.

rnk added a comment.Apr 20 2016, 11:39 AM

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

lib/Sema/SemaDecl.cpp
5570–5571 ↗(On Diff #53870)

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.

5595 ↗(On Diff #53870)

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.

5600–5602 ↗(On Diff #53870)

Use DLLExportAttr::CreateImplicit or Attr->setImplicit(true).

test/Sema/dllimport.c
52 ↗(On Diff #53870)

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}}
test/SemaCXX/dllimport.cpp
62 ↗(On Diff #53870)

ditto, after the first MS-specific diagnostic test case we don't need the duplication

179 ↗(On Diff #53870)

Can you check with MSVC 2015 update 2 actually does with definitions of dllimport variable templates? I bet it doesn't export them.

1137 ↗(On Diff #53870)

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?

rsmith edited edge metadata.Apr 20 2016, 3:18 PM
In 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.

avt77 added a comment.Apr 26 2016, 3:20 AM

In fact you introduced the plan of actions to fix the issue. Tnx.

avt77 added a comment.Apr 26 2016, 8:22 AM

It seems the latest MSVC changed the support of several features covering in this patch: more investigations needed.

test/SemaCXX/dllimport.cpp
179 ↗(On Diff #53870)

They don't support variable templates at all:

error C2399: variable templates are not supported in this release

1137 ↗(On Diff #53870)

They prohibit such a definition in the latest MSVC:

error C2491: 'ImportClassTmplMembers<T>::normalDef': definition of dllimport function not allowed

The same error I see for other definitions as well

majnemer added inline comments.
test/SemaCXX/dllimport.cpp
179 ↗(On Diff #53870)

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/

avt77 added inline comments.Apr 28 2016, 4:27 AM
test/SemaCXX/dllimport.cpp
179 ↗(On Diff #53870)

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?

1137 ↗(On Diff #53870)

With the latest compiler they disallow the definition of dllimport functions as well. I suppose Clang should do the same, right?

rnk added inline comments.May 13 2016, 4:01 PM
test/SemaCXX/dllimport.cpp
179 ↗(On Diff #53870)

Yep, sounds good for now.

1137 ↗(On Diff #53870)

Seems like a weird special case. I wouldn't worry about addressing it right now.

avt77 added a comment.May 23 2016, 3:52 AM

OK, as I see all issues were resolved, right?
Could I commit the patch?

rnk accepted this revision.May 23 2016, 8:57 AM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.May 23 2016, 8:57 AM
avt77 updated this revision to Diff 58233.May 24 2016, 7:35 AM
avt77 edited edge metadata.

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?

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 17 2017, 1:44 PM
thakis added inline comments.
cfe/trunk/lib/Sema/SemaDecl.cpp
5650

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.