[PATCH] D119049: [LLD] Allow usage of LLD as a library

[Zhuoran Yin via Phabricator via llvm-commits]

jerryyin accepted this revision.
jerryyin added a comment.
This revision is now accepted and ready to land.

LGTM with my limited knowledge of lld. Approving it as this is an extremely useful feature to have, and I want it to make progress after almost a year of staging.



================
Comment at: lld/Common/DriversMain.cpp:163-172
+    if (f == MinGW)
+      return mingw::link;
+    else if (f == Gnu)
+      return elf::link;
+    else if (f == WinLink)
+      return coff::link;
+    else if (f == Darwin)
----------------
aganea wrote:
> jerryyin wrote:
> > I have a naïve question to ask:
> >  - In my limited knowledge, previously `lldELF` is dependent on `lldCommon` target. `lld.cpp` would compile just fine because its target is an executable.
> >  - Now, since `DriversMain.cpp` become part of `lldCommon`, and in the implementation of `DriversMain,cpp`, we start to invoke those specific link target, it implicitly requires, for example, `lld::elf::link` to be available before `lldCommon` is compiled. In other words, `lldCommon` is dependent on `lldELF`. 
> > 
> > Did we just have a cyclic dependency here? And if it is, do you need to move the `lldMain()` and `safeLldMain()` into their own independent target such that we can avoid the existence of such a cycle in the compilation?
> The trickery is that we push lldCommon twice on the command-line, [[ https://github.com/llvm/llvm-project/blob/main/lld/ELF/CMakeLists.txt#L77 | once in lldELF ]] (same in all the other drivers), in another time in [[ https://github.com/llvm/llvm-project/blob/main/lld/tools/lld/CMakeLists.txt#L23 | the final module ]]. I retained the same mechanic for the unittests.
> 
> We could have another lib for LLD-as-lib usage, but I thought from the users' perspective it's simpler if we provided a single "common" lib + the specific drivers' libs.
Just want to let you know that I made an (very risky attempt) trying to test this patch in our fork, but this errors out unable to build lld. Based on Harbormaster build result, I don't believe this is due to issues from your patch, but rather our lld fork has been a couple of months old. (Refer to https://github.com/ROCmSoftwarePlatform/rocMLIR/commits/test-lld-update, I pulled the last 3 commits in between this period hoping they are enough narrow the gap.) However, `ninja lld` would yield a number of linker error as below:

> ld.lld: error: undefined symbol: lld::elf::link(llvm::ArrayRef<char const*>, llvm::raw_ostream&, llvm::raw_ostream&, bool, bool)
> referenced by DriversMain.cpp:0 (/root/rocMLIR/external/llvm-project/lld/Common/DriversMain.cpp:0)
>               external/llvm-project/llvm/tools/lld/Common/CMakeFiles/lldCommon.dir/DriversMain.cpp.o:(lld::lldMain(llvm::ArrayRef<char const*>, llvm::raw_ostream&, llvm::raw_ostream&, bool))

It would take a while for our fork to completely catch up with the tip of upstream, and I'll give this another test after then. In the mean time I will not hold this review up because of it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119049/new/

https://reviews.llvm.org/D119049