This is an archive of the discontinued LLVM Phabricator instance.

Allow dllimport/dllexport on inline functions and get the linkage right
ClosedPublic

Authored by hans on May 14 2014, 3:29 PM.

Diff Detail

Event Timeline

hans updated this revision to Diff 9405.May 14 2014, 3:29 PM
hans retitled this revision from to Allow dllimport/dllexport on inline functions and get the linkage right.
hans updated this object.
hans edited the test plan for this revision. (Show Details)
hans added reviewers: majnemer, rnk, nrieck.
hans added a subscriber: Unknown Object (MLST).
majnemer accepted this revision.May 14 2014, 3:39 PM
majnemer edited edge metadata.

LGTM.

This revision is now accepted and ready to land.May 14 2014, 3:39 PM
rnk added inline comments.May 14 2014, 3:53 PM
lib/CodeGen/CodeGenModule.cpp
1966

So, this is weird. We're saying that for discardable things, we need to use a linkage that prohibits discarding them.

Maybe it would be better to change the GVA linkage of dllexported things?

test/CodeGen/dllexport.cpp
1 ↗(On Diff #9405)

This should be in test/CodeGenCXX

21 ↗(On Diff #9405)

Sure, it seems fine to split this out as a separate change.

test/CodeGen/dllimport.cpp
1 ↗(On Diff #9405)

This should be in test/CodeGenCXX

rnk added inline comments.May 14 2014, 4:17 PM
lib/CodeGen/CodeGenModule.cpp
1991–1995

Hm, I think I wrote this. I probably should have pushed this up into ASTContext::GetGVALinkageForVariable() to make it return GVA_StrongODR.

hans added inline comments.May 14 2014, 5:24 PM
lib/CodeGen/CodeGenModule.cpp
1966

Yeah, fixing the GVA linkage is probably the right thing to do. I'll give it a shot.

test/CodeGen/dllexport.cpp
1 ↗(On Diff #9405)

Done.

test/CodeGen/dllimport.cpp
1 ↗(On Diff #9405)

Done.

hans updated this revision to Diff 9407.May 14 2014, 5:26 PM
hans edited edge metadata.

Moved the tests and moving the DLL-linkage logic to GVA computation.

rnk accepted this revision.May 14 2014, 6:33 PM
rnk edited edge metadata.

LGTM with a comment

lib/AST/ASTContext.cpp
7799

This should have a comment about the semantics of dllimport and dllexport, and link to the MSDN doc on dllexport with inline functions:
http://msdn.microsoft.com/en-us/library/xa0d9ste.aspx

I'm a bit concerned with moving the DLLImport/DLLExport related linkage calculation into Sema out of CodeGen. I have a feeling that this will trigger warn_static_local_in_extern_inline when a static local variable inside of a dllimport'd function.

Considering that other things like VTables and RTTI don't rely on the GVALinakge mechanism and will need the ability to "upgrade" some linkage to a DLLImport/DLLExport friendly linkage, I think I'd rather see a DLLImport/DLLExport handled via a function which takes an arbitrary llvm::GlobalVariable linkage and a Decl and figures out what the final linkage should be.

This kind of upgrading is common inside of CodeGen, GetAddrOfGlobalTemporary is a notable example of this.

ygao added a subscriber: ygao.May 14 2014, 11:14 PM
ygao added inline comments.May 14 2014, 11:22 PM
include/clang/Basic/DiagnosticSemaKinds.td
2095

Typo. "cannot by applied" => "cannot be applied"

lib/AST/ASTContext.cpp
7799

If the first argument to fixGVALinkageForDLLAttribute() is always the return result of
basicGVALinkageForFunction(), I wonder maybe it is more readable to just call the
basic linkage function from the fix linkage function.

Do we still check somewhere that we do not accept the combination of C99/GNU-inline with dllexport/dllimport?

lib/Sema/SemaDecl.cpp
9767

Do you have a test case for the isInherited() case where the dllimport definition should be allowed?

hans added inline comments.May 15 2014, 11:31 AM
include/clang/Basic/DiagnosticSemaKinds.td
2095

Thanks for catching! Fixed.

lib/AST/ASTContext.cpp
7799

No, with this patch we allow dllimport/export on all inline functions.

Reid pointed out that we would get the linkage wrong for exported functions in C99 mode, and that the reasonable thing to do would be to use MS semantics dllimport/export inline functions. I'll update the patch to do this.

lib/Sema/SemaDecl.cpp
9767

The original code had it, so I figured it was important for some reason.

The check goes all the way back to r61437, and it's not entirely clear what is was for even then. Back then it was possible to have both dllimport and dllexport attributes on the same function, and I suspect the isInherited check has to do with that.

I don't think that check is correct today, so I'm going to remove it and simplify the code here a bit.

hans updated this revision to Diff 9450.May 15 2014, 11:32 AM
hans edited edge metadata.

Fix typo, remove isInherited() check and use MS semantics for inline functions with dllexport in C.

hans added a comment.May 15 2014, 11:51 AM

Regarding handling the dll linkage in AST or Codegen, I was kind of hoping doing it in AST would help in functions such as ASTContext::DeclMustBeEmitted (but it currently doesn't).

I don't have enough experience here to feel strongly either way, but I do suspect that it will become clearer which solution is best as we get further along the train of patches towards full support for class-level dllimport/dllexport.

I would like to go ahead and commit this now, and am totally open to moving the linkage adjusting code if it seems like a good idea as we go forward.

rnk added inline comments.May 15 2014, 11:55 AM
lib/AST/ASTContext.cpp
7778

Sorry to run you around, but I tested and realized this is inconsistent with what GCC does. GCC seems to treat dllexport similar to extern. I recall Nico Rieck had a patch titled "Sema: Treat dllimport globals without explicit storage class as extern" that did somethign like this.

GCC emits a strong external definition for this code:
inline attribute((dllexport)) void foo() {}

We should do the same.

rnk added a comment.May 15 2014, 12:34 PM

LGTM

lib/AST/ASTContext.cpp
7778

Based on offline discussion, I'm OK punting on this for now. It's not like we supported it before. Please just put a FIXME here that this is ABI incompatible with GCC.

hans added a reviewer: rsmith.May 15 2014, 1:02 PM

We're having trouble reaching agreement about whether to adjust the GVALinkage for dllimport/export, or doing it in CodeGen as in the first version of the patch.

It would be interesting to hear your opinion on this.

ygao added a comment.May 15 2014, 1:38 PM

Reid pointed out that we would get the linkage wrong for exported functions in C99 mode, and that the reasonable thing to do would be to use MS semantics dllimport/export inline functions. I'll update the patch to do this.

@rnk. If I recall correctly, Microsoft does not support C99 inline. What does MinGw(64) do in this case?

hans closed this revision.May 15 2014, 3:15 PM

I've gone ahead and committed this in r208925.

Richard didn't feel strongly about either direction for the linkage adjustment.

I'm happy to change this either way if Reid and David wants, and it's possible that new information comes to light as we go down this dll rabbit hole.