diff mbox

[3/3] gdb: Show type summary for anonymous structures from c_print_typedef

Message ID 20190719210636.GD23204@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess July 19, 2019, 9:06 p.m. UTC
* Pedro Alves <palves@redhat.com> [2019-07-19 13:38:35 +0100]:

> On 7/12/19 12:37 PM, Andrew Burgess wrote:
> > Currently each language has a la_print_typedef method, this is only
> > used for the "info types" command.
> > 
> > The documentation for "info types" says:
> > 
> >    Print a brief description of all types whose names match the regular
> >    expression @var{regexp} (or all types in your program, if you supply
> >    no argument).
> > 
> > However, if we consider this C code:
> > 
> >    typedef struct {
> >      int a;
> >    } my_type;
> > 
> > Then currently with "info types" this will be printed like this:
> > 
> >    3:      typedef struct {
> >        int a;
> >    } my_type;
> > 
> > I see two problems with this, first the indentation is clearly broken,
> > second, if the struct contained more fields then the it feels like the
> > actual type names could easily get lost in the noise.
> 
> Something odd in "then the it feels"?
> 
> > 
> > Given that "info types" is about discovering type names, I think there
> > is an argument to be made that we should focus on giving _only_ the
> > briefest summary for "info types", and if the user wants to know more
> > they can take the type name and plug it into "ptype".  As such, I
> > propose that a better output would be:
> > 
> >    3:      typedef struct {...} my_type;
> > 
> 
> I think the same rationale applies to enums too?  We currently
> print anonymous enums like:
> 
>  16:     typedef enum {A, B, C} EEE;
> 
> The difference here is that we don't print newline between
> each enumerator.
> 
> It's as if we printed your struct example like this:
> 
>  3:     typedef struct {int a;} my_type;
> 
> which would be a bit more reasonable than the current output.
> 
> I did the 0 -> -1 change locally, and your patch addresses
> enums as well already:
> 
>  16:     typedef enum {...} EEE;
> 
> But I think you should add that to the testcase.
> 
> > The user understands that there is a type called `my_type`, and that
> > it's an alias for an anonymous structure type.
> 
> It's worth explicitly showing (in the commit log, IMO) that this
> only affects anonymous structs/enums.  For named structs/enums, we do
> print an abbreviated form with no fields/enumerators:
> 
>  11:     struct named_struct;
>  18:     enum named_enum;
> 
> So it makes sense to me to be consistent and use an abbreviated
> form for anonymous types too, like in your patch.

I've extended the test case to cover anonymous enums, and also unions
that I'd completely forgotten to cover at all before (though they are
mostly just handled like structs, so no surprises there).

I've extended the commit message to talk about the difference between
named and anonymous structs/enums, to mention that unions are like
structs, and also to mention the differences between C and C++ (with
C++ being less changes by this patch as its output before was more
compact anyway).

Thanks,
Andrew

