This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix gdb crash with tui


On 03/09/2013 02:13 PM, Hui Zhu wrote:
> Hi,
> 
> I got crash when I use tui.  The steps to reproduce is:
> gdb gdb
> b gdb_main
> r
> Ctrl-x A change to TUI mode.
> Keep click <UP> some times.
> Keep click <Down> some times.
> Then you can get "---Type <return> to continue, or q <return> to quit---"
> Click <return>.
> Then the GDB crash.
> 
> I think this issue is this part should not output "---Type <return> to
> continue, or q <return> to quit---".
> So I make a patch to let tui doesn't output "file" just output "fullname".
> Not sure is a good enough but this issue is fixed.
> 
> I suggest we fix this issue before next release because it will crash the GDB.
> 
> Thanks,
> Hui
> 
> 2013-03-09  Hui Zhu  <hui_zhu@mentor.com>
> 
> 	* source.c (print_source_lines_base): Doesn't output "file".
> 

Hmm.  print_source_lines_base does:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	{
	  ui_out_field_int (uiout, "line", line);
	  ui_out_text (uiout, "\tin ");
	  ui_out_field_string (uiout, "file",
			       symtab_to_filename_for_display (s));

	  /* TUI expects the "fullname" field.  While it is
	     !ui_out_is_mi_like_p compared to CLI it is !ui_source_list.  */
	  if (ui_out_is_mi_like_p (uiout)
	      || !ui_out_test_flags (uiout, ui_source_list))
	    {
	      const char *fullname = symtab_to_fullname (s);

	      ui_out_field_string (uiout, "fullname", fullname);
	    }
	  ui_out_text (uiout, "\n");

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


And tui_field_string does:


~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/* Other cli_field_* end up here so alignment and field separators are
   both handled by tui_field_string.  */

static void
tui_field_string (struct ui_out *uiout,
		  int fldno, int width,
		  enum ui_align align,
		  const char *fldname,
		  const char *string)
{
  tui_out_data *data = ui_out_data (uiout);

  if (data->base.suppress_output)
    return;

  if (fldname && data->line > 0 && strcmp (fldname, "fullname") == 0)
    {
      data->start_of_line ++;
      if (data->line > 0)
        {
          tui_show_source (string, data->line);
        }
      return;
    }

  data->start_of_line++;

  (*cli_ui_out_impl.field_string) (uiout, fldno,
				   width, align,
				   fldname, string);
}

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That "fullname" handling is gross...

It's there since forever though.

This:

703eecdd98022d08b362292ff79ac4087d1406de,
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date:   Sun Feb 3 16:16:40 2013 +0000

    gdb/
        * source.c (print_source_lines_base): Print for TUI also "fullname".
...
        * tui/tui-out.c (tui_field_string): Check for "fullname" now.

Did:

--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -84,7 +84,7 @@ tui_field_string (struct ui_out *uiout,
   if (data->base.suppress_output)
     return;

-  if (fldname && data->line > 0 && strcmp (fldname, "file") == 0)
+  if (fldname && data->line > 0 && strcmp (fldname, "fullname") == 0)
     {

So before, tui-out had the hack to call tui_show_source
whenever a string "file" was being output.  Are there any
other cases where we print a "file" string, but not a "filename"
string?  If so, that may have caused a TUI regression.

but the patch also made it so that tui_field_string is called
twice: once for "file", and another for "filename".  And "file",
having to special handling, causes tui_field_string to reach:

  if (fldname && data->line > 0 && strcmp (fldname, "fullname") == 0)
    {
.,..
    }

// ... this .... // ######

  data->start_of_line++;

  (*cli_ui_out_impl.field_string) (uiout, fldno,
				   width, align,
				   fldname, string);
}

And call the cli's field_string output, which goes
the the console window, which I guess causes the flashes
I see under valgrind, and fills up the pagination, ultimately
causing the pagination prompt and the crash as consequence of
that being unexpected.

Rolling back before that patch, the bug goes away.

Another bug that this caused (or rather another manifestation
of the bug), is that when you scroll up/down, you see the
highlighted line disappear rather than following the scroll.
Before the patch it worked correctly.

It seems one way to fix it would be revert these two
hunks:

    gdb/
        * source.c (print_source_lines_base): Print for TUI also "fullname".
...
        * tui/tui-out.c (tui_field_string): Check for "fullname" now.

But I don't know what motivated that change in the first place.

Another idea would be to make tui_field_string ignore "file" strings.

Just doing any of these does fix the issue with the highlighted
current line disappearing, so there's something else that needs
fixing in addition.

Yet another that came to mind when I saw the explicit
"fullname" check in tui_field_string would make to come up with new
ui_out_field_file and ui_out_field_filename methods (would also get
rid of the ui_out_is_mi_like_p check in print_source_lines_base), instead
of using tui_field_string for file names, which tui would implement
by ignoring one, and making the other refresh the source window,
though that still feels a little bit hacky (aren't there situations
we print a file name but don't want a TUI source refresh?).  Maybe
some deeper fix could be possible.  But I'd go with a simple
fix for now.

I'm not planing of addressing this further myself for now, but
I'd like it fixed in 7.6, being a TUI user myself.

Hui, could you create a PR in bugzilla so we can track this?

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]