Add discriminators to DILocations when multiple functions are inlined into a single point. by khuey · Pull Request #132613 · rust-lang/rust

khuey · 2024-11-04T19:56:32Z

LLVM does not expect to ever see multiple dbg_declares for the same variable at the same location with different values. proc-macros make it possible for arbitrary code, including multiple calls that get inlined, to happen at any given location in the source code. Add discriminators when that happens so these locations are different to LLVM.

This may interfere with the AddDiscriminators pass in LLVM, which is added by the unstable flag -Zdebug-info-for-profiling.

Fixes#131944

rustbot · 2024-11-04T19:56:40Z

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

workingjubilee · 2024-11-05T00:59:37Z

@bors try

bors · 2024-11-05T01:00:47Z

⌛ Trying commit c4b99a1 with merge 6bc6e20...

Add discriminators to DILocations when multiple functions are inlined into a single point. LLVM does not expect to ever see multiple dbg_declares for the same variable at the same location with different values. proc-macros make it possible for arbitrary code, including multiple calls that get inlined, to happen at any given location in the source code. Add discriminators when that happens so these locations are different to LLVM. This may interfere with the AddDiscriminators pass in LLVM, which is added by the unstable flag -Zdebug-info-for-profiling. Fixesrust-lang#131944

bors · 2024-11-05T02:55:23Z

☀️ Try build successful - checks-actions
Build commit: 6bc6e20 (6bc6e203f22db68ac5d1affcbc364737fad0f0d6)

jieyouxu · 2024-11-05T13:37:50Z

@bors try

bors · 2024-11-05T13:39:01Z

⌛ Trying commit c4b99a1 with merge 9517283...

Add discriminators to DILocations when multiple functions are inlined into a single point. LLVM does not expect to ever see multiple dbg_declares for the same variable at the same location with different values. proc-macros make it possible for arbitrary code, including multiple calls that get inlined, to happen at any given location in the source code. Add discriminators when that happens so these locations are different to LLVM. This may interfere with the AddDiscriminators pass in LLVM, which is added by the unstable flag -Zdebug-info-for-profiling. Fixesrust-lang#131944 try-job: x86_64-msvc try-job: i686-msvc

bors · 2024-11-05T14:38:16Z

💔 Test failed - checks-actions

jieyouxu · 2024-11-05T14:42:39Z

(I edited the PR description to include a few "selected" try-jobs which should bring about reasonable target variance, rest is probably full build)

workingjubilee · 2024-11-07T05:39:08Z

@bors try

bors · 2024-11-07T05:40:20Z

⌛ Trying commit b7b6b06 with merge 225656d...

Add discriminators to DILocations when multiple functions are inlined into a single point. LLVM does not expect to ever see multiple dbg_declares for the same variable at the same location with different values. proc-macros make it possible for arbitrary code, including multiple calls that get inlined, to happen at any given location in the source code. Add discriminators when that happens so these locations are different to LLVM. This may interfere with the AddDiscriminators pass in LLVM, which is added by the unstable flag -Zdebug-info-for-profiling. Fixesrust-lang#131944 try-job: x86_64-msvc try-job: i686-msvc try-job: x86_64-mingw try-job: aarch64-apple try-job: test-various try-job: dist-various-1 try-job: armhf-gnu

bors · 2024-11-07T06:38:34Z

💔 Test failed - checks-actions

jieyouxu · 2024-11-07T17:27:13Z

@bors try

bors · 2024-11-07T17:28:26Z

⌛ Trying commit 29f30c3 with merge 49e1343...

Add discriminators to DILocations when multiple functions are inlined into a single point. LLVM does not expect to ever see multiple dbg_declares for the same variable at the same location with different values. proc-macros make it possible for arbitrary code, including multiple calls that get inlined, to happen at any given location in the source code. Add discriminators when that happens so these locations are different to LLVM. This may interfere with the AddDiscriminators pass in LLVM, which is added by the unstable flag -Zdebug-info-for-profiling. Fixesrust-lang#131944 try-job: x86_64-msvc try-job: i686-msvc try-job: x86_64-mingw try-job: aarch64-apple try-job: test-various try-job: dist-various-1 try-job: armhf-gnu

