Page MenuHomePhabricator

Teach LTOModule to emit linker flags for dllexported symbols, plus interface cleanup.
ClosedPublic

Authored by pcc on Jun 18 2015, 1:50 PM.

Details

Summary

This change unifies how LTOModule and the backend obtain linker flags
for globals: via a new TargetLoweringObjectFile member function named
emitLinkerFlagsForGlobal. A new function LTOModule::getLinkerOpts() returns
the list of linker flags as a single concatenated string.

This change affects the C libLTO API: the function lto_module_get_*deplibs now
exposes an empty list, and lto_module_get_*linkeropts exposes a single element
which combines the contents of all observed flags. libLTO should never have
tried to parse the linker flags; it is the linker's job to do so. Because
linkers will need to be able to parse flags in regular object files, it
makes little sense for libLTO to have a redundant mechanism for doing so.

The new API is compatible with the old one. It is valid for a user to specify
multiple linker flags in a single pragma directive like this:

#pragma comment(linker, "/defaultlib:foo /defaultlib:bar")

The previous implementation would not have exposed
either flag via lto_module_get_*deplibs (as the test in
TargetLoweringObjectFileCOFF::getDepLibFromLinkerOpt was case sensitive)
and would have exposed "/defaultlib:foo /defaultlib:bar" as a single flag via
lto_module_get_*linkeropts. This may have been a bug in the implementation,
but it does give us a chance to fix the interface.

Diff Detail

Event Timeline

pcc updated this revision to Diff 27960.Jun 18 2015, 1:50 PM
pcc retitled this revision from to Teach LTOModule to emit linker flags for dllexported symbols, plus interface cleanup..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added reviewers: rnk, ygao.
pcc added subscribers: ruiu, Unknown Object (MLST).
rafael accepted this revision.Jun 23 2015, 3:58 AM
rafael added a reviewer: rafael.
rafael added a subscriber: rafael.

I really like this change.

The less IR specific logic we have for LTO the better.

RNK, would you mind a second review from a COFF perspective?

This revision is now accepted and ready to land.Jun 23 2015, 3:58 AM
rnk accepted this revision.Jun 29 2015, 1:15 PM
rnk edited edge metadata.

lgtm

To my knowledge, the only client of the dependent lib API was the Sony linker. I think they should be able to tolerate this change by parsing defaultlib internally. I don't think ld64 will be affected.

include/llvm-c/lto.h
296 ↗(On Diff #27960)

"Now always returns null."?

lib/CodeGen/TargetLoweringObjectFileImpl.cpp
995 ↗(On Diff #27960)

Wow, this seems too case sensitive.

test/CodeGen/X86/dllexport-x86_64.ll
74 ↗(On Diff #27960)

Can you use CHECK-SAME to keep the strings on separate lines like before? IMO it's easier to read and update.

This revision was automatically updated to reflect the committed changes.
pcc added inline comments.Jun 29 2015, 3:04 PM
include/llvm-c/lto.h
296 ↗(On Diff #27960)

Done

test/CodeGen/X86/dllexport-x86_64.ll
74 ↗(On Diff #27960)

Done

ygao added inline comments.Jun 29 2015, 3:04 PM
include/llvm-c/lto.h
286 ↗(On Diff #27960)

I'm wondering whether it might be better to remove these two functions instead of changing their behaviors silently.

lib/LTO/LTOModule.cpp
655 ↗(On Diff #27960)

Is it safe to concatenate all the linker options into one? What if the #pragma comment(lib, ...) or #pragma comment(linker, ...) string also contains space characters?

pcc added inline comments.Jun 29 2015, 3:27 PM
include/llvm-c/lto.h
286 ↗(On Diff #27960)

Works for me. I think your linker is the only one that depends on this interface, so if it wouldn't cause you too much pain I'd rather just do that.

lib/LTO/LTOModule.cpp
655 ↗(On Diff #27960)

In a regular object file, the contents of linker pragmas are concatenated with space characters between them before being emitted in the .directve section; i.e. the compiler does not quote the individual string literals that appear in the pragmas, they just appear as a bag of bytes in the section. lib pragmas are quoted early on if needed but go through the same mechanism as linker (see e.g. qualifyWindowsLibrary in clang). This means that there is no difference at the object file level between

#pragma comment(linker, "/defaultlib:foo /defaultlib:bar")

and

#pragma comment(linker, "/defaultlib:foo")
#pragma comment(linker, "/defaultlib:bar")

This is reflected in the LTO APIs.

ygao added inline comments.Jun 29 2015, 3:51 PM
include/llvm-c/lto.h
286 ↗(On Diff #27960)

I think I'd prefer to remove these two functions if no other client would be affected. Probably will have to bump up LTO_API_VERSION.
And with your change, lto_module_get_num_linkeropt() will always return 1, and therefore is not too useful either.

lib/LTO/LTOModule.cpp
655 ↗(On Diff #27960)

I was thinking of use cases like:

#pragma comment(lib, "spaced library");
#pragma comment(lib, "strange names /include:slashes");

It may have already been handled by the front end. Not sure.

pcc added inline comments.Jun 29 2015, 3:56 PM
include/llvm-c/lto.h
286 ↗(On Diff #27960)

Okay, I'll send out a separate change to clean up those interfaces.

lib/LTO/LTOModule.cpp
655 ↗(On Diff #27960)

Yes, as I mentioned in my first message lib pragmas are quoted by the front end. Sorry if that wasn't clear.

ygao edited edge metadata.Jul 29 2015, 6:01 PM

Hi Peter,
I'd like to point out a corner case:

#pragma comment(lib, "lib1\00lib2")
#pragma comment(lib, "lib3")

Testing on Visual Studio 2013, the intended behavior should be to auto-link with lib1.lib and lib3.lib,
but not lib2.
With the new LTO API, looking at the return value from lto_module_get_linkeropts(), the embedded
null character after lib1 seems to remove lib3 from the library list.

I am guessing that the fix is probably to make ParsePragma truncate the first string earlier,

  • Gao
pcc added a comment.Jul 29 2015, 6:08 PM

I am guessing that the fix is probably to make ParsePragma truncate the first string earlier,

That sounds correct, assuming that is how the directive looks in the object file.

espindola edited reviewers, added: espindola; removed: rafael.Mar 14 2018, 5:03 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 7:55 AM
ruiu added inline comments.Oct 7 2019, 10:09 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1038–1041

This code is now new, but I wonder if we need to distinguish MinGW and MSVC here because they both recognize -EXPORT. I believe in many projects option /foo and -foo are used interchangeably.

1057–1060

Ditto -- I believe both should work with DATA.