[profile] runtime counter relocation is needed on windows-msvc targets

[w2yehia]

Continuous profiling syncing is supported on windows, and it also relies on runtime counter relocation (based on this test)

Thanks to @anhtuyenibm for pointing it out to me.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Wael Yehia (w2yehia)

Changes

Continuous profiling syncing is supported on windows, and it also relies on runtime counter relocation (based on this test)

Thanks to @anhtuyenibm for pointing it out to me.

---

Full diff: https://github.com/llvm/llvm-project/pull/127858.diff

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Clang.cpp (+1-1)
• (modified) clang/test/Driver/fprofile-continuous.c (+1)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 96af466e067a8..3f76d034ee308 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -795,7 +795,7 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
     else {
       CmdArgs.push_back("-fprofile-continuous");
       // Platforms that require a bias variable:
-      if (T.isOSBinFormatELF() || T.isOSAIX()) {
+      if (T.isOSBinFormatELF() || T.isOSAIX() || T.isKnownWindowsMSVCEnvironment()) {
         CmdArgs.push_back("-mllvm");
         CmdArgs.push_back("-runtime-counter-relocation");
       }
diff --git a/clang/test/Driver/fprofile-continuous.c b/clang/test/Driver/fprofile-continuous.c
index 81719fb70cb1e..cc8e56c8f9fe0 100644
--- a/clang/test/Driver/fprofile-continuous.c
+++ b/clang/test/Driver/fprofile-continuous.c
@@ -6,6 +6,7 @@
 
 // RUN: %clang --target=powerpc64-ibm-aix -fprofile-generate -fprofile-continuous -### -c %s 2>&1 | FileCheck %s --check-prefix=RELOC
 // RUN: %clang --target=x86_64-unknown-fuchsia -fprofile-generate -fprofile-continuous -### -c %s 2>&1 | FileCheck %s --check-prefix=RELOC
+// RUN: %clang --target=x86_64-windows-msvc -fprofile-generate -fprofile-continuous -### -c %s 2>&1 | FileCheck %s --check-prefix=RELOC
 // RELOC: "-cc1" {{.*}} "-fprofile-continuous" "-mllvm" "-runtime-counter-relocation"
 
 // 2) test -fprofile-continuous with cs-profile-generate and -fprofile-instr-generate

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Andarwinux]

Doesn't mingw target support it?

[w2yehia]

Doesn't mingw target support it?

Sorry I don't know. I was hoping someone familiar with clang on windows to comment.

[anhtuyenibm]

Thanks for pointing out this reference page. The changes look good to me.

[mstorsjo]

Doesn't mingw target support it?

I'm pretty sure that profiling already works, both in mingw and MSVC configurations. I'm not familiar with all the nuances in various forms of profiling though, so it's unclear to me if this is some specific fringe configuration which didn't work before, or what this change actually changes.

Can you summarize the high level use case which didn't work before, and in which way it used to fail before, which gets fixed by this change?

[w2yehia]

Can you summarize the high level use case which didn't work before, and in which way it used to fail before, which gets fixed by this change?

Continuous profiling (that is continuous profile syncing to disk) is an existing feature, which is OFF by default on most platforms.
The feature mmap's the profile file rather than writing it at program exit (the default non-continuous mode behavior). There are two implementations in InstrProfilingFile.c, one that mmap's onto existing memory, and another that mmap's without an address hint. The latter relies on a corresponding compiler action (added in this commit), which is enabled using -mllvm -runtime-counter-relocation.

What I added, in a previous PR (#124353), was a clang option (-fprofile-continuous) to enable the feature and take care of the nuances of setting up the compiler and runtime: (1) override the profile file name by appending '%c' into it, and (2) pass -runtime-counter-relocation to the backend on certain platforms.
This PR added T.isKnownWindowsMSVCEnvironment() to the conditions under which the drivers passes -runtime-counter-relocation to the backend, and the question is whether this is sufficient for windows or more changes to the conditions are needed.
Note if we miss a condition/configuration then it doesn't break the feature there, it just would fail at runtime if you try enabling using this new clang option (-fprofile-continuous).

[mstorsjo]

Thanks for the very thorough explanation!

Note if we miss a condition/configuration then it doesn't break the feature there, it just would fail at runtime if you try enabling using this new clang option (-fprofile-continuous).

I see! With this, I was able to test the feature in a mingw configuration, and I do get LLVM Profile Error: Neither __llvm_profile_counter_bias nor __llvm_profile_bitmap_bias is defined at runtime there as well. If I modify the condition added by this patch to check T.isOSWindows() instead of T.isKnownWindowsMSVCEnvironment(), then it doesn't print any such error at runtime. So I believe this should be generalized to all of Windows (and/or generalized to check for a COFF object file format?) instead of singling out specifically MSVC only.

[w2yehia]

So I believe this should be generalized to all of Windows (and/or generalized to check for a COFF object file format?) instead of singling out specifically MSVC only.

Thanks for checking. I can fix the condition but can you please let me know which of the two options you listed is better?

[mstorsjo]

So I believe this should be generalized to all of Windows (and/or generalized to check for a COFF object file format?) instead of singling out specifically MSVC only.

Thanks for checking. I can fix the condition but can you please let me know which of the two options you listed is better?

I don't entirely know as I'm not familiar with what -runtime-counter-relocation; the main case where the distinction matters is e.g. if using ELF object files on Windows. (This isn't usually done for regular executables, but it can happen e.g. within a JIT.) I would perhaps just go with isOSWindows() here, that's the much more common condition. Then for the testcase, you can add a test with a <arch>-windows-gnu triple. Thanks!

[w2yehia]

@mstorsjo FYI: 9ef7287d425

[mstorsjo]

@mstorsjo FYI: 9ef7287d425

Thank you!