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

[Martell Malone via Phabricator via llvm-commits]

martell marked 3 inline comments as done.
martell added inline comments.


================
Comment at: MinGW/Driver.cpp:80
+  errs().flush();
+  _exit(1);
+}
----------------
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.


================
Comment at: include/lld/Driver/Driver.h:23
+namespace mingw {
+bool link(llvm::ArrayRef<const char *> Args,
+          llvm::raw_ostream &Diag = llvm::errs());
----------------
renamed the function to link for consistency also


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:70
+BumpPtrAllocator BAlloc;
+StringSaver Saver{BAlloc};
+
----------------
mstorsjo wrote:
> 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;
> ```
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.


================
Comment at: tools/lld/CMakeLists.txt:13
   lldELF
+  lldMinGW
   )
----------------
I also renamed this to MinGW, not sure if that is desirable for a lib name even though it is consistent.


================
Comment at: tools/lld/lld.cpp:116
+      return !mingw::link(Args);
     return !elf::link(Args, true);
   case WinLink:
----------------
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


Repository:
  rL LLVM

https://reviews.llvm.org/D33880