[PATCH] D96638: [Driver][Windows] Support per-target runtimes dir layout for profile instr generate

[Markus Böck via Phabricator via cfe-commits]

zero9178 marked an inline comment as done.
zero9178 added inline comments.


================
Comment at: clang/lib/Driver/ToolChain.cpp:468-469
   // Check for runtime files in the new layout without the architecture first.
-  std::string CRTBasename =
-      buildCompilerRTBasename(*this, Args, Component, Type, /*AddArch=*/false);
-  for (const auto &LibPath : getLibraryPaths()) {
-    SmallString<128> P(LibPath);
-    llvm::sys::path::append(P, CRTBasename);
-    if (getVFS().exists(P))
-      return std::string(P.str());
+  std::string CRTBasename = getCompilerRTBasename(Args, Component, Type);
+  if (auto value = findInLibraryPath(*this, CRTBasename)) {
+    return value.getValue();
----------------
rnk wrote:
> This does twice as many stat syscalls as we need. That seems worth optimizing. First, in getCompilerRTBasename, we search the library path, we return a result, and then we search it again here. I think the nicest solution is probably to have one shared implementation that takes a boolean and returns the full path or the basename.
Refactoring the current implementation out of getCompilerRTBasename is currently not possible as that would break the BareMetal toolchain as it overrides getCompilerRTBasename to build up a different scheme for it's runtime libraries. Not calling getCompilerRTBasename is what caused the test failure previously.

Unless you mean that I should add an optional boolean operand to getCompilerRTBasename that would get either the absolute path if possible or always return a relative path? That could work and would change getCompilerRT to not go through the library path but to instead check if that path is absolute and return if it is already. 






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

https://reviews.llvm.org/D96638