--

    gdb: Show type summary for anonymous structures from c_print_typedef
    
    Currently each language has a la_print_typedef method, this is only
    used for the "info types" command.
    
    The documentation for "info types" says:
    
       Print a brief description of all types whose names match the regular
       expression @var{regexp} (or all types in your program, if you supply
       no argument).
    
    However, if we consider this C code:
    
       typedef struct {
         int a;
       } my_type;
    
    Then currently with "info types" this will be printed like this:
    
       3:      typedef struct {
           int a;
       } my_type;
    
    I see two problems with this, first the indentation is clearly broken,
    second, if the struct contained more fields then it feels like the
    actual type names could easily get lost in the noise.
    
    Given that "info types" is about discovering type names, I think there
    is an argument to be made that we should focus on giving _only_ the
    briefest summary for "info types", and if the user wants to know more
    they can take the type name and plug it into "ptype".  As such, I
    propose that a better output would be:
    
       3:      typedef struct {...} my_type;
    
    The user understands that there is a type called `my_type`, and that
    it's an alias for an anonymous structure type.
    
    The change to achieve this turns out to be pretty simple, but only
    effects languages that make use of c_print_typedef, which are C, C++,
    asm, minimal, d, go, objc, and opencl.  Other languages will for now
    do whatever they used to do.
    
    The patch to change how anonymous structs are displayed also changes
    the display of anonymous enums, consider this code sample:
    
       typedef enum {
         AA, BB, CC
       } anon_enum_t;
    
    This used to be displayed like this:
    
       3:      typedef enum {AA, BB, CC} anon_enum_t;
    
    Which will quickly become cluttered for enums with a large number of
    values.  The modified output looks like this:
    
       3:      typedef enum {...} anon_enum_t;
    
    Again, the user can always make use of ptype if they want to see the
    details of the anon_enum_t type.
    
    It is worth pointing out that this change (to use {...}) only effects
    anonymous structs and enums, named types don't change with this patch,
    consider this code:
    
       struct struct_t {
         int i;
       };
       enum enum_t {
        AA, BB, CC
       };
    
    The output from 'info types' remains unchanged, like this:
    
       4:      enum enum_t;
       1:      struct struct_t;
    
    An additional area of interest is how C++ handles anonymous types used
    within a typedef; enums are handled basically inline with how C
    handles them, but structs (and classes) are slightly different.  The
    behaviour before the patch is different, and is unchanged by this
    patch.  Consider this code compiled for C++:
    
       typedef struct {
         int i;
       } struct_t;
    
    Both before and after this patch, this is show by 'info types' as:
    
       3:      typedef struct_t struct_t;
    
    Unions are displayed similarly to structs in both C and C++, the
    handling of anonymous unions changes for C in the same way that
    it changes for anonymous structs.
    
    I did look at ada, as this is the only language to actually have some
    tests for "info types", however, as I understand it ada doesn't really
    support typedefs, however, by forcing the language we can see what ada
    would print.  So, if we 'set language ada', then originally we printed
    this:
    
       3:      record
           a: int;
       end record
    
    Again the indentation is clearly broken, but we also have no mention
    of the type name at all, which is odd, but understandable given the
    lack of typedefs.  If I make a similar change as I'm proposing for C,
    then we now get this output:
    
       3:      record ... end record
    
    Which is even less informative I think.  However, the original output
    _is_ tested for in gdb.ada/info_auto_lang.exp, and its not clear to me
    if the change is a good one or not, so for now I have left this out.
    
    gdb/ChangeLog:
    
            * c-typeprint.c (c_print_typedef): Pass -1 instead of 0 to
            type_print.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.ada/info_auto_lang.exp: Update expected results.
            * gdb.base/info-types.c: Add additional types to check.
            * gdb.base/info-types.exp: Update expected results.

Comments

Pedro Alves July 20, 2019, 1:47 p.m. UTC | #1
On 7/19/19 10:06 PM, Andrew Burgess wrote:

>     An additional area of interest is how C++ handles anonymous types used
>     within a typedef; enums are handled basically inline with how C
>     handles them, but structs (and classes) are slightly different.  The
>     behaviour before the patch is different, and is unchanged by this
>     patch.  Consider this code compiled for C++:
>     
>        typedef struct {
>          int i;
>        } struct_t;
>     
>     Both before and after this patch, this is show by 'info types' as:
>     
>        3:      typedef struct_t struct_t;

Curious, and odd.

>     gdb/ChangeLog:
>     
>             * c-typeprint.c (c_print_typedef): Pass -1 instead of 0 to
>             type_print.
>     
>     gdb/testsuite/ChangeLog:
>     
>             * gdb.ada/info_auto_lang.exp: Update expected results.
>             * gdb.base/info-types.c: Add additional types to check.
>             * gdb.base/info-types.exp: Update expected results.

LGTM.

Thanks,
Pedro Alves
Tom Tromey July 22, 2019, 2:10 p.m. UTC | #2
>> typedef struct {
>> int i;
>> } struct_t;
>> 
>> Both before and after this patch, this is show by 'info types' as:
>> 
>> 3:      typedef struct_t struct_t;

Pedro> Curious, and odd.

There's a weird special case in C++ where this sort of typedef provides
the linkage name for an otherwise anonymous structure, and ages ago we
needed to work around this.

See

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48603#c5

