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

[zero9178]

Summary

When targeting a MSVC triple, --dependant-libs with the name of the clang runtime library for profiling is added to the command line args. In it's current implementations clang_rt.profile-<ARCH> is chosen as the name. When building a distribution using LLVM_ENABLE_PER_TARGET_RUNTIME_DIR this fails, due to the runtime file names not having an architecture suffix in the filename.

This patch refactors getCompilerRTBasename to first try and find the version without the architecture suffix before then falling back to using the architecture suffix. Since no callers have explicitly used the AddArch parameter, and the correct choice is now autodetected by default it has also been removed.

[zero9178]

Friendly bump :)

[rnk]

clang/lib/Driver/ToolChains/Clang.cpp

797

Is it possible to fix the problem in getCompilerRTBasename instead? I see 6-ish instances of this code pattern that need the same fix. It will require a small bit of refactoring to getCompilerRT.

[zero9178]

Refactored getCompilerRTBasename. It now simply refers to getCompilerRT, which gathers a full path to the specified runtime library. getCompilerRTBasename then simply returns only the filename component. The previous contents of getCompilerRTBasename have been refactored into a static function used by getCompilerRT. Only users of either of the two functions has been in clang-cl specific code and besides getCompilerRT, the AddArch parameter has never been explicitly specified. As it is now autodetected I think it makes sense to remove the parameter off of getCompilerRTBasename altogether as well.

[rnk]

lgtm

Thanks!

[thakis]

This broke check-clang on all platforms, e.g. here:

http://45.33.8.238/linux/40161/step_7.txt
http://45.33.8.238/macm1/4018/step_6.txt
http://45.33.8.238/win/33805/step_7.txt

Please run tests before committing.

