When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely #135643

khuey · 2025-01-17T18:20:31Z

Dropping them fails -Zverify-llvm-ir.

rustbot · 2025-01-17T18:20:42Z

Some changes occurred in compiler/rustc_codegen_gcc

jieyouxu

Thanks. One thing -- is it possible to conjure a regression test for this? I imagine that it's not going to be very feasible because the test case would have to exceed "LLVM's location discriminator value limit", but I wanted to ask in case you actually have a magical way. Based on the synthetic reproducer linked in #135332, we might be able to synthetic generate such a test case in a run-make test. What do you think?

EDIT: no that might be fragile, because it would be testing against an LLVM-internal limit which I'm not sure is guaranteed to remain the same between different LLVM versions.

jieyouxu · 2025-01-18T01:11:53Z

I would consider this for a beta backport + stable backport because a real-world crate (as reported in #135332) is impacted when trying to generate debuginfo for some configuration such as

debug = "line-tables-only" # any value that's not "none"
lto = "fat"

This PR changes locations to use dummy spans instead of being dropped entirely to inhibit generation of broken LLVM modules which fail to contain !dbg location metadata for a "inlinable function call in a function with debug info". This fixes a stable-to-stable regression Broken LLVM module: inlinable function call in a function with debug info must have a !dbg location #135332.

The stable backport nomination is because 1.84.0 still "unconditionally verif[ies] the LLVM IR in the backend (twice), ignoring the value of the verify-llvm-ir option". That was changed in #133499 which would be available for 1.85.0.

@rustbot label +beta-nominated +stable-nominated

khuey · 2025-01-18T03:55:22Z

I attempted to turn the reporter-provided testcase into a compile test but could not get it to fail. We have existing tests that exceed LLVM's limits (it's currently only 4096, so not hard to exceed). But the provided testcase also requires fat pgo and some other ingredient not present in compiletest that I failed to isolate ...

jieyouxu · 2025-01-18T04:11:23Z

I'll see if I can cook up a test that fails locally. If not, the change is mostly "obvious" enough.

jieyouxu · 2025-01-18T05:05:11Z

I was able to conjure a regression test based on the reproducer. It's not pretty, but notably it fails without the changes in this PR with the broken LLVM module error, and passes with the changes in this PR. Running the test locally on msvc to double check.

jieyouxu · 2025-01-18T05:26:09Z

Ok, the regression test fails as expected on both linux and msvc without the fix in this PR.

@khuey could you cherry-pick jieyouxu@1439412, and check that if we have three commits:

The cherry-picked regression test commit mentioned above
(Current PR commit 1) The revert commit
(Current PR commit 2) The refix commit

That the regression test fails at (1.) against master, but passes after (3.) for you locally as well? I could also just push the commit to your branch if you prefer.

Feel free to improve the test where suitable.

rustbot · 2025-01-18T19:24:57Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

khuey · 2025-01-18T19:24:58Z

Thanks. This test does fail after commits 1 and 2 but passes after 3. I trimmed the compiler flags a bit.

Assuming you're fine with the 30 second runtime (fat lto + verify-llvm-ir = ouch) it looks good to me.

jieyouxu · 2025-01-19T09:29:46Z

Thanks. This test does fail after commits 1 and 2 but passes after 3. I trimmed the compiler flags a bit.

Assuming you're fine with the 30 second runtime (fat lto + verify-llvm-ir = ouch) it looks good to me.

Yeah I noticed the test being slow which is not great. If we ever think this is too long versus the value that the test provides, then I wouldn't mind deleting this test.

jieyouxu

Thanks, two minor nits on the test, otherwise LGTM

tests/run-make/llvm-location-discriminator-limit-dummy-span/rmake.rs

khuey · 2025-01-19T14:41:12Z

Let me know what you think of those comment changes. If that's satisfactory I'll either squash those into the test commit or squash all the commits together, your choice.

jieyouxu

Thanks, looks good to me. Let's just squash the commits.

…tions with dummy spans instead of dropping them entirely Revert most of rust-lang#133194 (except the test and the comment fixes). Then refix not emitting locations at all when the correct location discriminator value exceeds LLVM's capacity.

