Page MenuHomePhabricator

COFF: Introduce LD shim around LINK
ClosedPublic

Authored by martell on Jun 4 2017, 11:41 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ruiu added a comment.Aug 4 2017, 2:42 AM

Please rename the directory MinGW, as MINGW is not an acronym.T

MINGW/Driver.cpp
36 ↗(On Diff #109695)

This should be mingw instead of coff.

Since you are writing code that is not in COFF directory, do not depend on anything in COFF directory, except elf::coff::link. Please remove dependency to COFF directory.

If you're still too busy and don't mind, I could also take over trying to get this in a mergeable form.

I can be on this to ensure it gets merged, there were 2 reasons why I left this slide for awhile.
I was having an issue with llvm-dlltool where if I used new to create memory for the strings it would work but if I done it the correct way as rui instructed it was not working correctly.
We now know that was because we were writing the strings beforehand the entry thanks to you :) So the 2 bugs because of the location of the the allocation seemed to nullify each other to some degree.
Which left me a little confused. So I left the lld support slide until llvm-dlltool was merged and also at the time I had started a new job.

I can still be somewhat useful here because I understand what is going on.
Where I probably won't be of any use is on the unwind exceptions, so might be an idea to commandeer that differential?
I'm sure this is something you know a lot more about and I can probably learn a lot from the patch that gets merged for it.

Also I am not sure how extreme I should go with test cases here, how many existing tests will I amend to test the mingw driver.

If you're still too busy and don't mind, I could also take over trying to get this in a mergeable form.

I can be on this to ensure it gets merged, there were 2 reasons why I left this slide for awhile.
I was having an issue with llvm-dlltool where if I used new to create memory for the strings it would work but if I done it the correct way as rui instructed it was not working correctly.
We now know that was because we were writing the strings beforehand the entry thanks to you :) So the 2 bugs because of the location of the the allocation seemed to nullify each other to some degree.
Which left me a little confused. So I left the lld support slide until llvm-dlltool was merged and also at the time I had started a new job.

Sure - it's great to have at least one of the components merged finally :-)

I can still be somewhat useful here because I understand what is going on.
Where I probably won't be of any use is on the unwind exceptions, so might be an idea to commandeer that differential?
I'm sure this is something you know a lot more about and I can probably learn a lot from the patch that gets merged for it.

