[clang][flang][windows] Prefer user-provided library paths (-L)

[DavidTruby]

Currently the paths to compiler-rt and the Flang runtimes from the LLVM build/install directory are preferred over any user-provided library paths. This means a user can't override compiler-rt or the Flang runtimes with custom versions.

This patch changes the link order to prefer library paths specified with -L over the LLVM paths. This matches the behaviour of clang and flang on Linux.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: David Truby (DavidTruby)

Changes

Currently the paths to compiler-rt and the Flang runtimes from the LLVM build/install directory are preferred over any user-provided library paths. This means a user can't override compiler-rt or the Flang runtimes with custom versions.

This patch changes the link order to prefer library paths specified with -L over the LLVM paths. This matches the behaviour of clang and flang on Linux.

---

Full diff: https://github.com/llvm/llvm-project/pull/90758.diff

1 Files Affected:

• (modified) clang/lib/Driver/ToolChains/MSVC.cpp (+4-4)

diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index fbf2f45b543844..b7021d4b996ddd 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -134,6 +134,10 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
           Args.MakeArgString(std::string("-libpath:") + WindowsSdkLibPath));
   }
 
+  if (!C.getDriver().IsCLMode() && Args.hasArg(options::OPT_L))
+    for (const auto &LibPath : Args.getAllArgValues(options::OPT_L))
+      CmdArgs.push_back(Args.MakeArgString("-libpath:" + LibPath));
+
   if (C.getDriver().IsFlangMode()) {
     addFortranRuntimeLibraryPath(TC, Args, CmdArgs);
     addFortranRuntimeLibs(TC, Args, CmdArgs);
@@ -154,10 +158,6 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   if (TC.getVFS().exists(CRTPath))
     CmdArgs.push_back(Args.MakeArgString("-libpath:" + CRTPath));
 
-  if (!C.getDriver().IsCLMode() && Args.hasArg(options::OPT_L))
-    for (const auto &LibPath : Args.getAllArgValues(options::OPT_L))
-      CmdArgs.push_back(Args.MakeArgString("-libpath:" + LibPath));
-
   CmdArgs.push_back("-nologo");
 
   if (Args.hasArg(options::OPT_g_Group, options::OPT__SLASH_Z7))

[MaskRay]

Thanks. Can you add a test? Possibly clang/test/Driver/msvc-link.c.
linux-cross.cpp has a good example about how to test the default -L paths.

[banach-space]

LGTM, thanks!

I've left some nits, feel free to ignore

[banach-space]

[nit] test -> random_test_dir or something else that will make this stand out a bit more (makes parsing tests a bit easier)

[banach-space]

Wondering how to check that there's no -libapth before -libpath:test 🤔

[MaskRay]

。

[MaskRay]

Add tests to existing msvc-link.c instead of a new file. Specify --target= to avoid reliance on the default target triple and avoid the need of REQUIRES.

[MaskRay]

The code checks whether the directory is present auto CRTPath = TC.getCompilerRTPath(); and only adds the libpath when it is present. This is related to the LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=off file hierarchy we are migrating way from.

With LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on, TC.getCompilerRTPath() will not be needed.

Since there is no good way to test this, I'm fine with the patch without a test.

[DavidTruby]

The code checks whether the directory is present auto CRTPath = TC.getCompilerRTPath(); and only adds the libpath when it is present. This is related to the LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=off file hierarchy we are migrating way from.

With LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on, TC.getCompilerRTPath() will not be needed.

Since there is no good way to test this, I'm fine with the patch without a test.

Hi, I'm not sure I understand exactly what you mean by without a test; do you mean I should just remove the tests I tried to add or is there some extra behaviour that you're talking about that can't be tested?

[DavidTruby]

I've left the flang test as the flang directory doesn't change with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR and removed the other test. I hope this is what you meant @MaskRay 👍

[banach-space]

I've left the flang test as the flang directory doesn't change with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR and removed the other test. I hope this is what you meant @MaskRay 👍

That's matches how I read that suggestion re LLVM_ENABLE_PER_TARGET_RUNTIME_DIR, thanks!

From what I can tell, you've already made all the changes requested from @MaskRay . If there are no new comments today, I suggest landing this.