This is an archive of the discontinued LLVM Phabricator instance.

COFF: Introduce LD shim around LINK
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.Jun 4 2017, 11:41 AM
martell added inline comments.Jun 4 2017, 11:44 AM
lld/lib/Shim/COFFLdOptions.td
21 ↗(On Diff #101361)

This is enough to get hello worlds working and cmake passing checks.

lld/lib/Shim/COFFLdShim.cpp
1 ↗(On Diff #101361)

Location Okay?

144 ↗(On Diff #101361)

Do I let LINK.exe handle such errors?

lld/tools/lld/lld.cpp
123 ↗(On Diff #101361)

I'm not quite sure if I should do this here or in the ELF's Driver.cpp where it checks the emulation.
Suggestions welcome

ruiu edited edge metadata.Jun 5 2017, 6:34 AM
  • lib is not the right place to put these files. Create a directory mingw as lld/mingw and move new files to that directory.
lld/lib/Shim/COFFLdOptions.td
1 ↗(On Diff #101361)

Rename this Options.td.

lld/lib/Shim/COFFLdShim.cpp
1 ↗(On Diff #101361)

See the comment above.

Rename this file MingwDriver.cpp.

12 ↗(On Diff #101361)

This is a linker driver for MinGW.

16–17 ↗(On Diff #101361)

Remove duplicate.

65 ↗(On Diff #101361)

COFFLdOptTable -> MingwOptTable.

73 ↗(On Diff #101361)

You should create a new namespace lld::mingw.

77 ↗(On Diff #101361)

Remove all occurences of llvm::.

80 ↗(On Diff #101361)

Remove llvm::.

92 ↗(On Diff #101361)

Remove {}.

107 ↗(On Diff #101361)

Remove IgnoreShared as you are not using it.

121 ↗(On Diff #101361)

Don't new std::string -- you are leaking memory.

144 ↗(On Diff #101361)

Either is probably fine, but don't leave this kind of comment.

163 ↗(On Diff #101361)

Maybe forward is a better name.

Don't return true/false for no reason. You are not using the return value, so return void.

173 ↗(On Diff #101361)

Don't return a value unless you are actually using it.

176 ↗(On Diff #101361)

Can you run clang-format before sending a new patch to review, please?

177 ↗(On Diff #101361)

Memory leak.

lld/tools/lld/lld.cpp
52 ↗(On Diff #101361)

isMingw

53 ↗(On Diff #101361)
for (auto It = V.begin(); It + 1 != V.end(); ++It) {
  if (StringRef(*It) != "-m")
    continue;
  StringRef S = *(It + 1);
  return S == "i386pe" || S == "i386pep" || S == "thumb2pe";
}
return false;
123 ↗(On Diff #101361)

Doing this right here is the correct approach

125 ↗(On Diff #101361)

Remove else because the last if ends with return.

martell updated this revision to Diff 105010.EditedJul 1 2017, 3:28 PM
martell marked 15 inline comments as done.

updated to HEAD.
added new options from rL305805
addressed as many comments as possible.

@ruiu Naming everything Mingw Driver doesn't make a lot of sense because this also supports cygwin. That and the possible future targets such as midipix.
Have you any suggestions on a more generic naming scheme?

martell updated this revision to Diff 105012.Jul 1 2017, 3:34 PM

prepend lld/ to the patch path to make it more readable

@ruiu Naming everything Mingw Driver doesn't make a lot of sense because this also supports cygwin. That and the possible future targets such as midipix.
Have you any suggestions on a more generic naming scheme?

Just my 2 cents; I also prefer naming this something like "ld style frontend to the COFF linker" instead of "mingw driver". but in the end, Rui's opinion is what matters of course.

ruiu added inline comments.Jul 11 2017, 6:34 PM
lld/include/lld/Driver/Driver.h
21 ↗(On Diff #105012)

Indentation. Please use clang-format-diff before sending patches so that human reviewers can focus on meaning of the code instead of formatting.

lld/lib/Shim/COFFLdShim.cpp
1 ↗(On Diff #101361)

Please move this file as I wrote above.

martell marked an inline comment as done.Jul 14 2017, 7:30 AM
martell added inline comments.
lld/lib/Shim/COFFLdShim.cpp
1 ↗(On Diff #101361)

reiterating my previous question. It probably got lost in the thread.

Naming everything Mingw Driver doesn't make a lot of sense because this also supports cygwin. That and the possible future targets such as midipix.

Have you any thoughts or suggestions on a more generic naming scheme rui?

Also a comment from @mstorsjo above.

Just my 2 cents; I also prefer naming this something like "ld style frontend to the COFF linker" instead of "mingw driver". but in the end, Rui's opinion is what matters of course.

martell added inline comments.Jul 14 2017, 7:36 AM
lld/include/lld/Driver/Driver.h
21 ↗(On Diff #105012)

I will use git diff -U999 HEAD^ | clang-format-diff.py -i -p1 going forward. Thanks.

ruiu added inline comments.Jul 14 2017, 2:00 PM
lld/include/lld/Driver/Driver.h
21 ↗(On Diff #105012)

Can you upload a clang-formatted version? This indentation is still wrong.

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

Move this file and other files in this directory to lld/Mingw (do not include lib or something, but just that directory).

Mingw is probably not the best name, but we already have COFF and ELF, and I can't think of any good name that briefly conveys the idea that this is "GNU-style ld command line frontend for Windows".

mstorsjo added inline comments.Jul 30 2017, 1:07 PM
lld/lib/Shim/COFFLdShim.cpp
70 ↗(On Diff #105012)

FWIW, this fails to link for me (on linux), with following error:

lib/liblldCOFFShim.a(COFFLdShim.cpp.o):(.bss._ZN3lld4coff5SaverE+0x0): multiple definition of `lld::coff::Saver'
lib/liblldCOFF.a(Driver.cpp.o):(.bss._ZN3lld4coff5SaverE+0x0): first defined here
lib/liblldCOFFShim.a(COFFLdShim.cpp.o):(.bss._ZN3lld4coff6BAllocE+0x0): multiple definition of `lld::coff::BAlloc'
lib/liblldCOFF.a(Driver.cpp.o):(.bss._ZN3lld4coff6BAllocE+0x0): first defined here

I did this modification locally to fix it:

-BumpPtrAllocator BAlloc;
-StringSaver Saver{BAlloc};
+extern StringSaver Saver;
mstorsjo added inline comments.Aug 2 2017, 12:36 PM
lld/lib/Shim/COFFLdShim.cpp
217 ↗(On Diff #105012)

Isn't this a typo here, shouldn't this be i386pep mapping to X64?

mstorsjo added inline comments.Aug 2 2017, 12:56 PM
lld/lib/Shim/COFFLdShim.cpp
231 ↗(On Diff #105012)

To get this working on i386, I had to do this modification:

auto M = Args.getLastArg(OPT_m);
if (M && StringRef(M->getValue()) == "i386pe")
  LinkArgs.push_back("/alternatename:__image_base__=___ImageBase");
else
  LinkArgs.push_back("/alternatename:__image_base__=__ImageBase");
martell updated this revision to Diff 109642.Aug 3 2017, 3:19 PM
martell added a reviewer: mstorsjo.
martell removed a subscriber: mstorsjo.

Have been super busy with work, should have got around to this sooner sorry.

mstorsjo added inline comments.Aug 4 2017, 1:39 AM
lib/CMakeLists.txt
4

Afaik Rui requested this to be a separate directory on the toplevel (next to COFF and ELF), not within lib.

lib/Mingw/CMakeLists.txt
10

I think Rui requested this to be named MingwDriver.cpp or Driver.cpp as well.

lib/Mingw/COFFLdShim.cpp
102

As far as I can see, nothing sets the Sysroot variable used here.

One might want to add a stub option --sysroot to Options.td as well, to handle the case if the user passes --sysroot to the clang frontend. In those cases, the clang driver already takes care of most of it (adding the right -L parameters to lld), so we can get pretty far by just ignoring it instead of erroring out.

mstorsjo edited edge metadata.Aug 4 2017, 1:44 AM

Have been super busy with work, should have got around to this sooner sorry.

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

martell updated this revision to Diff 109695.Aug 4 2017, 2:30 AM
martell marked an inline comment as done.

All the old comments got buried in the thread.
I see the comment about lld/Mingw all the other folders are named in all caps or all lowercase.
I went with all caps for now unless we specifically want Mingw with just a capital M.

Removed handling of sysroot
Though I am not sure this is correct behaviour because ELF handles this in the same manner.

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
80 ↗(On Diff #109699)

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
25

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
9 ↗(On Diff #109699)

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
212 ↗(On Diff #109699)

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
73 ↗(On Diff #109922)

remove this files global variable.

78–80 ↗(On Diff #109922)

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

83 ↗(On Diff #109922)

Do you really need this?

141 ↗(On Diff #109922)

Add {}

150 ↗(On Diff #109922)

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

154 ↗(On Diff #109922)

Use Args.getLastArgValue instead.

168 ↗(On Diff #109922)

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
63 ↗(On Diff #111685)

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
9 ↗(On Diff #109699)

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 ↗(On Diff #111701)

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 ↗(On Diff #111701)

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 ↗(On Diff #111701)

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 ↗(On Diff #111701)

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 ↗(On Diff #111701)

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.

71 ↗(On Diff #112287)

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().

97 ↗(On Diff #112287)

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.

120 ↗(On Diff #112287)

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

125 ↗(On Diff #112287)

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?

131 ↗(On Diff #112287)

You are using this function only once so inline this.

141 ↗(On Diff #112287)

By convention, use empty instead of size.

150–151 ↗(On Diff #112287)

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");
189 ↗(On Diff #112287)
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
97 ↗(On Diff #112287)

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
190 ↗(On Diff #112395)

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
97 ↗(On Diff #112287)

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

190 ↗(On Diff #112395)

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);
128–129 ↗(On Diff #112397)
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 = "".

131 ↗(On Diff #112397)

For consistency with addLibrary, remove {}.

139 ↗(On Diff #112397)

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

170–174 ↗(On Diff #112397)

I'd make them lowercase as well.

182–183 ↗(On Diff #112397)
if (Args.getLastArgValue(OPT_m) == "i386pe")
MinGW/Options.td
35–38 ↗(On Diff #112397)

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
179 ↗(On Diff #114455)

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

MinGW/Options.td
18 ↗(On Diff #114455)

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
86 ↗(On Diff #114525)

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.