Fix DWARF generation for enums #54004

tromey · 2018-09-06T18:50:15Z

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

rust-highfive · 2018-09-06T18:50:29Z

r? @nikomatsakis

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

src/librustc_codegen_llvm/debuginfo/metadata.rs

eddyb · 2018-09-06T19:04:22Z

src/librustc_codegen_llvm/debuginfo/metadata.rs

+
+        &layout::Variants::NicheFilling { ref niche, .. } => {
+            // Find the integer type of the correct size.
+            let discr_type = niche.value.to_ty(cx.tcx);


You can get the size and alignment from niche.value directly.

eddyb · 2018-09-06T19:04:37Z

src/librustc_codegen_llvm/debuginfo/metadata.rs

+                16 => Integer::I16,
+                32 => Integer::I32,
+                64 => Integer::I64,
+                bits => bug!("prepare_enum_metadata: unknown niche bit size {}", bits),


Is there no method on Integer to do this conversion?

Not directly (that I could find) but I can use Integer::fit_unsigned.

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.

nagisa · 2018-09-06T19:06:28Z

This definitely wants a test or two.

EDIT: though I won’t block this PR if they aren’t easy to write.

eddyb

r=me with nits fixed

eddyb · 2018-09-06T19:07:20Z

cc @michaelwoerister @alexcrichton

nikomatsakis · 2018-09-06T19:32:21Z

r? @eddyb

(I pinged him on Discord as I'm not really that familiar with this code. But I see he already left a review before I came back to re-assign. I agree with @nagisa that we could use more tests here :)

tromey · 2018-09-06T19:47:57Z

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.

tromey · 2018-09-07T19:49:03Z

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.

tromey · 2018-09-10T18:46:36Z

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.

tromey · 2018-09-11T13:10:00Z

I couldn't reproduce the bot's failure

I rebased & tried again and now it failed.

tromey · 2018-09-11T20:08:36Z

I rebased & tried again and now it failed.

At some point I thought I needed to give the variants names, but reverting this and re-testing didn't show any problems.

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.

tromey · 2018-09-13T11:59:55Z

@eddyb can you re-review? I don't think I have sufficient permissions to mark it. Thanks.

eddyb · 2018-09-13T12:14:27Z

@tromey Is the compiler-builtins submodule change intentional?

eddyb · 2018-09-13T12:20:48Z

src/librustc_codegen_llvm/debuginfo/metadata.rs

+                            None
+                        } else {
+                            Some((i.wrapping_sub(*niche_variants.start()) as u128)
+                                 .wrapping_add(niche_start) as u64)


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 · 2018-09-13T12:24:33Z

src/librustc_codegen_llvm/debuginfo/metadata.rs

+            file_metadata,
+            UNKNOWN_LINE_NUMBER,
+            enum_type_size.bits(),
+            enum_type_align.abi_bits() as u32,


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.

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.

Ah, so it's not per-variant?

tromey · 2018-09-13T12:45:59Z

@tromey Is the compiler-builtins submodule change intentional?

Nope.

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.

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.

tromey · 2018-10-30T20:48:38Z

@bors r+

bors · 2018-10-30T20:48:39Z

📌 Commit 98b2688 has been approved by tromey

bors · 2018-10-30T23:36:17Z

⌛ Testing commit 98b2688 with merge 0db7abe...

Fix DWARF generation for enums 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

bors · 2018-10-31T02:18:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: tromey
Pushing 0db7abe to master...

michaelwoerister · 2018-10-31T09:15:30Z

🎉 🎉 🎉 !

Bug rust-lang#55606 points out a regression introduced by rust-lang#54004; namely that an assertion can erroneously fire when a niche-filling discriminant value is emitted. This fixes the bug by removing the assertion, and furthermore by arranging for the discriminant value to be masked according to the size of the niche. This makes handling the discriminant a bit simpler for debuggers. The test case is from Jonathan Turner. Closes rust-lang#55606

Fix emission of niche-filling discriminant values Bug #55606 points out a regression introduced by #54004; namely that an assertion can erroneously fire when a niche-filling discriminant value is emitted. This fixes the bug by removing the assertion, and furthermore by arranging for the discriminant value to be masked according to the size of the niche. This makes handling the discriminant a bit simpler for debuggers. The test case is from Jonathan Turner. Closes #55606

A rust-enabled gdb already knows how to display string slices, structs, tuples, and enums (and after rust-lang#54004, the pretty-printers can't handle enums at all). This patch disables these pretty-printers when gdb is rust-enabled. The "gdb-pretty-struct-and-enums-pre-gdb-7-7.rs" test is renamed, because it does not seem to depend on any behavior of that version of gdb, and because gdb 7.7 is 4 years old now.

…, r=nikomatsakis Disable some pretty-printers when gdb is rust-enabled A rust-enabled gdb already knows how to display string slices, structs, tuples, and enums (and after rust-lang#54004, the pretty-printers can't handle enums at all). This patch disables these pretty-printers when gdb is rust-enabled. The "gdb-pretty-struct-and-enums-pre-gdb-7-7.rs" test is renamed, because it does not seem to depend on any behavior of that version of gdb, and because gdb 7.7 is 4 years old now.

A rust-enabled gdb already knows how to display string slices, structs, tuples, and enums (and after rust-lang#54004, the pretty-printers can't handle enums at all). This patch disables these pretty-printers when gdb is rust-enabled. The "gdb-pretty-struct-and-enums-pre-gdb-7-7.rs" test is renamed, because it does not seem to depend on any behavior of that version of gdb, and because gdb 7.7 is 4 years old now.

…, r=alexcrichton Disable some pretty-printers when gdb is rust-enabled A rust-enabled gdb already knows how to display string slices, structs, tuples, and enums (and after rust-lang#54004, the pretty-printers can't handle enums at all). This patch disables these pretty-printers when gdb is rust-enabled. The "gdb-pretty-struct-and-enums-pre-gdb-7-7.rs" test is renamed, because it does not seem to depend on any behavior of that version of gdb, and because gdb 7.7 is 4 years old now.

rust-highfive assigned nikomatsakis Sep 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2018

eddyb reviewed Sep 6, 2018

View reviewed changes

src/librustc_codegen_llvm/debuginfo/metadata.rs Show resolved Hide resolved

eddyb reviewed Sep 6, 2018

View reviewed changes

src/librustc_codegen_llvm/debuginfo/metadata.rs Outdated Show resolved Hide resolved

eddyb reviewed Sep 6, 2018

View reviewed changes

eddyb requested changes Sep 6, 2018

View reviewed changes

rust-highfive assigned eddyb and unassigned nikomatsakis Sep 6, 2018

This comment has been minimized.

Sign in to view

tromey force-pushed the enum-debuginfo branch from 8f6ce23 to 1bf986e Compare September 11, 2018 20:39

eddyb reviewed Sep 13, 2018

View reviewed changes

tromey mentioned this pull request Sep 13, 2018

Implement MaybeUninit #53508

Merged

tromey force-pushed the enum-debuginfo branch from 1bf986e to 8d0df1c Compare September 13, 2018 15:38

tromey force-pushed the enum-debuginfo branch from 09009d3 to 98b2688 Compare October 30, 2018 18:58

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2018

bors merged commit 98b2688 into rust-lang:master Oct 31, 2018

bors mentioned this pull request Oct 31, 2018

Add template parameter debuginfo to generic types #55010

Merged

tromey deleted the enum-debuginfo branch October 31, 2018 13:29

tromey mentioned this pull request Nov 1, 2018

Several bugs related to enum debugging #55586

Closed

nikomatsakis mentioned this pull request Nov 2, 2018

Debuginfo changes causing ICE #55606

Closed

tromey mentioned this pull request Nov 5, 2018

Fix emission of niche-filling discriminant values #55701

Merged

tromey mentioned this pull request Nov 7, 2018

Disable some pretty-printers when gdb is rust-enabled #55767

Merged

tromey mentioned this pull request Nov 29, 2018

Add bundled GDB pretty-printers intellij-rust/intellij-rust#3045

Merged

eddyb mentioned this pull request Mar 28, 2019

Add FromStr impl for NonZero types #58717

Merged

Kobzol mentioned this pull request Jul 20, 2019

DWARF variant metadata for compressed enums with niche is ambiguous #62839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DWARF generation for enums #54004

Fix DWARF generation for enums #54004

tromey commented Sep 6, 2018 •

edited by killercup

Loading

rust-highfive commented Sep 6, 2018

eddyb Sep 6, 2018

eddyb Sep 6, 2018

tromey Sep 7, 2018

eddyb Sep 7, 2018

nagisa commented Sep 6, 2018 •

edited

Loading

eddyb left a comment

eddyb commented Sep 6, 2018

nikomatsakis commented Sep 6, 2018

tromey commented Sep 6, 2018

This comment has been minimized.

tromey commented Sep 7, 2018

tromey commented Sep 10, 2018

tromey commented Sep 11, 2018

tromey commented Sep 11, 2018

tromey commented Sep 13, 2018

eddyb commented Sep 13, 2018

eddyb Sep 13, 2018

eddyb Sep 13, 2018

tromey Sep 13, 2018

eddyb Sep 13, 2018

tromey Sep 13, 2018

tromey commented Sep 13, 2018

tromey commented Oct 30, 2018

bors commented Oct 30, 2018

bors commented Oct 30, 2018

bors commented Oct 31, 2018

michaelwoerister commented Oct 31, 2018

Fix DWARF generation for enums #54004

Fix DWARF generation for enums #54004

Conversation

tromey commented Sep 6, 2018 • edited by killercup Loading

rust-highfive commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Sep 6, 2018 • edited Loading

eddyb left a comment

Choose a reason for hiding this comment

eddyb commented Sep 6, 2018

nikomatsakis commented Sep 6, 2018

tromey commented Sep 6, 2018

This comment has been minimized.

tromey commented Sep 7, 2018

tromey commented Sep 10, 2018

tromey commented Sep 11, 2018

tromey commented Sep 11, 2018

tromey commented Sep 13, 2018

eddyb commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromey commented Sep 13, 2018

tromey commented Oct 30, 2018

bors commented Oct 30, 2018

bors commented Oct 30, 2018

bors commented Oct 31, 2018

michaelwoerister commented Oct 31, 2018

tromey commented Sep 6, 2018 •

edited by killercup

Loading

nagisa commented Sep 6, 2018 •

edited

Loading