[profile] Change __llvm_profile_counter_bias type to match llvm #107362
Conversation
As detailed in Issue llvm#101667, two `profile` tests `FAIL` on 32-bit SPARC, both Linux/sparc64 and Solaris/sparcv9 (where the tests work when enabled): ``` Profile-sparc :: ContinuousSyncMode/runtime-counter-relocation.c Profile-sparc :: ContinuousSyncMode/set-file-object.c ``` The Solaris linker provides the crucial clue as to what's wrong: ``` ld: warning: symbol '__llvm_profile_counter_bias' has differing sizes: (file runtime-counter-relocation-17ff25.o value=0x8; file libclang_rt.profile-sparc.a(InstrProfilingFile.c.o) value=0x4); runtime-counter-relocation-17ff25.o definition taken ``` In fact, the types in `llvm` and `compiler-rt` differ: - `__llvm_profile_counter_bias`/`INSTR_PROF_PROFILE_COUNTER_BIAS_VAR` is created in `llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp` (`InstrLowerer::getCounterAddress`) as `int64_t`, while `compiler-rt/lib/profile/InstrProfilingFile.c` uses `intptr_t`. While this doesn't matter in the 64-bit case, the type sizes differ for 32-bit. This patch changes the `compiler-rt` type to match `llvm`. At the same time, the affected testcases are enabled on Solaris, too, where they now just `PASS`. Tested on `sparc64-unknown-linux-gnu`, `sparcv9-sun-solaris2.11`, `x86_64-pc-linux-gnu`, and `amd64-pc-solaris2.11. This is a backport of PR llvm#102747, adjusted for the lack of `__llvm_profile_bitmap_bias` on the `release/19.x` branch.
|
@llvm/pr-subscribers-pgo Author: Rainer Orth (rorth) ChangesAs detailed in Issue #101667, two The Solaris linker provides the crucial clue as to what's wrong: In fact, the types in
This patch changes the Tested on This is a backport of PR #102747, adjusted for the lack of Full diff: https://github.com/llvm/llvm-project/pull/107362.diff 4 Files Affected:
diff --git a/compiler-rt/lib/profile/InstrProfilingFile.c b/compiler-rt/lib/profile/InstrProfilingFile.c
index 1c58584d2d4f73..3bb2ae068305c9 100644
--- a/compiler-rt/lib/profile/InstrProfilingFile.c
+++ b/compiler-rt/lib/profile/InstrProfilingFile.c
@@ -198,12 +198,12 @@ static int mmapForContinuousMode(uint64_t CurrentFileOffset, FILE *File) {
#define INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR \
INSTR_PROF_CONCAT(INSTR_PROF_PROFILE_COUNTER_BIAS_VAR, _default)
-COMPILER_RT_VISIBILITY intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR = 0;
+COMPILER_RT_VISIBILITY int64_t INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR = 0;
/* This variable is a weak external reference which could be used to detect
* whether or not the compiler defined this symbol. */
#if defined(_MSC_VER)
-COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
+COMPILER_RT_VISIBILITY extern int64_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
#if defined(_M_IX86) || defined(__i386__)
#define WIN_SYM_PREFIX "_"
#else
@@ -214,7 +214,7 @@ COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
INSTR_PROF_PROFILE_COUNTER_BIAS_VAR) "=" WIN_SYM_PREFIX \
INSTR_PROF_QUOTE(INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR))
#else
-COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR
+COMPILER_RT_VISIBILITY extern int64_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR
__attribute__((weak, alias(INSTR_PROF_QUOTE(
INSTR_PROF_PROFILE_COUNTER_BIAS_DEFAULT_VAR))));
#endif
diff --git a/compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c b/compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c
index fdcb82e4d72baf..65b7bdaf403da4 100644
--- a/compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c
+++ b/compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c
@@ -35,7 +35,7 @@
#include "InstrProfilingUtil.h"
/* This variable is an external reference to symbol defined by the compiler. */
-COMPILER_RT_VISIBILITY extern intptr_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
+COMPILER_RT_VISIBILITY extern int64_t INSTR_PROF_PROFILE_COUNTER_BIAS_VAR;
COMPILER_RT_VISIBILITY unsigned lprofProfileDumped(void) {
return 1;
diff --git a/compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c b/compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
index 4ca8bf62455371..19a7aae70cb0d3 100644
--- a/compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
+++ b/compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
@@ -1,4 +1,4 @@
-// REQUIRES: linux || windows
+// REQUIRES: target={{.*(linux|solaris|windows-msvc).*}}
// RUN: %clang -fprofile-instr-generate -fcoverage-mapping -mllvm -runtime-counter-relocation=true -o %t.exe %s
// RUN: echo "garbage" > %t.profraw
diff --git a/compiler-rt/test/profile/ContinuousSyncMode/set-file-object.c b/compiler-rt/test/profile/ContinuousSyncMode/set-file-object.c
index b52324d7091eb2..53609f5838f753 100644
--- a/compiler-rt/test/profile/ContinuousSyncMode/set-file-object.c
+++ b/compiler-rt/test/profile/ContinuousSyncMode/set-file-object.c
@@ -1,4 +1,4 @@
-// REQUIRES: darwin || linux
+// REQUIRES: target={{.*(darwin|linux|solaris).*}}
// Test using __llvm_profile_set_file_object in continuous mode (%c).
// Create & cd into a temporary directory.
|
|
Hi, since we are wrapping up LLVM 19.1.0 we are very strict with the fixes we pick at this point. Can you please respond to the following questions to help me understand if this has to be included in the final release or not. Is this PR a fix for a regression or a critical issue? What is the risk of accepting this into the release branch? What is the risk of NOT accepting this into the release branch? |
I guess it's best for @petrhosek to make the final assessment. That said...
... it's not a regression but a long(er)-standing bug. Critical? Probably not.
Although this has been on |
|
I agree, while the risk is low, this issue has existed for several releases so it should fine do delay the fix a bit further. |
As detailed in Issue #101667, two
profiletestsFAILon 32-bit SPARC, both Linux/sparc64 and Solaris/sparcv9 (where the tests work when enabled):The Solaris linker provides the crucial clue as to what's wrong:
In fact, the types in
llvmandcompiler-rtdiffer:__llvm_profile_counter_bias/INSTR_PROF_PROFILE_COUNTER_BIAS_VARis created inllvm/lib/Transforms/Instrumentation/InstrProfiling.cpp(InstrLowerer::getCounterAddress) asint64_t, whilecompiler-rt/lib/profile/InstrProfilingFile.cusesintptr_t. While this doesn't matter in the 64-bit case, the type sizes differ for 32-bit.This patch changes the
compiler-rttype to matchllvm. At the same time, the affected testcases are enabled on Solaris, too, where they now justPASS.Tested on
sparc64-unknown-linux-gnu,sparcv9-sun-solaris2.11,x86_64-pc-linux-gnu, and `amd64-pc-solaris2.11.This is a backport of PR #102747, adjusted for the lack of
__llvm_profile_bitmap_biason therelease/19.xbranch.