Support GDB with native rust support #37410

TimNN · 2016-10-25T21:53:44Z

This PR aims at making the debuginfo tests pass with the newer GDB versions which have native rust support (RGDB)

To that end debuginfo tests now support three GDB prefixes:

gdb applicable to both GDB varieties
gdbg (Generic) only applicable to the old GDB
gdbr (Rust) only applicable to the new RGDB

Whether the GDB has rust support is detected based on it's version: GDB >= 7.11.10 is considered to have rust support.

Test updates

All tests have been updated to the new GDB version. Note that some tests currently require the GDB trunk¹.

How to move forward with this PR:

I propose the following steps for moving forward with this PR:

Validate the general approach of this PR (the gdb-, gdbg- and gdbr- split)
Validate the approach taken for updating the debuginfo tests (I've checked this since there's (almost) no set language c left, which was my main concern)
Determine how to distinguish between the new and old GDB (and implement that)
Add one or more non-gating build bots with the new GDB (blocked on the previous item, can happen after this PR has been merged)
If the new bots pass the tests, gate on them
(Maybe) update the remaining tests to run without set syntax c (in a separate PR)
(Maybe) add tests specifically for the new GDB (in a separate PR / open an issue about this)

I'm not completely sure about the build bot related steps (cc @alexcrichton), the current approach was suggested to prevent any downtime / broken build time between a new GDB gating builder being added and this PR being merged.

Suboptimal RGDB Output

I've found several places where the output of RGDB is not ideal. They are tagged with // FIXME, here is an overview:

