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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Mostly ok with me, a couple minor comments and a small discussion item.

The change about tolerating relative paths where we previously only accepted filenames, is separate from the rest IMO, and it would be good to have a testcase for that too.



================
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);
----------------
thieta wrote:
> 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.
Isn't this part of the patch in principle a separate functional change with a different subject?

The rest of the patch is about adding extra directories to the libpath, while this particular change is about allowing/handling relative paths in the way we previously handled only bare file names? And I don't see this functional change covered by the tests?


================
Comment at: lld/COFF/Driver.cpp:646
+
+  // We need to prepend the paths here in order to make sure that we always
+  // try to link the clang versions of the builtins over the ones supplied by MSVC.
----------------
To clarify the context of this comment, maybe move it down to exactly before the `searchPaths.insert()`?


================
Comment at: lld/COFF/Driver.cpp:653
+  SmallString<128> runtimeLibDir(rootDir);
+  sys::path::append(runtimeLibDir, "lib", "clang", std::to_string(LLVM_VERSION_MAJOR), "lib");
+  searchPaths.insert(searchPaths.begin(), saver().save(runtimeLibDir.str()));
----------------
Curious - is the clang version always exactly identical to `LLVM_VERSION_MAJOR`, or can it change? E.g. previously, we used to have the full version number here, 16.0.0 etc, while it now is only the major number. If nothing else, it would be good to have a comment at the other code site where that is decided, to remind people to update these two code sites in sync.


================
Comment at: lld/COFF/Driver.cpp:2079
     config->writeCheckSum = true;
-  
   // Handle /safeseh, x86 only, on by default, except for mingw.
----------------
Nit: stray change


================
Comment at: lld/test/COFF/print-search-paths.s:17
 
-
         .text
----------------
Nit: Stray change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151188