Don't internalize __llvm_profile_counter_bias

[abrachet]

Currently, LLVM profiling runtime counter relocation cannot be used by rust during LTO because symbols are being internalized before all symbol information is known.

This mode makes LLVM emit a __llvm_profile_counter_bias symbol which is referenced by the profiling initialization, which itself is pulled in by the rust driver here [1].

It is enabled with -Cllvm-args=-runtime-counter-relocation for platforms which are opt-in to this mode like Linux. On these platforms there will be no link error, rather just surprising behavior for a user which request runtime counter relocation. The profiling runtime will not see that symbol go on as if it were never there. On Fuchsia, the profiling runtime must have this symbol which will cause a hard link error.

As an aside, I don't have enough context as to why rust's LTO model is how it is. AFAICT, the internalize pass is only safe to run at link time when all symbol information is actually known, this being an example as to why. I think special casing this symbol as a known one that LLVM can emit which should not have it's visbility de-escalated should be fine given how seldom this pattern of defining an undefined symbol to get initilization code pulled in is. From a quick grep, __llvm_profile_runtime is the only symbol that rustc does this for.

[1]

rust/compiler/rustc_codegen_ssa/src/back/linker.rs

Line 598 in 0265a3e

self.cmd.arg("__llvm_profile_runtime");

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

[bjorn3]

I think it should be added to

rust/compiler/rustc_codegen_ssa/src/back/symbol_export.rs

Line 216 in bb93450

["__llvm_profile_raw_version", "__llvm_profile_filename"];

instead.

[abrachet]

I think it should be added to

rust/compiler/rustc_codegen_ssa/src/back/symbol_export.rs

Line 216 in bb93450

["__llvm_profile_raw_version", "__llvm_profile_filename"];

instead.

Done

[klensy]

You can squash commits to remove now unneeded changes.

[abrachet]

You can squash commits to remove now unneeded changes.

Done

[cjgillot]

r? @bjorn3

[bjorn3]

Could you add a test for this?

[abrachet]

Could you add a test for this?

Sure, let me know if that test looks ok.

[bjorn3]

@bors r+

[bors]

📌 Commit 2f7b4d9 has been approved by bjorn3

It is now in the queue for this repository.

[bors]

⌛ Testing commit 2f7b4d9 with merge a16abce7a31818d541760097059538aa6c5aa1e4...

[bors]

💔 Test failed - checks-actions

[abrachet]

It looks like this creates an undefined reference to these symbols (in /tmp/.../symbols.o), which is fine on other platforms but I guess doesn't work so well with how weak symbols are implemented in Windows, see https://github.com/llvm/llvm-project/blob/6db71b8f1418170324b49d20f1f7b3f7c5086066/compiler-rt/lib/profile/InstrProfilingFile.c#L168-L178.

As far as I can tell the fix is to stop indiscriminately adding this to the exported symbols list and only do so when __llvm_profile_counter_bias is emitted, which seems untenable given llvm decides to do this based on target (which currently is only one target but could likely grow) or backend flag. Alternatively we could go back to putting this in prepare_lto, though here is admittedly much better because this is where other similar symbols are handled. WDYT?

[bjorn3]

I'm not quite sure. Maybe @nikic has an idea?

[abrachet]

Looks like -runtime-counter-relocation is ignored for Mach-O targets [1]. I've added an // ignore-macos line in the test which should hopefully fix the test failure on the bots. Sorry about that

[1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L443

[bjorn3]

@bors r+

[bors]

📌 Commit 5d88d36 has been approved by bjorn3

It is now in the queue for this repository.

[bors]

⌛ Testing commit 5d88d36 with merge d31d00eef1d034f6e478f0088c74516ae3a98f44...

[bors]

💔 Test failed - checks-actions

[ehuss]

@bors retry

Failed to update toolstate
(has been fixed)

[abrachet]

Looks like the same error, should we retry again?

[bjorn3]

@bors retry Failed to update toolstate

[bjorn3]

@bors retry

[ehuss]

This PR is already in the queue. The rust-log-analyzer comment was just delayed by about 12 hours.

[bors]

⌛ Testing commit 5d88d36 with merge d137783...

[bors]

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing d137783 to master...

[rust-timer]

Finished benchmarking commit (d137783): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.0%, 2.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.7%, -2.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0