In the DWARF this looks like:

 <1><1d>: Abbrev Number: 2 (DW_TAG_structure_type)
    <1e>   DW_AT_byte_size   : 4
    <1f>   DW_AT_decl_file   : 1
    <20>   DW_AT_decl_line   : 1
    <21>   DW_AT_decl_column : 16
    <22>   DW_AT_linkage_name: (indirect string, offset: 0x0): 8struct_t
    <26>   DW_AT_sibling     : <0x36>

... in particular the linkage name is picked up by gdb.

Search for "47510" in dwar2read.c.  There are a few spots.

Tom
diff mbox

Patch

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 6690ca53bcd..43ad3b3e0e6 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -205,7 +205,7 @@  c_print_typedef (struct type *type,
 {
   type = check_typedef (type);
   fprintf_filtered (stream, "typedef ");
-  type_print (type, "", stream, 0);
+  type_print (type, "", stream, -1);
   if (TYPE_NAME ((SYMBOL_TYPE (new_symbol))) == 0
       || strcmp (TYPE_NAME ((SYMBOL_TYPE (new_symbol))),
 		 SYMBOL_LINKAGE_NAME (new_symbol)) != 0
diff --git a/gdb/testsuite/gdb.ada/info_auto_lang.exp b/gdb/testsuite/gdb.ada/info_auto_lang.exp
index be1deae99ef..68457827d2f 100644
--- a/gdb/testsuite/gdb.ada/info_auto_lang.exp
+++ b/gdb/testsuite/gdb.ada/info_auto_lang.exp
@@ -53,10 +53,7 @@  set func_in_c(ada_syntax)    "${decimal}:	procedure proc_in_c;"
 set func_in_ada(c_syntax)    "${decimal}:	void proc_in_ada\\\(void\\\);"
 set func_in_ada(ada_syntax)  "${decimal}:	procedure proc_in_ada;"
 
-set type_in_c(c_syntax) [multi_line \
-			    "${decimal}:	typedef struct {" \
-			    "    int some_component_in_c;" \
-			    "} some_type_in_c;" ]
+set type_in_c(c_syntax) "${decimal}:	typedef struct {\\.\\.\\.} some_type_in_c;"
 set type_in_c(ada_syntax) [multi_line \
 			      "${decimal}:	record" \
 			      "    some_component_in_c: int;" \
diff --git a/gdb/testsuite/gdb.base/info-types.c b/gdb/testsuite/gdb.base/info-types.c
index 6746238801d..de9320da888 100644
--- a/gdb/testsuite/gdb.base/info-types.c
+++ b/gdb/testsuite/gdb.base/info-types.c
@@ -38,6 +38,37 @@  enum enum_t
 typedef enum enum_t my_enum_t;
 typedef my_enum_t nested_enum_t;
 
+typedef struct
+{
+  double d;
+  float f;
+} anon_struct_t;
+
+typedef anon_struct_t nested_anon_struct_t;
+
+typedef enum
+{
+ DD, EE, FF
+} anon_enum_t;
+
+typedef anon_enum_t nested_anon_enum_t;
+
+union union_t
+{
+  int i;
+  float f;
+};
+
+typedef union union_t nested_union_t;
+
+typedef union
+{
+  int i;
+  double d;
+} anon_union_t;
+
+typedef anon_union_t nested_anon_union_t;
+
 volatile int var_a;
 volatile float var_b;
 volatile my_int_t var_c;
@@ -53,6 +84,14 @@  volatile baz_ptr var_l;
 volatile enum enum_t var_m;
 volatile my_enum_t var_n;
 volatile nested_enum_t var_o;
+volatile anon_struct_t var_p;
+volatile nested_anon_struct_t var_q;
+volatile anon_enum_t var_r;
+volatile nested_anon_enum_t var_s;
+volatile union union_t var_t;
+volatile nested_union_t var_u;
+volatile anon_union_t var_v;
+volatile nested_anon_union_t var_w;
 
 #ifdef __cplusplus
 
diff --git a/gdb/testsuite/gdb.base/info-types.exp b/gdb/testsuite/gdb.base/info-types.exp
index a5808425398..3a514b5bc19 100644
--- a/gdb/testsuite/gdb.base/info-types.exp
+++ b/gdb/testsuite/gdb.base/info-types.exp
@@ -57,25 +57,35 @@  proc run_test { lang } {
 		 "All defined types:" \
 		 "" \
 		 "File .*:" \
-		 "59:\[\t \]+CL;" \
+		 "98:\[\t \]+CL;" \
+		 "42:\[\t \]+anon_struct_t;" \
+		 "65:\[\t \]+anon_union_t;" \
 		 "21:\[\t \]+baz_t;" \
 		 "33:\[\t \]+enum_t;" \
+		 "56:\[\t \]+union_t;" \
+		 "52:\[\t \]+typedef enum {\\.\\.\\.} anon_enum_t;" \
+		 "45:\[\t \]+typedef anon_struct_t anon_struct_t;" \
+		 "68:\[\t \]+typedef anon_union_t anon_union_t;" \
 		 "28:\[\t \]+typedef baz_t baz;" \
 		 "31:\[\t \]+typedef baz_t \\* baz_ptr;" \
 		 "27:\[\t \]+typedef baz_t baz_t;" \
 		 "\[\t \]+double" \
 		 "\[\t \]+float" \
 		 "\[\t \]+int" \
-		 "64:\[\t \]+typedef CL my_cl;" \
+		 "103:\[\t \]+typedef CL my_cl;" \
 		 "38:\[\t \]+typedef enum_t my_enum_t;" \
 		 "17:\[\t \]+typedef float my_float_t;" \
 		 "16:\[\t \]+typedef int my_int_t;" \
-		 "65:\[\t \]+typedef CL \\* my_ptr;" \
+		 "104:\[\t \]+typedef CL \\* my_ptr;" \
+		 "54:\[\t \]+typedef enum {\\.\\.\\.} nested_anon_enum_t;" \
+		 "47:\[\t \]+typedef anon_struct_t nested_anon_struct_t;" \
+		 "70:\[\t \]+typedef anon_union_t nested_anon_union_t;" \
 		 "30:\[\t \]+typedef baz_t nested_baz;" \
 		 "29:\[\t \]+typedef baz_t nested_baz_t;" \
 		 "39:\[\t \]+typedef enum_t nested_enum_t;" \
 		 "19:\[\t \]+typedef float nested_float_t;" \
 		 "18:\[\t \]+typedef int nested_int_t;" \
+		 "62:\[\t \]+typedef union_t nested_union_t;" \
 		 "\[\t \]+unsigned int"]
     } else {
 	set output_re \
@@ -83,6 +93,9 @@  proc run_test { lang } {
 		 "All defined types:" \
 		 "" \
 		 "File .*:" \
+		 "52:\[\t \]+typedef enum {\\.\\.\\.} anon_enum_t;" \
+		 "45:\[\t \]+typedef struct {\\.\\.\\.} anon_struct_t;" \
+		 "68:\[\t \]+typedef union {\\.\\.\\.} anon_union_t;" \
 		 "28:\[\t \]+typedef struct baz_t baz;" \
 		 "31:\[\t \]+typedef struct baz_t \\* baz_ptr;" \
 		 "21:\[\t \]+struct baz_t;" \
@@ -93,11 +106,16 @@  proc run_test { lang } {
 		 "38:\[\t \]+typedef enum enum_t my_enum_t;" \
 		 "17:\[\t \]+typedef float my_float_t;" \
 		 "16:\[\t \]+typedef int my_int_t;" \
+		 "54:\[\t \]+typedef enum {\\.\\.\\.} nested_anon_enum_t;" \
+		 "47:\[\t \]+typedef struct {\\.\\.\\.} nested_anon_struct_t;" \
+		 "70:\[\t \]+typedef union {\\.\\.\\.} nested_anon_union_t;" \
 		 "30:\[\t \]+typedef struct baz_t nested_baz;" \
 		 "29:\[\t \]+typedef struct baz_t nested_baz_t;" \
 		 "39:\[\t \]+typedef enum enum_t nested_enum_t;" \
 		 "19:\[\t \]+typedef float nested_float_t;" \
 		 "18:\[\t \]+typedef int nested_int_t;" \
+		 "62:\[\t \]+typedef union union_t nested_union_t;" \
+		 "56:\[\t \]+union union_t;" \
 		 "\[\t \]+unsigned int" ]
     }