[PATCH] D151188: [LLD][COFF] Add LLVM toolchain library paths by default.

Thu Jul 13 01:59:30 PDT 2023

thieta marked an inline comment as done.
thieta added a comment.

This seems to work fine now and have tests. Next up would be to add some dependent-lib statements in various places. Please review this and let me know if there is something else that should be fixed here.

================
Comment at: lld/COFF/Driver.cpp:468

-  bool hasPathSep = (filename.find_first_of("/\\") != StringRef::npos);
-  if (hasPathSep)
+  if (sys::path::is_absolute(filename))
     return getFilename(filename);
----------------
rnk wrote:
> thieta wrote:
> > In order to get relative paths to work, I had to change this line. I am NOT sure what the original intent of this block was. It will make any relative path only return the filename. And none of the tests seems to cover this case, everything works after my change.
> > 
> > But if someone have a bit more context on this - I'll be happy to understand what's going on in this block.
> I think the old logic was based on observed MSVC link.exe behavior.
> 
> Can you check that the new strategy is actually compatible with MSVC link.exe? As in, if we change clang to embed relative paths, does that actually work with MSVC link.exe with `-libpath:${resource}/lib`?
Verified that it worked with link.exe as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151188