Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[mbilal]

Hi All,
The following patch is a fix for PR gdb/15224, that is 'history save'
command should be enabled by default
so that user can always be able to see their commands history by default

Index: gdb/top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.231
diff -u -p -r1.231 top.c
--- gdb/top.c    27 Mar 2013 12:14:09 -0000    1.231
+++ gdb/top.c    29 Mar 2013 11:39:40 -0000
@@ -1604,7 +1604,7 @@ init_main (void)
    /* Set the important stuff up for command editing.  */
    command_editing_p = 1;
    history_expansion_p = 0;
-  write_history_p = 0;
+  write_history_p = 1;

    /* Setup important stuff for command line editing.  */
    rl_completion_word_break_hook = gdb_completion_word_break_characters;
Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.577
diff -u -p -r1.577 NEWS
--- gdb/NEWS    28 Mar 2013 21:58:03 -0000    1.577
+++ gdb/NEWS    29 Mar 2013 11:41:42 -0000
@@ -3,6 +3,8 @@

  *** Changes since GDB 7.6

+* The command 'history save' is now default enabled
+
  * New commands:
  maint set|show per-command
  maint set|show per-command space
Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15339
diff -u -p -r1.15339 ChangeLog
--- gdb/ChangeLog    29 Mar 2013 02:04:14 -0000    1.15339
+++ gdb/ChangeLog    29 Mar 2013 11:43:27 -0000
@@ -1,3 +1,10 @@
+2013-03-29  Muhammad Bilal  <mbilal@codesourcery.com>
+
+     PR gdb/15224
+     * top.c (init_main):Modified write_history_p.
+
+     * NEWS: Update.
+
  2013-03-28  Joel Brobecker  <brobecker@adacore.com>

      * coffread.c (is_import_fixup_symbol): New function.
mbilal@pkl-mbilal-ubuntu:~/src/gdb/doc$ cvs diff gdb.texinfo
Index: gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.1067
diff -u -p -r1.1067 gdb.texinfo
--- gdb.texinfo    28 Mar 2013 21:58:03 -0000    1.1067
+++ gdb.texinfo    29 Mar 2013 12:22:31 -0000
@@ -21218,7 +21218,7 @@ is not set.
  @item set history save
  @itemx set history save on
  Record command history in a file, whose name may be specified with the
-@code{set history filename} command.  By default, this option is disabled.
+@code{set history filename} command.  By default, this option is enabled.

  @item set history save off
  Stop recording command history in a file.
Index: gdb/doc/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/doc/ChangeLog,v
retrieving revision 1.1430
diff -u -p -r1.1430 ChangeLog
--- gdb/doc/ChangeLog    28 Mar 2013 21:58:03 -0000    1.1430
+++ gdb/doc/ChangeLog    29 Mar 2013 11:51:07 -0000
@@ -1,3 +1,8 @@
+2013-03-29  Muhammad Bilal  <mbilal@codesourcery.com>
+
+    PR gdb/15224
+    * gdb.texinfo (Command history): Set history save enabled
+
  2013-03-28  Pedro Alves  <palves@redhat.com>

      * gdb.texinfo (Remote Configuration) <set remote @var{name}-packet


Ok to Commit?


Thanks
-Bilal

Re: Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[Jan Kratochvil]

On Fri, 29 Mar 2013 13:53:25 +0100, mbilal wrote:
> The following patch is a fix for PR gdb/15224, that is 'history save'
> command should be enabled by default
> so that user can always be able to see their commands history by default

I would also like it that way for many years but it is not that simple.

If you run GDB testsuite afterwards it will fill up (and discard due to its
maximum size) your personal history.  Normally it does not happen because
testsuite runs gdb -nx so ~/.gdbinit is not read and therefore the history
saving is not enabled.  Eclipse CDT also uses "gdb --interpreter mi2 --nx".

While that could be fixed in GDB testsuite the same problem will affect any
GDB frontend.

Could you propose a patch compatible at least with existing front ends (like
Eclipse CDT)?  I was thinking to just bind the default WRITE_HISTORY_P to
whether ~/.gdbinit is being read or not.


Thanks,
Jan

Re: Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[Pedro Alves]

On 03/29/2013 02:15 PM, Jan Kratochvil wrote:

> While that could be fixed in GDB testsuite the same problem will affect any
> GDB frontend.
> 
> Could you propose a patch compatible at least with existing front ends (like
> Eclipse CDT)?

The commands issued in MI sessions aren't added to the history, as when the top
level interpreter is MI, GDB doesn't use readline/history (look for add_history).

