Fix DWARF generation for enums#54004
Merged
bors merged 8 commits intorust-lang:masterfrom Oct 31, 2018
Merged
Conversation
Contributor
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
eddyb
reviewed
Sep 6, 2018
eddyb
reviewed
Sep 6, 2018
eddyb
reviewed
Sep 6, 2018
Member
There was a problem hiding this comment.
You can get the size and alignment from niche.value directly.
eddyb
reviewed
Sep 6, 2018
Member
There was a problem hiding this comment.
Is there no method on Integer to do this conversion?
Contributor
Author
There was a problem hiding this comment.
Not directly (that I could find) but I can use Integer::fit_unsigned.
Member
There was a problem hiding this comment.
Oh, I think you should match on Primitive, convert floats to I32 / I64, and pointers to cx.data_layout().ptr_sized_integer(). That way you don't need to turn an integer back into itself by looking at its size.
Member
|
This definitely wants a test or two. EDIT: though I won’t block this PR if they aren’t easy to write. |
Member
Contributor
Contributor
Author
|
I can write more tests but note that there are existing debuginfo tests that already cover the ordinary cases. The new test is testing the case that could not be made to work previously. |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Still investigating that failure. I'd tried things out with llvm 6 but not 5. I somewhat recall having to make a change here for 6; I will try all 3 ways again but if something has to regress, I think we should let it be 5. |
Contributor
Author
|
I built rust against LLVM 5, 6, and rust-llvm; and then I tested each one against gdb 7.11, 8.1, and 8.2. This found a crash in 8.2, filed: https://sourceware.org/bugzilla/show_bug.cgi?id=23626. I'll fix this before 8.2.1. I couldn't reproduce the bot's failure; but the bot is using 7.11, and my feeling is that a minor output regression there is nothing to worry about. So, if it persists and there are no objections, I'll just bump the minimum gdb version for the test. |
Contributor
Author
I rebased & tried again and now it failed. |
Contributor
Author
At some point I thought I needed to give the variants names, but reverting this and re-testing didn't show any problems. |
tromey
force-pushed
the
enum-debuginfo
branch
from
8f6ce23 to
1bf986e
Compare
tromey
added a commit
to tromey/gdb
that referenced
this pull request
Sep 11, 2018
While testing my Rust compiler patch to fix the DWARF representation of Rust enums (rust-lang/rust#54004), I found a gdb crash coming from one of the Rust test cases. The bug here is that the new variant support in gdb does not handle the case where there are no variants in the enum. This patch fixes the problem in a straightforward way. Note that the new tests are somewhat lax because I did not want to try to fully fix this corner case for older compilers. If you think that's unacceptable, let meknow. Tested on x86-64 Fedora 28 using several versions of the Rust compiler. I intend to push this to the 8.2 branch as well. gdb/ChangeLog 2018-09-11 Tom Tromey <tom@tromey.com> PR rust/23626: * rust-lang.c (rust_enum_variant): Now static. (rust_empty_enum_p): New function. (rust_print_enum, rust_evaluate_subexp, rust_print_struct_def): Handle empty enum. gdb/testsuite/ChangeLog 2018-09-11 Tom Tromey <tom@tromey.com> PR rust/23626: * gdb.rust/simple.rs (EmptyEnum): New type. (main): Use it. * gdb.rust/simple.exp (test_one_slice): Add empty enum test.
Contributor
Author
|
@eddyb can you re-review? I don't think I have sufficient permissions to mark it. Thanks. |
Member
|
@tromey Is the compiler-builtins submodule change intentional? |
eddyb
reviewed
Sep 13, 2018
Member
There was a problem hiding this comment.
Hmm, I think if you do this then we're "insured" from weird 128-bit bugs:
let niche = (i as u128)
.wrapping_sub(*niche_variants.start() as u128)
.wrapping_add(niche_start);
assert_eq!(niche as u64 as u128, niche);
Some(niche as u64)Note that other than the assert, you also had an overflow bug: if niche_variants.start() is larger than i, subtracting u64s and casting the "negative" result to u128 puts you close to 1 << 64 (because it zero-extends), instead of being a "negative" 128-bit integer.
eddyb
reviewed
Sep 13, 2018
Member
There was a problem hiding this comment.
Hmm, do all variants use these size/alignment values? Because each variant has its own "sub-layout" that "fits" inside the enum's layout, and it might make more sense to use those individual ones. Also, instead of UNKNOWN_LINE_NUMBER, you can get each variant's declaration span.
Contributor
Author
There was a problem hiding this comment.
This part is just a bit of DWARF formalism. This is emitting a DW_TAG_variant_part, which according to DWARF is the part of an enclosing structure that can vary. For Rust, the entire thing can vary, so we emit a structure wrapping a variant part, which wraps the variants. Since there can only be one variant part, it makes sense for it to fill the enclosing structure. And, the location of this thing is unimportant as it is basically entirely artificial -- a synthetic construct only necessary because this is how DWARF mandates it.
Contributor
Author
Nope. |
Merged
tromey
force-pushed
the
enum-debuginfo
branch
from
1bf986e to
8d0df1c
Compare
wallento
pushed a commit
to wallento/binutils-gdb
that referenced
this pull request
Sep 13, 2018
While testing my Rust compiler patch to fix the DWARF representation of Rust enums (rust-lang/rust#54004), I found a gdb crash coming from one of the Rust test cases. The bug here is that the new variant support in gdb does not handle the case where there are no variants in the enum. This patch fixes the problem in a straightforward way. Note that the new tests are somewhat lax because I did not want to try to fully fix this corner case for older compilers. If you think that's unacceptable, let meknow. Tested on x86-64 Fedora 28 using several versions of the Rust compiler. I intend to push this to the 8.2 branch as well. gdb/ChangeLog 2018-09-13 Tom Tromey <tom@tromey.com> PR rust/23626: * rust-lang.c (rust_enum_variant): Now static. (rust_empty_enum_p): New function. (rust_print_enum, rust_evaluate_subexp, rust_print_struct_def): Handle empty enum. gdb/testsuite/ChangeLog 2018-09-13 Tom Tromey <tom@tromey.com> PR rust/23626: * gdb.rust/simple.rs (EmptyEnum): New type. (main): Use it. * gdb.rust/simple.exp (test_one_slice): Add empty enum test.
kraj
pushed a commit
to kraj/binutils-gdb
that referenced
this pull request
Sep 13, 2018
While testing my Rust compiler patch to fix the DWARF representation of Rust enums (rust-lang/rust#54004), I found a gdb crash coming from one of the Rust test cases. The bug here is that the new variant support in gdb does not handle the case where there are no variants in the enum. This patch fixes the problem in a straightforward way. Note that the new tests are somewhat lax because I did not want to try to fully fix this corner case for older compilers. If you think that's unacceptable, let meknow. Tested on x86-64 Fedora 28 using several versions of the Rust compiler. I intend to push this to the 8.2 branch as well. 2018-09-13 Tom Tromey <tom@tromey.com> PR rust/23626: * rust-lang.c (rust_enum_variant): Now static. (rust_empty_enum_p): New function. (rust_print_enum, rust_evaluate_subexp, rust_print_struct_def): Handle empty enum. gdb/testsuite/ChangeLog 2018-09-13 Tom Tromey <tom@tromey.com> PR rust/23626: * gdb.rust/simple.rs (EmptyEnum): New type. (main): Use it. * gdb.rust/simple.exp (test_one_slice): Add empty enum test.
kennytm
added
the
S-waiting-on-review
Member
|
@tromey should this be approved? I think you're approved to merge this as well? |
Contributor
Author
|
Yes, it should be approved. I'll try. @bors r+ |
Collaborator
|
📌 Commit 09009d386de739bccc2763e37feb9d5426cf162d has been approved by |
Collaborator
|
⌛ Testing commit 09009d386de739bccc2763e37feb9d5426cf162d with merge fc5cf77a07f3bc8db8db5912b39ee3b27380a4bf... |
Collaborator
|
💔 Test failed - status-travis |
Contributor
|
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 |
Contributor
Author
|
Some difference between the apple lldb and the (non-rust-enabled) linux lldb I suppose. Will do another macOS build next week. |
Contributor
Author
|
Forgot to make those tests rust-lldb-only. |
The DWARF generated for Rust enums was always somewhat unusual. Rather than using DWARF constructs directly, it would emit magic field names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since PR rust-lang#45225, though, even this has not worked -- the ad hoc scheme was not updated to handle the wider variety of niche-filling layout optimizations now available. This patch changes the generated DWARF to use the standard tags meant for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part. The patch to implement this went in to LLVM 7. In order to work with older versions of LLVM, and because LLVM doesn't do anything here for PDB, the existing code is kept as a fallback mode. Support for this DWARF is in the Rust lldb and in gdb 8.2. Closes rust-lang#32920 Closes rust-lang#32924 Closes rust-lang#52762 Closes rust-lang#53153
This fixes the issues pointed out in review.
Update the new enum-debug to ensure that field "D" does not have a discrimnant.
Rename the previous enum debug info test, and add more tests to cover c-like enums and tagged (ordinary) enums.
@eddyb pointed out in review that the niche value computation had a possible integer overflow problem, fixed here as he suggested.
Bug rust-lang#52452 notes some debuginfo test regressions when moving to gdb 8.1. This series will also cause versions of gdb before 8.2 to fail when a recent LLVM is used -- DW_TAG_variant_part support was not added until 8.2. This patch updates one of the builders to a later version of Ubuntu, which comes with gdb 8.2. It updates the relevant tests to require both a new-enough LLVM and a new-enough gdb; the subsequent patch arranges to continue testing the fallback mode. The "gdbg" results are removed from these tests because the tests now require a rust-enabled gdb. If you read closely, you'll see that some of the lldb results in this patch still look a bit strange. This will be addressed in a subsequent patch; I believe the fix is to disable the Python pretty-printers when lldb is rust-enabled.
The enum debuginfo patch includes a legacy mode that is used when building against LLVM 5 and LLVM 6. The main enum debuginfo tests have been updated to rely on the new approach and a new-enough gdb. This patch makes a copy of these tests so that the fallback mode will continue to be tested. Note that nil-enum.rs is not copied; it seemed not to provide enough value to bother. A new header directive is added, "ignore-llvm-version". I will send a patch to update the rustc documentation once this lands.
Update src/tools/lldb to pick up a needed bug fix in the DW_TAG_variant_part handling.
Contributor
Author
|
@bors r+ |
Collaborator
|
📌 Commit 98b2688 has been approved by |
Collaborator
Collaborator
|
☀️ Test successful - status-appveyor, status-travis |
Member
|
🎉 🎉 🎉 ! |
This was referenced Nov 5, 2018
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The DWARF generated for Rust enums was always somewhat unusual.
Rather than using DWARF constructs directly, it would emit magic field
names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since
PR #45225, though, even this has not worked -- the ad hoc scheme was
not updated to handle the wider variety of niche-filling layout
optimizations now available.
This patch changes the generated DWARF to use the standard tags meant
for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part.
The patch to implement this went in to LLVM 7. In order to work with
older versions of LLVM, and because LLVM doesn't do anything here for
PDB, the existing code is kept as a fallback mode.
Support for this DWARF is in the Rust lldb and in gdb 8.2.
Closes #32920
Closes #32924
Closes #52762
Closes #53153