[Driver][Windows] Add dependent lib argument for profile instr generate

[russell.gallop]

Summary

This is needed so lld-link can find clang_rt.profile when self hosting on Windows with PGO.

Trying to self host on Windows with PGO runs into undefined symbols as lld-link doesn't know to link clang_rt.profile as discussed here:
http://lists.llvm.org/pipermail/llvm-dev/2019-January/129500.html

Using clang-cl as a linker knows to add the library but self hosting, using -DCMAKE_LINKER=<...>/lld-link.exe doesn't. I think that the cleanest way to make this work is to add a dependent library when using profile-instr-generate on Windows.

Note some cmake changes are also required to get PGO self hosting on Windows working. I will post them separately.

[rnk]

Thanks, I would like to do this for the sanitizers as well, since this is a constant pain point for users, who have to come up with an architecture-dependent filename (clang_rt.asan-dynamic-$arch.lib or something).

clang/lib/Driver/ToolChains/Clang.cpp

787 ↗

(On Diff #198861)

This won't work with ld.bfd, so I would restrict this to a Windows MSVC environment. For mingw, I'd expect the user to call the compiler to link, and the right library path to be supplied to the linker by the compiler driver.

[russell.gallop]

Prevent adding -dependent-lib on mingw32 and add tests.

[russell.gallop]

In D61742#1497259, @rnk wrote:

Thanks, I would like to do this for the sanitizers as well, since this is a constant pain point for users

That would be good. I think that this might already work for UBSan.

[rnk]

lgtm

[thakis]

Hello, this embeds an absolute path into the generated .obj file, which means the output now is no longer deterministic (since it depends on the absolute path to clang_rt.profile-x86_64.lib). This means the output will be different on different machines, which breaks things like caching in distributed builds and whatnot.

I can't see an obvious way to save this patch either, since you won't know at compile time what the CWD will be at link time.

As-is, this breaks Chromium's deterministic builds. ("Only" the coverage builds, and they don't yet check for determinism, which is why it took us a while to notice.)

Suggestions?

[russell.gallop]

Hello, this embeds an absolute path into the generated .obj file, which means the output now is no longer deterministic (since it depends on the absolute path to clang_rt.profile-x86_64.lib).

Yes, it embeds the absolute path. Note that -fsanitize=undefined has done that since r241225 (in 2015).

This means the output will be different on different machines, which breaks things like caching in distributed builds and whatnot.

I assume that your distributed build sandboxes tool paths so this is deterministic from one users point of view, just not deterministic across users with tools installed in different locations so it defeats the cache. Yes, that's not ideal.

I can't see an obvious way to save this patch either, since you won't know at compile time what the CWD will be at link time.

I assume you're considering relative paths here. Yes, I don't think that would work nicely.

As-is, this breaks Chromium's deterministic builds. ("Only" the coverage builds, and they don't yet check for determinism, which is why it took us a while to notice.)
Suggestions?

If this was made into a library name instead of a path (e.g. --dependent-lib=clang_rt.profile-x86_64.lib) then it would solve the problem of different paths on different nodes, but then you would have to tell the linker where to find this library.

That is *almost* as bad as having to specify the full path to the library to the linker in the first place! It's not quite as bad as you don't have to specify the architecture and you only need to specify it once regardless of whether you're using profiling or sanitizers. If lld-link added the clang library path itself, then the library name would be all that was needed from the compiler side.

Rui, would it be possible for lld-link to search the path to the clang libraries (e.g. C:\Program Files\LLVM\lib\clang\8.0.0\lib\windows) by default? Would this have any undesirable side effects?

[thakis]