It could be argued that commands executed through "-interpreter-exec console"
should make it to the history.  Then we'd have the problem that cli
commands that frontends issue because there are no MI alternatives would
appear on the history.  But, we already have a solution for that -- the
"server" command/prefix
(http://sourceware.org/gdb/onlinedocs/gdb/Server-Prefix.html#Server-Prefix).

> I was thinking to just bind the default WRITE_HISTORY_P to
> whether ~/.gdbinit is being read or not.

The history's filename is also tweakable with the GDBHISTFILE
environment variable (there's HISTSIZE too; 'GDBHISTFILE= gdb' effectively
disables history load/save).  Doesn't feel quite right to tie this to .gdbinit.

One possible counter argument to flipping this on would that since history
files are saved to the current directory by default, this has potential for
littering users' directories with (hidden) .gdb_history files without the
users noticing.  We should at least make that fact explicit in NEWS and
perhaps make it prominent in the manual.

-- 
Pedro Alves

Re: Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[Jan Kratochvil]

On Mon, 01 Apr 2013 13:33:18 +0200, Pedro Alves wrote:
> The commands issued in MI sessions aren't added to the history, as when the top
> level interpreter is MI, GDB doesn't use readline/history (look for add_history).

That's better than what I assumed.

This would mean 'set history save off' in GDB testsuite could be sufficient.


> It could be argued that commands executed through "-interpreter-exec console"
> should make it to the history.  Then we'd have the problem that cli
> commands that frontends issue because there are no MI alternatives would
> appear on the history.  But, we already have a solution for that -- the
> "server" command/prefix
> (http://sourceware.org/gdb/onlinedocs/gdb/Server-Prefix.html#Server-Prefix).

There is also needed backward compatibility with existing frontends.

The default for -i=mi could be made "set history save off" and new frontends
which properly recognize user commands vs. "server" commands could use during
initialization:
	-interpreter-exec console "set history save on"


> > I was thinking to just bind the default WRITE_HISTORY_P to
> > whether ~/.gdbinit is being read or not.
> 
> The history's filename is also tweakable with the GDBHISTFILE
> environment variable (there's HISTSIZE too; 'GDBHISTFILE= gdb' effectively
> disables history load/save).  Doesn't feel quite right to tie this to .gdbinit.

It matters how are the existing environments set up.  And by default neither
HISTSIZE nor GDBHISTFILE environment variables are set.

Binding it to ~/.gdbinit is really ugly but I did not have a better idea
compatible with existing GDB scripts/frontends/testsuites.


> One possible counter argument to flipping this on would that since history
> files are saved to the current directory by default, this has potential for
> littering users' directories with (hidden) .gdb_history files without the
> users noticing.  We should at least make that fact explicit in NEWS and
> perhaps make it prominent in the manual.

I see there should be also changed the default to:
	set history filename ~/.gdb_history

Otherwise it is a similar pain to ./.gdbinit, it is also non-standard to any
other normal application which use history files in $HOME, I do not find using
./.gdb_history by default as acceptable.


Jan

Re: Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[Pedro Alves]

On 04/01/2013 01:29 PM, Jan Kratochvil wrote:
>>> > > I was thinking to just bind the default WRITE_HISTORY_P to
>>> > > whether ~/.gdbinit is being read or not.
>> > 
>> > The history's filename is also tweakable with the GDBHISTFILE
>> > environment variable (there's HISTSIZE too; 'GDBHISTFILE= gdb' effectively
>> > disables history load/save).  Doesn't feel quite right to tie this to .gdbinit.
> It matters how are the existing environments set up.  And by default neither
> HISTSIZE nor GDBHISTFILE environment variables are set.

Not clear to me which environments you're referring to here, if you're
referring to any specific environments.  (HISTSIZE won't be as rare,
because that's also the variable used by bash.  We should probably
have a GDBHISTSIZE too.)

> Binding it to ~/.gdbinit is really ugly but I did not have a better idea
> compatible with existing GDB scripts/frontends/testsuites.

MI frontends are not an issue currently, as discussed.  We could add
a check for annotations too, if necessary.

We don't add commands to history unless we're interactive debugging:

  /* Add line to history if appropriate.  */
  if (instream == stdin
      && ISATTY (stdin) && *linebuffer)
    add_history (linebuffer);

and init_history (what loads history) isn't reached with either

 $ gdb -ex "quit"

or

 $ echo "quit" > cmd
 $ gdb -x ./cmd

Also,

 $ gdb --batch

never reaches either add_history or init_history.

I think however, that whether to write history to file should also
check whether we're in interactive debugging mode:

  /* Save the history information if it is appropriate to do so.  */
  if (write_history_p && history_filename)
    write_history (history_filename);

That is, e.g., this touches history:

 $ ls -als ~/.gdbhist
 4 -rw-------. 1 pedro pedro 27 Apr  1 14:10 /home/pedro/.gdbhist
 $ date
 Mon Apr  1 14:37:01 WEST 2013
 $ gdb -ex "set history filename ~/.gdbhist" < /dev/null
 (gdb) quit
 $ ls -als ~/.gdbhist
 4 -rw-------. 1 pedro pedro 27 Apr  1 14:37 /home/pedro/.gdbhist

That, I'd call a bug.

Probably, it'd be best to use input_from_terminal_p, which also
checks for --batch mode, among other things.

I think that all covers cases like abrt and other crash catchers that
spawn gdb?

As for testsuites and similar setups, I'm thinking the cases that could
be affected would be restricted to setups like ours and gcc's, that drive
gdb with expect, which is precisely designed to launch programs
interactively, as if a user was running them, so it'd make a lot of sense
to me to not treat them specially (having them adjust instead).

>> > One possible counter argument to flipping this on would that since history
>> > files are saved to the current directory by default, this has potential for
>> > littering users' directories with (hidden) .gdb_history files without the
>> > users noticing.  We should at least make that fact explicit in NEWS and
>> > perhaps make it prominent in the manual.
> I see there should be also changed the default to:
> 	set history filename ~/.gdb_history
> 
> Otherwise it is a similar pain to ./.gdbinit, it is also non-standard to any
> other normal application which use history files in $HOME, I do not find using
> ./.gdb_history by default as acceptable.

That'd be fine with me.  However, we'd need a way to be able to specify
"./.gdbinit" back.  We have precedent for "$cwd", so perhaps that would
best be addressed by making "set history filename $cwd/.gdb_history" work.

-- 
Pedro Alves

Re: Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[Jan Kratochvil]

On Mon, 01 Apr 2013 16:15:11 +0200, Pedro Alves wrote:
> On 04/01/2013 01:29 PM, Jan Kratochvil wrote:
> >> > The history's filename is also tweakable with the GDBHISTFILE
> >> > environment variable (there's HISTSIZE too; 'GDBHISTFILE= gdb' effectively
> >> > disables history load/save).  Doesn't feel quite right to tie this to .gdbinit.
> > It matters how are the existing environments set up.  And by default neither
> > HISTSIZE nor GDBHISTFILE environment variables are set.
> 
> Not clear to me which environments you're referring to here,

How >90% of GDB users run it.


> if you're referring to any specific environments.  (HISTSIZE won't be as rare,
> because that's also the variable used by bash.  We should probably
> have a GDBHISTSIZE too.)

I meant >90% of GDB users do not have GDBHISTFILE environment variables set.


> I think however, that whether to write history to file should also
> check whether we're in interactive debugging mode:
> 
>   /* Save the history information if it is appropriate to do so.  */
>   if (write_history_p && history_filename)
>     write_history (history_filename);
> 
> That is, e.g., this touches history:
> 
>  $ ls -als ~/.gdbhist
>  4 -rw-------. 1 pedro pedro 27 Apr  1 14:10 /home/pedro/.gdbhist
>  $ date
>  Mon Apr  1 14:37:01 WEST 2013
>  $ gdb -ex "set history filename ~/.gdbhist" < /dev/null
>  (gdb) quit
>  $ ls -als ~/.gdbhist
>  4 -rw-------. 1 pedro pedro 27 Apr  1 14:37 /home/pedro/.gdbhist
> 
> That, I'd call a bug.

I agree.


> 
> Probably, it'd be best to use input_from_terminal_p, which also
> checks for --batch mode, among other things.
> 
> I think that all covers cases like abrt and other crash catchers that
> spawn gdb?

Hopefully they usually use redirection from file so ISATTY should catch that
which I did not realize before.

OK, that seems as a good plan - with patching the GDB testsuite to disable
history saving.  I expect it will still affect some 3rd party testsuite using
GDB but you have convinced me such cases will not be significant enough.


I tested now that "ddd" will store the history commands like "step".
But as it runs GBD with "gdb -q -fullname PROGNAME" it already happens if one
has "set history save on" in ~/.gdbinit so I would not call that a regression.

http://pkgs.fedoraproject.org/cgit/gdb.git/plain/gdb-6.5-BEA-testsuite.patch
is using:
  cat gdbscript.gdb /dev/zero | gdb -nx "./$(basename "$RUNME")" > $LOG
which should get handled by existing ISATTY in GDB.


> > I see there should be also changed the default to:
> > 	set history filename ~/.gdb_history
> > 
> > Otherwise it is a similar pain to ./.gdbinit, it is also non-standard to any
> > other normal application which use history files in $HOME, I do not find using
> > ./.gdb_history by default as acceptable.
> 
> That'd be fine with me.  However, we'd need a way to be able to specify
> "./.gdbinit" back.  We have precedent for "$cwd", so perhaps that would
> best be addressed by making "set history filename $cwd/.gdb_history" work.

This command works for me:
	unset GDBHISTFILE; gdb -ex "set history filename .gdb_history"

I do not understand why you mention $cwd, what $cwd should do differently?

I was expecting that by default
	cd /tmp; (unset GDBHISTFILE; gdb -nx)
will change from
	(gdb) show history filename 
	The filename in which to record the command history is "/tmp/.gdb_history".
to
	(gdb) show history filename 
	The filename in which to record the command history is "/home/user/.gdb_history".
.


Jan

Re: Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[Pedro Alves]

On 04/02/2013 04:26 PM, Jan Kratochvil wrote:
> On Mon, 01 Apr 2013 16:15:11 +0200, Pedro Alves wrote:
>> On 04/01/2013 01:29 PM, Jan Kratochvil wrote:
>>>>> The history's filename is also tweakable with the GDBHISTFILE
>>>>> environment variable (there's HISTSIZE too; 'GDBHISTFILE= gdb' effectively
>>>>> disables history load/save).  Doesn't feel quite right to tie this to .gdbinit.
>>> It matters how are the existing environments set up.  And by default neither
>>> HISTSIZE nor GDBHISTFILE environment variables are set.
>>
>> Not clear to me which environments you're referring to here,
> 
> How >90% of GDB users run it.
> 
> 
>> if you're referring to any specific environments.  (HISTSIZE won't be as rare,
>> because that's also the variable used by bash.  We should probably
>> have a GDBHISTSIZE too.)
> 
> I meant >90% of GDB users do not have GDBHISTFILE environment variables set.