jieyouxu · 2025-01-19T16:49:02Z

r=me after PR CI is green

@bors delegate+ rollup=never

bors · 2025-01-19T16:49:05Z

✌️ @khuey, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

khuey · 2025-01-19T17:55:30Z

@bors r=@jieyouxu

bors · 2025-01-19T17:55:32Z

📌 Commit 45ef927 has been approved by jieyouxu

It is now in the queue for this repository.

jieyouxu · 2025-01-20T13:45:49Z

@bors p=5 (rollup scheduling)

bors · 2025-01-20T14:16:25Z

⌛ Testing commit 45ef927 with merge 6a64e3b...

bors · 2025-01-20T17:06:22Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 6a64e3b to master...

rust-timer · 2025-01-20T18:36:03Z

Finished benchmarking commit (6a64e3b): 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 (primary 2.5%, secondary 2.8%)

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)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	1

Cycles

Results (secondary 2.6%)

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.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 767.44s -> 766.257s (-0.15%)
Artifact size: 325.97 MiB -> 325.96 MiB (-0.00%)

apiraino · 2025-01-23T15:45:41Z

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them.

Stable backport approved as well. Leaning to motivate for a dot release.

@rustbot label +beta-accepted +stable-accepted

[beta] backports - Always force non-trimming of path in `unreachable_patterns` lint rust-lang#135310 - Add Profile Override for Non-Git Sources rust-lang#135433 - resolve symlinks of LLVM tool binaries before copying them rust-lang#135585 - add cache to `AmbiguityCausesVisitor` rust-lang#135618 - When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely rust-lang#135643 - Temporarily bring back `Rvalue::Len` rust-lang#135709 - make it possible to use ci-rustc on tarball sources rust-lang#135722 - Remove test panic from File::open rust-lang#135837 - Only assert the `Parser` size on specific arches rust-lang#135855 r? cuviper

[beta] backports - Always force non-trimming of path in `unreachable_patterns` lint rust-lang#135310 - Add Profile Override for Non-Git Sources rust-lang#135433 - resolve symlinks of LLVM tool binaries before copying them rust-lang#135585 - add cache to `AmbiguityCausesVisitor` rust-lang#135618 - When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely rust-lang#135643 - Temporarily bring back `Rvalue::Len` rust-lang#135709 - make it possible to use ci-rustc on tarball sources rust-lang#135722 - Remove test panic from File::open rust-lang#135837 - Only assert the `Parser` size on specific arches rust-lang#135855 - [beta] TRPL: more backward-compatible Edition changes rust-lang#135843 r? cuviper

[beta] backports - Always force non-trimming of path in `unreachable_patterns` lint rust-lang#135310 - Add Profile Override for Non-Git Sources rust-lang#135433 - resolve symlinks of LLVM tool binaries before copying them rust-lang#135585 - add cache to `AmbiguityCausesVisitor` rust-lang#135618 - When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely rust-lang#135643 - make it possible to use ci-rustc on tarball sources rust-lang#135722 - Remove test panic from File::open rust-lang#135837 - Only assert the `Parser` size on specific arches rust-lang#135855 - [beta] TRPL: more backward-compatible Edition changes rust-lang#135843 r? cuviper