bors · 2024-11-07T18:21:52Z

💔 Test failed - checks-actions

cuviper · 2024-11-09T00:10:19Z

Nominating since #131944 was rated critical -- although it doesn't seem like anyone else has had trouble with it.

@rustbot label beta-nominated

workingjubilee · 2024-11-09T04:47:33Z

@bors rollup=never

bors · 2024-11-09T13:12:02Z

☔ The latest upstream changes (presumably #132584) made this pull request unmergeable. Please resolve the merge conflicts.

jieyouxu · 2024-11-09T13:45:02Z

Unfortunately needs a rebase, r=me after that.

@bors delegate

jieyouxu · 2024-11-09T13:46:06Z

Er maybe @bors delegate+

bors · 2024-11-09T13:46:10Z

✌️ @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

… into a single point. LLVM does not expect to ever see multiple dbg_declares for the same variable at the same location with different values. proc-macros make it possible for arbitrary code, including multiple calls that get inlined, to happen at any given location in the source code. Add discriminators when that happens so these locations are different to LLVM. This may interfere with the AddDiscriminators pass in LLVM, which is added by the unstable flag -Zdebug-info-for-profiling. Fixesrust-lang#131944

jieyouxu · 2024-11-09T17:13:42Z

@bors r+ rollup=never p=1 (fixes a P-critical, but also make it easier to bisect)

bors · 2024-11-09T17:13:45Z

📌 Commit 1dc1061 has been approved by jieyouxu

It is now in the queue for this repository.

bors · 2024-11-09T17:23:31Z

⌛ Testing commit 1dc1061 with merge b026d85...

bors · 2024-11-09T19:51:39Z

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

rust-timer · 2024-11-09T21:32:38Z

Finished benchmarking commit (b026d85): 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 -1.7%, secondary -3.2%)

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)	-	-	0
Improvements ✅ (primary)	-1.7%	[-2.0%, -1.4%]	2
Improvements ✅ (secondary)	-3.2%	[-4.5%, -1.9%]	2
All ❌✅ (primary)	-1.7%	[-2.0%, -1.4%]	2

Cycles

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

Binary size

Results (primary 0.0%)

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.0%	[0.0%, 0.0%]	31
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	31

Bootstrap: 781.253s -> 780.559s (-0.09%)
Artifact size: 335.38 MiB -> 335.38 MiB (0.00%)

Add discriminators to DILocations when multiple functions are inlined into a single point. LLVM does not expect to ever see multiple dbg_declares for the same variable at the same location with different values. proc-macros make it possible for arbitrary code, including multiple calls that get inlined, to happen at any given location in the source code. Add discriminators when that happens so these locations are different to LLVM. This may interfere with the AddDiscriminators pass in LLVM, which is added by the unstable flag -Zdebug-info-for-profiling.

apiraino · 2024-11-14T15:19:42Z

Beta backport visited during triage meeting on Zulip.

We're not completely clear about the value added by backporting this. Is this regression really critical? Also the patch fixing this is a bit suspect of causing another ICE (#132900).

So let's postpone the backport decision for a week (release in 2 weeks from now) waiting to have more info.

Drop debug info instead of panicking if we exceed LLVM's capability to represent it Recapping a bit of history here: In rust-lang#128861 I made debug info correctly represent parameters to inline functions by removing a fake lexical block that had been inserted to suppress LLVM assertions and by deduplicating those parameters. LLVM, however, expects to see a single parameter _with distinct locations_, particularly distinct inlinedAt values on the DILocations. This generally worked because no matter how deep the chain of inlines it takes two different call sites in the original function to result in the same function being present multiple times, and a function call requires a non-zero number of characters, but macros threw a wrench in that in rust-lang#131944. At the time I thought the issue there was limited to proc-macros, where an arbitrary amount of code can be generated at a single point in the source text. In rust-lang#132613 I added discriminators to DILocations that would otherwise be the same to repair rust-lang#131944[^1]. This works, but LLVM's capacity for discriminators is not infinite (LLVM actually only allocates 12 bits for this internally). At the time I thought it would be very rare for anyone to hit the limit, but rust-lang#132900 proved me wrong. In the relatively-minimized test case it also became clear to me that the issue affects regular macros too, because the call to the inlined function will (without collapse_debuginfo on the macro) be attributed to the (repeated, if the macro is used more than once) textual callsite in the macro definition. This PR fixes the panic by dropping debug info when we exceed LLVM's maximum discriminator value. There's also a preceding commit for a related but distinct issue: macros that use collapse_debuginfo should in fact have their inlinedAts collapsed to the macro callsite and thus not need discriminators at all (and not panic/warn accordingly when the discriminator limit is exhausted). Fixesrust-lang#132900 r? `@jieyouxu` [^1]: Editor's note: `fix` is a magic keyword in PR description that apparently will close the linked issue (it's closed already in this case, but still).