Ack.  It just sounded from the wording you were thinking of some specific
case.

> I tested now that "ddd" will store the history commands like "step".
> But as it runs GBD with "gdb -q -fullname PROGNAME" it already happens if one
> has "set history save on" in ~/.gdbinit so I would not call that a regression.

Yeah.  The history size is 256 by default, so IMO it's no big issue.  It'd be
worse if the history file could grow unbounded by default, IMO.

I didn't realize ddd used annotations...  I've updated the wiki's frontend
hall of shame^Wfame: http://sourceware.org/gdb/wiki/GDB%20Front%20Ends

> 
> http://pkgs.fedoraproject.org/cgit/gdb.git/plain/gdb-6.5-BEA-testsuite.patch
> is using:
>   cat gdbscript.gdb /dev/zero | gdb -nx "./$(basename "$RUNME")" > $LOG
> which should get handled by existing ISATTY in GDB.

Good.


> This command works for me:
> 	unset GDBHISTFILE; gdb -ex "set history filename .gdb_history"

Indeed it works.  I actually didn't think to try it.  I read:

  tmpenv = getenv ("GDBHISTFILE");
  if (tmpenv)
    history_filename = xstrdup (tmpenv);
  else if (!history_filename)
    {
      /* We include the current directory so that if the user changes
         directories the file written will be the same as the one
         that was read.  */
#ifdef __MSDOS__
      /* No leading dots in file names are allowed on MSDOS.  */
      history_filename = concat (current_directory, "/_gdb_history",
				 (char *)NULL);
#else
      history_filename = concat (current_directory, "/.gdb_history",
				 (char *)NULL);
#endif
    }
  read_history (history_filename);