Trait references are not syntactically correct: type_names::&Trait2<...> (WontFix: the issue is minor and from @Manishearth below: "to properly address the trait issue we should wait for trait object support")
Univariant enums are printed as <error reading variable> (Fixed in GDB trunk^1⁾
Unions are printed as <error reading variable> (Fixed in GDB trunk¹)
"Nil Enums" (enum Foo {}) are printed as <error reading variable> (WontFix: the are not supposed to exist)
I have found no alternative for sizeof(var) in rust mode, so had to resort to set language c (Fixed in GDB trunk¹)
I have found not way of interpreting a value as a specific enum variant, so had to resort to set language c (Fixed in GDB trunk¹)
Before the initial run command, gdb does not realise it should be in rust mode (thus, if one want's to print statics before the run one has to explicitly set language rust) (maybe this is intended / expected behaviour, if so, someone please tell me ;) ("Expected" / current behaviour of GDB: picks up jemalloc, see Support GDB with native rust support #37410 (comment))

¹: Or rather in @Manishearth's trunk, waiting to be merged upstream.

cc @alexcrichton, @michaelwoerister, #36323

rust-highfive · 2016-10-25T21:53:57Z

r? @nikomatsakis

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

TimNN · 2016-10-26T10:05:30Z

Edit: I moved the content / checklist of this comment to the original post.

michaelwoerister · 2016-10-26T15:02:44Z

Oh nice! This has been near the top of my list of things to do for months. I give you more feedback later today.

michaelwoerister · 2016-10-26T15:47:31Z

Validate the general approach of this PR (the gdb-, gdbg- and gdbr- split)

I think we can check this box. This is basically what we planned to do in the tools team and it allows for nice sharing of commands when writing test cases.

michaelwoerister · 2016-10-26T15:58:53Z

Determine how to distinguish between the new and old GDB (and implement that)

There is some version detection we already do for LLDB via the output of lldb --version. What we have learned from this is:

the output of --version can be rather unreliable, because distros and vendors will modify it as they please,
but that's not much of a problem because there's still a rather limited number of general formats and we can just add logic for those as we come across them.

So I'd say it's definitely possible to go down that road. I'm not sure the feature detection approach is more stable (it depends on how stable the error message is). I'd slightly favor using the same approach we are using for LLDB.

michaelwoerister · 2016-10-26T18:07:53Z

Ideally I'd like both versions of GDB to be tested on all gating build bots. We could let configure define a PRE_RUST_GDB and a RUST_ENABLED_GDB, both of which will be used by compiletest to run debuginfo tests. configure would take explicit arguments for both and fall back to detecting which category gdb on PATH falls into if nothing is specified. That would be backwards compatible.

Alternatively we could do auto-detection only and install a good mix of old and new GDB versions on the build bots.

What do you think, @alexcrichton?

michaelwoerister · 2016-10-26T18:23:16Z

Specifically, in rust mode, for the command print 'basic_types_globals::I', the '...' is interpreted as an unterminated char literal, and I don't know how to fix this.

For me, just specifying I (i.e. the unqualified name of the global) seems to work in the new version.

TimNN · 2016-10-26T18:28:31Z

Specifically, in rust mode, for the command print 'basic_types_globals::I', the '...' is interpreted as an unterminated char literal, and I don't know how to fix this.

For me, just specifying I (i.e. the unqualified name of the global) seems to work in the new version.

Ok, I can try that.

TimNN · 2016-10-26T18:54:58Z

Specifically, in rust mode, for the command print 'basic_types_globals::I', the '...' is interpreted as an unterminated char literal, and I don't know how to fix this.

For me, just specifying I (i.e. the unqualified name of the global) seems to work in the new version.

That seems to have worked, I updated the two tests where I encountered this situation (basic-types-(mut)-globals) to work without set language c.

TimNN · 2016-10-27T19:23:07Z

I've started removing some of the set syntax c lines. While doing so, I have tagged several places as // FIXME where RGDB behaves suboptimal (type_names::&Trait2<...>) or outright bad ($4 = <error reading variable> for univariant enums).

cc @tromey, @Manishearth on the last issue.

TimNN · 2016-10-27T22:11:38Z

I've been removing some more set syntax c lines, and found two more FIXME's, I'm gonna start tracking all these in the top post.

Manishearth · 2016-10-27T22:27:27Z

src/test/debuginfo/borrowed-enum.rs

-// gdb-check:$3 = {{__0 = 4820353753753434}}
+// gdbg-check:$3 = {{__0 = 4820353753753434}}
+// FIXME
+// gdbr-check:$3 = <error reading variable>


This seems fixed. Trying on trunk in a moment, but this works from a gdb I have from august.

(gdb) p univariant_ref $1 = (borrowed_enum::Univariant *) 0x7fff5fbff820 (gdb) ptype univariant_ref type = union borrowed_enum::Univariant { borrowed_enum::TheOnlyCase; } *

Ok, I'm currently working from the GDB that ships with Ubuntu Yakkety Yak (GNU gdb (Ubuntu 7.11.90.20161005-0ubuntu1) 7.11.90.20161005-git). I'll see about updating to a newer version tomorrow.

Try and build trunk, and let me know which bugs persist.

@Manishearth: I tried again with a trunk build (GNU gdb (GDB) 7.12.50.20161027-git) and got four different failures (in pretty tests, I see warning: Unsupported auto-load script at offset 0 in section .debug_gdb_scripts but haven't investigated further yet). Edit: I may have build gdb without python support, trying again now.

Also, I noticed, that in your snippet you did not dereference the univariant_ref, is it possible that the <error reading variable> occurs only when dereferencing?

Alright, I got myself a gdb with python support and the pretty tests pass again, along with all other tests.

That is, gdb-command:print *univariant_ref still prints <error reading variable>.

Manishearth · 2016-10-27T23:53:01Z

Yeah, the univariant thing is a bug. Fixed in https://sourceware.org/ml/gdb-patches/2016-10/msg00778.html. Try applying that patch and let me know how many errors are left, it should fix all the univariant issues. I think to properly address the trait issue we should wait for trait object support (which may take some time), though I could try to make it look prettier for now.

TimNN · 2016-10-28T00:02:19Z

@Manishearth: I'll try the patch ~~tomorrow~~ later today.

The trait issue doesn't seem that important to me - sure, it doesn't look great but it's still easy to understand. So if there's an easy fix / you've got the time, go for it, otherwise I wouldn't bother and wait for "trait object support".

Manishearth · 2016-10-28T00:15:30Z

The fix would be to print it as &Trait instead of struct type_name::&Trait, but that's not a major difference and really it kind of is struct type_name::&Trait (internally). I think type_name::&Trait is a string generated by rustc, too. I'll wait for trait object support, though if you want this fixed earlier, see if you can make rustc generate the right string.

TimNN · 2016-10-28T10:40:33Z

@Manishearth: the patch worked flawlessly, thanks!

There are currently two issues left (although I've not yet gone through all the tests which were "fixed" by simply adding set language c at the top):

Unions are printed as <error reading variable>
Before the initial run command, gdb does not realise it should be in rust mode (thus, if one want's to print statics before the run one has to explicitly set language rust) (maybe this is intended / expected behaviour, if so, someone please tell me ;) (see https://github.com/rust-lang/rust/pull/37410/files#diff-314f334bf26a5c3f111b2d20aba1f3ed)

TimNN · 2016-10-28T13:24:20Z

I went through all the tests to which I previously added set language c and was able to remove all but two instances of set language c (see the FIXME section in the top post).

@Manishearth: at this point I'm most interested in whether it is possible to treat an enum value as being a specific variant in an expressions (see the recursive-struct test).

Manishearth · 2016-10-28T14:31:35Z

You can access enum values already. Pretend the type is the struct (in case of a struct variant) or tuple struct corresponding to the variant and use named or numbered field access. E.g. use some.0 to get the innards of a Some.

That test uses arrow operators btw, those will not work for rust and you have to explicitly deref.

alexcrichton · 2016-10-28T14:33:11Z

Alternatively we could do auto-detection only and install a good mix of old and new GDB versions on the build bots.

My absolutely ideal world would look like:

The configure scripts auto-detects what gdb is being used (somehow)
The configure script also has a flag along the lines of --assert-pre-rust-gdb
The bots pass these flags appropriately on two bots

It may unfortunately take awhile to get the bots online and working here, but I don't think that should block this PR. We should land this ahead of that and then flesh out the bugs once we get a real builder up and running.

TimNN · 2016-10-28T14:47:10Z

@Manishearth: I think I understand what you said however now the following does not seem to make much sense:

(gdb) print stack_unique.next.val
Attempting to access named field val of tuple variant recursive_struct::Opt<Box<recursive_struct::UniqueNode<u16>>>::Val, which has only anonymous fields
(gdb) print stack_unique.next.0
Variant recursive_struct::Opt<Box<recursive_struct::UniqueNode<u16>>>::Val is not a tuple variant
(gdb)

The two error messages seem contradictory to me.

This is from the recursive-struct test (without set language c) but I've extracted the relevant definitions here:

enum Opt<T> {
    Empty,
    Val { val: T }
}

struct UniqueNode<T> {
    next: Opt<Box<UniqueNode<T>>>,
    value: T
}


fn main() {
    let stack_unique: UniqueNode<u16> = UniqueNode {
        next: Val {
            val: box UniqueNode {
                next: Empty,
                value: 1,
            }
        },
        value: 0,
    };
}

Edit: The problem goes away, when adding a third variant to Opt, so I guess the problem is with option like enums (which have non-zero / pointer optimizations).

Manishearth · 2016-10-28T15:25:18Z

Hm, that sounds like a bug. cc @tromey

Manishearth · 2016-10-28T15:45:55Z

"Nil Enums" (enum Foo {}) are printed as <error reading variable> (probably expected? I'm not sure what to make of that test)

These aren't supposed to exist as values. GDB throws an error in this case (not sure why the error text isn't shown, that might be a bug)

Unions are printed as <error reading variable>

There's no support for these yet (they didn't exist when we did the rust support). I'll try to add it later.

Regarding set language rust, I think there was an issue at one point with gdb not autodetecting this but it should work now. If this still happens I think it's a bug. IIRC last time it was a rustc bug (not emitting the right dwarf language tag). It works for me on last week's nightly + gdb master.

Manishearth · 2016-10-28T15:46:54Z

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 9d13353..9569584 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -1736,7 +1736,9 @@ tuple structs, and tuple-like enum variants"));
        variant_type = TYPE_FIELD_TYPE (type, disr.field_no);

        if (variant_type == NULL
-       || rust_tuple_variant_type_p (variant_type))
+           || (disr.is_encoded
+               ? rust_tuple_struct_type_p (variant_type)
+               : rust_tuple_variant_type_p (variant_type)))
          error(_("Attempting to access named field %s of tuple variant %s, \
 which has only anonymous fields"),
            field_name, disr.name);

This should fix the struct variant issue. Not yet probed properly into this, it could be the wrong fix.

TimNN · 2016-10-28T16:51:16Z

This should fix the struct variant issue.

This did indeed fix my struct variant issues.

That test uses arrow operators btw, those will not work for rust and you have to explicitly deref.

(Emphasis mine) Just, FYI, I found that explicitly dereferencing was not necessary, just using the dot operator was sufficient.

TimNN · 2016-10-28T16:56:27Z

"Nil Enums" (enum Foo {}) are printed as (probably expected? I'm not sure what to make of that test)

These aren't supposed to exist as values. GDB throws an error in this case (not sure why the error text isn't shown, that might be a bug)

Yeah, I thought so, which is why I was confused that this test even exists.

Regarding set language rust, I think there was an issue at one point with gdb not autodetecting this but it should work now. If this still happens I think it's a bug. IIRC last time it was a rustc bug (not emitting the right dwarf language tag). It works for me on last week's nightly + gdb master.

set language rust is still required in to places on GDB master. Note that as soon as the gdb run command is executed, gdb correctly detects the language as rust. The problem only occurs when inspecting statics before executing run.

TimNN · 2016-10-28T17:42:36Z

Since the test have all been updated now, I'm gonna come back on the gdb version handling:

auto-detection

No matter what approach we choose it seems as if auto-detecting is desirable / required. Since @michaelwoerister expressed concerns about the stability of the error message (which I verified: gdb 6.6 for example, has a different error message) we should choose a version based approach.

I'm not familiar with the versioning scheme used by gdb (what's the relation between 7.11.1 and 7.11.10?) but based on some guessing I would suggest treating gdb >= 7.11.10 as having rust support.

build system

@alexcrichton and @michaelwoerister have expressed somewhat contrary opinions on this point. Irrespective of which approach we implement, I propose

to have the gdb-kind detection in compiletest (otherwise we'll have to duplicate that the logic between rustbuild & configure)
to add an option to compiletest, rustbuild & configure to specify the gdb executable (defaulting to gdb)

Which leaves us with two other requests (which may both be implemented):

@michaelwoerister expressed interest in having one bot test two gdb versions. To accomplish this I propose adding a gdb-extra option to rustbuild & configure, which defaults to "unset" and, if set, causes compiletest to be invoked twice, once for gdb and once for gdb-extra.
@alexcrichton expressed interest in asserting the kind of gdb. To accomplish this I propose adding a assert-gdb-kind=pre-rust|post-rust option to rustbuild, configure & compiletest, defaulting to "unset". If this option is set, compiletest will validate the assertion. This has the drawback of delaying the assertion until compiletest is run. (An analogous option could be added for gdb-extra.

Manishearth · 2016-10-28T23:39:30Z

I have found not way of interpreting a value as a specific enum variant, so had to resort to set language c

Is this still the case?

@alexcrichton

Support GDB with native rust support This PR aims at making the debuginfo tests pass with the newer GDB versions which have native rust support (RGDB) To that end debuginfo tests now support three GDB prefixes: - `gdb` applicable to both GDB varieties - `gdbg` (**G**eneric) only applicable to the old GDB - `gdbr` (**R**ust) only applicable to the new RGDB Whether the GDB has rust support is detected based on it's version: GDB >= 7.11.10 is considered to have rust support. --- **Test updates** All tests have been updated to the new GDB version. Note that some tests currently require the GDB trunk1. --- **How to move forward with this PR:** I propose the following steps for moving forward with this PR: - [x] Validate the general approach of this PR (the `gdb-`, `gdbg-` and `gdbr-` split) - [x] Validate the approach taken for updating the debuginfo tests (I've checked this since there's (almost) no `set language c` left, which was my main concern) - [x] Determine how to distinguish between the new and old GDB (and implement that) - [ ] Add one or more non-gating build bots with the new GDB (blocked on the previous item, can happen after this PR has been merged) - [ ] If the new bots pass the tests, gate on them - [x] \(Maybe) update the remaining tests to run without `set syntax c` (in a separate PR) - [ ] \(Maybe) add tests specifically for the new GDB (in a separate PR / open an issue about this) I'm not completely sure about the build bot related steps (cc @alexcrichton), the current approach was suggested to prevent any downtime / broken build time between a new GDB gating builder being added and this PR being merged. --- **Suboptimal RGDB Output** I've found several places where the output of RGDB is not ideal. They are tagged with `// FIXME`, here is an overview: - [x] Trait references are not syntactically correct: `type_names::&Trait2<...>` (**WontFix**: the issue is minor and from @Manishearth below: "to properly address the trait issue we should wait for trait object support") - [x] Univariant enums are printed as `<error reading variable>` (**Fixed** in GDB trunk1) - [x] Unions are printed as `<error reading variable>` (**Fixed** in GDB trunk1) - [x] "Nil Enums" (`enum Foo {}`) are printed as `<error reading variable>` (**WontFix**: the are not supposed to exist) - [x] I have found no alternative for `sizeof(var)` in rust mode, so had to resort to `set language c` (**Fixed** in GDB trunk1) - [x] I have found not way of interpreting a value as a specific enum variant, so had to resort to `set language c` (**Fixed** in GDB trunk1) - [x] Before the initial `run` command, gdb does not realise it should be in rust mode (thus, if one want's to print statics before the run one has to explicitly `set language rust`) (maybe this is intended / expected behaviour, if so, someone please tell me ;) (**"Expected"** / current behaviour of GDB: picks up jemalloc, see rust-lang#37410 (comment)) 1: Or rather in @Manishearth's trunk, waiting to be merged upstream. --- cc @alexcrichton, @michaelwoerister, rust-lang#36323

bors · 2016-11-05T00:43:55Z

⌛ Testing commit f7107f3 with merge 9d34708...

bors · 2016-11-05T00:48:06Z

💔 Test failed - auto-win-msvc-64-cargotest

Rollup of 24 pull requests - Successful merges: #37255, #37317, #37408, #37410, #37422, #37427, #37470, #37501, #37537, #37556, #37557, #37564, #37565, #37566, #37569, #37574, #37577, #37579, #37583, #37585, #37586, #37587, #37589, #37596 - Failed merges: #37521, #37547

TimNN · 2016-11-05T09:24:57Z

I fixed the android failure (#37597 (comment)).

alexcrichton · 2016-11-05T17:49:59Z

@TimNN awesome thanks! I'll update the rollup

alexcrichton · 2016-11-05T17:52:03Z

@bors: r+ force clean

bors · 2016-11-05T17:52:04Z

📌 Commit d2cb515 has been approved by alexcrichton

bors · 2016-11-05T21:11:32Z

⌛ Testing commit d2cb515 with merge 366636e...

bors · 2016-11-05T21:15:07Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

TimNN · 2016-11-05T21:25:57Z

I fixed the windows failure (although that seems to have been fixed in the rollup already).

Rollup of 24 pull requests - Successful merges: #37255, #37317, #37408, #37410, #37422, #37427, #37470, #37501, #37537, #37556, #37557, #37564, #37565, #37566, #37569, #37574, #37577, #37579, #37583, #37585, #37586, #37587, #37589, #37596 - Failed merges: #37521, #37547

alexcrichton · 2016-11-06T16:56:25Z

Hm I must have botched the merge but this was landed in #37597, so closing.

michaelwoerister · 2016-11-06T18:41:15Z

🎉

rust-highfive assigned nikomatsakis Oct 25, 2016

TimNN force-pushed the gdb-next-gen branch from f11797d to 9efbef7 Compare October 26, 2016 18:51

Manishearth reviewed Oct 27, 2016

View reviewed changes

TimNN force-pushed the gdb-next-gen branch from b322fb3 to a95ba45 Compare October 28, 2016 13:18

alexcrichton mentioned this pull request Nov 4, 2016

Rollup of 24 pull requests #37597

Merged

ignore gdb check for android

d2cb515

add missing import for windows

95337f7

alexcrichton closed this Nov 6, 2016

TimNN deleted the gdb-next-gen branch November 6, 2016 16:57

Support GDB with native rust support #37410

Support GDB with native rust support #37410

Conversation

TimNN commented Oct 25, 2016 • edited Loading

rust-highfive commented Oct 25, 2016

TimNN commented Oct 26, 2016 • edited Loading

michaelwoerister commented Oct 26, 2016

michaelwoerister commented Oct 26, 2016

michaelwoerister commented Oct 26, 2016

michaelwoerister commented Oct 26, 2016

michaelwoerister commented Oct 26, 2016

TimNN commented Oct 26, 2016 • edited Loading

TimNN commented Oct 26, 2016

TimNN commented Oct 27, 2016

TimNN commented Oct 27, 2016

Manishearth Oct 27, 2016

Choose a reason for hiding this comment

TimNN Oct 27, 2016

Choose a reason for hiding this comment

Manishearth Oct 27, 2016

Choose a reason for hiding this comment

TimNN Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

TimNN Oct 27, 2016

Choose a reason for hiding this comment

Manishearth commented Oct 27, 2016 • edited Loading

TimNN commented Oct 28, 2016

Manishearth commented Oct 28, 2016

TimNN commented Oct 28, 2016

TimNN commented Oct 28, 2016

Manishearth commented Oct 28, 2016

alexcrichton commented Oct 28, 2016

TimNN commented Oct 28, 2016 • edited Loading

Manishearth commented Oct 28, 2016 • edited Loading

Manishearth commented Oct 28, 2016 • edited Loading

Manishearth commented Oct 28, 2016

TimNN commented Oct 28, 2016

TimNN commented Oct 28, 2016 • edited Loading

TimNN commented Oct 28, 2016

Manishearth commented Oct 28, 2016

bors commented Nov 5, 2016

bors commented Nov 5, 2016

TimNN commented Nov 5, 2016

alexcrichton commented Nov 5, 2016

alexcrichton commented Nov 5, 2016

bors commented Nov 5, 2016

bors commented Nov 5, 2016

bors commented Nov 5, 2016

TimNN commented Nov 5, 2016

alexcrichton commented Nov 6, 2016

michaelwoerister commented Nov 6, 2016

TimNN commented Oct 25, 2016 •

edited

Loading

TimNN commented Oct 26, 2016 •

edited

Loading

TimNN commented Oct 26, 2016 •

edited

Loading

TimNN Oct 27, 2016 •

edited

Loading

Manishearth commented Oct 27, 2016 •

edited

Loading

TimNN commented Oct 28, 2016 •

edited

Loading

Manishearth commented Oct 28, 2016 •

edited

Loading

Manishearth commented Oct 28, 2016 •

edited

Loading

TimNN commented Oct 28, 2016 •

edited

Loading