(It also breaks check-clang in a local cmake build. The official llvm buildbot seems to be down atm, so I can't check if llvm's regular bots are all red, but I'd guess so.)

[thakis]

Reverted for now in ab5b00ada9e77437ed2c76f73cfb8481bb9826a5

[zero9178]

Undid refactoring of getCompilerRTBasename. Previous diffs introduced a new internal function that built the basename. In that version getCompilerRTBasename simply called getCompilerRT and extracted the filename component.

This lead to issues as Toolchains such as Baremetal override getCompilerRTBasename to provide a different basename then default.

This diff now instead changes the default implementation of getCompilerRTBasename to also auto detect whether an architecture suffix should be used or not. Therefore callers of getCompilerRTBasename can rely on it picking the right filenames as well. getCompilerRT still calls into getCompilerRTBasename to get the filename, allowing Toolchains to override the virtual function.

There is now sadly a bit of repetition as both getCompilerRT and getCompilerRTBasename have to look through the library path but this should not cause any issues I believe.

Sorry for the inconvenience and the test failure.

[zero9178]

I think this change, while functionality fixing the same as my previous diff, but also fixing the test failure, does deviate a bit from the original review, so I'd like it to be reviewed again if that isn't a problem.

[rnk]

clang/lib/Driver/ToolChain.cpp

446 ↗

(On Diff #326022)

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.

468–469 ↗

(On Diff #326022)

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.

[zero9178]

Addressed one of the reviewer requests and also uploaded additional files part of the patch that were accidently left out.

[zero9178]

clang/lib/Driver/ToolChain.cpp

468–469 ↗

(On Diff #326022)

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.

[rnk]

clang/lib/Driver/ToolChain.cpp

468–469 ↗

(On Diff #326022)

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.

[zero9178]

Avoid going through the library directories, checking for the existence of a runtime library, twice by having getCompilerRTBasename call getCompilerRT and simply extract the filename component. To allow other ToolChains to override the names of runtime libraries, getCompilerRT now calls the virtual function buildCompilerRTBasename. This is used by the BareMetal toolchain.

I chose the name buildCompilerRTBasename as the call chain getCompilerRTBasename -> getCompilerRT -> getCompilerRTBasenameImpl would be a bit confusing. I am not too attached to the name though if better suggestions come along.

[rnk]

lgtm

clang/lib/Driver/ToolChains/Clang.cpp

if (ProfileGenerateArg) {
     if (ProfileGenerateArg->getOption().matches(
             options::OPT_fprofile_instr_generate_EQ))
       CmdArgs.push_back(Args.MakeArgString(Twine("-fprofile-instrument-path=") +
                                            ProfileGenerateArg->getValue()));
     // The default is to use Clang Instrumentation.
     CmdArgs.push_back("-fprofile-instrument=clang");
     if (TC.getTriple().isWindowsMSVCEnvironment()) {
       // Add dependent lib for clang_rt.profile
+      std::string ClangRtProfilePath = TC.getCompilerRT(Args, "profile");
       CmdArgs.push_back(Args.MakeArgString(
-          "--dependent-lib=" + TC.getCompilerRTBasename(Args, "profile")));
+          "--dependent-lib=" + llvm::sys::path::filename(ClangRtProfilePath)));
     }
   }
 
   Arg *PGOGenArg = nullptr;
   if (PGOGenerateArg) {
     assert(!CSPGOGenerateArg);
     PGOGenArg = PGOGenerateArg;
     CmdArgs.push_back("-fprofile-instrument=llvm");

clang/test/Driver/cl-options.c

 // fpprecise-NOT: -ffast-math
 
 // RUN: %clang_cl /fp:fast /fp:strict -### -- %s 2>&1 | FileCheck -check-prefix=fpstrict %s
 // fpstrict-NOT: -menable-unsafe-fp-math
 // fpstrict-NOT: -ffast-math
 
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-INSTR-GENERATE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-INSTR-GENERATE-FILE %s
-// CHECK-PROFILE-INSTR-GENERATE: "-fprofile-instrument=clang" "--dependent-lib=clang_rt.profile-{{[^"]*}}.lib"
+// CHECK-PROFILE-INSTR-GENERATE: "-fprofile-instrument=clang" "--dependent-lib=clang_rt.profile{{[^"]*}}.lib"
 // CHECK-PROFILE-INSTR-GENERATE-FILE: "-fprofile-instrument-path=/tmp/somefile.profraw"
 
 // RUN: %clang_cl -### /FA -fprofile-generate -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=llvm" "--dependent-lib=clang_rt.profile-{{[^"]*}}.lib"
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=llvm" "--dependent-lib=clang_rt.profile{{[^"]*}}.lib"
 
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file -- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 
 // RUN: %clang_cl -### /FA -fprofile-instr-use -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-use=/tmp/somefile.prof -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-USE-FILE %s
 // CHECK-PROFILE-USE: "-fprofile-instrument-use-path=default.profdata"

clang/test/Driver/instrprof-ld.c

 // CHECK-WATCHOS-ARMV7: "{{.*}}/Inputs/resource_dir{{/|\\\\}}lib{{/|\\\\}}darwin{{/|\\\\}}libclang_rt.profile_watchos.a"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     -target i386-pc-win32 -fprofile-instr-generate \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-I386 %s
 //
 // CHECK-WINDOWS-I386: "{{.*}}link{{(.exe)?}}"
-// CHECK-WINDOWS-I386: "{{.*}}clang_rt.profile-i386.lib"
+// CHECK-WINDOWS-I386: "{{.*}}clang_rt.profile{{(-i386)?}}.lib"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     -target x86_64-pc-win32 -fprofile-instr-generate \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-X86-64 %s
 //
 // CHECK-WINDOWS-X86-64: "{{.*}}link{{(.exe)?}}"
-// CHECK-WINDOWS-X86-64: "{{.*}}clang_rt.profile-x86_64.lib"
+// CHECK-WINDOWS-X86-64: "{{.*}}clang_rt.profile{{(-x86_64)?}}.lib"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN:     -target x86_64-mingw32 -fprofile-instr-generate -fuse-ld=ld \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:   | FileCheck --check-prefix=CHECK-MINGW-X86-64 %s
 //
 // CHECK-MINGW-X86-64: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-MINGW-X86-64: "{{.*}}/Inputs/resource_dir{{/|\\\\}}lib{{/|\\\\}}windows{{/|\\\\}}libclang_rt.profile-x86_64.a"
 
 // Test instrumented profiling dependent-lib flags
 //
 // RUN: %clang %s -### -o %t.o -target x86_64-pc-win32 \
 // RUN:     -fprofile-instr-generate 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-X86-64-DEPENDENT-LIB %s
 //
-// CHECK-WINDOWS-X86-64-DEPENDENT-LIB: "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+// CHECK-WINDOWS-X86-64-DEPENDENT-LIB: "--dependent-lib={{[^"]*}}clang_rt.profile{{[^"]*}}.lib"
 //
 // RUN: %clang %s -### -o %t.o -target x86_64-mingw32 \
 // RUN:     -fprofile-instr-generate 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-MINGW-X86-64-DEPENDENT-LIB %s
 //
 // CHECK-MINGW-X86-64-DEPENDENT-LIB-NOT: "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.a"