* Re: [PATCH 1/2][PR gdb/20239] Make evaluation and type-printing of all NonZero-optimized enums work
[not found] <CAFOnWknif9WwaRdDYzxx8ngNJBwgfN24w02KGPVnYA6-VtMcUg@mail.gmail.com>
@ 2016-06-22 19:10 ` Tom Tromey
2016-06-25 6:00 ` Manish Goregaokar
1 sibling, 0 replies; 8+ messages in thread
From: Tom Tromey @ 2016-06-22 19:10 UTC (permalink / raw)
To: Manish Goregaokar; +Cc: gdb-patches, Tom Tromey
>>>>> "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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2][PR gdb/20239] Make evaluation and type-printing of all NonZero-optimized enums work
[not found] <CAFOnWknif9WwaRdDYzxx8ngNJBwgfN24w02KGPVnYA6-VtMcUg@mail.gmail.com>
2016-06-22 19:10 ` [PATCH 1/2][PR gdb/20239] Make evaluation and type-printing of all NonZero-optimized enums work Tom Tromey
@ 2016-06-25 6:00 ` Manish Goregaokar
2016-06-29 8:55 ` Thomas Preudhomme
1 sibling, 1 reply; 8+ messages in thread
From: Manish Goregaokar @ 2016-06-25 6:00 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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
^ permalink raw reply [flat|nested] 8+ messages in thread