Bug with lists in tables in ui-out.c

Bug with lists in tables in ui-out.c

[Jim Ingham]

Hi all,

Turns out if you have an element of a table that is a list or tuple, then
the current ui-out table code chokes.  verify_field_alignment doesn't know
that each of the elements of the sublist are not separate table elements, so
it throws an error at the first one it sees.  The following patch fixes this
bug.  

Note that in the FSF gdb code, no one ever uses this feature, because the
only client of the ui_out_table is the breakpoint printer, and it cleverly
only declares the first 5 or 6 columns of the breakpoint info table in the
table header, and sticks on the rest of the info as undeclared freebies
(which is kind of lame, but another story...), so the commands, which are a
list, don't get added to the table proper, and thus don't trip the bug.

I have a patch which will fix this and make the table header accurate, but I
am not submitting it here because (a) it is a separate issue and (b) it will
change the output of the "info breakpoint" command (though only the header),
and so may not necessarily please everyone...

Jim

Index: ui-out.c
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.c,v
retrieving revision 1.18
diff -p -r1.18 ui-out.c
*** ui-out.c    2001/07/06 03:53:11     1.18
--- ui-out.c    2001/11/29 01:40:59
*************** ui_out_end (struct ui_out *uiout,
*** 367,372 ****
--- 367,384 ----
            enum ui_out_type type)
  {
    int old_level = pop_level (uiout, type);
+ 
+   /* If we are building up a table, and we were making a table element
+      which was a list or tuple, then when we end the list, we have to
+      increment the header. Watch out, however, the current breakpoint
+      code actually emits more columns than it defines in the header
+      so we have to be careful not to walk off the end. */
+ 
+   if (uiout->table_flag && uiout->body_flag
+       && uiout->level == 1
+       && uiout->headercurr != NULL)
+     uiout->headercurr = uiout->headercurr->next;
+   
    uo_end (uiout, type, old_level);
  }
  
*************** append_header_to_list (struct ui_out *ui
*** 1018,1024 ****
    uiout->headercurr = uiout->headerlast;
  }
  
! /* returns 0 if there is no more headers */
  
  static int
  get_curr_header (struct ui_out *uiout,
--- 1030,1038 ----
    uiout->headercurr = uiout->headerlast;
  }
  
! /* Returns information on the current header.  ALSO Increments the
!    current header in UI_OUT.  Returns 0 if there is no more headers, 1
!    otherwise. */
  
  static int
  get_curr_header (struct ui_out *uiout,
*************** specified after table_body and inside a
*** 1056,1062 ****
      }
  }
  
! /* determines what is the alignment policy */
  
  static void
  verify_field_alignment (struct ui_out *uiout,
--- 1070,1079 ----
      }
  }
  
