[PATCH] Print void types correctly in Rust

[PATCH] Print void types correctly in Rust

[Manish Goregaokar]

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

Re: [PATCH] Print void types correctly in Rust

[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

Re: [PATCH] Print void types correctly in Rust

[Manish Goregaokar]

> 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

Re: [PATCH] Print void types correctly in Rust

[Tom Tromey]

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

Re: [PATCH] Print void types correctly in Rust

[Tom Tromey]

>>>>> "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

Re: [PATCH] Print void types correctly in Rust

[Manish Goregaokar]

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