[RFA] Fix "Segmentation fault" when "gdb -v"

[RFA] Fix "Segmentation fault" when "gdb -v"

[Hui Zhu]

Hi,

I found that when "gdb -v", it "Segmentation fault".

This is because:
  if (print_version)
    {
      print_gdb_version (gdb_stdout);
      wrap_here ("");
      printf_filtered ("\n");
      exit (0);
    }
All of this code use "printf_filtered".  But this function must be
call after "interp_set".
But this part of code call before "interp_set".
So I make a patch to change "printf_filtered" to "printf_unfiltered"
in this part of code.

Please help me review it.

Thanks,
Hui

2010-03-04  Hui Zhu  <teawater@gmail.com>

	* main.c (captured_main): Change "printf_filtered" to
	"printf_unfiltered".
	* top.c (print_gdb_version): Ditto.

---
 main.c |    2 +-
 top.c  |   18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

--- a/main.c
+++ b/main.c
@@ -704,7 +704,7 @@ Excess command line arguments ignored. (
     {
       print_gdb_version (gdb_stdout);
       wrap_here ("");
-      printf_filtered ("\n");
+      printf_unfiltered ("\n");
       exit (0);
     }

--- a/top.c
+++ b/top.c
@@ -1054,18 +1054,18 @@ print_gdb_version (struct ui_file *strea
      program to parse, and is just canonical program name and version
      number, which starts after last space. */

-  fprintf_filtered (stream, "GNU gdb %s%s\n", PKGVERSION, version);
+  fprintf_unfiltered (stream, "GNU gdb %s%s\n", PKGVERSION, version);

   /* Second line is a copyright notice. */

-  fprintf_filtered (stream, "Copyright (C) 2010 Free Software
Foundation, Inc.\n");
+  fprintf_unfiltered (stream, "Copyright (C) 2010 Free Software
Foundation, Inc.\n");

   /* Following the copyright is a brief statement that the program is
      free software, that users are free to copy and change it on
      certain conditions, that it is covered by the GNU GPL, and that
      there is no warranty. */

-  fprintf_filtered (stream, "\
+  fprintf_unfiltered (stream, "\
 License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>\n\
 This is free software: you are free to change and redistribute it.\n\
 There is NO WARRANTY, to the extent permitted by law.  Type \"show copying\"\n\
@@ -1073,22 +1073,22 @@ and \"show warranty\" for details.\n");

   /* After the required info we print the configuration information. */

-  fprintf_filtered (stream, "This GDB was configured as \"");
+  fprintf_unfiltered (stream, "This GDB was configured as \"");
   if (strcmp (host_name, target_name) != 0)
     {
-      fprintf_filtered (stream, "--host=%s --target=%s", host_name,
target_name);
+      fprintf_unfiltered (stream, "--host=%s --target=%s", host_name,
target_name);
     }
   else
     {
-      fprintf_filtered (stream, "%s", host_name);
+      fprintf_unfiltered (stream, "%s", host_name);
     }
-  fprintf_filtered (stream, "\".");
+  fprintf_unfiltered (stream, "\".");

   if (REPORT_BUGS_TO[0])
     {
-      fprintf_filtered (stream,
+      fprintf_unfiltered (stream,
 			_("\nFor bug reporting instructions, please see:\n"));
-      fprintf_filtered (stream, "%s.", REPORT_BUGS_TO);
+      fprintf_unfiltered (stream, "%s.", REPORT_BUGS_TO);
     }
 }

Re: [RFA] Fix "Segmentation fault" when "gdb -v"

[Joel Brobecker]

> All of this code use "printf_filtered".  But this function must be
> call after "interp_set".
> But this part of code call before "interp_set".

I think that this was an unforseen side-effect of a recent change.
IMO, it's better to get rid of this side-effect (needing the interpreter
to be set) in printf_filtered.

We should NOT change printf_filtered into printf_unfiltered in this case
because the same function is used in two different situations:
  - when the user uses -v
  - when the user types "show version"
In the latter case, the printf_filtered is appropriate.

Now, we need to decide whether pagination should be enabled if
the interpreter is not set. I think it makes sense to disable pagination
in this case.  If the interpreter is not set yet, we're just printing
stuff on stdout, we haven't started the interactive session (if any) yet...

-- 
Joel

Re: [RFA] Fix "Segmentation fault" when "gdb -v"

[Hui Zhu]

On Thu, Mar 4, 2010 at 15:26, Joel Brobecker <brobecker@adacore.com> wrote:
>> All of this code use "printf_filtered".  But this function must be
>> call after "interp_set".
>> But this part of code call before "interp_set".
>
> I think that this was an unforseen side-effect of a recent change.
> IMO, it's better to get rid of this side-effect (needing the interpreter
> to be set) in printf_filtered.
>
> We should NOT change printf_filtered into printf_unfiltered in this case
> because the same function is used in two different situations:
>  - when the user uses -v
>  - when the user types "show version"
> In the latter case, the printf_filtered is appropriate.
>
> Now, we need to decide whether pagination should be enabled if
> the interpreter is not set. I think it makes sense to disable pagination
> in this case.  If the interpreter is not set yet, we're just printing
> stuff on stdout, we haven't started the interactive session (if any) yet...
>

The way is make it output after interp_set.

Thanks,
Hui

2010-03-04  Hui Zhu  <teawater@gmail.com>

	* main.c (captured_main): Change version output after
	interp_set.

---
 main.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

--- a/main.c
+++ b/main.c
@@ -695,19 +695,6 @@ Excess command line arguments ignored. (
      gdb_init.  */
   get_init_files (&system_gdbinit, &home_gdbinit, &local_gdbinit);

-  /* Do these (and anything which might call wrap_here or *_filtered)
-     after initialize_all_files() but before the interpreter has been
-     installed.  Otherwize the help/version messages will be eaten by
-     the interpreter's output handler.  */
-
-  if (print_version)
-    {
-      print_gdb_version (gdb_stdout);
-      wrap_here ("");
-      printf_filtered ("\n");
-      exit (0);
-    }
-
   if (print_help)
     {
       print_gdb_help (gdb_stdout);
@@ -750,6 +737,19 @@ Excess command line arguments ignored. (
       }
   }

+  /* Do these (and anything which might call wrap_here or *_filtered)
+     after initialize_all_files() but before the interpreter has been
+     installed.  Otherwize the help/version messages will be eaten by
+     the interpreter's output handler.  */
+
+  if (print_version)
+    {
+      print_gdb_version (gdb_stdout);
+      wrap_here ("");
+      printf_filtered ("\n");
+      exit (0);
+    }
+
   /* FIXME: cagney/2003-02-03: The big hack (part 2 of 2) that lets
      GDB retain the old MI1 interpreter startup behavior.  Output the
      copyright message after the interpreter is installed when it is

Re: [RFA] Fix "Segmentation fault" when "gdb -v"

[Daniel Jacobowitz]

On Thu, Mar 04, 2010 at 04:06:00PM +0800, Hui Zhu wrote:
> The way is make it output after interp_set.

Take a look at the comment you moved.

> -  /* Do these (and anything which might call wrap_here or *_filtered)
> -     after initialize_all_files() but before the interpreter has been
> -     installed.  Otherwize the help/version messages will be eaten by
> -     the interpreter's output handler.  */

That says to do it before interp_set.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Fix "Segmentation fault" when "gdb -v"

[Pedro Alves]

On Thursday 04 March 2010 07:26:20, Joel Brobecker wrote:
> > All of this code use "printf_filtered".  But this function must be
> > call after "interp_set".
> > But this part of code call before "interp_set".

Sorry I missed this could happen.  Quite obvious in hindsight.

> I think that this was an unforseen side-effect of a recent change.
> IMO, it's better to get rid of this side-effect (needing the interpreter
> to be set) in printf_filtered.

That could mean that GDB could paginate before figuring out if the
top level interpreter is MI, which isn't desirable.  Unless we
always disable pagination until there's an interpreter set, but,
now that I look, actually, we already do:

I started GDB in a really short window (3 lines height), and noticed
that "gdb --version" doesn't paginate either.  prompt_for_continue
_is_ called, but it's eneffective: it calls gdb_readline_wrapper
which first, which tries to print the pagination prompt, but that
fails because current_interp_display_prompt_p just doesn't do
anything if there's no current interpreter yet.  Then it goes into the
event loop waiting for input to the prompt, which just returns
almost immediately, because there's no waitable file registered
in the event loop yet.

> 
> We should NOT change printf_filtered into printf_unfiltered in this case
> because the same function is used in two different situations:
>   - when the user uses -v
>   - when the user types "show version"
> In the latter case, the printf_filtered is appropriate.
> 
> Now, we need to decide whether pagination should be enabled if
> the interpreter is not set. I think it makes sense to disable pagination
> in this case.  If the interpreter is not set yet, we're just printing
> stuff on stdout, we haven't started the interactive session (if any) yet...

I agree, and turns out this is what's already happening, albeit in
a "works almost by accident" way.  I think this patch below wouldn't
be that bad afterall.  It does fix the problem.

-- 
Pedro Alves

2010-03-04  Pedro Alves  <pedro@codesourcery.com>

	* utils.c (fputs_maybe_filtered): Check if there's already a top
	level interpreter before dereferencing it.  If there isn't one,
	don't paginate either.

---
 gdb/utils.c |    1 +
 1 file changed, 1 insertion(+)

Index: src/gdb/utils.c
===================================================================
--- src.orig/gdb/utils.c	2010-03-04 13:27:17.000000000 +0000
+++ src/gdb/utils.c	2010-03-04 13:29:11.000000000 +0000
@@ -2213,6 +2213,7 @@ fputs_maybe_filtered (const char *linebu
   if (stream != gdb_stdout
       || !pagination_enabled
       || (lines_per_page == UINT_MAX && chars_per_line == UINT_MAX)
+      || top_level_interpreter () == NULL
       || ui_out_is_mi_like_p (interp_ui_out (top_level_interpreter ())))
     {
       fputs_unfiltered (linebuffer, stream);

Re: [RFA] Fix "Segmentation fault" when "gdb -v"

[Pedro Alves]

On Thursday 04 March 2010 14:02:07, Pedro Alves wrote:
> I agree, and turns out this is what's already happening, albeit in
> a "works almost by accident" way.  I think this patch below wouldn't
> be that bad afterall.  It does fix the problem.

I went ahead and applied this.  Thanks.

> 2010-03-04  Pedro Alves  <pedro@codesourcery.com>
> 
>         * utils.c (fputs_maybe_filtered): Check if there's already a top
>         level interpreter before dereferencing it.  If there isn't one,
>         don't paginate either.

-- 
Pedro Alves

Re: [RFA] Fix "Segmentation fault" when "gdb -v"

[Joel Brobecker]

> > 2010-03-04  Pedro Alves  <pedro@codesourcery.com>
> > 
> >         * utils.c (fputs_maybe_filtered): Check if there's already a top
> >         level interpreter before dereferencing it.  If there isn't one,
> >         don't paginate either.

Thanks for fixing this, it's exactly what I had in mind!

-- 
Joel