noticed the explicit current directory handling and assumed it didn't.

But, it doesn't work the same.  Vis.:

 $ gdb
 (gdb) show history filename
 The filename in which to record the command history is "/tmp/.gdb_history".
 (gdb) q

Notice, absolute path above.

 $ cd /tmp
 $ mkdir a
 $ gdb -ex "set history filename .gdbhist"
 (gdb) show history filename
 The filename in which to record the command history is ".gdbhist".
 (gdb) cd a
 Working directory /tmp/a.
 (gdb) q
 $ cat a/.gdbhist
 show history filename
 cd a
 q
 $ ls .gdbhist a/.gdbhist
 ls: cannot access .gdbhist: No such file or directory
 a/.gdbhist
 $

So, the current default resolves the relative .gdb_history path
early on, meaning, the history file written is the same that was
read, while "set history filename .gdb_history" resolves to
different files at read and at write times, as seen in the example
above.  I'd call that a bug too.

> 
> I do not understand why you mention $cwd, what $cwd should do differently?

Not necessary it seems then.  The above issue should be addressed though.
Probably by making "set history filename"'s set hook resolve the path.
Whether to make "show history filename" show just the resolved path, or
both, I'm undecided.

> 
> I was expecting that by default
> 	cd /tmp; (unset GDBHISTFILE; gdb -nx)
> will change from
> 	(gdb) show history filename 
> 	The filename in which to record the command history is "/tmp/.gdb_history".
> to
> 	(gdb) show history filename 
> 	The filename in which to record the command history is "/home/user/.gdb_history".

