* [PATCH] Print void types correctly in Rust @ 2016-06-27 15:52 Manish Goregaokar 2016-06-27 16:15 ` Tom Tromey 2016-06-27 16:53 ` Manish Goregaokar 0 siblings, 2 replies; 6+ messages in thread From: Manish Goregaokar @ 2016-06-27 15:52 UTC (permalink / raw) To: gdb-patches, Tom Tromey Rust prefers to not specify the return type of a function when it is unit (`()`). The type is also referred to as "void" in debuginfo but not in actual usage, so we should never be printing "void" when the language is Rust. gdb/ChangeLog: * rust-lang.c: Print unit types as "()" * rust-lang.c: Omit return type for functions returning unit gdb/testsuite/ChangeLog: 2016-06-27 Manish Goregaokar <manish@mozilla.com> * gdb.rust/simple.rs: Add test for returning unit in a function * gdb.rust/simple.exp: Add expectation for functions returning unit --- gdb/rust-lang.c | 22 ++++++++++++++++++---- gdb/testsuite/gdb.rust/simple.exp | 1 + gdb/testsuite/gdb.rust/simple.rs | 7 +++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c index 0c56a0f..2b115ee 100644 --- a/gdb/rust-lang.c +++ b/gdb/rust-lang.c @@ -446,7 +446,7 @@ static const struct generic_val_print_decorations rust_decorations = " * I", "true", "false", - "void", + "()", "[", "]" }; @@ -729,13 +729,22 @@ rust_print_type (struct type *type, const char *varstring, if (show <= 0 && TYPE_NAME (type) != NULL) { - fputs_filtered (TYPE_NAME (type), stream); + /* Rust calls the unit type "void" in its debuginfo, + but we don't want to print it as that. */ + if (TYPE_CODE (type) == TYPE_CODE_VOID) + fputs_filtered ("()", stream); + else + fputs_filtered (TYPE_NAME (type), stream); return; } type = check_typedef (type); switch (TYPE_CODE (type)) { + case TYPE_CODE_VOID: + fputs_filtered ("()", stream); + break; + case TYPE_CODE_FUNC: /* Delegate varargs to the C printer. */ if (TYPE_VARARGS (type)) @@ -753,8 +762,13 @@ rust_print_type (struct type *type, const char *varstring, rust_print_type (TYPE_FIELD_TYPE (type, i), "", stream, -1, 0, flags); } - fputs_filtered (") -> ", stream); - rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); + fputs_filtered (")", stream); + /* If it returns unit, we can omit the return type. */ + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID) + { + fputs_filtered (" -> ", stream); + rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); + } break; case TYPE_CODE_ARRAY: diff --git a/gdb/testsuite/gdb.rust/simple.exp b/gdb/testsuite/gdb.rust/simple.exp index 88f1c89..4622f75 100644 --- a/gdb/testsuite/gdb.rust/simple.exp +++ b/gdb/testsuite/gdb.rust/simple.exp @@ -149,6 +149,7 @@ gdb_test "print self::diff2(8, 9)" " = -1" gdb_test "print ::diff2(23, -23)" " = 46" gdb_test "ptype diff2" "fn \\(i32, i32\\) -> i32" +gdb_test "ptype empty" "fn \\(\\)" gdb_test "print (diff2 as fn(i32, i32) -> i32)(19, -2)" " = 21" diff --git a/gdb/testsuite/gdb.rust/simple.rs b/gdb/testsuite/gdb.rust/simple.rs index 32da580..3d28e27 100644 --- a/gdb/testsuite/gdb.rust/simple.rs +++ b/gdb/testsuite/gdb.rust/simple.rs @@ -48,6 +48,12 @@ fn diff2(x: i32, y: i32) -> i32 { x - y } +// Empty function, should not have "void" +// or "()" in its return type +fn empty() { + +} + pub struct Unit; // This triggers the non-zero optimization that yields a different @@ -111,4 +117,5 @@ fn main () { println!("{}, {}", x.0, x.1); // set breakpoint here println!("{}", diff2(92, 45)); + empty(); } -- 2.8.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Print void types correctly in Rust 2016-06-27 15:52 [PATCH] Print void types correctly in Rust Manish Goregaokar @ 2016-06-27 16:15 ` Tom Tromey 2016-06-27 16:25 ` Manish Goregaokar 2016-06-27 16:26 ` Tom Tromey 2016-06-27 16:53 ` Manish Goregaokar 1 sibling, 2 replies; 6+ messages in thread From: Tom Tromey @ 2016-06-27 16:15 UTC (permalink / raw) To: Manish Goregaokar; +Cc: gdb-patches, Tom Tromey >>>>> "Manish" == Manish Goregaokar <manish@mozilla.com> writes: Manish> Rust prefers to not specify the return type of a function when it is unit Manish> (`()`). The type is also referred to as "void" in debuginfo but not in actual Manish> usage, so we should never be printing "void" when the language is Rust. Thanks for finding and fixing this. The patch looks good overall but I have a few questions and some nits. Manish> * rust-lang.c: Print unit types as "()" Manish> * rust-lang.c: Omit return type for functions returning unit A ChangeLog entry is supposed to refer to function and variable names, so more like: * rust-lang.c (rust_decorations): Spell "void" as "()". (rust_print_type): Likewise. ... Manish> + /* Rust calls the unit type "void" in its debuginfo, Manish> + but we don't want to print it as that. */ Manish> + if (TYPE_CODE (type) == TYPE_CODE_VOID) Manish> + fputs_filtered ("()", stream); Manish> + else Manish> + fputs_filtered (TYPE_NAME (type), stream); This is fine but there is another case in rust_val_print: case TYPE_CODE_INT: /* Recognize the unit type. */ if (TYPE_UNSIGNED (type) && TYPE_LENGTH (type) == 0 && TYPE_NAME (type) != NULL && strcmp (TYPE_NAME (type), "()") == 0) { fputs_filtered ("()", stream); break; } goto generic_print; ... I wonder if that is something that changed after 1.8, or if it's the case that the unit type can be represented in multiple ways. (Or maybe this only handles the unit type constructed by rust_language_arch_info?) Anyway I wonder if a case like this is needed in rust_print_type, or if the one in rust_val_print can be removed or changed. Manish> + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID) Manish> + { Manish> + fputs_filtered (" -> ", stream); Manish> + rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); This should be formatted like: + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID) + { + fputs_filtered (" -> ", stream); + rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); Manish> +// Empty function, should not have "void" Manish> +// or "()" in its return type Manish> +fn empty() { I'm curious what happens if it does say "-> ()"? Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Print void types correctly in Rust 2016-06-27 16:15 ` Tom Tromey @ 2016-06-27 16:25 ` Manish Goregaokar 2016-06-27 16:35 ` Tom Tromey 2016-06-27 16:26 ` Tom Tromey 1 sibling, 1 reply; 6+ messages in thread From: Manish Goregaokar @ 2016-06-27 16:25 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches > This is fine but there is another case in rust_val_print: > > case TYPE_CODE_INT: > /* Recognize the unit type. */ > if (TYPE_UNSIGNED (type) && TYPE_LENGTH (type) == 0 > && TYPE_NAME (type) != NULL && strcmp (TYPE_NAME (type), "()") == 0) > { > fputs_filtered ("()", stream); > break; > } > goto generic_print; > > > ... I wonder if that is something that changed after 1.8, or if it's the > case that the unit type can be represented in multiple ways. (Or maybe > this only handles the unit type constructed by rust_language_arch_info?) > > Anyway I wonder if a case like this is needed in rust_print_type, or if > the one in rust_val_print can be removed or changed. I don't think this has changed. The issue is with functions, where the return type of functions that return unit is called void because we pretend to be C. Values have a different type code. > Manish> + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID) > Manish> + { > Manish> + fputs_filtered (" -> ", stream); > Manish> + rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); > > This should be formatted like: > > + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID) > + { > + fputs_filtered (" -> ", stream); > + rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); > > Manish> +// Empty function, should not have "void" > Manish> +// or "()" in its return type > Manish> +fn empty() { > > I'm curious what happens if it does say "-> ()"? It's okay to do so, just not idiomatic Rust. You rarely see `-> ()` for void functions. ========= new patch below (changelog and formatting fixed): gdb/ChangeLog: * rust-lang.c (rust_print_type): Print unit types as "()" * rust-lang.c (rust_print_type): Omit return type for functions returning unit gdb/testsuite/ChangeLog: 2016-06-27 Manish Goregaokar <manish@mozilla.com> * gdb.rust/simple.rs: Add test for returning unit in a function * gdb.rust/simple.exp: Add expectation for functions returning unit --- gdb/rust-lang.c | 22 ++++++++++++++++++---- gdb/testsuite/gdb.rust/simple.exp | 1 + gdb/testsuite/gdb.rust/simple.rs | 7 +++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c index 0c56a0f..23ddd5a 100644 --- a/gdb/rust-lang.c +++ b/gdb/rust-lang.c @@ -446,7 +446,7 @@ static const struct generic_val_print_decorations rust_decorations = " * I", "true", "false", - "void", + "()", "[", "]" }; @@ -729,13 +729,22 @@ rust_print_type (struct type *type, const char *varstring, if (show <= 0 && TYPE_NAME (type) != NULL) { - fputs_filtered (TYPE_NAME (type), stream); + /* Rust calls the unit type "void" in its debuginfo, + but we don't want to print it as that. */ + if (TYPE_CODE (type) == TYPE_CODE_VOID) + fputs_filtered ("()", stream); + else + fputs_filtered (TYPE_NAME (type), stream); return; } type = check_typedef (type); switch (TYPE_CODE (type)) { + case TYPE_CODE_VOID: + fputs_filtered ("()", stream); + break; + case TYPE_CODE_FUNC: /* Delegate varargs to the C printer. */ if (TYPE_VARARGS (type)) @@ -753,8 +762,13 @@ rust_print_type (struct type *type, const char *varstring, rust_print_type (TYPE_FIELD_TYPE (type, i), "", stream, -1, 0, flags); } - fputs_filtered (") -> ", stream); - rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); + fputs_filtered (")", stream); + /* If it returns unit, we can omit the return type. */ + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID) + { + fputs_filtered (" -> ", stream); + rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); + } break; case TYPE_CODE_ARRAY: diff --git a/gdb/testsuite/gdb.rust/simple.exp b/gdb/testsuite/gdb.rust/simple.exp index 88f1c89..4622f75 100644 --- a/gdb/testsuite/gdb.rust/simple.exp +++ b/gdb/testsuite/gdb.rust/simple.exp @@ -149,6 +149,7 @@ gdb_test "print self::diff2(8, 9)" " = -1" gdb_test "print ::diff2(23, -23)" " = 46" gdb_test "ptype diff2" "fn \\(i32, i32\\) -> i32" +gdb_test "ptype empty" "fn \\(\\)" gdb_test "print (diff2 as fn(i32, i32) -> i32)(19, -2)" " = 21" diff --git a/gdb/testsuite/gdb.rust/simple.rs b/gdb/testsuite/gdb.rust/simple.rs index 32da580..3d28e27 100644 --- a/gdb/testsuite/gdb.rust/simple.rs +++ b/gdb/testsuite/gdb.rust/simple.rs @@ -48,6 +48,12 @@ fn diff2(x: i32, y: i32) -> i32 { x - y } +// Empty function, should not have "void" +// or "()" in its return type +fn empty() { + +} + pub struct Unit; // This triggers the non-zero optimization that yields a different @@ -111,4 +117,5 @@ fn main () { println!("{}, {}", x.0, x.1); // set breakpoint here println!("{}", diff2(92, 45)); + empty(); } -- 2.8.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Print void types correctly in Rust 2016-06-27 16:25 ` Manish Goregaokar @ 2016-06-27 16:35 ` Tom Tromey 0 siblings, 0 replies; 6+ messages in thread From: Tom Tromey @ 2016-06-27 16:35 UTC (permalink / raw) To: Manish Goregaokar; +Cc: Tom Tromey, gdb-patches >>>>> "Manish" == Manish Goregaokar <manish@mozilla.com> writes: Manish> gdb/ChangeLog: Manish> * rust-lang.c (rust_print_type): Print unit types as "()" Manish> * rust-lang.c (rust_print_type): Omit return type for functions Manish> returning unit The first one should mention rust_decorations. Also a given file is listed just once per entry. Full docs here: https://www.gnu.org/prep/standards/html_node/Change-Logs.html#Change-Logs It's ok with this nit fixed, thanks again. Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Print void types correctly in Rust 2016-06-27 16:15 ` Tom Tromey 2016-06-27 16:25 ` Manish Goregaokar @ 2016-06-27 16:26 ` Tom Tromey 1 sibling, 0 replies; 6+ messages in thread From: Tom Tromey @ 2016-06-27 16:26 UTC (permalink / raw) To: Tom Tromey; +Cc: Manish Goregaokar, gdb-patches Tom> ... I wonder if that is something that changed after 1.8, or if it's the Tom> case that the unit type can be represented in multiple ways. (Or maybe Tom> this only handles the unit type constructed by rust_language_arch_info?) [...] Manish> +// Empty function, should not have "void" Manish> +// or "()" in its return type Manish> +fn empty() { Tom> I'm curious what happens if it does say "-> ()"? I did the experiment myself and I think I see what's going on now. For an explicit "()", like "let y = ()", we'll end up with a zero-sized integer type: <1><98>: Abbrev Number: 7 (DW_TAG_base_type) <99> DW_AT_name : (indirect string, offset: 0x88): () <9d> DW_AT_encoding : 7 (unsigned) <9e> DW_AT_byte_size : 0 However, "fn empty()" (or in my experiment also "fn empty()->()"), the type is left unspecified in the DWARF, which gdb turns into TYPE_CODE_VOID (see dwarf2read.c:read_unspecified_type). I think both have to be handled. However I think it's fine to do some cleanup in a follow-up patch, which I'm happy to do. So your patch is OK with the ChangeLog entry and the indentation fixed. Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Print void types correctly in Rust 2016-06-27 15:52 [PATCH] Print void types correctly in Rust Manish Goregaokar 2016-06-27 16:15 ` Tom Tromey @ 2016-06-27 16:53 ` Manish Goregaokar 1 sibling, 0 replies; 6+ messages in thread From: Manish Goregaokar @ 2016-06-27 16:53 UTC (permalink / raw) To: gdb-patches, Tom Tromey Landed as https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=921d8f549f9e35d3f83c7b1a381146a7dc1246f4, with a changelog fix at https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=6763d566a8d30d1ad65dfd060a919c621dad86c4. Thanks, -Manish On Mon, Jun 27, 2016 at 9:22 PM, Manish Goregaokar <manish@mozilla.com> wrote: > Rust prefers to not specify the return type of a function when it is unit > (`()`). The type is also referred to as "void" in debuginfo but not in actual > usage, so we should never be printing "void" when the language is Rust. > > gdb/ChangeLog: > * rust-lang.c: Print unit types as "()" > * rust-lang.c: Omit return type for functions returning unit > > gdb/testsuite/ChangeLog: > 2016-06-27 Manish Goregaokar <manish@mozilla.com> > * gdb.rust/simple.rs: Add test for returning unit in a function > * gdb.rust/simple.exp: Add expectation for functions returning unit > --- > gdb/rust-lang.c | 22 ++++++++++++++++++---- > gdb/testsuite/gdb.rust/simple.exp | 1 + > gdb/testsuite/gdb.rust/simple.rs | 7 +++++++ > 3 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c > index 0c56a0f..2b115ee 100644 > --- a/gdb/rust-lang.c > +++ b/gdb/rust-lang.c > @@ -446,7 +446,7 @@ static const struct generic_val_print_decorations > rust_decorations = > " * I", > "true", > "false", > - "void", > + "()", > "[", > "]" > }; > @@ -729,13 +729,22 @@ rust_print_type (struct type *type, const char *varstring, > if (show <= 0 > && TYPE_NAME (type) != NULL) > { > - fputs_filtered (TYPE_NAME (type), stream); > + /* Rust calls the unit type "void" in its debuginfo, > + but we don't want to print it as that. */ > + if (TYPE_CODE (type) == TYPE_CODE_VOID) > + fputs_filtered ("()", stream); > + else > + fputs_filtered (TYPE_NAME (type), stream); > return; > } > > type = check_typedef (type); > switch (TYPE_CODE (type)) > { > + case TYPE_CODE_VOID: > + fputs_filtered ("()", stream); > + break; > + > case TYPE_CODE_FUNC: > /* Delegate varargs to the C printer. */ > if (TYPE_VARARGS (type)) > @@ -753,8 +762,13 @@ rust_print_type (struct type *type, const char *varstring, > rust_print_type (TYPE_FIELD_TYPE (type, i), "", stream, -1, 0, > flags); > } > - fputs_filtered (") -> ", stream); > - rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); > + fputs_filtered (")", stream); > + /* If it returns unit, we can omit the return type. */ > + if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID) > + { > + fputs_filtered (" -> ", stream); > + rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags); > + } > break; > > case TYPE_CODE_ARRAY: > diff --git a/gdb/testsuite/gdb.rust/simple.exp > b/gdb/testsuite/gdb.rust/simple.exp > index 88f1c89..4622f75 100644 > --- a/gdb/testsuite/gdb.rust/simple.exp > +++ b/gdb/testsuite/gdb.rust/simple.exp > @@ -149,6 +149,7 @@ gdb_test "print self::diff2(8, 9)" " = -1" > gdb_test "print ::diff2(23, -23)" " = 46" > > gdb_test "ptype diff2" "fn \\(i32, i32\\) -> i32" > +gdb_test "ptype empty" "fn \\(\\)" > > gdb_test "print (diff2 as fn(i32, i32) -> i32)(19, -2)" " = 21" > > diff --git a/gdb/testsuite/gdb.rust/simple.rs b/gdb/testsuite/gdb.rust/simple.rs > index 32da580..3d28e27 100644 > --- a/gdb/testsuite/gdb.rust/simple.rs > +++ b/gdb/testsuite/gdb.rust/simple.rs > @@ -48,6 +48,12 @@ fn diff2(x: i32, y: i32) -> i32 { > x - y > } > > +// Empty function, should not have "void" > +// or "()" in its return type > +fn empty() { > + > +} > + > pub struct Unit; > > // This triggers the non-zero optimization that yields a different > @@ -111,4 +117,5 @@ fn main () { > > println!("{}, {}", x.0, x.1); // set breakpoint here > println!("{}", diff2(92, 45)); > + empty(); > } > -- > 2.8.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-27 16:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-27 15:52 [PATCH] Print void types correctly in Rust Manish Goregaokar 2016-06-27 16:15 ` Tom Tromey 2016-06-27 16:25 ` Manish Goregaokar 2016-06-27 16:35 ` Tom Tromey 2016-06-27 16:26 ` Tom Tromey 2016-06-27 16:53 ` Manish Goregaokar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).