[patch] ptype: show members of an unnamed struct inside an union

[patch] ptype: show members of an unnamed struct inside an union

[Carlos Eduardo Seo]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello!

The attached patched fixes an issue described on this post:

http://sourceware.org/ml/gdb-patches/2002-04/msg01114.html

Any comments?

Thanks and regards,

- --
Carlos Eduardo Seo
Software Engineer
IBM Linux Technology Center
E-Mail: cseo@linux.vnet.ibm.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.5 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGxNINqvq7Aov/qQARAvUXAKCLQ5hF+jNzreDyiubG2cHFRbDMNQCfYudG
Kuve0/kwqXrVF9k8rxLwtto=
=8Qjd
-----END PGP SIGNATURE-----

[-- Attachment #2: ptype_issue.diff --]
[-- Type: text/plain, Size: 715 bytes --]

2007-08-16   Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>

	gdb/c-typeprint.c (c_type_print_base): check value of 
	TYPE_NFIELDS (type) when displaying members of an
	unnamed struct inside an union.

Index: src-git/gdb/c-typeprint.c
===================================================================
--- src-git.orig/gdb/c-typeprint.c	2007-08-16 15:28:04.000000000 -0700
+++ src-git/gdb/c-typeprint.c	2007-08-16 15:29:07.000000000 -0700
@@ -735,7 +735,7 @@
 	    fputs_filtered (" ", stream);
 	}
       wrap_here ("    ");