apiraino · 2024-11-21T15:31:35Z

Beta backport declined as per compiler team on Zulip.

@rustbot label -beta-nominated

rustbot assigned chenyukang Nov 4, 2024

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 Nov 4, 2024

This comment has been minimized.
Sign in to view

This comment was marked as resolved.
Sign in to view

This comment has been minimized.
Sign in to view

bors 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 Nov 5, 2024

This comment has been minimized.
Sign in to view

jieyouxu reviewed Nov 7, 2024
View reviewed changes

Comment threadtests/codegen/debuginfo-proc-macro/auxiliary/macro_def.rs

This comment has been minimized.
Sign in to view

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 9, 2024

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2024

khuey force-pushed the master branch from d8c74b5 to 1dc1061Compare November 9, 2024 16:12

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 Nov 9, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 9, 2024

bors merged commit b026d85 into rust-lang:masterNov 9, 2024

rustbot added this to the 1.84.0 milestone Nov 9, 2024

Millione mentioned this pull request Nov 11, 2024
ICE: Failed to encode discriminator in DILocation#132900
Closed

saethlin mentioned this pull request Nov 17, 2024
LLVM debuginfo assertion "overlapping or duplicate fragments" #129206
Closed

khuey mentioned this pull request Nov 19, 2024
Drop debug info instead of panicking if we exceed LLVM's capability to represent it #133194
Merged

rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 21, 2024

DaniPopes mentioned this pull request Jan 10, 2025
Broken LLVM module: inlinable function call in a function with debug info must have a !dbg location #135332
Closed

Uh oh!

Uh oh!

Conversation

khuey commented Nov 4, 2024• edited by workingjubilee Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 4, 2024

Uh oh!

This comment has been minimized.

workingjubilee commented Nov 5, 2024

Uh oh!

bors commented Nov 5, 2024

Uh oh!

bors commented Nov 5, 2024

Uh oh!

jieyouxu commented Nov 5, 2024

Uh oh!

bors commented Nov 5, 2024

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

bors commented Nov 5, 2024

Uh oh!

jieyouxu commented Nov 5, 2024

Uh oh!

workingjubilee commented Nov 7, 2024

Uh oh!

bors commented Nov 7, 2024

Uh oh!

This comment has been minimized.

bors commented Nov 7, 2024

Uh oh!

Uh oh!

jieyouxu commented Nov 7, 2024

Uh oh!

bors commented Nov 7, 2024

Uh oh!

This comment has been minimized.

bors commented Nov 7, 2024

Uh oh!

cuviper commented Nov 9, 2024

Uh oh!

workingjubilee commented Nov 9, 2024

Uh oh!

bors commented Nov 9, 2024

Uh oh!

jieyouxu commented Nov 9, 2024

Uh oh!

jieyouxu commented Nov 9, 2024

Uh oh!

bors commented Nov 9, 2024

Uh oh!

jieyouxu commented Nov 9, 2024

Uh oh!

bors commented Nov 9, 2024

Uh oh!

bors commented Nov 9, 2024

Uh oh!

bors commented Nov 9, 2024

Uh oh!

rust-timer commented Nov 9, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

apiraino commented Nov 14, 2024

Uh oh!

apiraino commented Nov 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

khuey commented Nov 4, 2024•
edited by workingjubilee
Loading