Nope, I know much less about that; I'll probably stay away from your libc++/libunwind patches until I've got everything necessary for building C programs upstreamed. The things left are this and the mingw ctor section handling (even though it's not used for C projects, it causes linker errors if not handled in one way or another).

martell updated this revision to Diff 109699.Aug 4 2017, 3:49 AM
martell marked 3 inline comments as done.Aug 4 2017, 4:00 AM
martell added inline comments.
MinGW/Driver.cpp
81

Is this desirable. I had to do this in the case of a link error because I would get a stack crash.
I believe this might have been caused by bad libs generated by llvm-dlltool as I described in the comments. In general I will follow what rui thinks here about exiting early.

include/lld/Driver/Driver.h
23

renamed the function to link for consistency also

lld/lib/Shim/COFFLdShim.cpp
70 ↗(On Diff #105012)

This was because it was sharing the COFF namespace which already has those for the coff linker.
Now that we are using the mingw namespace as rui asked this change should not be needed.

tools/lld/CMakeLists.txt
13

I also renamed this to MinGW, not sure if that is desirable for a lib name even though it is consistent.

tools/lld/lld.cpp
116

The description of what CanExitEarly is makes sense by why do we need a bool to even track this.
I don't think it is relevant to mingw::link because we use the COFF linker under the hood.
I think this is for error messaging?
I Just want to be sure because I use _exit(1); in the driver

martell marked an inline comment as done.Aug 4 2017, 4:01 AM
martell added inline comments.
MinGW/CMakeLists.txt
10

I also renamed this to MinGW, not sure if that is desirable for a lib name even though it is consistent.

mstorsjo added inline comments.Aug 5 2017, 1:11 PM
MinGW/Driver.cpp
213

As I got the mingw-arm64 stuff complete enough to publish patches now and this isn't merged yet, you could add a mapping from arm64pe (or whatever we choose in D36364) to ARM64 here.

tools/lld/lld.cpp
57

... and here, and in ELF/Driver.cpp

martell added inline comments.Aug 6 2017, 6:48 AM
tools/lld/lld.cpp
57

I think I should remove the code in ELF/Driver.cpp because that is no longer a code path that can be followed

martell updated this revision to Diff 109922.Aug 6 2017, 7:00 AM

removed defunct ELF path.
added support for arm64pe

martell marked 3 inline comments as done.Aug 6 2017, 7:01 AM
ruiu added inline comments.Aug 6 2017, 6:48 PM
MinGW/Driver.cpp
74

remove this files global variable.

79–81

Just call exit() instead of _exit(). _exit is a performance hack that don't want to replicate to a thin wrapper.

84

Do you really need this?

142

Add {}

151

Just check if there's at least one OPT_l or OPT_INPUT.

155

Use Args.getLastArgValue instead.

169

Please spend a bit more time on reviewing your patch yourself before sending it to review so that everything follows the LLVM style. You want to read your patch from top to bottom several times (that's what I do when I send a patch.)

outArg -> OutArg.

Ping @martell - do you have time to update this patch according to Rui's review?

martell updated this revision to Diff 111685.Aug 18 2017, 9:14 AM
martell marked 6 inline comments as done.
martell added inline comments.
MinGW/Driver.cpp
64

infoTable doesn't seem to follow the variable naming guide but this is done consistently across LLVM

martell updated this revision to Diff 111686.Aug 18 2017, 9:17 AM
martell updated this revision to Diff 111688.Aug 18 2017, 9:24 AM

remove AddFile function.
I thought it looked cleaner with it but it is not needed anymore.

martell added a comment.EditedAug 18 2017, 9:40 AM

I'm currently testing shared library building(dll's) and adding testcases.
Will be back with an update today/tomorrow once that is ironed out.

I'm currently testing shared library building(dll's) and adding testcases.
Will be back with an update today/tomorrow once that is ironed out.

I know of some issues around that, but had meant to send my patches once this goes in, in order not to stall this one forever.

martell updated this revision to Diff 111701.Aug 18 2017, 10:35 AM

I know of some issues around that, but had meant to send my patches once this goes in, in order not to stall this one forever.

With that in mind I updated this with just a test for .exe and ensured the default for exe generation is a.exe vs a.out for elf targets.
You might want to override this for a.dll when -shared is passed etc

ruiu added inline comments.Aug 22 2017, 3:56 PM
MinGW/CMakeLists.txt
10

Yeah, I think MinGW is not exactly right, but I can't come up with a better name. It is probably not that bad, as it is a bit inaccurate but clear on what it refers to.

MinGW/Driver.cpp
53

infoTable -> InfoTable

(Yet another style issue, and honestly I'm a bit tired of pointing them out, so please please fix them before sending your patch to review. If you do not pay attention to the documented and obvious coding style rules, I would probably suspend doing "real" code review and instead pointing out only style issues until your code follows the coding style. I understand that we cannot and do not want to reduce the error rate to zero, and I really appreciate your work on lld/mingw, but pointing out that variables are in CamelCase in the LLVM coding style on virtually every patch update is a waste of both your and my time.)

77–78

I don't think you need to call flush() because exit() will indirectly call them.

martell added inline comments.Aug 22 2017, 6:33 PM
MinGW/Driver.cpp
53

I addressed ALL the CamelCase variables in the patch and left a note for this one specifically before you reviewed it again. You should see a comment a few lines below where infoTable is used with the following text
infoTable doesn't seem to follow the variable naming guide but this is done consistently across LLVM

Here are references to the locations I found using this exact spelling which is why I left it.

LLD

COFF/DriverUtils.cpp
lib/Driver/DarwinLdDriver.cpp

LLVM

tools/llvm-mt/llvm-mt.cpp
tools/llvm-rc/llvm-rc.cpp
lib/ToolDrivers/llvm-lib/LibDriver.cpp
tools/llvm-cvtres/llvm-cvtres.cpp
unittests/Option/OptionParsingTest.cpp
etc

Do you still want me to change it?

I followed up with rL311517 and rL311518 to ensure there is no confusion in future.

martell updated this revision to Diff 112287.Aug 22 2017, 7:43 PM
martell marked an inline comment as done.
mstorsjo added inline comments.Aug 22 2017, 10:16 PM
MinGW/Driver.cpp
53

I understand you frustration Rui, I truly do (especially given earlier revisions of this and other patches), but this one was surely clearly pointed out by Martell before - unfortunately I guess the comment was buried in the log and not at the right spot in the inline diff.

If we give Martell the benefit of doubt that it is ok wrt to variable naming (since he did point this one out explicitly before with a reasoning) and other style issues I assume he's fixed with clang-format by now, what's the status of functional review of this patch? Is it almost done with a few minor issues yet to be found, or has that not been done thoroughly yet at all due to major style issues in all the previous iterations?

ruiu added inline comments.Aug 23 2017, 9:01 AM
MinGW/Driver.cpp
53

I'm sorry that I missed your previous comment, Martell. But infoTables in many files are just copypasta and shouldn't be intentional.

As to the functionality, I think this patch is towards a right direction, though there are a few things that have to be addressed.

72

I'd change the type of this variable to std::vector<std::string> and repack to std::vector<const char *> at end of link(). This should eliminate a lot of uses of Saver.save().

98

You are using this function only once, and at that call site IgnoreShared is not passed, so it is always false. Please remove that parameter.

121

Use Arg->getOption().getUnaliasedOption().getID() instead of Arg->getOption().getID() so that we don't need to worry about aliases.

126

Do you really have to call Saver.save here? The use of Saver.save is not consistent in this patch, and it might be saving a string that's already been saved.

I believe we should remove Saver entirely from this patch. Can you do that if you replace some occurrences of StringRef with std::strings in this patch, no?

132

You are using this function only once so inline this.

142

By convention, use empty instead of size.

151–152

Merge OutArg and OutValueArg and do that at caller side. For example, instead of doing

forwardValue(Args, OPT_m, "i386pe", "MACHINE", "X86");

, you can just do

forwardValue(Args, OPT_m, "i386pe", "MACHINE:X86");

Also, lowercase options are probably preferred, so it's even better if you do

forwardValue(Args, OPT_m, "i386pe", "machine:x86");
190
for (auto *Arg : Args.filtered(OPT_L))
  SearchPaths.push_back(Arg->getValue());
martell updated this revision to Diff 112395.Aug 23 2017, 9:53 AM
martell marked 5 inline comments as done.
martell added inline comments.Aug 23 2017, 9:56 AM
MinGW/Driver.cpp
98

It is based on the -Bstatic flag.

addLibrary(Arg->getValue(), Args.hasArg(OPT_Bstatic));
which in turn calls
searchLibrary(Name, StaticOnly))
passing in the bool

martell added inline comments.Aug 23 2017, 10:01 AM
MinGW/Driver.cpp
191

I could probably use auto here instead of std::vector<std::string>::size_type?
Other then that I think I have addressed all the comments

martell updated this revision to Diff 112397.Aug 23 2017, 10:15 AM

auto gives a cast warning so leave it with the full type.

martell marked an inline comment as done.Aug 23 2017, 10:16 AM
ruiu added a comment.Aug 23 2017, 5:42 PM

I forgot to mention that this patch needs a test. Please create a subdirectory MinGW under lld/tests and put a test file there for this driver.

MinGW/Driver.cpp
98

You can at least remove = false. It is nice to keep the variable name, so I'd rename this StaticOnly.

129–130
StringRef OutArg -> const std::string &OutArg

to remove Twine and str().

You generally want to avoid C strings in C++, so const char *Default = nullptr -> std::string Default = "".

132

For consistency with addLibrary, remove {}.

140

If you change StringRef OutArg to const std::string &OutArg, then you can remove Twine and str().

171–175

I'd make them lowercase as well.

183–184
if (Args.getLastArgValue(OPT_m) == "i386pe")
191

I think just size_t suffices. However, probably the best way of doing this is something like this:

std::vector<const char *> Vec;
for (const std::string &S : LinkArgs)
  Vec.push_back(S.c_str());
return coff::link(Vec);
MinGW/Options.td
36–39

You are not handling this one, so please remove.

In D33880#850959, @ruiu wrote:

I forgot to mention that this patch needs a test. Please create a subdirectory MinGW under lld/tests and put a test file there for this driver.

He actually already adds one test case within test/COFF/out.test - linking and checking an implicitly named output a.exe. I guess you at least want that test split out to a separate file in a separate directory then?

ruiu added a comment.Aug 24 2017, 12:28 PM

He actually already adds one test case within test/COFF/out.test - linking and checking an implicitly named output a.exe. I guess you at least want that test split out to a separate file in a separate directory then?

Yes, I think we want to have a separate directory under test as we have one at the top level directory.

Also, the test seems too simple. I want to see a test file that tests -o, -entry, -subs, -shared, -outlib, -stack and image_base.

In D33880#851739, @ruiu wrote:

He actually already adds one test case within test/COFF/out.test - linking and checking an implicitly named output a.exe. I guess you at least want that test split out to a separate file in a separate directory then?

Yes, I think we want to have a separate directory under test as we have one at the top level directory.

Also, the test seems too simple. I want to see a test file that tests -o, -entry, -subs, -shared, -outlib, -stack and image_base.

Ok, makes sense.

Although I think we should just drop handling of -shared from this patch and focus on getting the rest of it OK'd. Actually producing working DLLs is incomplete in this patch anyway.

Ping @martell - do you have time to follow up on Rui's latest review comments? Do you want assistance in e.g. writing the necessary test cases for it?

mati865 added a subscriber: mati865.Sep 5 2017, 5:47 AM
martell updated this revision to Diff 114455.Sep 8 2017, 4:12 PM

address feedback
add MinGW tests
add -shared

mstorsjo added inline comments.Sep 8 2017, 10:51 PM
MinGW/Driver.cpp
180

Shouldn't you swap a.exe and a.dll here?

MinGW/Options.td
19

If testing -stack, you'll notice that this will recurse infinitely - remove the alias.

test/MinGW/output.test
10 ↗(On Diff #114455)

This doesn't test that a.exe is produced by the right command.

Missing tests for -subs, -stack, -o, -outlib and actually referring to the image_base symbol.

If setting e.g. -subs, I think it would make sense to check for it in the readobj output, and check that the dll actually is a dll etc.

Also, since the input file is used only once, Rui has earlier preferred to move that data into this file, and rename it to .yaml.

@martell I took the liberty to improve the tests - if you squash http://martin.st/temp/lld-mingw-tests.patch into it, it should test most things that are added in this patch, as suggested by Rui.

martell updated this revision to Diff 114525.Sep 10 2017, 12:53 PM

update tests

martell added a comment.EditedSep 10 2017, 12:57 PM

@mstorsjo I had actually done a bunch of these tests but as separate files yesterday.
Given that it is just a wrapper having just output.test probably makes more sense here.

I was actually looking through tests/COFF wondering what would be the best test for the __image_base__ symbol.
Thanks for the help there.

the x64 test did not actually check for IMAGE_FILE_MACHINE_AMD64 so I added that.
also I wanted to add a test to ensure that __ImageBase is still supported because long term I want binutils to use that also so we can use LINK.exe with mingw-w64 if needed.
So for that I added an arm and aarch64 test that uses __ImageBase instead, so we can check that and thumb2pe and arm64pe all in the one test.

@mstorsjo I had actually done a bunch of these tests but as separate files yesterday.
Given that it is just a wrapper having just output.test probably makes more sense here.

Yeah, so far it's probably easiest to squeeze them all in one. When adding more tests later we can see what looks like it'd make most sense - and we'll see what Rui thinks of course.

I was actually looking through tests/COFF wondering what would be the best test for the __image_base__ symbol.
Thanks for the help there.

the x64 test did not actually check for IMAGE_FILE_MACHINE_AMD64 so I added that.

Oh, apparently I lost that line in some test/edit cycle when I ripped out parts of them. Thanks for noticing!

also I wanted to add a test to ensure that __ImageBase is still supported because long term I want binutils to use that also so we can use LINK.exe with mingw-w64 if needed.
So for that I added an arm and aarch64 test that uses __ImageBase instead, so we can check that and thumb2pe and arm64pe all in the one test.

Ok, that makes sense to test as well - thanks!

MinGW/Driver.cpp
87

A comment on the level of nitpickery: Some of the functions (e.g. findFile above) are static, while this one (findFromSearchPaths) isn't - it'd be nice to have all functions static unless they really need to be non-static.

martell updated this revision to Diff 114529.Sep 10 2017, 3:43 PM

make findFromSearchPaths, searchLibrary, addLibrary and createFiles static

Thanks @martell for the updates! @ruiu, any further comments on this now?

ruiu accepted this revision.Sep 11 2017, 9:20 AM

LGTM. There are a few things that I want to fix in this patch, but we can do that later.

This revision is now accepted and ready to land.Sep 11 2017, 9:20 AM

Thanks Rui for the review effort! Hopefully later changes are easier to work with, as smaller incremental additions/changes.

This revision was automatically updated to reflect the committed changes.