Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Do not forget to pass DWARF fragment information to LLVM. #115139

Merged
merged 4 commits into from
Aug 27, 2023
Merged

Do not forget to pass DWARF fragment information to LLVM. #115139

merged 4 commits into from
Aug 27, 2023

Conversation

cjgillot
Copy link
Contributor

Fixes #115113 for the rustc part

@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2023

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@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 Aug 23, 2023
@cuviper
Copy link
Member

cuviper commented Aug 23, 2023

I tried this on cargo release+debuginfo (per #115113 (comment)) and with LLVM assertions enabled, and it trips here:

rustc: /checkout/src/llvm-project/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:323: void llvm::DbgVariable::addMMIEntry(const DbgVariable &): Assertion `(FrameIndexExprs.size() == 1 || llvm::all_of(FrameIndexExprs, [](FrameIndexExpr &FIE) { return FIE.Expr && FIE.Expr->isFragment(); })) && "conflicting locations for variable"' failed.

Actually, that does trigger once before your change too, at the very end on cargo itself, but with your change it seems to trigger on almost every crate in the build, even build scripts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@saethlin saethlin linked an issue Aug 23, 2023 that may be closed by this pull request
Crash (SEGFAULT) when building Firefox for ARMv7 or x86 Android with 1.72 #115142
Closed
nikic
nikic reviewed Aug 23, 2023
View reviewed changes
tests/codegen/sroa-fragment-debuginfo.rs Outdated
// CHECK: %slice.dbg.spill = alloca %Endian,
// CHECK: %s.dbg.spill = alloca { ptr, i64 },
// CHECK: call void @llvm.dbg.declare(metadata ptr %s.dbg.spill, metadata ![[S:.*]], metadata !DIExpression()),
// CHECK: call void @llvm.dbg.declare(metadata ptr %slice.dbg.spill, metadata ![[SLICE:.*]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 0)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be something like 128, 0 rather than 0, 0?

It would probably be clearer to use a non-zero sized type here.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@apiraino apiraino mentioned this pull request Aug 25, 2023
Closed
@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot cjgillot mentioned this pull request Aug 26, 2023
Merged
nikic
nikic reviewed Aug 26, 2023
View reviewed changes
tests/codegen/sroa-fragment-debuginfo.rs
pub fn extra(s: &[u8]) {
// CHECK: void @extra(
// CHECK: %slice.dbg.spill1 = alloca i32,
// CHECK: %slice.dbg.spill = alloca { ptr, i64 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Test should require 64-bit target, otherwise the sizes / offset aren't correct.

@nikic
Copy link
Contributor

nikic commented Aug 27, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 27, 2023

📌 Commit 5529e2f has been approved by nikic

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2023
@bors
Copy link
Contributor

bors commented Aug 27, 2023

⌛ Testing commit 5529e2f with merge f072775...

@bors
Copy link
Contributor

bors commented Aug 27, 2023

☀️ Test successful - checks-actions
Approved by: nikic
Pushing f072775 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 27, 2023
@bors bors merged commit f072775 into rust-lang:master Aug 27, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 27, 2023
@cjgillot cjgillot deleted the llvm-fragment branch August 27, 2023 16:13
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f072775): 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)		 4.4% [4.4%, 4.4%] 1
Improvements ✅
(primary)		 -0.9% [-0.9%, -0.9%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.9% [-0.9%, -0.9%] 1

Cycles

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

Binary size

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.7% [0.5%, 0.8%] 2
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.7% [0.5%, 0.8%] 2

Bootstrap: 630.812s -> 631.43s (0.10%)
Artifact size: 315.96 MiB -> 316.11 MiB (0.04%)

@cuviper
Copy link
Member

cuviper commented Aug 27, 2023

For #115113 in 1.73, we should either backport this or disable SROA again.

@rustbot label +beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 27, 2023
@apiraino
Copy link
Contributor

apiraino commented Aug 31, 2023 •
edited
Loading

I feel this patch can also be evaluated for a stable backport (if there will be plans for a 1.72.1 release).

@rustbot label stable-nominated

@rustbot rustbot added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Aug 31, 2023
@apiraino
Copy link
Contributor

apiraino commented Sep 1, 2023

Beta backport approved as per compiler team on Zulip.

Stable backport declined (#115140 already disabled the MIR opt that caused fragment debuginfo to be generated from rustc, comment on Zulip).

@rustbot label +beta-accepted -stable-nominated

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed stable-nominated Nominated for backporting to the compiler in the stable channel. labels Sep 1, 2023
@cuviper cuviper mentioned this pull request Sep 1, 2023
Merged
@cuviper cuviper modified the milestones: 1.74.0, 1.73.0 Sep 1, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 1, 2023
@bors
Auto merge of rust-lang#115442 - cuviper:beta-next, r=cuviper
9f37cd4 
[beta] backports

- Contents of reachable statics is reachable rust-lang#115114
- Revert "Suggest using `Arc` on `!Send`/`!Sync` types" rust-lang#115311
- Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
- Do not forget to pass DWARF fragment information to LLVM. rust-lang#115139
- rustdoc: use unicode-aware checks for redundant explicit link fastpath rust-lang#115070

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2023
@bors
Auto merge of rust-lang#115252 - cjgillot:mir-composite, r=davidtwco
a5b2ac6 
Represent MIR composite debuginfo as projections instead of aggregates

Composite debuginfo for MIR is currently represented as
```
debug name => Type { projection1 => place1, projection2 => place2 };
```
ie. a single `VarDebugInfo` object with that name, and its value a `VarDebugInfoContents::Composite`.

This PR proposes to reverse the representation to be
```
debug name.projection1 => place1;
debug name.projection2 => place2;
```
ie. multiple `VarDebugInfo` objects with each their projection.

This simplifies the handling of composite debuginfo by the compiler by avoiding weird nesting.

Based on rust-lang#115139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

Assignees

@compiler-errors compiler-errors

Labels
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
9 participants
@cjgillot @rustbot @cuviper @rust-log-analyzer @nikic @bors @rust-timer @apiraino @compiler-errors