-      if (show < 0)
+      if ((show < 0) && (TYPE_NFIELDS (type) == 0))
 	{
 	  /* If we just printed a tag name, no need to print anything else.  */
 	  if (TYPE_TAG_NAME (type) == NULL)

[-- Attachment #3: ptype_issue.diff.sig --]
[-- Type: application/octet-stream, Size: 65 bytes --]

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

Carlos,

> The attached patched fixes an issue described on this post:
> 
> http://sourceware.org/ml/gdb-patches/2002-04/msg01114.html

> 2007-08-16   Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>
> 
> 	gdb/c-typeprint.c (c_type_print_base): check value of 
> 	TYPE_NFIELDS (type) when displaying members of an
> 	unnamed struct inside an union.

Could you add a testcase for this issue? Being new at reviewing
other people's patches, it's sometimes hard for me to understand
what you are trying to do. A testcase would help in two ways: not
only ensure that we do not regress in the future, but also help
me understand clearly what it is that we're trying to fix.

Thank you!

> Index: src-git/gdb/c-typeprint.c
> ===================================================================
> --- src-git.orig/gdb/c-typeprint.c	2007-08-16 15:28:04.000000000 -0700
> +++ src-git/gdb/c-typeprint.c	2007-08-16 15:29:07.000000000 -0700
> @@ -735,7 +735,7 @@
>  	    fputs_filtered (" ", stream);
>  	}
>        wrap_here ("    ");
> -      if (show < 0)
> +      if ((show < 0) && (TYPE_NFIELDS (type) == 0))
>  	{
>  	  /* If we just printed a tag name, no need to print anything else.  */
>  	  if (TYPE_TAG_NAME (type) == NULL)

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Carlos Eduardo Seo]

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Joel

Here's a testcase. In order to see the issue, do the following:

1. Compile it with -g flag
2. Run gdb
3. b main
4. ptype struct my_page

You'll see:

type = struct my_page {
    union {
        struct {...};
    };
}

Using the proposed patch, you'll see:


type = struct my_page {
    union {
        struct {int mapping; };
    };
}

If you want, you may use a more complex union, such as:

struct my_page {
       union {
           struct {
               int mapping;
               long foo;
               char *pointer;
           };
           struct {
               char blah;
               int test;
           };
           struct {
               char blahblah;
           };
           struct {
           };
       };
};

Any questions, feel free to contact me.

Thanks and regards,

- --
Carlos Eduardo Seo
Software Engineer
IBM Linux Technology Center



Joel Brobecker wrote:
> Carlos,
>
>> The attached patched fixes an issue described on this post:
>>
>> http://sourceware.org/ml/gdb-patches/2002-04/msg01114.html
>
>> 2007-08-16   Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>
>>
>>     gdb/c-typeprint.c (c_type_print_base): check value of
>>     TYPE_NFIELDS (type) when displaying members of an
>>     unnamed struct inside an union.
>
> Could you add a testcase for this issue? Being new at reviewing
> other people's patches, it's sometimes hard for me to understand
> what you are trying to do. A testcase would help in two ways: not
> only ensure that we do not regress in the future, but also help
> me understand clearly what it is that we're trying to fix.
>
> Thank you!
>
>> Index: src-git/gdb/c-typeprint.c
>> ===================================================================
>> --- src-git.orig/gdb/c-typeprint.c    2007-08-16 15:28:04.000000000 -0700
>> +++ src-git/gdb/c-typeprint.c    2007-08-16 15:29:07.000000000 -0700
>> @@ -735,7 +735,7 @@
>>          fputs_filtered (" ", stream);
>>      }
>>        wrap_here ("    ");
>> -      if (show < 0)
>> +      if ((show < 0) && (TYPE_NFIELDS (type) == 0))
>>      {
>>        /* If we just printed a tag name, no need to print anything
else.  */
>>        if (TYPE_TAG_NAME (type) == NULL)
>


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGzwDaqvq7Aov/qQARArN6AJ9i06xbZqjUGhRXRZg5LReVZtmi6gCghOZB
PgEYEpPTi3YfX6rXvQoXlZY=
=sCUb
-----END PGP SIGNATURE-----


[-- Attachment #2: mypage.c --]
[-- Type: text/plain, Size: 149 bytes --]

struct my_page {
       union {
           struct {
               int mapping;
           };
       };
};

main()
{
       struct my_page mypage;
}

[-- Attachment #3: mypage.c.sig --]
[-- Type: application/octet-stream, Size: 65 bytes --]

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

> Here's a testcase. In order to see the issue, do the following:

Thanks, I'll use that to review your patch. But I meant a testcase
that we could add to our testsuite (a .exp file, a .c file, etc)...
It should be pretty straightforward to write one from the example
you just provided.

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

Carlos,

> The attached patched fixes an issue described on this post:
> 
> http://sourceware.org/ml/gdb-patches/2002-04/msg01114.html
> 
> Any comments?

It actually seems to me that the current behavior is expected.
It's not actually a question of named vs unnamed structure,
but rather a question of nesting. Consider the following example:

    struct inner
    {
      int mapping;
    };
    
    struct outer
    {
      int a;
      struct inner b;
      struct
      {
        int c;
        struct { int d; } e;
      } f;
    };

Doing a "ptype" on type "outer" yields:

    type = struct outer {
        int a;
        struct inner b;
        struct {
            int c;
            struct {...} e;
        } f;
    }

Basically, the idea here is to avoid going too deep into the type
structure by only printing complete information for the immediate
fields of our structure (that's nesting level 1). For nesting level 2,
we print the type name only (eg: "struct inner b"), or, when the field
has an unnamed type, a concise description of that type: That's why
we printed "struct { int c; ... }". For nesting level 3, we try to
be even more concise, which means the type name if we have one, or
"struct {...}" if we don't.

See also the description of the function you tried to modify:

   SHOW positive means print details about the type (e.g. enum values),
   and print structure elements passing SHOW - 1 for show.
   SHOW negative means just print the type name or struct tag if there is one.
   If there is no name, print something sensible but concise like
   "struct {...}".
   SHOW zero means just print the type name or struct tag if there is one.
   If there is no name, print something sensible but not as concise like
   "struct {int x; int y;}".

So, in order to get the type description for our nested structure,
we just need to get one level deeper by doing the following:

    (gdb) ptype my_outer.f
    type = struct {
        int c;
        struct {
            int d;
        } e;
    }

Similarly in your example:

    (gdb) ptype mypage.u
    type = union {
        struct {
            int mapping;
        };
    }

I looked at the current GDB documentation for "ptype" and couldn't
find anything that confirmed my analysis. I think a contribution
to the documentation would be very much appreciated.

In the meantime, I will submit a testcase in our testsuite that would have
failed if you patch had been applied.

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Luis Machado]

Joel Brobecker wrote:
> Similarly in your example:
>
>     (gdb) ptype mypage.u
>     type = union {
>         struct {
>             int mapping;
>         };
>     }

Are you considering a named Union structure here? I could not reproduce 
this locally, since there's no such member "u" in the data structure.

Regards,
Luis

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

> >Similarly in your example:
> >
> >    (gdb) ptype mypage.u
> >    type = union {
> >        struct {
> >            int mapping;
> >        };
> >    }
> 
> Are you considering a named Union structure here? I could not reproduce 
> this locally, since there's no such member "u" in the data structure.

Yes, I had to add a name to this field. Otherwise, there is no way
to name it!

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Carlos Eduardo Seo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


>
> Yes, I had to add a name to this field. Otherwise, there is no way
> to name it!
>
What about the case of an unnamed union, such as in my example?  GDB
won't support displaying its members then?

- --
Carlos Eduardo Seo
Software Engineer
IBM Linux Technology Center
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG1IGMqvq7Aov/qQARAlrEAJ0Tv/euI+KWdxbRQN8/FgvxrhrNcQCeKSTG
XM+jIvWzey2AiWypsmhbOAA=
=ugn2
-----END PGP SIGNATURE-----

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

> > Yes, I had to add a name to this field. Otherwise, there is no way
> > to name it!
> >
> What about the case of an unnamed union, such as in my example?  GDB
> won't support displaying its members then?

Does this case actually make any sense? I don't think you can access
this union in your C program either, or can you?

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Daniel Jacobowitz]

On Tue, Aug 28, 2007 at 01:34:20PM -0700, Joel Brobecker wrote:
> > > Yes, I had to add a name to this field. Otherwise, there is no way
> > > to name it!
> > >
> > What about the case of an unnamed union, such as in my example?  GDB
> > won't support displaying its members then?
> 
> Does this case actually make any sense? I don't think you can access
> this union in your C program either, or can you?

GCC supports anonymous unions.  There's a test for it in the GDB
testsuite, too.

I don't remember whether this was imported from C++ or from C99.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

> > Does this case actually make any sense? I don't think you can access
> > this union in your C program either, or can you?
> 
> GCC supports anonymous unions.  There's a test for it in the GDB
> testsuite, too.

But how can you access this union if the field doesn't have a name?
More precisely, using the example that Carlos sent:

   struct my_page {
     union {
       struct { int mapping; };
     };
   };

How do you access the union? That's why I gave the union field a name
"u" as follow:

   struct my_page {
     union {
       struct { int mapping; };
     } u;
   };

So that I can reference it using "my_page.u".

Just to be clear, I am not trying to object to the idea of enhancing
GDB for such situations, I'm just trying to understand whether these
situations correspond to something actually useful. I've seen this
being used in the past, when some fields were inserted as a way of
inserting some padding. I don't think it's really essential in that
case to show the actual description of the padding, or maybe it is?

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Daniel Jacobowitz]

On Tue, Aug 28, 2007 at 09:36:33PM -0700, Joel Brobecker wrote:
> > > Does this case actually make any sense? I don't think you can access
> > > this union in your C program either, or can you?
> > 
> > GCC supports anonymous unions.  There's a test for it in the GDB
> > testsuite, too.
> 
> But how can you access this union if the field doesn't have a name?
> More precisely, using the example that Carlos sent:
> 
>    struct my_page {
>      union {
>        struct { int mapping; };
>      };
>    };
> 
> How do you access the union?

In C, it would be "my_page_var.mapping".  The union is transparent.
I think GDB has some failures for anon-union.exp with recent GCC
related to this...

More interesting example.  First you have:

struct s {
  int x;
  int y;
};

Later you switch to:

struct s {
  int x;
  union {
    int y;
    void *z;
  };
};

The anonymous union lets s.y keep working.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

> In C, it would be "my_page_var.mapping".  The union is transparent.

Oh, I see. (love to learn new things :).

Back to the issue at hand, it therefore seems pretty desirable to
enhance the debugger in that case. Unfortunately, I don't think
the proposed patch is correct, because it bases the logic on the
size of the union/struct instead of whether the field is anonymous
or not. Do you agree?

Just for easier reference, the patch was:

2007-08-16   Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>

        gdb/c-typeprint.c (c_type_print_base): check value of
        TYPE_NFIELDS (type) when displaying members of an
        unnamed struct inside an union.

This patch breaks the tests that I just posted for RFA:
http://www.sourceware.org/ml/gdb-patches/2007-08/msg00498.html
But otherwise is reported to not cause any other regression on
the testsuite.

-- 
Joel

[-- Attachment #2: ptype_issue.diff --]
[-- Type: text/plain, Size: 715 bytes --]

2007-08-16   Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>

	gdb/c-typeprint.c (c_type_print_base): check value of 
	TYPE_NFIELDS (type) when displaying members of an
	unnamed struct inside an union.

Index: src-git/gdb/c-typeprint.c
===================================================================
--- src-git.orig/gdb/c-typeprint.c	2007-08-16 15:28:04.000000000 -0700
+++ src-git/gdb/c-typeprint.c	2007-08-16 15:29:07.000000000 -0700
@@ -735,7 +735,7 @@
 	    fputs_filtered (" ", stream);
 	}
       wrap_here ("    ");
-      if (show < 0)
+      if ((show < 0) && (TYPE_NFIELDS (type) == 0))
 	{
 	  /* If we just printed a tag name, no need to print anything else.  */
 	  if (TYPE_TAG_NAME (type) == NULL)

Re: [patch] ptype: show members of an unnamed struct inside an union

[Daniel Jacobowitz]

On Wed, Aug 29, 2007 at 11:32:15AM -0700, Joel Brobecker wrote:
> Back to the issue at hand, it therefore seems pretty desirable to
> enhance the debugger in that case. Unfortunately, I don't think
> the proposed patch is correct, because it bases the logic on the
> size of the union/struct instead of whether the field is anonymous
> or not. Do you agree?

Right.  I would be happy to treat anonymous unions specially here.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

> > Back to the issue at hand, it therefore seems pretty desirable to
> > enhance the debugger in that case. Unfortunately, I don't think
> > the proposed patch is correct, because it bases the logic on the
> > size of the union/struct instead of whether the field is anonymous
> > or not. Do you agree?
> 
> Right.  I would be happy to treat anonymous unions specially here.

Cool!  Carlos, would you be able to look into that? I promise to
review any patch promptly.

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Carlos Eduardo Seo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


>
> Cool!  Carlos, would you be able to look into that? I promise to
> review any patch promptly.
>
Sure, will work on that.

- --
Carlos Eduardo Seo
Software Engineer
IBM Linux Technology Center
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG1b/Zqvq7Aov/qQARAooVAJ4mBevQHd1UofAO5Zb4RAWAtyg1vQCgh4wd
bSTzNb0+/lJXsnxnlVvbn4E=
=Hkqg
-----END PGP SIGNATURE-----

Re: [patch] ptype: show members of an unnamed struct inside an union

[Carlos Eduardo Seo]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


>
> Cool!  Carlos, would you be able to look into that? I promise to
> review any patch promptly.
>
Joel

I believe this simple fix addresses the issue. It also fixes one thing
that was wrong with the previous patch: GDB was showing "{...}" when a
struct had no members, instead of <no data fields>.

What do you think?

- --
Carlos Eduardo Seo
Software Engineer
IBM Linux Technology Center
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG1dOuqvq7Aov/qQARAuiXAJ9l6SplkC3rw0yWirpfykMUIeozKQCdE+Nz
ozaauc2l8paWiDR/t3NUCtQ=
=egcI
-----END PGP SIGNATURE-----


[-- Attachment #2: ptype_unnamed.diff --]
[-- Type: text/plain, Size: 717 bytes --]

2007-08-16   Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>

        gdb/c-typeprint.c (c_type_print_base): allows GDB to
	show members of unnamed struct/union that couldn't be
	displayed otherwise using ptype.

Index: src/gdb/c-typeprint.c
===================================================================
--- src.orig/gdb/c-typeprint.c	2007-08-29 12:36:09.000000000 -0700
+++ src/gdb/c-typeprint.c	2007-08-29 12:57:51.000000000 -0700
@@ -733,7 +733,7 @@
 	    fputs_filtered (" ", stream);
 	}
       wrap_here ("    ");
-      if (show < 0)
+      if ((show < 0) && (TYPE_TAG_NAME (type) != NULL))
 	{
 	  /* If we just printed a tag name, no need to print anything else.  */
 	  if (TYPE_TAG_NAME (type) == NULL)

[-- Attachment #3: ptype_unnamed.diff.sig --]
[-- Type: application/octet-stream, Size: 65 bytes --]

Re: [patch] ptype: show members of an unnamed struct inside an union

[Daniel Jacobowitz]

On Wed, Aug 29, 2007 at 05:14:38PM -0300, Carlos Eduardo Seo wrote:
> I believe this simple fix addresses the issue. It also fixes one thing
> that was wrong with the previous patch: GDB was showing "{...}" when a
> struct had no members, instead of <no data fields>.
> 
> What do you think?

This isn't the same as what we were talking about.  This is when the
type has no name; an anonymous union is when the field has no name.

This might be a good idea too - I'm not decided on that...


-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] ptype: show members of an unnamed struct inside an union

[Carlos Eduardo Seo]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


>
> This isn't the same as what we were talking about.  This is when
> the type has no name; an anonymous union is when the field has no
> name.
>
> This might be a good idea too - I'm not decided on that...
>
IMHO, I think this is more informative to the user. But that's just my
$0.02... :)

- --
Carlos Eduardo Seo
Software Engineer
IBM Linux Technology Center
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG1dZ0qvq7Aov/qQARAq0KAJ0dNA8YT7AEanIIDocmS424UUAg8ACbBKK8
IL/S0K3lLWEIOUO7tQ/Qf+w=
=CHHg
-----END PGP SIGNATURE-----

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

> > This isn't the same as what we were talking about.  This is when
> > the type has no name; an anonymous union is when the field has no
> > name.
> >
> > This might be a good idea too - I'm not decided on that...
> >
> IMHO, I think this is more informative to the user. But that's just my
> $0.02... :)

I agree with Daniel. Doing what you propose is partly defeating what
the current code is doing, only because the type is anonymous. In
the case of large structs which include other structs which include
other structs, this makes a big difference in readability.

The one reason I think it's a good idea to expand the field description
when the field does not have a name, is because there is no way to
get that information otherwise. But in the case of a named field that
has an anonymous type, we can easily get the type information simply
by selecting it (I mean by that: "ptype my_page.u" where u is the field
name of your union).

One last comment: The change you made is affecting both structs and
unions. Is that what we want? So far, the discussion was only geared
towards unions.

-- 
Joel

Re: [patch] ptype: show members of an unnamed struct inside an union

[Daniel Jacobowitz]

On Wed, Aug 29, 2007 at 01:41:14PM -0700, Joel Brobecker wrote:
> The one reason I think it's a good idea to expand the field description
> when the field does not have a name, is because there is no way to
> get that information otherwise. But in the case of a named field that
> has an anonymous type, we can easily get the type information simply
> by selecting it (I mean by that: "ptype my_page.u" where u is the field
> name of your union).

That's not quite true.  You need a variable, or to mess around with
casting a pointer; you can't say "ptype my_type.member".

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] ptype: show members of an unnamed struct inside an union

[Joel Brobecker]

> That's not quite true.  You need a variable, or to mess around with
> casting a pointer; you can't say "ptype my_type.member".

I hadn't realized this. I'm so used to doing it when debugging Ada
programs:

   (gdb) ptype rec
   type = record
       a: integer;
       b: integer;
   end record
   (gdb) ptype rec.a
   type = <4-byte integer>

-- 
Joel