Patch: 'show args' -vs- '--args'

Patch: 'show args' -vs- '--args'

[Tom Tromey]

I use "gdb --args" a lot, and I've noticed a weird behavior.  If I
start gdb this way, "show args" will initially say that there are no
arguments, even if there are.  However, if I run "show args" twice in
a row, the second one will print the correct answer.

I tracked this down to notice_args_read.  This calls get_inferior_args
to set the correct value, but too late.  This is why it fails the
first time (it uses the old value) but succeeds the second time (the
first call sets the value).

This patch fixes the problem by using the (apparently as-yet-unused)
'pre_show_hook' of the show command.

I checked the other callers of add_setshow_optional_filename_cmd to
make sure they weren't making the same mistake (they weren't).  I
didn't check any other setshow commands, though.

Also, I ran the test suite against this on x86 FC-6.  I forgot to make
a baseline, sorry.. but the results look sane to me.

Tom


ChangeLog:
2008-02-03  Tom Tromey  <tromey@redhat.com>

	* symfile.c (_initialize_symfile): Update.
	* solib.c (_initialize_solib): Update.
	* cli/cli-decode.c (add_setshow_optional_filename_cmd): Set
	pre_show_hook.
	* infcmd.c (notice_args_read): Change arguments.  Don't print
	value.
	(_initialize_infcmd): Update.
	* command.h (add_setshow_optional_filename_cmd): Add
	'pre_show_func' argument.

Index: command.h
===================================================================
RCS file: /cvs/src/src/gdb/command.h,v
retrieving revision 1.58
diff -u -r1.58 command.h
--- command.h	1 Jan 2008 22:53:09 -0000	1.58
+++ command.h	3 Feb 2008 20:24:30 -0000
@@ -297,6 +297,7 @@
 					       const char *show_doc,
 					       const char *help_doc,
 					       cmd_sfunc_ftype *set_func,
+					       void (*pre_show_func) (struct cmd_list_element *),
 					       show_value_ftype *show_func,
 					       struct cmd_list_element **set_list,
 					       struct cmd_list_element **show_list);
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.169
diff -u -r1.169 infcmd.c
--- infcmd.c	31 Jan 2008 13:37:21 -0000	1.169
+++ infcmd.c	3 Feb 2008 20:24:31 -0000
@@ -268,12 +268,10 @@
   inferior_argv = 0;
 }
 
-/* Notice when `show args' is run.  */
+/* Called to compute the value before `show args' is run.  */
 static void
-notice_args_read (struct ui_file *file, int from_tty,
-		  struct cmd_list_element *c, const char *value)
+notice_args_read (struct cmd_list_element *c)
 {
-  deprecated_show_value_hack (file, from_tty, c, value);
   /* Might compute the value.  */
   get_inferior_args ();
 }
@@ -2058,6 +2056,7 @@
 _initialize_infcmd (void)
 {
   struct cmd_list_element *c = NULL;
+  struct cmd_list_element *show_args;
 
   /* add the filename of the terminal connected to inferior I/O */
   add_setshow_filename_cmd ("inferior-tty", class_run,
@@ -2074,6 +2073,7 @@
 Follow this command with any number of args, to be passed to the program."),
 				     notice_args_set,
 				     notice_args_read,
+				     NULL,
 				     &setlist, &showlist);
 
   c = add_cmd ("environment", no_class, environment_info, _("\
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.101
diff -u -r1.101 solib.c
--- solib.c	7 Jan 2008 15:19:58 -0000	1.101
+++ solib.c	3 Feb 2008 20:24:31 -0000
@@ -1033,6 +1033,7 @@
 Show the search path for loading non-absolute shared library symbol files."), _("\
 This takes precedence over the environment variables PATH and LD_LIBRARY_PATH."),
 				     reload_shared_libraries,
+				     NULL,
 				     show_solib_search_path,
 				     &setlist, &showlist);
 }
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.198
diff -u -r1.198 symfile.c
--- symfile.c	29 Jan 2008 22:47:20 -0000	1.198
+++ symfile.c	3 Feb 2008 20:24:31 -0000
@@ -4181,6 +4181,7 @@
 and lastly at the path of the directory of the binary with\n\
 the global debug-file directory prepended."),
 				     NULL,
+				     NULL,
 				     show_debug_file_directory,
 				     &setlist, &showlist);
 }