! /* Determines what is the alignment policy.  NB: as a side-effect of
!  calling get_curr_header, this also increments the current header.  So
!  you have to call verify_field_alignment in any call that adds a
!  complete bit of data to a table column. */
  
  static void
  verify_field_alignment (struct ui_out *uiout,
*************** verify_field_alignment (struct ui_out *u
*** 1066,1073 ****
  {
    int colno;
    char *text;
  
!   if (uiout->table_flag
        && get_curr_header (uiout, &colno, width, align, &text))
      {
        if (fldno != colno)
--- 1083,1097 ----
  {
    int colno;
    char *text;
+ 
+   /* Be careful - if you are building up a list or tuple AS AN ELEMENT
+      of a table, you don't want to call get_curr_header, since that will
+      increment the current header, which would be wrong. Currently, tables
+      can only be built at level 1, so I am using that as a check.  If this
+      stuff is ever generalized, we will need to stuff the level of the
+      current table into the (putative) ui_table structure. */
  
!   if (uiout->table_flag && uiout->level == 1
        && get_curr_header (uiout, &colno, width, align, &text))
      {
        if (fldno != colno)

-- 
+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==
Jim Ingham                              jingham@apple.com
Developer Tools - gdb

Re: Bug with lists in tables in ui-out.c

[Eli Zaretskii]

On Wed, 28 Nov 2001, Jim Ingham wrote:

> Turns out if you have an element of a table that is a list or tuple, then
> the current ui-out table code chokes.  verify_field_alignment doesn't know
> that each of the elements of the sublist are not separate table elements, so
> it throws an error at the first one it sees.  The following patch fixes this
> bug.  

Jim, could you please skim over the documentation of UI_OUT functions
in gdbint.texinfo and see if anything there needs to be updated due to
this patch?  For example, perhaps the warnings you put in comments
should be repeated there.

Thanks.

Bug with lists in tables in ui-out.c

[Jim Ingham]

Hi all,

Turns out if you have an element of a table that is a list or tuple, then
the current ui-out table code chokes.  verify_field_alignment doesn't know
that each of the elements of the sublist are not separate table elements, so
it throws an error at the first one it sees.  The following patch fixes this
bug.  

Note that in the FSF gdb code, no one ever uses this feature, because the
only client of the ui_out_table is the breakpoint printer, and it cleverly
only declares the first 5 or 6 columns of the breakpoint info table in the
table header, and sticks on the rest of the info as undeclared freebies
(which is kind of lame, but another story...), so the commands, which are a
list, don't get added to the table proper, and thus don't trip the bug.

I have a patch which will fix this and make the table header accurate, but I
am not submitting it here because (a) it is a separate issue and (b) it will
change the output of the "info breakpoint" command (though only the header),
and so may not necessarily please everyone...

Jim

Index: ui-out.c
===================================================================
RCS file: /cvs/src/src/gdb/ui-out.c,v
retrieving revision 1.18
diff -p -r1.18 ui-out.c
*** ui-out.c    2001/07/06 03:53:11     1.18
--- ui-out.c    2001/11/29 01:40:59
*************** ui_out_end (struct ui_out *uiout,
*** 367,372 ****
--- 367,384 ----
            enum ui_out_type type)
  {
    int old_level = pop_level (uiout, type);
+ 
+   /* If we are building up a table, and we were making a table element
+      which was a list or tuple, then when we end the list, we have to
+      increment the header. Watch out, however, the current breakpoint
+      code actually emits more columns than it defines in the header
+      so we have to be careful not to walk off the end. */
+ 
+   if (uiout->table_flag && uiout->body_flag
+       && uiout->level == 1
+       && uiout->headercurr != NULL)
+     uiout->headercurr = uiout->headercurr->next;
+   
    uo_end (uiout, type, old_level);
  }
  
*************** append_header_to_list (struct ui_out *ui
*** 1018,1024 ****
    uiout->headercurr = uiout->headerlast;
  }
  
! /* returns 0 if there is no more headers */
  
  static int
  get_curr_header (struct ui_out *uiout,
--- 1030,1038 ----
    uiout->headercurr = uiout->headerlast;
  }
  
! /* Returns information on the current header.  ALSO Increments the
!    current header in UI_OUT.  Returns 0 if there is no more headers, 1
!    otherwise. */
  
  static int
  get_curr_header (struct ui_out *uiout,
*************** specified after table_body and inside a
*** 1056,1062 ****
      }
  }
  
! /* determines what is the alignment policy */
  
  static void
  verify_field_alignment (struct ui_out *uiout,
--- 1070,1079 ----
      }
  }
  
! /* Determines what is the alignment policy.  NB: as a side-effect of
!  calling get_curr_header, this also increments the current header.  So
!  you have to call verify_field_alignment in any call that adds a
!  complete bit of data to a table column. */
  
  static void
  verify_field_alignment (struct ui_out *uiout,
*************** verify_field_alignment (struct ui_out *u
*** 1066,1073 ****
  {
    int colno;
    char *text;
  
!   if (uiout->table_flag
        && get_curr_header (uiout, &colno, width, align, &text))
      {
        if (fldno != colno)
--- 1083,1097 ----
  {
    int colno;
    char *text;
+ 
+   /* Be careful - if you are building up a list or tuple AS AN ELEMENT
+      of a table, you don't want to call get_curr_header, since that will
+      increment the current header, which would be wrong. Currently, tables
+      can only be built at level 1, so I am using that as a check.  If this
+      stuff is ever generalized, we will need to stuff the level of the
+      current table into the (putative) ui_table structure. */
  
!   if (uiout->table_flag && uiout->level == 1
        && get_curr_header (uiout, &colno, width, align, &text))
      {
        if (fldno != colno)

-- 
+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==
Jim Ingham                              jingham@apple.com
Developer Tools - gdb

Re: Bug with lists in tables in ui-out.c

[Eli Zaretskii]

On Wed, 28 Nov 2001, Jim Ingham wrote:

> Turns out if you have an element of a table that is a list or tuple, then
> the current ui-out table code chokes.  verify_field_alignment doesn't know
> that each of the elements of the sublist are not separate table elements, so
> it throws an error at the first one it sees.  The following patch fixes this
> bug.  

Jim, could you please skim over the documentation of UI_OUT functions
in gdbint.texinfo and see if anything there needs to be updated due to
this patch?  For example, perhaps the warnings you put in comments
should be repeated there.

Thanks.

Re: Bug with lists in tables in ui-out.c

[Jim Ingham]

On 11/29/01 12:20 AM, "Eli Zaretskii" <eliz@is.elta.co.il> wrote:

> 
> On Wed, 28 Nov 2001, Jim Ingham wrote:
> 
>> Turns out if you have an element of a table that is a list or tuple, then
>> the current ui-out table code chokes.  verify_field_alignment doesn't know
>> that each of the elements of the sublist are not separate table elements, so
>> it throws an error at the first one it sees.  The following patch fixes this
>> bug.  
> 
> Jim, could you please skim over the documentation of UI_OUT functions
> in gdbint.texinfo and see if anything there needs to be updated due to
> this patch?  For example, perhaps the warnings you put in comments
> should be repeated there.
> 
> Thanks.
> 

Eli,

I think the only thing that needs to be added is:

Index: gdbint.texinfo
===================================================================
RCS file: /cvs/Darwin/Commands/GNU/cygnus/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.12
diff -c -w -r1.12 gdbint.texinfo
*** gdbint.texinfo      2001/11/27 17:34:09     1.12
--- gdbint.texinfo      2001/12/03 21:46:39
***************
*** 860,867 ****
  even when you are listing just one object but you still want the header.
  
  @cindex nesting level in @code{ui_out} functions
! Tables can not be nested.  Tuples and lists can be nested up to a
! maximum of five levels.
  
  The overall structure of the table output code is something like this:
  
--- 860,867 ----
  even when you are listing just one object but you still want the header.
  
  @cindex nesting level in @code{ui_out} functions
! Tables can not be nested, nor can a tuple or list element be a table.
! Tuples and lists can be nested up to a maximum of five levels.
  
  The overall structure of the table output code is something like this:
  

The other warnings are for writers of ui_out functions, not for their users,
and as such are not really appropriate in the gdbint doc, I think.  With the
suggested patch, no behaviors change, things will now just work as expected.

Jim 
-- 
+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==
Jim Ingham                              jingham@apple.com
Developer Tools - gdb

Re: Bug with lists in tables in ui-out.c

[Eli Zaretskii]

On Mon, 3 Dec 2001, Jim Ingham wrote:

> I think the only thing that needs to be added is:
> 
> Index: gdbint.texinfo
> ===================================================================
> RCS file: /cvs/Darwin/Commands/GNU/cygnus/src/gdb/doc/gdbint.texinfo,v
> retrieving revision 1.12
> diff -c -w -r1.12 gdbint.texinfo
> *** gdbint.texinfo      2001/11/27 17:34:09     1.12
> --- gdbint.texinfo      2001/12/03 21:46:39
> ***************
> *** 860,867 ****
>   even when you are listing just one object but you still want the header.
>   
>   @cindex nesting level in @code{ui_out} functions
> ! Tables can not be nested.  Tuples and lists can be nested up to a
> ! maximum of five levels.
>   
>   The overall structure of the table output code is something like this:
>   
> --- 860,867 ----
>   even when you are listing just one object but you still want the header.
>   
>   @cindex nesting level in @code{ui_out} functions
> ! Tables can not be nested, nor can a tuple or list element be a table.
> ! Tuples and lists can be nested up to a maximum of five levels.
>   

Thanks.  Please commit this change to the docs.

Re: Bug with lists in tables in ui-out.c

[Andrew Cagney]

> Hi all,

This code my head hurt.

> Turns out if you have an element of a table that is a list or tuple, then
> the current ui-out table code chokes.  verify_field_alignment doesn't know
> that each of the elements of the sublist are not separate table elements, so
> it throws an error at the first one it sees.  The following patch fixes this
> bug.  

Can this be fixed in a simpler cleaner way by just moving the table 
stuff into `struct ui_out_level'?

When I was last working on this code I very nearly did just this change :-/

enjoy,
Andrew

> Note that in the FSF gdb code, no one ever uses this feature, because the
> only client of the ui_out_table is the breakpoint printer, and it cleverly
> only declares the first 5 or 6 columns of the breakpoint info table in the
> table header, and sticks on the rest of the info as undeclared freebies
> (which is kind of lame, but another story...), so the commands, which are a
> list, don't get added to the table proper, and thus don't trip the bug.
> 
> I have a patch which will fix this and make the table header accurate, but I
> am not submitting it here because (a) it is a separate issue and (b) it will
> change the output of the "info breakpoint" command (though only the header),
> and so may not necessarily please everyone...
> 
> Jim
> 
> Index: ui-out.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/ui-out.c,v
> retrieving revision 1.18
> diff -p -r1.18 ui-out.c
> *** ui-out.c    2001/07/06 03:53:11     1.18
> --- ui-out.c    2001/11/29 01:40:59
> *************** ui_out_end (struct ui_out *uiout,
> *** 367,372 ****
> --- 367,384 ----
>             enum ui_out_type type)
>   {
>     int old_level = pop_level (uiout, type);
> + 
> +   /* If we are building up a table, and we were making a table element
> +      which was a list or tuple, then when we end the list, we have to
> +      increment the header. Watch out, however, the current breakpoint
> +      code actually emits more columns than it defines in the header
> +      so we have to be careful not to walk off the end. */
> + 
> +   if (uiout->table_flag && uiout->body_flag
> +       && uiout->level == 1
> +       && uiout->headercurr != NULL)
> +     uiout->headercurr = uiout->headercurr->next;
> +   
>     uo_end (uiout, type, old_level);
>   }
>   
> *************** append_header_to_list (struct ui_out *ui
> *** 1018,1024 ****
>     uiout->headercurr = uiout->headerlast;
>   }
>   
> ! /* returns 0 if there is no more headers */
>   
>   static int
>   get_curr_header (struct ui_out *uiout,
> --- 1030,1038 ----
>     uiout->headercurr = uiout->headerlast;
>   }
>   
> ! /* Returns information on the current header.  ALSO Increments the
> !    current header in UI_OUT.  Returns 0 if there is no more headers, 1
> !    otherwise. */
>   
>   static int
>   get_curr_header (struct ui_out *uiout,
> *************** specified after table_body and inside a
> *** 1056,1062 ****
>       }
>   }
>   
> ! /* determines what is the alignment policy */
>   
>   static void
>   verify_field_alignment (struct ui_out *uiout,
> --- 1070,1079 ----
>       }
>   }
>   
> ! /* Determines what is the alignment policy.  NB: as a side-effect of
> !  calling get_curr_header, this also increments the current header.  So
> !  you have to call verify_field_alignment in any call that adds a
> !  complete bit of data to a table column. */
>   
>   static void
>   verify_field_alignment (struct ui_out *uiout,
> *************** verify_field_alignment (struct ui_out *u
> *** 1066,1073 ****
>   {
>     int colno;
>     char *text;
>   
> !   if (uiout->table_flag
>         && get_curr_header (uiout, &colno, width, align, &text))
>       {
>         if (fldno != colno)
> --- 1083,1097 ----
>   {
>     int colno;
>     char *text;
> + 
> +   /* Be careful - if you are building up a list or tuple AS AN ELEMENT
> +      of a table, you don't want to call get_curr_header, since that will
> +      increment the current header, which would be wrong. Currently, tables
> +      can only be built at level 1, so I am using that as a check.  If this
> +      stuff is ever generalized, we will need to stuff the level of the
> +      current table into the (putative) ui_table structure. */
>   
> !   if (uiout->table_flag && uiout->level == 1
>         && get_curr_header (uiout, &colno, width, align, &text))
>       {
>         if (fldno != colno)
> 
> 

Re: Bug with lists in tables in ui-out.c

[Andrew Cagney]

> Turns out if you have an element of a table that is a list or tuple, then
> the current ui-out table code chokes.  verify_field_alignment doesn't know
> that each of the elements of the sublist are not separate table elements, so
> it throws an error at the first one it sees.  The following patch fixes this
> bug. 

Thinking about this some more, I suspect you're constructing something like:

	table={hdr={...},body=[{{<THIS>}, ...},...}]}

or as a code sequence:

	..._body()
	  ui_out_tuple_begin(); /*row*/
	    ui_out_tupple_begin(); /* element - non-standard */
	      ui_out_field (...<THIS>..)

I'm wondering if a table is even applicable in this case.  How exactly 
should the header formatting information, for instance, be interpreted 
when there is more than one entry.

Would you have an example?

	Andrew

Re: Bug with lists in tables in ui-out.c

[Jim Ingham]

On 12/4/01 11:36 PM, "Andrew Cagney" <ac131313@cygnus.com> wrote:

>> Turns out if you have an element of a table that is a list or tuple, then
>> the current ui-out table code chokes.  verify_field_alignment doesn't know
>> that each of the elements of the sublist are not separate table elements, so
>> it throws an error at the first one it sees.  The following patch fixes this
>> bug. 
> 
> Thinking about this some more, I suspect you're constructing something like:
> 
> table={hdr={...},body=[{{<THIS>}, ...},...}]}
> 
> or as a code sequence:
> 
> ..._body()
>  ui_out_tuple_begin(); /*row*/
>    ui_out_tupple_begin(); /* element - non-standard */
>      ui_out_field (...<THIS>..)
> 
> I'm wondering if a table is even applicable in this case.  How exactly
> should the header formatting information, for instance, be interpreted
> when there is more than one entry.
> 
> Would you have an example?
> 
> Andrew
> 
> 
> 

Andrew,

The example is the table that "info break" makes.  In FSF gdb, you only emit
table headers for the first 6 columns, and if there are commands, they are
added to the bkpt tuple (and so formally should be other columns in the
table), but there is no equivalent entry in the header list.  The table code
doesn't require that all data in a table row have an equivalent header
entry, so this works, but this is just a bit bogus...  Somebody here got
annoyed by this and added table headers for the other columns - mostly so
users could see what the columns mean.  In this case, the list that is being
added to the table is the list of breakpoint commands.

Jim
-- 
+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==+==
Jim Ingham                              jingham@apple.com
Developer Tools - gdb

Re: Bug with lists in tables in ui-out.c

[Andrew Cagney]

> Andrew,
> 
> The example is the table that "info break" makes.  In FSF gdb, you only emit
> table headers for the first 6 columns, and if there are commands, they are
> added to the bkpt tuple (and so formally should be other columns in the
> table), but there is no equivalent entry in the header list.  The table code
> doesn't require that all data in a table row have an equivalent header
> entry, so this works, but this is just a bit bogus...  Somebody here got
> annoyed by this and added table headers for the other columns - mostly so
> users could see what the columns mean.  In this case, the list that is being
> added to the table is the list of breakpoint commands.

Ah, yes. Ok, got it (head butts table).  I think I'll check it in 
slightly different though - add the code that advances the headings to 
the ..._begin() instead of ..._end() function.  I think that will sit 
better with the other logic already handling tables.

Make sense?
Andrew

Re: Bug with lists in tables in ui-out.c

[Jim Ingham]

On 12/6/01 3:06 PM, "Andrew Cagney" <ac131313@cygnus.com> wrote:

> 
>> Andrew,
>> 
>> The example is the table that "info break" makes.  In FSF gdb, you only emit
>> table headers for the first 6 columns, and if there are commands, they are
>> added to the bkpt tuple (and so formally should be other columns in the
>> table), but there is no equivalent entry in the header list.  The table code
>> doesn't require that all data in a table row have an equivalent header
>> entry, so this works, but this is just a bit bogus...  Somebody here got
>> annoyed by this and added table headers for the other columns - mostly so
>> users could see what the columns mean.  In this case, the list that is being
>> added to the table is the list of breakpoint commands.
> 
> Ah, yes. Ok, got it (head butts table).  I think I'll check it in
> slightly different though - add the code that advances the headings to
> the ..._begin() instead of ..._end() function.  I think that will sit
> better with the other logic already handling tables.
> 
> Make sense?

Sounds fine.  If you are going to check this in, will you check the doc
patch too?  Thanks.

Jim
-- 
++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=
Jim Ingham                              jingham@apple.com
Developer Tools - gdb

Re: Bug with lists in tables in ui-out.c

[Andrew Cagney]

> Sounds fine.  If you are going to check this in, will you check the doc
> patch too?  Thanks.

Hmm, I think there is another thinko bug lurking.  I suspect the 
current->field_count needs to be updated as part of consuming the header.

If you don't and the table has a tuple/list followed by as simple field 
then the simple field would find a field nr mismatch.

	Andrew