[stable] Prepare Rust 1.84.1 point release - [Fix ICE 132920 in duplicate-crate diagnostics.](rust-lang#133304) - [Fix errors for overlapping impls in incremental rebuilds.](rust-lang#133828) - [Fix slow compilation related to the next-generation trait solver.](rust-lang#135618) - [Fix debuginfo when LLVM's location discriminator value limit is exceeded.](rust-lang#135643) - Fixes for building Rust from source: - [Only try to distribute `llvm-objcopy` if llvm tools are enabled.](rust-lang#134240) - [Add Profile Override for Non-Git Sources.](rust-lang#135433) - [Resolve symlinks of LLVM tool binaries before copying them.](rust-lang#135585) - [Make it possible to use ci-rustc on tarball sources.](rust-lang#135722) cc `@rust-lang/release` r? ghost

rustbot assigned jieyouxu Jan 17, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 17, 2025

jieyouxu approved these changes Jan 18, 2025

View reviewed changes

jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2025

rustbot added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Jan 18, 2025

khuey force-pushed the 135332 branch from cb95984 to 613d877 Compare January 18, 2025 19:24

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Jan 18, 2025

jieyouxu approved these changes Jan 19, 2025

View reviewed changes

tests/run-make/llvm-location-discriminator-limit-dummy-span/rmake.rs Outdated Show resolved Hide resolved

tests/run-make/llvm-location-discriminator-limit-dummy-span/rmake.rs Outdated Show resolved Hide resolved

jieyouxu approved these changes Jan 19, 2025

View reviewed changes

khuey force-pushed the 135332 branch from f31e436 to 45ef927 Compare January 19, 2025 16:42

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 20, 2025

bors merged commit 6a64e3b into rust-lang:master Jan 20, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 20, 2025

apiraino mentioned this pull request Jan 20, 2025

Broken LLVM module: inlinable function call in a function with debug info must have a !dbg location #135332

Closed

rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Jan 23, 2025

cuviper mentioned this pull request Jan 24, 2025

[beta] backports #136017

Merged

cuviper modified the milestones: 1.86.0, 1.85.0 Jan 24, 2025

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 24, 2025

cuviper mentioned this pull request Jan 27, 2025

[stable] Prepare Rust 1.84.1 point release #136158

Merged

cuviper modified the milestones: 1.85.0, 1.84.1 Jan 27, 2025

cuviper removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Jan 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely #135643

When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely #135643

khuey commented Jan 17, 2025

rustbot commented Jan 17, 2025

jieyouxu left a comment •

edited

Loading

jieyouxu commented Jan 18, 2025 •

edited

Loading

khuey commented Jan 18, 2025

jieyouxu commented Jan 18, 2025

jieyouxu commented Jan 18, 2025 •

edited

Loading

jieyouxu commented Jan 18, 2025 •

edited

Loading

rustbot commented Jan 18, 2025

khuey commented Jan 18, 2025 •

edited

Loading

jieyouxu commented Jan 19, 2025 •

edited

Loading

jieyouxu left a comment

khuey commented Jan 19, 2025

jieyouxu left a comment

jieyouxu commented Jan 19, 2025

bors commented Jan 19, 2025

khuey commented Jan 19, 2025

bors commented Jan 19, 2025

jieyouxu commented Jan 20, 2025

bors commented Jan 20, 2025

bors commented Jan 20, 2025

rust-timer commented Jan 20, 2025

apiraino commented Jan 23, 2025

When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely #135643

When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely #135643

Conversation

khuey commented Jan 17, 2025

rustbot commented Jan 17, 2025

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu commented Jan 18, 2025 • edited Loading

khuey commented Jan 18, 2025

jieyouxu commented Jan 18, 2025

jieyouxu commented Jan 18, 2025 • edited Loading

jieyouxu commented Jan 18, 2025 • edited Loading

rustbot commented Jan 18, 2025

khuey commented Jan 18, 2025 • edited Loading

jieyouxu commented Jan 19, 2025 • edited Loading

jieyouxu left a comment

Choose a reason for hiding this comment

khuey commented Jan 19, 2025

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu commented Jan 19, 2025

bors commented Jan 19, 2025

khuey commented Jan 19, 2025

bors commented Jan 19, 2025

jieyouxu commented Jan 20, 2025

bors commented Jan 20, 2025

bors commented Jan 20, 2025

rust-timer commented Jan 20, 2025

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

apiraino commented Jan 23, 2025

jieyouxu left a comment •

edited

Loading

jieyouxu commented Jan 18, 2025 •

edited

Loading

jieyouxu commented Jan 18, 2025 •

edited

Loading

jieyouxu commented Jan 18, 2025 •

edited

Loading

khuey commented Jan 18, 2025 •

edited

Loading

jieyouxu commented Jan 19, 2025 •

edited

Loading