This is an archive of the discontinued LLVM Phabricator instance.

[LLD] [COFF] Add support for the -wholearchive option
ClosedPublic

Authored by mstorsjo on Sep 11 2017, 1:58 PM.

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 11 2017, 1:58 PM
ruiu added inline comments.Sep 11 2017, 2:12 PM
COFF/Options.td
101–102

Since they are lld-specific flags, I'd name -lld-whole-archive and -lld-no-whole-archive.

pcc added inline comments.Sep 11 2017, 2:17 PM
COFF/Options.td
101–102

I don't think you need to add lld-specific flags to support --whole-archive. Instead, you could implement support for the equivalent link.exe flag, /wholearchive. See also PR31824.

mstorsjo added inline comments.Sep 11 2017, 11:16 PM
COFF/Options.td
101–102

@pcc - Oh, that probably makes sense - I hadn't seen that option before. Then the MinGW driver just needs to inject -wholearchive: before each library between --whole-archive and --no-whole-archive. Since the driver doesn't do file magic detection, it probably needs to only match this for *.a, *.lib and -lfoo entries, but that's probably fine for most sensible use cases.

pcc added inline comments.Sep 11 2017, 11:38 PM
COFF/Options.td
101–102

Right. I think you wouldn't even need to do the matching; MSVC link.exe allows /wholearchive to be used with object files, and we can do the same.

mstorsjo updated this revision to Diff 114880.Sep 12 2017, 12:49 PM
mstorsjo edited the summary of this revision. (Show Details)

Adjusted the COFF driver to implement the link.exe option -wholearchive instead.

ruiu edited edge metadata.Sep 12 2017, 1:29 PM

Generally looking good.

Could you split this patch into two, one for COFF and one for MinGW?

COFF/Driver.cpp
124–125

nit: add a blank line befor ethis line.

124–125

It's not new code, but can you write it in two lines?

Symtab->addFile(make<ArchiveFile>(MBRef));
return;

Because addFile returns void and this function returns void too, it is legal to write return Symtab->addFile(...), but this is still an odd feature of C++, and I believe we generally want to avoid this unless it's generated as instantiated templates.

125–142

Likewise, separate into two lines.

978

Replacing WholeArchive with Args.hasArg(OPT_wholearchive_flag) might improve readability.

MinGW/Driver.cpp
175 ↗(On Diff #114880)

I think I'd name this just Prefix as we generally prefer shorter variable names in lld.

183 ↗(On Diff #114880)

80 cols (please run clang-format if you have installed it.)

In D37709#868490, @ruiu wrote:

Could you split this patch into two, one for COFF and one for MinGW?

Sure, will repost with it split, and with your suggestions applied.

COFF/Driver.cpp
124–125

Sure, I can fix that at the same time.

mstorsjo updated this revision to Diff 114900.Sep 12 2017, 1:46 PM
mstorsjo retitled this revision from [MinGW] Add support for the options --[no-]whole-archive to [LLD] [COFF] Add support for the -wholearchive option.
mstorsjo edited the summary of this revision. (Show Details)

Split the patch, moving the MinGW specific parts to a separate patch. Applied Rui's style suggestions.

ruiu accepted this revision.Sep 12 2017, 1:55 PM

LGTM

COFF/Driver.cpp
131

nit: I'd rename Member M.

132

Instead of "*", let's pass something like "<whole-archive>" instead.

This revision is now accepted and ready to land.Sep 12 2017, 1:55 PM
pcc added inline comments.Sep 12 2017, 2:13 PM
COFF/Driver.cpp
233

Are you sure that /wholearchive has no effect on archives specified by linker directives?

978

Wouldn't that create O(N^2) complexity in the size of the argument list though?

mstorsjo added inline comments.Sep 12 2017, 2:19 PM
COFF/Driver.cpp
233

No, I can try to construct a test to figure out how it's supposed to behave.

mstorsjo added inline comments.Sep 13 2017, 12:26 AM
COFF/Driver.cpp
233

I checked - link.exe doesn't apply the -wholearchive option to -defaultlib options, neither on the command line nor in directives. So I'll update the handling of the command line version of -defaultlib accordingly as well.

978

So it seems, based on reading ArgList.h. Will push with this changed back to a bool WholeArchiveFlag variable, to avoid the O(N^2) complexity.

This revision was automatically updated to reflect the committed changes.