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

cg_llvm: Use constants for DWARF opcodes, instead of FFI calls #135115

Merged
merged 3 commits into from
Jan 6, 2025
Merged

cg_llvm: Use constants for DWARF opcodes, instead of FFI calls #135115

merged 3 commits into from
Jan 6, 2025
+64 −48

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Jan 5, 2025 •
edited
Loading

Split off from #134009 to incorporate feedback from #134009 (comment).

Most of the constant values now come from gimli, which is already a compiler dependency.

I noticed that DW_OP_LLVM_fragment is an LLVM detail that is not defined by DWARF and could hypothetically change, so I added a static assertion on the C++ side to detect that if it ever happens.

r? workingjubilee

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
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

@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 5, 2025
@workingjubilee workingjubilee self-assigned this Jan 5, 2025
@Zalathar
Copy link
Contributor Author

Zalathar commented Jan 5, 2025

For historical reference, the removal of fake DW_TAG_* values:

Introduction of DW_OP_LLVM_fragment:

Zalathar
Zalathar commented Jan 5, 2025
View reviewed changes
compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs
@@ -87,8 +75,6 @@ type SmallVec<T> = smallvec::SmallVec<[T; 16]>;
mod enums;
mod type_map;

pub(crate) use type_map::TypeMap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this to where the other imports are, so that it doesn't trick rust-analyzer into adding new imports in the wrong place.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Zalathar
Copy link
Contributor Author

Zalathar commented Jan 5, 2025

I'll rebase this and also incorporate the new constant added by #133990.

@Zalathar

This comment was marked as resolved.

Sign in to view
@rustbot rustbot 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 5, 2025
@Zalathar
Use gimli to get the values of DWARF constants needed by codegen
e267106 
The `gimli` crate is already a dependency of `thorin-dwp`, which is already a
dependency of `rustc_codegen_ssa`.
@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@Zalathar
Copy link
Contributor Author

Zalathar commented Jan 5, 2025

I switched over to using gimli's constants, though we still redeclare them to give them more convenient types (diff).

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 5, 2025
@Noratrieb Noratrieb removed their assignment Jan 5, 2025
workingjubilee
workingjubilee reviewed Jan 5, 2025
View reviewed changes
compiler/rustc_codegen_llvm/Cargo.toml
@@ -9,6 +9,7 @@ test = false
[dependencies]
# tidy-alphabetical-start
bitflags = "2.4.1"
gimli = "0.30"
Copy link
Member

Choose a reason for hiding this comment

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

mmm, three versions of gimli in the repo... should fix that...

Copy link
Contributor Author

@Zalathar Zalathar Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it looks like we have:

  • 0.31 (latest) used by run-make-support, which actually needs it
  • 0.30 used by most stuff, which can probably be bumped to 0.31
  • 0.28 used by an old addr2line, which is pulled in by anyhow's backtrace feature, which is perhaps not even needed nowadays?
    • also pulled in by color-eyre

@workingjubilee
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 5, 2025

📌 Commit f50721e has been approved by workingjubilee

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 Jan 5, 2025
@jhpratt jhpratt mentioned this pull request Jan 5, 2025
Merged
@Zalathar Zalathar mentioned this pull request Jan 6, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2025
@bors
Auto merge of rust-lang#135140 - jhpratt:rollup-pn2gi84, r=jhpratt
56f9e6f 
Rollup of 3 pull requests

Successful merges:

 - rust-lang#135115 (cg_llvm: Use constants for DWARF opcodes, instead of FFI calls)
 - rust-lang#135118 (Clarified the documentation on `core::iter::from_fn` and `core::iter::successors`)
 - rust-lang#135121 (Mark `slice::reverse` unstably const)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b9f971a into rust-lang:master Jan 6, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 6, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2025
@rust-timer
Unrolled build for rust-lang#135115
92ac019 
Rollup merge of rust-lang#135115 - Zalathar:dwarf-const, r=workingjubilee

cg_llvm: Use constants for DWARF opcodes, instead of FFI calls

Split off from rust-lang#134009 to incorporate feedback from rust-lang#134009 (comment).

Most of the constant values now come from gimli, which is already a compiler dependency.

I noticed that `DW_OP_LLVM_fragment` is an LLVM detail that is not defined by DWARF and could hypothetically change, so I added a static assertion on the C++ side to detect that if it ever happens.

r? workingjubilee
@Zalathar Zalathar deleted the dwarf-const branch January 6, 2025 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@workingjubilee workingjubilee

Labels
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.86.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Zalathar @rustbot @rust-log-analyzer @workingjubilee @bors @bjorn3 @Noratrieb