After some more reading, it looks like passing -no-canonical-prefixes makes this path relative, and as long as compiles and links run from the same CWD things will work and be path-independent. So I think this is all good. Sorry about the false alarm! (Maybe we should have a test that fprofile-instr-generate + no-canonical-prefixes doesn't generate an absolute path?)

[russell.gallop]

it looks like passing -no-canonical-prefixes makes this path relative

Do you see this working? I tried writing a test and it doesn't appear to work for me.

[thakis]

It seems to work for me from what I can tell:

$ out/gn/bin/clang-cl /c -fprofile-instr-generate /TC /dev/null -### -no-canonical-prefixes 2>&1 | grep -o 'dependent-lib=[^"]*'
dependent-lib=libcmt
dependent-lib=oldnames
dependent-lib=out/gn/lib/clang/9.0.0/lib/windows/clang_rt.profile-x86_64.lib

[rnk]

I was going to suggest that maybe what we should do is just embed the basename, i.e. /nodefaultlib:clang_rt.profile-x86_64.lib, and then we just ask users to add one /libpath: flag to their linker invocation. That saves users from having to come up with a mapping from their build system CPU name to whatever compiler-rt is using (i686, i386, x86-64, x86_64h, etc). Users should probably put clang's library directory on libpath anyway if they want to be able to find the builtins library, which might be necessary even in a standard (no-pgo, no-coverage) build configuration. This is also consistent with MSVC in some ways, which autolinks the CRT this way.

[russell.gallop]

I was going to suggest that maybe what we should do is just embed the basename, i.e. /nodefaultlib:clang_rt.profile-x86_64.lib ...

Do you mean /defaultlib:clang_rt.profile-x86_64.lib?

... and then we just ask users to add one /libpath: flag to their linker invocation

I'll try that. If it works, should we revisit the ubsan dependent lib support, to be consistent?

[rnk]

In D61742#1519539, @russell.gallop wrote:

I was going to suggest that maybe what we should do is just embed the basename, i.e. /nodefaultlib:clang_rt.profile-x86_64.lib ...

Do you mean /defaultlib:clang_rt.profile-x86_64.lib?

... and then we just ask users to add one /libpath: flag to their linker invocation

I'll try that. If it works, should we revisit the ubsan dependent lib support, to be consistent?

Yes, definitely. I'd like to do the same for asan, but things there are complicated by the various CRT linking modes.

[thakis]

Since this seems to work with no-canonical-prefixes, I think this is good as-is since people who want build path independent builds need that flag anyway. (At least if Russel can reproduce it working with no-canonical-prefixes; not sure why it wouldn't.)

lib/Driver/ToolChains/Clang.cpp

#ifdef LLVM_ON_UNIX
   std::string UID = llvm::utostr(getuid());
 #else
   // FIXME: Windows seems to have an 'SID' that might work.
   std::string UID = "9999";
 #endif
   Result.append(UID.begin(), UID.end());
 }
 
-static void addPGOAndCoverageFlags(Compilation &C, const Driver &D,
-                                   const InputInfo &Output, const ArgList &Args,
+static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
+                                   const Driver &D, const InputInfo &Output,
+                                   const ArgList &Args,
                                    ArgStringList &CmdArgs) {
 
   auto *PGOGenerateArg = Args.getLastArg(options::OPT_fprofile_generate,
                                          options::OPT_fprofile_generate_EQ,
                                          options::OPT_fno_profile_generate);
   if (PGOGenerateArg &&
       PGOGenerateArg->getOption().matches(options::OPT_fno_profile_generate))
     PGOGenerateArg = nullptr;
static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
 
   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
+      CmdArgs.push_back(Args.MakeArgString("--dependent-lib=" +
+                                           TC.getCompilerRT(Args, "profile")));
+    }
   }
 
   Arg *PGOGenArg = nullptr;
   if (PGOGenerateArg) {
     assert(!CSPGOGenerateArg);
     PGOGenArg = PGOGenerateArg;
     CmdArgs.push_back("-fprofile-instrument=llvm");
   }
if (auto *A = Args.getLastArg(
       options::OPT_finstrument_functions_after_inlining,
       options::OPT_finstrument_function_entry_bare))
     A->render(Args, CmdArgs);
 
   // NVPTX doesn't support PGO or coverage. There's no runtime support for
   // sampling, overhead of call arc collection is way too high and there's no
   // way to collect the output.
   if (!Triple.isNVPTX())
-    addPGOAndCoverageFlags(C, D, Output, Args, CmdArgs);
+    addPGOAndCoverageFlags(TC, C, D, Output, Args, CmdArgs);
 
   if (auto *ABICompatArg = Args.getLastArg(options::OPT_fclang_abi_compat_EQ))
     ABICompatArg->render(Args, CmdArgs);
 
   // Add runtime flag for PS4 when PGO, coverage, or sanitizers are enabled.
   if (RawTriple.isPS4CPU() &&
       !Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
     PS4cpu::addProfileRTArgs(TC, Args, CmdArgs);

test/Driver/cl-options.c

 
 // RUN: %clang_cl /Z7 -### -- %s 2>&1 | FileCheck -check-prefix=gdefcolumn %s
 // gdefcolumn-NOT: -dwarf-column-info
 
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
 // 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-PROFILE-GENERATE: "-fprofile-instrument=clang"
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
 // CHECK-PROFILE-GENERATE-FILE: "-fprofile-instrument-path=/tmp/somefile.profraw"
 // 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"
 // CHECK-PROFILE-USE-FILE: "-fprofile-instrument-use-path=/tmp/somefile.prof"
 

test/Driver/instrprof-ld.c

 //
 // 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"
+//
+// 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"