rustc_codegen_llvm: support 128-bit discriminants in debuginfo. #59520
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@JohnTitor We should try the alternative, but there's no guarantee it will work (I have no idea what LLVM requires internally, so it might or it might not). |
|
I think 128-bit values will still get truncated going into LLVM -- see #59509 (comment). |
|
Huh, I thought everything went through a LLVM value created with I'm readding the assert as a drive-by fix in #59519, since I had to touch that code anyway. We should keep this PR open to document "the proper fix", or maybe even merge it now, with the assertion moved to the point where we need to pass an |
|
@cuviper Hang on, this PR only touches values passed through We should add codegen tests, and even debuginfo (cc @nagisa @michaelwoerister). |
|
Ah, yeah, maybe they're disconnected. |
|
@cuviper Yupp, see #59509 (comment) @JohnTitor Can you run your local rust build with |
|
I think we should merge this with a codegen test and debuginfo one, but debuggers might not be able to use the information, so I'm not sure. Anyway, if you have a |
|
Okay, I want to add the tests but I don't know proper that. |
|
@JohnTitor That's why I pinged @nagisa and @michaelwoerister above, I'm not sure either. Then |
|
Ah, wait, nowadays we have documents like this: https://rust-lang.github.io/rustc-guide/tests/adding.html |
|
@eddyb Uh, I added temporary tests for the moment. I don't know what good |
|
☔ The latest upstream changes (presumably #59590) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors retry |
bors
added
S-waiting-on-bors
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
bors
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #59897) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I rebased and fixed debuginfo test. |
src/test/debuginfo/repr-u128.rs
Outdated
| // gdb-command: run | ||
|
|
||
| // gdb-command:print vals | ||
| // gdbr-check:$1 = (<error reading variable>, <error reading variable>) |
There was a problem hiding this comment.
We no longer catch Options after rebasing, is this okay? @eddyb
There was a problem hiding this comment.
That doesn't make sense, what broke this?
There was a problem hiding this comment.
There was a problem hiding this comment.
Is #59897 (comment) related?
There was a problem hiding this comment.
No, that's just affecting miri, and also you're not using generators at all here.
There was a problem hiding this comment.
If this is regressing something, it's likely because something changed. The only thing that really looks like it's changing is that we're always telling LLVM that there's a 128-bit discriminant. My guess is that LLVM and/or GDB isn't really prepared for that and there's some pessimization that happens somewhere. This can probably be fixed by always telling LLVM about a 64-bit discriminant when it fits and 128-bit otherwise.
Or it could be something completely different. I don't really know why I'm cc'd here and I don't really know much about this test, this PR, or debugging and descriminants being passed.
There was a problem hiding this comment.
This test worked before a recent rebase, according to @JohnTitor - see ea7af71 for the post-rebase diff.
However, I don't know how when or how we ever run debuginfo tests anymore, so I can't be sure of anything.
EDIT: I was wondering whether we changed anything about our LLVM recently, or anything like that. I guess I could try bisect this PR, except for the fact that debuginfo tests have been broken for me for a long while now.
There was a problem hiding this comment.
Hmm, it seems that we need more investigation.
@eddyb How we try to bisect this PR?
There was a problem hiding this comment.
I'm not sure, can you squash all the changes into one commit?
That would make it easy, since you can just git cherry-pick the-hash-of-that-commit on top of any older master and build that (or even use git bisect but I never remember how to).
That's assuming this test can even be tested, that is.
|
@eddyb Hmm, I'm not sure but we can catch |
|
ping @eddyb, what do you think? Should we do |
|
😪 I'm awake I'm awake |
|
|
||
| // compile-flags: -g -C no-prepopulate-passes | ||
|
|
||
| // === LLDB TESTS ================================================================================== |
There was a problem hiding this comment.
@eddyb Sorry for being late. I fixed test.
There was a problem hiding this comment.
I'm not seeing Some or None in the tested output.
There was a problem hiding this comment.
Hmm, we cannot catch these now. This is a part of repr-u128.out.
print vals
((core::option::Option<repr_u128::Foo>, core::option::Option<repr_u128::Foo>)) $0 = (Option<repr_u128::Foo> { }, Option<repr_u128::Foo> { })
quit
None
There was a problem hiding this comment.
I really don't understand what's happening :/
|
r? @nagisa |
|
Could you please squash your commits? |
|
Right, I do not think we can do this quite yet – if LLVM is built with debug assertions enabled, it aborts with following assert when building the repr-u128 test: with following backtrace: This would be one possible reason why the output from debuggers ends up showing no useful data, for example. |
|
@nagisa Okay, I squashed commits. |
|
@JohnTitor will you be investigating the assert in LLVM? It will likely be the case that the DWARF debug info generator needs to be improved to support outputting 128-bit discriminants which may or may-not need non-trivial changes upstream. If not, then perhaps we should update the relevant issues and close the PR? |
|
You would fix it here, |
|
It seems we should investigate more so it's reasonable to close this PR. |
CC: #59509
If we should fix like this issue comment, I'll do.
r? @eddyb