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.

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

[Debuginfo] Force enum DISCR_* to static const u64 to allow for inspection via LLDB #133990

Merged
merged 2 commits into from
Jan 5, 2025

Conversation

Walnut356
Copy link
Contributor

see here for more info.

This change mainly helps *-msvc debugged with LLDB. Currently, LLDB cannot inspect static struct fields, so the intended visualization for enums is only borderline functional, and niche enums with ranges of discriminant cannot be determined at all .

LLDB can inspect static const values (though for whatever reason, non-enum/non-u64 consts don't work).

This change adds the LLVMRustDIBuilderCreateQualifiedType to the rust FFI layer to wrap the discr type with a const modifier, as well as forcing all generated integer enum DISCR_* values to be u64's. Those values will only ever be used by debugger visualizers anyway, so it shouldn't be a huge deal, but I left a fixme comment for it just in case.. The tag also still properly reflects the discriminant type, so no information is lost.

@rustbot
Copy link
Collaborator

rustbot commented Dec 7, 2024

r? @cjgillot

rustbot has assigned @cjgillot.
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 Dec 7, 2024
@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

workingjubilee commented Dec 7, 2024

Please git-clang-format this.

@rustbot author

@Zalathar
Copy link
Contributor

Zalathar commented Dec 7, 2024

To format the C++ code automatically, you should be able to do:

./x test tidy --extra-checks=cpp:fmt --bless

@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 Dec 7, 2024
@workingjubilee
Copy link
Member

@Walnut356 Just to check, there's no debuginfo api in the LLVM C API, right?

Comment on lines +539 to +576
// FIXME: Currently we force all DISCR_* values to be u64's as LLDB seems to have
// problems inspecting other value types. Since DISCR_* is typically only going to be
// directly inspected via the debugger visualizer - which compares it to the `tag` value
// (whose type is not modified at all) it shouldn't cause any real problems.
Copy link
Member

Choose a reason for hiding this comment

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

can we file a bug for this against lldb? I think they don't have a separate repo from llvm, so it would just be against https://github.com/llvm/llvm-project

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've opened an issue for it llvm/llvm-project#119101

@Zalathar
Copy link
Contributor

Zalathar commented Dec 7, 2024

@Walnut356 Just to check, there's no debuginfo api in the LLVM C API, right?

At a glance it looks like there is debuginfo stuff in the LLVM C API, but we currently don't use any of it.

@rust-log-analyzer

This comment has been minimized.

@Walnut356
Copy link
Contributor Author

@Walnut356 Just to check, there's no debuginfo api in the LLVM C API, right?

If i'm understanding your question correctly, everything prefixed by LLVMDI is purely debug info generation. There's a handful of things it's used for currently, but we're definitely not taking full advantage of it.

@workingjubilee
Copy link
Member

@jieyouxu
Copy link
Member

jieyouxu commented Dec 7, 2024

Also, is there a way to add a debuginfo test for this, to catch changes in the output?

@workingjubilee
Copy link
Member

workingjubilee commented Dec 7, 2024

Well, since we currently use the C++ API for debuginfo it seems, you don't have to use the C API version of this function. I can't imagine it's trivial to make that interoperate with our current C++-using handling of debuginfo.

It would be better to do that port as its own PR, so I created an issue for it.

@Walnut356
Copy link
Contributor Author

Also, is there a way to add a debuginfo test for this, to catch changes in the output?

At the moment, I don't think so. Debuginfo tests in CI currently only run on macos, whose output shouldn't change at all after these changes. There's also the problem that debug info is generated slightly differently for DAWRF vs PDB (afaik mostly due to the requirements of the formats themselves). As such, the tests would need to be written in a platoform-agnostic way, or they would need to have separate tests per platform. I haven't looked too much into how the test runner functions so i'm not sure how possible that is, but it fixing up the test suite is a priority for those of us working on debug info

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Dec 24, 2024
@Walnut356
Copy link
Contributor Author

@rustbot review

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 24, 2024
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 24, 2024
@cjgillot cjgillot assigned workingjubilee and unassigned cjgillot Dec 29, 2024
@workingjubilee
Copy link
Member

@Walnut356 Hello! Squashy squashy?

…s `SBTypeStaticField::GetConstantValue()`
@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 4, 2025

📌 Commit a1191e3 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 4, 2025
@bors
Copy link
Contributor

bors commented Jan 4, 2025

⌛ Testing commit a1191e3 with merge 3dc3c52...

@bors
Copy link
Contributor

bors commented Jan 5, 2025

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 3dc3c52 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 5, 2025
@bors bors merged commit 3dc3c52 into rust-lang:master Jan 5, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 5, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3dc3c52): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -7.3%, secondary -0.4%)

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.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
-7.3% [-7.3%, -7.3%] 1
Improvements ✅
(secondary)
-1.8% [-1.8%, -1.8%] 2
All ❌✅ (primary) -7.3% [-7.3%, -7.3%] 1

Cycles

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

Binary size

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

Bootstrap: 763.656s -> 763.423s (-0.03%)
Artifact size: 325.61 MiB -> 325.64 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

9 participants