Index: cli/cli-decode.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-decode.c,v
retrieving revision 1.63
diff -u -r1.63 cli-decode.c
--- cli/cli-decode.c	1 Jan 2008 22:53:14 -0000	1.63
+++ cli/cli-decode.c	3 Feb 2008 20:24:31 -0000
@@ -523,15 +523,18 @@
 				   const char *set_doc, const char *show_doc,
 				   const char *help_doc,
 				   cmd_sfunc_ftype *set_func,
+				   void (*pre_show_func) (struct cmd_list_element *),
 				   show_value_ftype *show_func,
 				   struct cmd_list_element **set_list,
 				   struct cmd_list_element **show_list)
 {
+  struct cmd_list_element *show_cmd;
   add_setshow_cmd_full (name, class, var_optional_filename, var,
 			set_doc, show_doc, help_doc,
 			set_func, show_func,
 			set_list, show_list,
-			NULL, NULL);
+			NULL, &show_cmd);
+  show_cmd->pre_show_hook = pre_show_func;
 }
 
 /* Add element named NAME to both the set and show command LISTs (the

Re: Patch: 'show args' -vs- '--args'

[Tom Tromey]

Ping.  (I hope it isn't too early for that.)

Tom> This patch fixes the problem by using the (apparently as-yet-unused)
Tom> 'pre_show_hook' of the show command.

Tom> ChangeLog:
Tom> 2008-02-03  Tom Tromey  <tromey@redhat.com>
Tom> 	* symfile.c (_initialize_symfile): Update.
Tom> 	* solib.c (_initialize_solib): Update.
Tom> 	* cli/cli-decode.c (add_setshow_optional_filename_cmd): Set
Tom> 	pre_show_hook.
Tom> 	* infcmd.c (notice_args_read): Change arguments.  Don't print
Tom> 	value.
Tom> 	(_initialize_infcmd): Update.
Tom> 	* command.h (add_setshow_optional_filename_cmd): Add
Tom> 	'pre_show_func' argument.

Tom

Re: Patch: 'show args' -vs- '--args'

[Daniel Jacobowitz]

On Sun, Feb 03, 2008 at 01:03:10PM -0700, Tom Tromey wrote:
> I use "gdb --args" a lot, and I've noticed a weird behavior.  If I
> start gdb this way, "show args" will initially say that there are no
> arguments, even if there are.  However, if I run "show args" twice in
> a row, the second one will print the correct answer.
> 
> I tracked this down to notice_args_read.  This calls get_inferior_args
> to set the correct value, but too late.  This is why it fails the
> first time (it uses the old value) but succeeds the second time (the
> first call sets the value).
> 
> This patch fixes the problem by using the (apparently as-yet-unused)
> 'pre_show_hook' of the show command.

This is from your original --args patch as posted on gdb-patches:

@@ -1785,8 +1836,10 @@
   	      "Set argument list to give program being debugged when
	      it is started.\n\
 Follow this command with any number of args, to be passed to the
 program.",
		   &setlist);
-  add_show_from_set (c, &showlist);
   c->completer = filename_completer;
+  c->function.sfunc = notice_args_set;
+  c = add_show_from_set (c, &showlist);
+  c->pre_show_hook = notice_args_read;
 
   c = add_cmd
     ("environment", no_class, environment_info,

It was removed in a typo later and eventually ended up readded as
a show_func rather than a pre_show_hook.

Anyway, now that it's a show_func...

> +notice_args_read (struct cmd_list_element *c)
>  {
> -  deprecated_show_value_hack (file, from_tty, c, value);
>    /* Might compute the value.  */
>    get_inferior_args ();
>  }

wouldn't just swapping those two function calls fix it?


-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch: 'show args' -vs- '--args'

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> Anyway, now that it's a show_func...

