When LLVM's location discriminator value limit is exceeded, emit locations with dummy spans instead of dropping them entirely by khuey · Pull Request #135643 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation24 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

khuey

@rustbot 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

@rustbot

jieyouxu

Member

@jieyouxu jieyouxu left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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

@jieyouxu

@jieyouxu jieyouxu added A-debuginfo

Area: Debugging information in compiled programs (DWARF, PDB, etc.)

beta-nominated

Nominated for backporting to the compiler in the beta channel.

stable-nominated

Nominated for backporting to the compiler in the stable channel.

and removed 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

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

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

@jieyouxu

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

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:

  1. The cherry-picked regression test commit mentioned above
  2. (Current PR commit 1) The revert commit
  3. (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

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, two minor nits on the test, otherwise LGTM

@khuey

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@khuey

…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

r=me after PR CI is green

@bors delegate+ rollup=never

@bors

📌 Commit 45ef927 has been approved by jieyouxu

It is now in the queue for this repository.

@bors 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

@jieyouxu

@bors p=5 (rollup scheduling)

@bors

@bors

@rust-timer

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

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

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 24, 2025

@bors

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 25, 2025

@bors

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 26, 2025

@bors

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 27, 2025

@bors

[stable] Prepare Rust 1.84.1 point release

cc @rust-lang/release r? ghost

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Feb 2, 2025

@he32

Pkgsrc changes:

Upstream changes:

Version 1.84.1 (2025-01-30)

Version 1.84.0 (2025-01-09)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts

Cargo

Rustdoc

Compatibility Notes

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

Feb 4, 2025

@tmeijn

This MR contains the following updates:

Package Update Change
rust patch 1.84.0 -> 1.84.1

MR created with the help of el-capitano/tools/renovate-bot.

Proposed changes to behavior should be submitted there as MRs.

Release Notes

rust-lang/rust (rust)

v1.84.1

Compare Source

==========================

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

This MR has been generated by Renovate Bot.

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request

Mar 11, 2025

@bmwiedemann

Labels

A-debuginfo

Area: Debugging information in compiled programs (DWARF, PDB, etc.)

A-run-make

Area: port run-make Makefiles to rmake.rs

beta-accepted

Accepted for backporting to the compiler in the beta channel.

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

stable-accepted

Accepted for backporting to the compiler in the stable channel.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.