Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
| 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. |
- 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. |
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?
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.
| lld/lib/Shim/COFFLdShim.cpp | ||
|---|---|---|
| 1 ↗ | (On Diff #101361) | reiterating my previous question. It probably got lost in the thread.
Have you any thoughts or suggestions on a more generic naming scheme rui? Also a comment from @mstorsjo above.
|
| 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. |
| 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". |
| 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; |
| lld/lib/Shim/COFFLdShim.cpp | ||
|---|---|---|
| 217 ↗ | (On Diff #105012) | Isn't this a typo here, shouldn't this be i386pep mapping to X64? |
| 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"); |
| lib/CMakeLists.txt | ||
|---|---|---|
| 4 ↗ | (On Diff #109642) | 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 ↗ | (On Diff #109642) | I think Rui requested this to be named MingwDriver.cpp or Driver.cpp as well. |
| lib/Mingw/COFFLdShim.cpp | ||
| 102 ↗ | (On Diff #109642) | 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. |
If you're still too busy and don't mind, I could also take over trying to get this in a mergeable form.
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.
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. |
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.
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).
| 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. |
| include/lld/Driver/Driver.h | ||
| 23 ↗ | (On Diff #109699) | 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. |
| tools/lld/CMakeLists.txt | ||
| 13 ↗ | (On Diff #109699) | 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 ↗ | (On Diff #109699) | The description of what CanExitEarly is makes sense by why do we need a bool to even track this. |
| 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. |
| tools/lld/lld.cpp | ||
|---|---|---|
| 57 ↗ | (On Diff #109699) | I think I should remove the code in ELF/Driver.cpp because that is no longer a code path that can be followed |
| 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. |
| MinGW/Driver.cpp | ||
|---|---|---|
| 63 ↗ | (On Diff #111685) | infoTable doesn't seem to follow the variable naming guide but this is done consistently across LLVM |
remove AddFile function.
I thought it looked cleaner with it but it is not needed anymore.
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.
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
| 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. |
| 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 Here are references to the locations I found using this exact spelling which is why I left it. LLD COFF/DriverUtils.cpp LLVM tools/llvm-mt/llvm-mt.cpp Do you still want me to change it? |
| 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? |
| 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()); |
| MinGW/Driver.cpp | ||
|---|---|---|
| 97 ↗ | (On Diff #112287) | It is based on the -Bstatic flag. addLibrary(Arg->getValue(), Args.hasArg(OPT_Bstatic)); |
| MinGW/Driver.cpp | ||
|---|---|---|
| 190 ↗ | (On Diff #112395) | I could probably use auto here instead of std::vector<std::string>::size_type? |
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. |
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?
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?
| 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.
@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.
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. |
LGTM. There are a few things that I want to fix in this patch, but we can do that later.
Thanks Rui for the review effort! Hopefully later changes are easier to work with, as smaller incremental additions/changes.