>> +notice_args_read (struct cmd_list_element *c)
>> {
>> -  deprecated_show_value_hack (file, from_tty, c, value);
>> /* Might compute the value.  */
>> get_inferior_args ();
>> }

Daniel> wouldn't just swapping those two function calls fix it?

I suppose it could if we did this:

   deprecated_show_value_hack (file, from_tty, c, get_inferior_args ());

i.e., we didn't print the passed-in value -- that value is already
known to be wrong.

It seems a little weird to do this, given that this function is named
'deprecated_...', and given that there is an existing hook that does
exactly what is needed here.  That aid I'm happy to resubmit a patch
based on this idea if that is what you want, just let me know.

Tom

Re: Patch: 'show args' -vs- '--args'

[Daniel Jacobowitz]

On Thu, Feb 28, 2008 at 09:00:43AM -0700, Tom Tromey wrote:
> I suppose it could if we did this:
> 
>    deprecated_show_value_hack (file, from_tty, c, get_inferior_args ());
> 
> i.e., we didn't print the passed-in value -- that value is already
> known to be wrong.
> 
> It seems a little weird to do this, given that this function is named
> 'deprecated_...', and given that there is an existing hook that does
> exactly what is needed here.  That aid I'm happy to resubmit a patch
> based on this idea if that is what you want, just let me know.

I'm fine with either the above, or setting pre_show_hook manually; I
didn't like add_setshow_optional_filename_cmd having a different
signature than all the similar setshow functions.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch: 'show args' -vs- '--args'

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> I'm fine with either the above, or setting pre_show_hook manually; I
Daniel> didn't like add_setshow_optional_filename_cmd having a different
Daniel> signature than all the similar setshow functions.

Ok, in that case, I will go with reordering the lines.

AFAICT add_setshow_optional_filename_cmd does not return the new
command element, and of course adding this would require changing the
signature.  I'm assuming here that it is not ok to assume that the
first element of 'showlist' will be the new command.

Tom

Re: Patch: 'show args' -vs- '--args'

[Daniel Jacobowitz]

On Thu, Feb 28, 2008 at 09:22:13AM -0700, Tom Tromey wrote:
> >>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:
> 
> Daniel> I'm fine with either the above, or setting pre_show_hook manually; I
> Daniel> didn't like add_setshow_optional_filename_cmd having a different
> Daniel> signature than all the similar setshow functions.
> 
> Ok, in that case, I will go with reordering the lines.
> 
> AFAICT add_setshow_optional_filename_cmd does not return the new
> command element, and of course adding this would require changing the
> signature.  I'm assuming here that it is not ok to assume that the
> first element of 'showlist' will be the new command.

Right you are.  I thought it did.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch: 'show args' -vs- '--args'

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> I'm fine with either the above [...]

How about this?

Tom

ChangeLog:
2008-02-28  Tom Tromey  <tromey@redhat.com>

	* infcmd.c (notice_args_read): Print result of get_inferior_args.

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.170
diff -u -r1.170 infcmd.c
--- infcmd.c	28 Feb 2008 16:26:17 -0000	1.170
+++ infcmd.c	28 Feb 2008 17:26:48 -0000
@@ -273,9 +273,9 @@
 notice_args_read (struct ui_file *file, int from_tty,
 		  struct cmd_list_element *c, const char *value)
 {
-  deprecated_show_value_hack (file, from_tty, c, value);
-  /* Might compute the value.  */
-  get_inferior_args ();
+  /* Note that we ignore the passed-in value in favor of computing it
+     directly.  */
+  deprecated_show_value_hack (file, from_tty, c, get_inferior_args ());
 }
 
 \f

Re: Patch: 'show args' -vs- '--args'

[Daniel Jacobowitz]

On Thu, Feb 28, 2008 at 09:38:48AM -0700, Tom Tromey wrote:
> ChangeLog:
> 2008-02-28  Tom Tromey  <tromey@redhat.com>
> 
> 	* infcmd.c (notice_args_read): Print result of get_inferior_args.

OK.

-- 
Daniel Jacobowitz
CodeSourcery