Page MenuHomePhabricator

[COFF] Don't error if the only inputs are from /wholearchive:
ClosedPublic

Authored by rnk on Nov 7 2019, 1:52 PM.

Diff Detail

Event Timeline

rnk created this revision.Nov 7 2019, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2019, 1:52 PM
amccarth requested changes to this revision.Nov 12 2019, 9:50 AM

This looks like the right approach, but I have a concern or two about the ramifications as noted inline. Let me know if my concerns are unwarranted.

lld/COFF/Driver.cpp
1176

I don't entirely understand the meaning of noEntry here, but your change affects whether this happens or not. Is that important? I don't know. See my comment on your new test.

1752–1753

This seems like a possible problem. Before, you couldn't get here without OPT_INPUTs--you either had some, or you had a DEF file which would provide the output file name. Now we can get here with just an archive, so we might be dereferencing an iterator at the beginning of an empty list.

lld/test/COFF/wholearchive.s
18

I think the -out flag is preventing the potential crasher I mentioned above. Can you try it without the -out?

Maybe we need one with a DEF file as well.

This revision now requires changes to proceed.Nov 12 2019, 9:50 AM

Somewhat related to the comments, I believe lld-link's ability to generate an import library from a def file directly is non-standard. MSVC's toolchain requires you to go through lib for that.

rnk marked 3 inline comments as done.Nov 15 2019, 3:03 PM
rnk added inline comments.
lld/COFF/Driver.cpp
1703

This also ended up being a bug caught by the entry inference test.

1752–1753

Thanks, that's a real bug. I checked the Visual C++ linker's behavior, and they name the output after the first wholearchive argument. I did this: link -wholearchive:foo.lib and it produced foo.exe.

lld/test/COFF/wholearchive.s
18

I updated two existing tests that deal with infering things from inputs:

  1. out.test, this infers the .exe name from the wholearchive name
  2. entry-inference.test, this deals with inferring the entry point, which interacts with deffile and noentry.

I don't think the case of -def: -wholearchive: is interesting, the def file is going to control the name of the DLL in that case.

rnk updated this revision to Diff 229649.Nov 15 2019, 3:03 PM
  • fix bugs
amccarth accepted this revision.Nov 15 2019, 3:20 PM
This revision is now accepted and ready to land.Nov 15 2019, 3:20 PM
This revision was automatically updated to reflect the committed changes.