Right.

-- 
Pedro Alves

Re: Fwd: [PATCH] Fix for PR gdb/15224 should "set history save on" by default

[Jan Kratochvil]

Hello Muhammad,

as you asked on IRC for a summary of tasks to do.

These tasks should be posted as separate (independent if possible) patches.


(1) Fix relative "set history filename" to by immediately converted to absolute
    form:

On Wed, 03 Apr 2013 15:44:39 +0200, Pedro Alves wrote:
>  $ gdb
>  (gdb) show history filename
>  The filename in which to record the command history is "/tmp/.gdb_history".
>  (gdb) q
> 
> Notice, absolute path above.
> 
>  $ cd /tmp
>  $ mkdir a
>  $ gdb -ex "set history filename .gdbhist"
>  (gdb) show history filename
>  The filename in which to record the command history is ".gdbhist".
>  (gdb) cd a
>  Working directory /tmp/a.
>  (gdb) q
>  $ cat a/.gdbhist
>  show history filename
>  cd a
>  q
>  $ ls .gdbhist a/.gdbhist
>  ls: cannot access .gdbhist: No such file or directory
>  a/.gdbhist
>  $
> 
> So, the current default resolves the relative .gdb_history path
> early on, meaning, the history file written is the same that was
> read, while "set history filename .gdb_history" resolves to
> different files at read and at write times, as seen in the example
> above.  I'd call that a bug too.
+
> Not necessary it seems then.  The above issue should be addressed though.
> Probably by making "set history filename"'s set hook resolve the path.
> Whether to make "show history filename" show just the resolved path, or
> both, I'm undecided.


(2) Unify interactivity tests to use input_from_terminal_p:

On Mon, 01 Apr 2013 16:15:11 +0200, Pedro Alves wrote:
> We don't add commands to history unless we're interactive debugging:
> 
>   /* Add line to history if appropriate.  */
>   if (instream == stdin
>       && ISATTY (stdin) && *linebuffer)
>     add_history (linebuffer);
+
> Probably, it'd be best to use input_from_terminal_p, which also
> checks for --batch mode, among other things.


(3) Fix currently incorrectly touched history file even if no commands get
    added to it:

On Mon, 01 Apr 2013 16:15:11 +0200, Pedro Alves wrote:
> I think however, that whether to write history to file should also
> check whether we're in interactive debugging mode:
> 
>   /* Save the history information if it is appropriate to do so.  */
>   if (write_history_p && history_filename)
>     write_history (history_filename);
> 
> That is, e.g., this touches history:
> 
>  $ ls -als ~/.gdbhist
>  4 -rw-------. 1 pedro pedro 27 Apr  1 14:10 /home/pedro/.gdbhist
>  $ date
>  Mon Apr  1 14:37:01 WEST 2013
>  $ gdb -ex "set history filename ~/.gdbhist" < /dev/null
>  (gdb) quit
>  $ ls -als ~/.gdbhist
>  4 -rw-------. 1 pedro pedro 27 Apr  1 14:37 /home/pedro/.gdbhist
> 
> That, I'd call a bug.
> 
> Probably, it'd be best to use input_from_terminal_p, which also
> checks for --batch mode, among other things.


(4) Modify GDB testsuite to always disable history saving.

> As for testsuites and similar setups, I'm thinking the cases that could
> be affected would be restricted to setups like ours and gcc's, that drive
> gdb with expect, which is precisely designed to launch programs
> interactively, as if a user was running them, so it'd make a lot of sense
> to me to not treat them specially (having them adjust instead).


(5) No action required - MI command "-interpreter-exec console" does not save
    it to the history, it is questionable whether it should or not, it could
    be changed in the future.


(6) Change the default set history filename to ~/.gdb_history.

On Mon, 01 Apr 2013 14:29:30 +0200, Jan Kratochvil wrote:
> I see there should be also changed the default to:
>       set history filename ~/.gdb_history


(7) Enable "set history save on" by default as you already did in your patch.


Thanks,
Jan