[1/2,PR,gdb/20239] Make evaluation and type-printing of all NonZero-optimized enums work

  Moved over from https://sourceware.org/bugzilla/show_bug.cgi?id=20239.
Regarding the xstrdup, this is done because strsep edits the string
itself.

Built and tested on OS X El Capitan. Methods tests fail, but they are
a preexisting failure due to Rust's debuginfo changing.



gdb/ChangeLog:
2016-06-21  Manish Goregaokar  <manish@mozilla.com>

    PR gdb/20239
    * rust-lang.c (rust_get_disr_info): Correctly interpret
    NonZero-optimized enums of arbitrary depth.
    (rust_print_type): Correctly print NonZero-optimized
    enums.
---
 gdb/rust-lang.c | 72 +++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 17 deletions(-)

+      do_cleanups (cleanup);
       return ret;
     }

@@ -841,6 +868,7 @@ rust_print_type (struct type *type, const char *varstring,
       {
     /* ADT enums */
     int i, len = 0;
+  int skip_to = 1; /* Skip the discriminant field */

     fputs_filtered ("enum ", stream);
     if (TYPE_TAG_NAME (type) != NULL)
@@ -849,7 +877,17 @@ rust_print_type (struct type *type, const char *varstring,
         fputs_filtered (" ", stream);
         len = strlen (TYPE_TAG_NAME (type));
       }
-    fputs_filtered ("{\n", stream);
+    fputs_filtered ("{\n", stream);
+
+  if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
+       strlen (RUST_ENUM_PREFIX)) == 0) {
+    char *zero_field = strrchr (TYPE_FIELD_NAME (type, 0), '$');
+    if (zero_field != NULL && strlen (zero_field) > 1) {
+      fprintfi_filtered (level+2, stream, "%s,\n", zero_field+1);
+      /* there is no explicit discriminant field, skip nothing */
+      skip_to = 0;
+    }
+  }

     for (i = 0; i < TYPE_NFIELDS (type); ++i)
       {
@@ -859,14 +897,14 @@ rust_print_type (struct type *type, const char *varstring,

         fprintfi_filtered (level + 2, stream, "%s", name);

-        if (TYPE_NFIELDS (variant_type) > 1)
+        if (TYPE_NFIELDS (variant_type) > skip_to)
           {
-        int first = 1;
+    int first = 1;
         int is_tuple = rust_tuple_variant_type_p (variant_type);
         int j;

         fputs_filtered (is_tuple ? "(" : "{", stream);
-        for (j = 1; j < TYPE_NFIELDS (variant_type); j++)
+        for (j = skip_to; j < TYPE_NFIELDS (variant_type); j++)
           {
             if (first)
               first = 0;
  

>>>>> "Manish" == Manish Goregaokar <manish@mozilla.com> writes:

Manish> Regarding the xstrdup, this is done because strsep edits the string
Manish> itself.

Indeed it does.  Somehow I didn't realize this, sorry about that.

Manish> Methods tests fail, but they are
Manish> a preexisting failure due to Rust's debuginfo changing.

Yes, the situation with 1.9 is not that good :(


This patch looks good.  There are formatting nits but really nothing serious.

The normal procedure is that once you've written one good patch you can
get write-after-approval access to gdb.  If you want to go this route,
let me know.  Otherwise (and also let me know) I can push the final
patches for you.

Manish> +         char *type_name;

I think this is assigned to but not used, so you might as well remove
it.

Manish> +      /* Optimized enums have only one field */

gdb style is period with two spaces at the end of a comment.

Manish> +      while ((token = strsep (&tail, "$")) != NULL)
Manish> +      {

The "{" should be indented 2 more spaces, and then the body as well.

Manish> +         if (sscanf (token, "%lu", &fieldno) != 1)
Manish> +         {

Here too.

Manish> +            /* We have reached the enum name, which cannot start with a digit */

The period thing again.  Also this line might be too long.

Manish> +      };

Extra ";".

Manish> +        value = unpack_long (member_type,
Manish> +           valaddr + embedded_offset);

Make sure the continuation line is indented to line up under "member_type".
(Maybe it is but I can't tell ...)

Manish>      int i, len = 0;
Manish> +  int skip_to = 1; /* Skip the discriminant field */

Indentation?  Also gdb doesn't tend to use trailing comments like that;
you can put it before the declaration.

Manish> +    char *zero_field = strrchr (TYPE_FIELD_NAME (type, 0), '$');

const char *

Manish> +    if (zero_field != NULL && strlen (zero_field) > 1) {

"{" on the next line and indented.

Manish> +      fprintfi_filtered (level+2, stream, "%s,\n", zero_field+1);

Spaces around "+"s.

Manish> +      /* there is no explicit discriminant field, skip nothing */

Upper case "There", period.

Manish> -        int first = 1;
Manish> +    int first = 1;

Some spurious change maybe?

Tom
  

After further fixups, these patches have been pushed as
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=b5a4b3c5e711be9096423f9765623eda449d8f4d
and https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=fccb08f8cd2035b50a2b0a5e09983180b7411685

Thanks!
-Manish


On Tue, Jun 21, 2016 at 3:10 PM, Manish Goregaokar <manish@mozilla.com> wrote:
> Moved over from https://sourceware.org/bugzilla/show_bug.cgi?id=20239.
> Regarding the xstrdup, this is done because strsep edits the string
> itself.
>
> Built and tested on OS X El Capitan. Methods tests fail, but they are
> a preexisting failure due to Rust's debuginfo changing.
>
>
>
> gdb/ChangeLog:
> 2016-06-21  Manish Goregaokar  <manish@mozilla.com>
>
>     PR gdb/20239
>     * rust-lang.c (rust_get_disr_info): Correctly interpret
>     NonZero-optimized enums of arbitrary depth.
>     (rust_print_type): Correctly print NonZero-optimized
>     enums.
> ---
>  gdb/rust-lang.c | 72 +++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
> index 5df99ce..baabf76 100644
> --- a/gdb/rust-lang.c
> +++ b/gdb/rust-lang.c
> @@ -120,42 +120,69 @@ rust_get_disr_info (struct type *type, const
> gdb_byte *valaddr,
>    if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
>             strlen (RUST_ENUM_PREFIX)) == 0)
>      {
> -      char *tail;
> +      char *name, *tail, *token;
>        unsigned long fieldno;
>        struct type *member_type;
>        LONGEST value;
>
> +         char *type_name;
> +
>        ret.is_encoded = 1;
>
>        if (TYPE_NFIELDS (type) != 1)
>      error (_("Only expected one field in %s type"), RUST_ENUM_PREFIX);
>
> -      fieldno = strtoul (TYPE_FIELD_NAME (type, 0) + strlen (RUST_ENUM_PREFIX),
> -             &tail, 10);
> -      if (*tail != '$')
> +      /* Optimized enums have only one field */
> +      member_type = TYPE_FIELD_TYPE (type, 0);
> +
> +      name = xstrdup (TYPE_FIELD_NAME (type, 0));
> +      cleanup = make_cleanup (xfree, name);
> +      tail = name + strlen (RUST_ENUM_PREFIX);
> +
> +      /* The location of the value that doubles as a discriminant is
> +         stored in the name of the field, as
> +         RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
> +         where the fieldnos are the indices of the fields that should be
> +         traversed in order to find the field (which may be several
> fields deep)
> +         and the variantname is the name of the variant of the case when the
> +         field is zero */
> +      while ((token = strsep (&tail, "$")) != NULL)
> +      {
> +         if (sscanf (token, "%lu", &fieldno) != 1)
> +         {
> +            /* We have reached the enum name, which cannot start with
> a digit */
> +            break;
> +         }
> +        if (fieldno >= TYPE_NFIELDS (member_type))
> +    error (_("%s refers to field after end of member type"),
> +           RUST_ENUM_PREFIX);
> +
> +        embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
> +        member_type = TYPE_FIELD_TYPE (member_type, fieldno);
> +        type_name = TYPE_NAME (member_type);
> +      };
> +
> +      if (token >= name + strlen (TYPE_FIELD_NAME (type, 0)))
>      error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
> +        value = unpack_long (member_type,
> +           valaddr + embedded_offset);
> +
>
> -      member_type = TYPE_FIELD_TYPE (type, 0);
> -      if (fieldno >= TYPE_NFIELDS (member_type))
> -    error (_("%s refers to field after end of member type"),
> -           RUST_ENUM_PREFIX);
>
> -      embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
> -      value = unpack_long (TYPE_FIELD_TYPE (member_type, fieldno),
> -               valaddr + embedded_offset);
>        if (value == 0)
>      {
>        ret.field_no = RUST_ENCODED_ENUM_HIDDEN;
> -      ret.name = concat (TYPE_NAME (type), "::", tail + 1, (char *) NULL);
> +      ret.name = concat (TYPE_NAME (type), "::", token, (char *) NULL);
>      }
>        else
>      {
>        ret.field_no = RUST_ENCODED_ENUM_REAL;
>        ret.name = concat (TYPE_NAME (type), "::",
> -                 rust_last_path_segment (TYPE_NAME (member_type)),
> +                 rust_last_path_segment (TYPE_NAME (TYPE_FIELD_TYPE
> (type, 0))),
>                   (char *) NULL);
>      }
>
> +      do_cleanups (cleanup);
>        return ret;
>      }
>
> @@ -841,6 +868,7 @@ rust_print_type (struct type *type, const char *varstring,
>        {
>      /* ADT enums */
>      int i, len = 0;
> +  int skip_to = 1; /* Skip the discriminant field */
>
>      fputs_filtered ("enum ", stream);
>      if (TYPE_TAG_NAME (type) != NULL)
> @@ -849,7 +877,17 @@ rust_print_type (struct type *type, const char *varstring,
>          fputs_filtered (" ", stream);
>          len = strlen (TYPE_TAG_NAME (type));
>        }
> -    fputs_filtered ("{\n", stream);
> +    fputs_filtered ("{\n", stream);
> +
> +  if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
> +       strlen (RUST_ENUM_PREFIX)) == 0) {
> +    char *zero_field = strrchr (TYPE_FIELD_NAME (type, 0), '$');
> +    if (zero_field != NULL && strlen (zero_field) > 1) {
> +      fprintfi_filtered (level+2, stream, "%s,\n", zero_field+1);
> +      /* there is no explicit discriminant field, skip nothing */
> +      skip_to = 0;
> +    }
> +  }
>
>      for (i = 0; i < TYPE_NFIELDS (type); ++i)
>        {
> @@ -859,14 +897,14 @@ rust_print_type (struct type *type, const char *varstring,
>
>          fprintfi_filtered (level + 2, stream, "%s", name);
>
> -        if (TYPE_NFIELDS (variant_type) > 1)
> +        if (TYPE_NFIELDS (variant_type) > skip_to)
>            {
> -        int first = 1;
> +    int first = 1;
>          int is_tuple = rust_tuple_variant_type_p (variant_type);
>          int j;
>
>          fputs_filtered (is_tuple ? "(" : "{", stream);
> -        for (j = 1; j < TYPE_NFIELDS (variant_type); j++)
> +        for (j = skip_to; j < TYPE_NFIELDS (variant_type); j++)
>            {
>              if (first)
>                first = 0;
> --
> 2.8.3
  

Hi Manish,

On Saturday 25 June 2016 11:30:39 Manish Goregaokar wrote:
> After further fixups, these patches have been pushed as
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=b5a4b3c5
> e711be9096423f9765623eda449d8f4d and
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=fccb08f
> 8cd2035b50a2b0a5e09983180b7411685

GDB fails to build on Windows (mingw build) due to this patch as strsep is not 
available. Was there something preventing the use of strtok?

Best regards,

Thomas
  

No, strsep was just a better fit here. I wasn't aware that it doesn't
work on windows.

I'll fix it to use strtok.
-Manish


On Wed, Jun 29, 2016 at 2:25 PM, Thomas Preudhomme
<thomas.preudhomme@foss.arm.com> wrote:
> Hi Manish,
>
> On Saturday 25 June 2016 11:30:39 Manish Goregaokar wrote:
>> After further fixups, these patches have been pushed as
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=b5a4b3c5
>> e711be9096423f9765623eda449d8f4d and
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=fccb08f
>> 8cd2035b50a2b0a5e09983180b7411685
>
> GDB fails to build on Windows (mingw build) due to this patch as strsep is not
> available. Was there something preventing the use of strtok?
>
> Best regards,
>
> Thomas
  

On Wednesday 29 June 2016 14:27:56 Manish Goregaokar wrote:
> No, strsep was just a better fit here. I wasn't aware that it doesn't
> work on windows.

I just found it when investigating the build failure: the manual says it 
conforms to 4.4BSD and suggest the use of strtok for portability.

> 
> I'll fix it to use strtok.

Great, many thanks! While you are at it, shouldn't an include of string.h be 
added as well? It clearly does not fail now without it because I suppose 
another header is including it.

Best regards,

Thomas
  

>
> Great, many thanks! While you are at it, shouldn't an include of string.h be
> added as well? It clearly does not fail now without it because I suppose
> another header is including it.
>

It seems to be included as a part of common-defs.h, so I don't think
we're supposed to explicitly include it.
  

On Wednesday 29 June 2016 14:47:27 Manish Goregaokar wrote:
> > Great, many thanks! While you are at it, shouldn't an include of string.h
> > be added as well? It clearly does not fail now without it because I
> > suppose another header is including it.
> 
> It seems to be included as a part of common-defs.h, so I don't think
> we're supposed to explicitly include it.

Oh right, indeed.

Best regards,

Thomas
  

On 06/29/2016 09:57 AM, Manish Goregaokar wrote:
> No, strsep was just a better fit here. I wasn't aware that it doesn't
> work on windows.
> 
> I'll fix it to use strtok.

You can use strtok_r.  We pull the gnulib module that ensures it's
always available.

Thanks,
Pedro Alves