[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: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();
----------------
zero9178 wrote:
> 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. 
> 
> 
> 
> 
I see, good point. I'd prefer to avoid extra boolean parameters when possible. My next best idea is to separate getCompilerRTBasename into two methods, one public non-virtual, and one protected virtual (getCompilerRTBasenameImpl? ech) to allow baremetal to override. The public one would do the obvious thing of returning sys::fs::filename of getCompilerRT. The protected one would retain the current implementation, and we'd update the bare metal toolchains to override the protected one.

We have prior history of doing excessive amounts of Linux Distro detection (D87187), which is why I'm being a bit fussy about the stat calls.


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

https://reviews.llvm.org/D96638