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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/Driver/ToolChain.cpp:446
   case ToolChain::FT_Shared:
-    Suffix = TT.isOSWindows()
-                 ? (TT.isWindowsGNUEnvironment() ? ".dll.a" : ".lib")
+    Suffix = Triple.isOSWindows()
+                 ? (Triple.isWindowsGNUEnvironment() ? ".dll.a" : ".lib")
----------------
This is the same as `TT`, right? Please use TT here or remove TT and use Triple across this function. I guess I lean towards keeping TT, but it's up to you.


================
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();
----------------
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.


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

https://reviews.llvm.org/D96638