[PATCH] D33880: COFF: Introduce LD shim around LINK

Martin Storsjö via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Sep 10 13:20:55 PDT 2017


mstorsjo added a comment.

In https://reviews.llvm.org/D33880#865996, @martell wrote:

> @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!



================
Comment at: MinGW/Driver.cpp:86
+
+Optional<std::string> findFromSearchPaths(StringRef Path) {
+  for (StringRef Dir : SearchPaths)
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D33880





More information about the llvm-commits mailing list