[PATCH] Append to input history file instead of overwriting it

[PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

This patch makes readline append new history lines to the GDB history
file on exit instead of overwriting the entire history file on exit.
This change allows us to run multiple simultaneous GDB sessions without
having each session overwrite the added history of each other session on
exit.  It is particularly helpful when debugging GDB with GDB.

Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.

gdb/ChangeLog:

	* top.h (gdb_add_history): Declare.
	* top.c (command_count): New variable.
	(gdb_add_history): New function.
	(quit_force): Append to history file instead of overwriting it.
	(command_line_input): Use gdb_add_history instead of
	add_history.
	* event-top.c (command_line_handler): Likewise.
---
 gdb/event-top.c |  2 +-
 gdb/top.c       | 19 ++++++++++++++++---
 gdb/top.h       |  2 ++
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index cb438ac..490bca6 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -667,7 +667,7 @@ command_line_handler (char *rl)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Note: lines consisting solely of comments are added to the command
      history.  This is useful when you type a command, and then
diff --git a/gdb/top.c b/gdb/top.c
index c4b5c2c..6ec3acc 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -895,7 +895,20 @@ gdb_rl_operate_and_get_next (int count, int key)
 
   return rl_newline (1, key);
 }
-\f
+
+/* Number of user commands executed during this session.  */
+
+static int command_count;
+
+/* Add the user command COMMAND to the input history list.  */
+
+void
+gdb_add_history (const char *command)
+{
+  add_history (command);
+  command_count++;
+}
+
 /* Read one line from the command input stream `instream'
    into the local static buffer `linebuffer' (whose current length
    is `linelength').
@@ -1090,7 +1103,7 @@ command_line_input (char *prompt_arg, int repeat, char *annotation_suffix)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Save into global buffer if appropriate.  */
   if (repeat)
@@ -1441,7 +1454,7 @@ quit_force (char *args, int from_tty)
     {
       if (write_history_p && history_filename
 	  && input_from_terminal_p ())
-	write_history (history_filename);
+	append_history (command_count, history_filename);
     }
   DO_PRINT_EX;
 
diff --git a/gdb/top.h b/gdb/top.h
index 94f6c48..d8baea8 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -79,6 +79,8 @@ extern int history_expansion_p;
 extern int server_command;
 extern char *lim_at_start;
 
+extern void gdb_add_history (const char *);
+
 extern void show_commands (char *args, int from_tty);
 
 extern void set_history (char *, int);
-- 
2.2.0.rc1.23.gf570943

Re: [PATCH] Append to input history file instead of overwriting it

[Sergio Durigan Junior]

On Friday, November 28 2014, Patrick Palka wrote:

> This patch makes readline append new history lines to the GDB history
> file on exit instead of overwriting the entire history file on exit.
> This change allows us to run multiple simultaneous GDB sessions without
> having each session overwrite the added history of each other session on
> exit.  It is particularly helpful when debugging GDB with GDB.
>
> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.

Thanks, Patrick.

The patch makes sense to me, and it is a good thing to have, I agree.

> gdb/ChangeLog:
>
> 	* top.h (gdb_add_history): Declare.
> 	* top.c (command_count): New variable.
> 	(gdb_add_history): New function.
> 	(quit_force): Append to history file instead of overwriting it.
> 	(command_line_input): Use gdb_add_history instead of
> 	add_history.
> 	* event-top.c (command_line_handler): Likewise.
> ---
>  gdb/event-top.c |  2 +-
>  gdb/top.c       | 19 ++++++++++++++++---
>  gdb/top.h       |  2 ++
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index cb438ac..490bca6 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -667,7 +667,7 @@ command_line_handler (char *rl)
>  
>    /* Add line to history if appropriate.  */
>    if (*linebuffer && input_from_terminal_p ())
> -    add_history (linebuffer);
> +    gdb_add_history (linebuffer);
>  
>    /* Note: lines consisting solely of comments are added to the command
>       history.  This is useful when you type a command, and then
> diff --git a/gdb/top.c b/gdb/top.c
> index c4b5c2c..6ec3acc 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -895,7 +895,20 @@ gdb_rl_operate_and_get_next (int count, int key)
>  
>    return rl_newline (1, key);
>  }
> -\f
> +
> +/* Number of user commands executed during this session.  */
> +
> +static int command_count;

You should initialize this variable.

Other than that, looks good.  Let's wait for a maintainer to take a
look.

> +
> +/* Add the user command COMMAND to the input history list.  */
> +
> +void
> +gdb_add_history (const char *command)
> +{
> +  add_history (command);
> +  command_count++;
> +}
> +
>  /* Read one line from the command input stream `instream'
>     into the local static buffer `linebuffer' (whose current length
>     is `linelength').
> @@ -1090,7 +1103,7 @@ command_line_input (char *prompt_arg, int repeat, char *annotation_suffix)
>  
>    /* Add line to history if appropriate.  */
>    if (*linebuffer && input_from_terminal_p ())
> -    add_history (linebuffer);
> +    gdb_add_history (linebuffer);
>  
>    /* Save into global buffer if appropriate.  */
>    if (repeat)
> @@ -1441,7 +1454,7 @@ quit_force (char *args, int from_tty)
>      {
>        if (write_history_p && history_filename
>  	  && input_from_terminal_p ())
> -	write_history (history_filename);
> +	append_history (command_count, history_filename);
>      }
>    DO_PRINT_EX;
>  
> diff --git a/gdb/top.h b/gdb/top.h
> index 94f6c48..d8baea8 100644
> --- a/gdb/top.h
> +++ b/gdb/top.h
> @@ -79,6 +79,8 @@ extern int history_expansion_p;
>  extern int server_command;
>  extern char *lim_at_start;
>  
> +extern void gdb_add_history (const char *);
> +
>  extern void show_commands (char *args, int from_tty);
>  
>  extern void set_history (char *, int);
> -- 
> 2.2.0.rc1.23.gf570943

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Append to input history file instead of overwriting it

[Pedro Alves]

On 11/29/2014 02:01 AM, Patrick Palka wrote:
> This patch makes readline append new history lines to the GDB history
> file on exit instead of overwriting the entire history file on exit.
> This change allows us to run multiple simultaneous GDB sessions without
> having each session overwrite the added history of each other session on
> exit.  It is particularly helpful when debugging GDB with GDB.
> 
> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.

Does this mean the history file will keep growing forever, rather than
be trimmed by the history size?

Thanks,
Pedro Alves

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

On Thu, 4 Dec 2014, Pedro Alves wrote:

> On 11/29/2014 02:01 AM, Patrick Palka wrote:
>> This patch makes readline append new history lines to the GDB history
>> file on exit instead of overwriting the entire history file on exit.
>> This change allows us to run multiple simultaneous GDB sessions without
>> having each session overwrite the added history of each other session on
>> exit.  It is particularly helpful when debugging GDB with GDB.
>>
>> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.
>
> Does this mean the history file will keep growing forever, rather than
> be trimmed by the history size?

That it does... my .gdb_history is up to 2200 lines already!

Looks like we have to explicitly truncate the history file on exit after
appending to it.  Here's v2 of the patch that initializes the static
variable command_count, and calls history_truncate_file() appropriately.
Does it look OK?

>
> Thanks,
> Pedro Alves
>
>

-- >8 --

Subject: [PATCH] Append to input history file instead of overwriting it

This patch makes readline append new history lines to the GDB history
file on exit instead of overwriting the entire history file on exit.
This change allows us to run multiple simultaneous GDB sessions without
having each session overwrite the added history of each other session on
exit.  It is particularly helpful when debugging GDB with GDB.

gdb/ChangeLog:

 	* top.h (gdb_add_history): Declare.
 	* top.c (command_count): New variable.
 	(gdb_add_history): New function.
 	(quit_force): Append to history file instead of overwriting it.
 	Truncate the history file afterwards.
 	(command_line_input): Use gdb_add_history instead of
 	add_history.
 	* event-top.c (command_line_handler): Likewise.
---
  gdb/event-top.c |  2 +-
  gdb/top.c       | 20 +++++++++++++++++---
  gdb/top.h       |  2 ++
  3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index cb438ac..490bca6 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -667,7 +667,7 @@ command_line_handler (char *rl)

    /* Add line to history if appropriate.  */
    if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);

    /* Note: lines consisting solely of comments are added to the command
       history.  This is useful when you type a command, and then
diff --git a/gdb/top.c b/gdb/top.c
index c4b5c2c..9a5ed1e 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -895,7 +895,20 @@ gdb_rl_operate_and_get_next (int count, int key)

    return rl_newline (1, key);
  }
- 
+
+/* Number of user commands executed during this session.  */
+
+static int command_count = 0;
+
+/* Add the user command COMMAND to the input history list.  */
+
+void
+gdb_add_history (const char *command)
+{
+  add_history (command);
+  command_count++;
+}
+
  /* Read one line from the command input stream `instream'
     into the local static buffer `linebuffer' (whose current length
     is `linelength').
@@ -1090,7 +1103,7 @@ command_line_input (char *prompt_arg, int repeat, char *annotation_suffix)

    /* Add line to history if appropriate.  */
    if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);

    /* Save into global buffer if appropriate.  */
    if (repeat)
@@ -1441,7 +1454,8 @@ quit_force (char *args, int from_tty)
      {
        if (write_history_p && history_filename
  	  && input_from_terminal_p ())
-	write_history (history_filename);
+	append_history (command_count, history_filename);
+	history_truncate_file (history_filename, history_max_entries);
      }
    DO_PRINT_EX;

diff --git a/gdb/top.h b/gdb/top.h
index 94f6c48..d8baea8 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -79,6 +79,8 @@ extern int history_expansion_p;
  extern int server_command;
  extern char *lim_at_start;

+extern void gdb_add_history (const char *);
+
  extern void show_commands (char *args, int from_tty);

  extern void set_history (char *, int);
-- 
2.2.0.rc1.23.gf570943

Re: [PATCH] Append to input history file instead of overwriting it

[Pedro Alves]

On 12/05/2014 12:19 AM, Patrick Palka wrote:
> On Thu, 4 Dec 2014, Pedro Alves wrote:
> 
>> On 11/29/2014 02:01 AM, Patrick Palka wrote:
>>> This patch makes readline append new history lines to the GDB history
>>> file on exit instead of overwriting the entire history file on exit.
>>> This change allows us to run multiple simultaneous GDB sessions without
>>> having each session overwrite the added history of each other session on
>>> exit.  It is particularly helpful when debugging GDB with GDB.
>>>
>>> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.
>>
>> Does this mean the history file will keep growing forever, rather than
>> be trimmed by the history size?
> 
> That it does... my .gdb_history is up to 2200 lines already!
> 
> Looks like we have to explicitly truncate the history file on exit after
> appending to it.  Here's v2 of the patch that initializes the static
> variable command_count, and calls history_truncate_file() appropriately.
> Does it look OK?

I'd like to hear how does bash (the canonical readline/history user)
handle this scenario, if at all.  It seems like we're opening ourselves
up for more chances of history file corruption, considering that e.g.,
you may be quitting several gdb's simultaneously (e.g., when SIGTERM
is sent to all processes).  A single write call with O_APPEND should
be atomic, but history_do_write uses mmap if available.  And then
seems like the truncation operation could end up with a broken hist
file as well.

ISTM that if we go this path, we should be doing an atomic update:
create a new file under a parallel name, and then move to final destination.

> This patch makes readline append new history lines to the GDB history
> file on exit instead of overwriting the entire history file on exit.

> This change allows us to run multiple simultaneous GDB sessions without
> having each session overwrite the added history of each other session on
> exit.  

> It is particularly helpful when debugging GDB with GDB.

I'd like to have the description of how this helps that use case
expanded.  I mean, why would you want the history of the top
level gdb mixed with the inferior gdb's history?  Like, in:

 $ ./gdb ./gdb
 (top-gdb) b foo; whatever
 (top-gdb) run
 (gdb) whatever commands to trigger bug
 (gdb) quit // appends "whatever commands to trigger bug" to history
 (top-gdb) quit // appends commands to history
 $ ./gdb ./gdb
 (top-gdb) show commands  // history contains top gdb's commands.
 (top-gdb) run
 (gdb) most recent history is the top-gdb's history


I'd suggest that a better way to handle that use case is to start
the inferior gdb with a different history file, like, e.g.:

 $ export g="./gdb -data-directory=data-directory"
 $ gdb --args $g -ex "set history file /tmp/hist"

(I have that $g export in my .bashrc, since I use it so often.)

> 
> gdb/ChangeLog:
> 
>  	* top.h (gdb_add_history): Declare.
>  	* top.c (command_count): New variable.
>  	(gdb_add_history): New function.
>  	(quit_force): Append to history file instead of overwriting it.
>  	Truncate the history file afterwards.
>  	(command_line_input): Use gdb_add_history instead of
>  	add_history.
>  	* event-top.c (command_line_handler): Likewise.
> ---
>   gdb/event-top.c |  2 +-
>   gdb/top.c       | 20 +++++++++++++++++---
>   gdb/top.h       |  2 ++
>   3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index cb438ac..490bca6 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -667,7 +667,7 @@ command_line_handler (char *rl)
> 
>     /* Add line to history if appropriate.  */
>     if (*linebuffer && input_from_terminal_p ())
> -    add_history (linebuffer);
> +    gdb_add_history (linebuffer);
> 
>     /* Note: lines consisting solely of comments are added to the command
>        history.  This is useful when you type a command, and then
> diff --git a/gdb/top.c b/gdb/top.c
> index c4b5c2c..9a5ed1e 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -895,7 +895,20 @@ gdb_rl_operate_and_get_next (int count, int key)
> 
>     return rl_newline (1, key);
>   }
> - 
> +
> +/* Number of user commands executed during this session.  */
> +
> +static int command_count = 0;
> +
> +/* Add the user command COMMAND to the input history list.  */
> +
> +void
> +gdb_add_history (const char *command)
> +{
> +  add_history (command);
> +  command_count++;
> +}
> +
>   /* Read one line from the command input stream `instream'
>      into the local static buffer `linebuffer' (whose current length
>      is `linelength').
> @@ -1090,7 +1103,7 @@ command_line_input (char *prompt_arg, int repeat, char *annotation_suffix)
> 
>     /* Add line to history if appropriate.  */
>     if (*linebuffer && input_from_terminal_p ())
> -    add_history (linebuffer);
> +    gdb_add_history (linebuffer);
> 
>     /* Save into global buffer if appropriate.  */
>     if (repeat)
> @@ -1441,7 +1454,8 @@ quit_force (char *args, int from_tty)
>       {
>         if (write_history_p && history_filename
>   	  && input_from_terminal_p ())
> -	write_history (history_filename);
> +	append_history (command_count, history_filename);
> +	history_truncate_file (history_filename, history_max_entries);
>       }
>     DO_PRINT_EX;
> 
> diff --git a/gdb/top.h b/gdb/top.h
> index 94f6c48..d8baea8 100644
> --- a/gdb/top.h
> +++ b/gdb/top.h
> @@ -79,6 +79,8 @@ extern int history_expansion_p;
>   extern int server_command;
>   extern char *lim_at_start;
> 
> +extern void gdb_add_history (const char *);
> +
>   extern void show_commands (char *args, int from_tty);
> 
>   extern void set_history (char *, int);
> 


Thanks,
Pedro Alves

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

On Fri, Dec 5, 2014 at 5:45 AM, Pedro Alves <palves@redhat.com> wrote:
> On 12/05/2014 12:19 AM, Patrick Palka wrote:
>> On Thu, 4 Dec 2014, Pedro Alves wrote:
>>
>>> On 11/29/2014 02:01 AM, Patrick Palka wrote:
>>>> This patch makes readline append new history lines to the GDB history
>>>> file on exit instead of overwriting the entire history file on exit.
>>>> This change allows us to run multiple simultaneous GDB sessions without
>>>> having each session overwrite the added history of each other session on
>>>> exit.  It is particularly helpful when debugging GDB with GDB.
>>>>
>>>> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.
>>>
>>> Does this mean the history file will keep growing forever, rather than
>>> be trimmed by the history size?
>>
>> That it does... my .gdb_history is up to 2200 lines already!
>>
>> Looks like we have to explicitly truncate the history file on exit after
>> appending to it.  Here's v2 of the patch that initializes the static
>> variable command_count, and calls history_truncate_file() appropriately.
>> Does it look OK?
>
> I'd like to hear how does bash (the canonical readline/history user)
> handle this scenario, if at all.

It looks like bash truncates the history file size whenever the
$HISTFILESIZE variable is changed (which is usually at startup when it
reads ~/.bashrc), not on exit like this patch does.  It doesn't do any
kind of file-level locking for the truncation operation or for
write_history() or append_history().  It writes directly to $HISTFILE.

> It seems like we're opening ourselves
> up for more chances of history file corruption, considering that e.g.,
> you may be quitting several gdb's simultaneously (e.g., when SIGTERM
> is sent to all processes).  A single write call with O_APPEND should
> be atomic, but history_do_write uses mmap if available.  And then
> seems like the truncation operation could end up with a broken hist
> file as well.
> ISTM that if we go this path, we should be doing an atomic update:
> create a new file under a parallel name, and then move to final destination.

history_truncate_file() is definitely not atomic and does not look
obviously thread-safe, but if bash gets away with not doing any kind
of file-level locking, then can't we? :)

>
>> This patch makes readline append new history lines to the GDB history
>> file on exit instead of overwriting the entire history file on exit.
>
>> This change allows us to run multiple simultaneous GDB sessions without
>> having each session overwrite the added history of each other session on
>> exit.
>
>> It is particularly helpful when debugging GDB with GDB.
>
> I'd like to have the description of how this helps that use case
> expanded.  I mean, why would you want the history of the top
> level gdb mixed with the inferior gdb's history?  Like, in:

I don't necessarily want to, but I think such behavior is more useful
than not retaining the inferior gdb's history at all.  Besides I don't
debug GDB that way.

In one shell I open "gdb", in another I open "gdb -p $(pidof gdb)" and
I execute commands in both processes from time to time.  In such a
scenario, the contents of the history file depends on which gdb
process I close first.  I would rather like to have the history file
retain the commands of both processes.  This problem is not peculiar
to debugging GDB with GDB, rather it's encountered whenever there are
multiple GDB processes running simultaneously.  So I would rather
remove that remark from the commit message ("It is particularly
helpful when debugging GDB with GDB.") because it's not really true.

>
>  $ ./gdb ./gdb
>  (top-gdb) b foo; whatever
>  (top-gdb) run
>  (gdb) whatever commands to trigger bug
>  (gdb) quit // appends "whatever commands to trigger bug" to history
>  (top-gdb) quit // appends commands to history
>  $ ./gdb ./gdb
>  (top-gdb) show commands  // history contains top gdb's commands.
>  (top-gdb) run
>  (gdb) most recent history is the top-gdb's history
>
>
> I'd suggest that a better way to handle that use case is to start
> the inferior gdb with a different history file, like, e.g.:
>
>  $ export g="./gdb -data-directory=data-directory"
>  $ gdb --args $g -ex "set history file /tmp/hist"
>
> (I have that $g export in my .bashrc, since I use it so often.)
>
>>
>> gdb/ChangeLog:
>>
>>       * top.h (gdb_add_history): Declare.
>>       * top.c (command_count): New variable.
>>       (gdb_add_history): New function.
>>       (quit_force): Append to history file instead of overwriting it.
>>       Truncate the history file afterwards.
>>       (command_line_input): Use gdb_add_history instead of
>>       add_history.
>>       * event-top.c (command_line_handler): Likewise.
>> ---
>>   gdb/event-top.c |  2 +-
>>   gdb/top.c       | 20 +++++++++++++++++---
>>   gdb/top.h       |  2 ++
>>   3 files changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/event-top.c b/gdb/event-top.c
>> index cb438ac..490bca6 100644
>> --- a/gdb/event-top.c
>> +++ b/gdb/event-top.c
>> @@ -667,7 +667,7 @@ command_line_handler (char *rl)
>>
>>     /* Add line to history if appropriate.  */
>>     if (*linebuffer && input_from_terminal_p ())
>> -    add_history (linebuffer);
>> +    gdb_add_history (linebuffer);
>>
>>     /* Note: lines consisting solely of comments are added to the command
>>        history.  This is useful when you type a command, and then
>> diff --git a/gdb/top.c b/gdb/top.c
>> index c4b5c2c..9a5ed1e 100644
>> --- a/gdb/top.c
>> +++ b/gdb/top.c
>> @@ -895,7 +895,20 @@ gdb_rl_operate_and_get_next (int count, int key)
>>
>>     return rl_newline (1, key);
>>   }
>> -
>> +
>> +/* Number of user commands executed during this session.  */
>> +
>> +static int command_count = 0;
>> +
>> +/* Add the user command COMMAND to the input history list.  */
>> +
>> +void
>> +gdb_add_history (const char *command)
>> +{
>> +  add_history (command);
>> +  command_count++;
>> +}
>> +
>>   /* Read one line from the command input stream `instream'
>>      into the local static buffer `linebuffer' (whose current length
>>      is `linelength').
>> @@ -1090,7 +1103,7 @@ command_line_input (char *prompt_arg, int repeat, char *annotation_suffix)
>>
>>     /* Add line to history if appropriate.  */
>>     if (*linebuffer && input_from_terminal_p ())
>> -    add_history (linebuffer);
>> +    gdb_add_history (linebuffer);
>>
>>     /* Save into global buffer if appropriate.  */
>>     if (repeat)
>> @@ -1441,7 +1454,8 @@ quit_force (char *args, int from_tty)
>>       {
>>         if (write_history_p && history_filename
>>         && input_from_terminal_p ())
>> -     write_history (history_filename);
>> +     append_history (command_count, history_filename);
>> +     history_truncate_file (history_filename, history_max_entries);
>>       }
>>     DO_PRINT_EX;
>>
>> diff --git a/gdb/top.h b/gdb/top.h
>> index 94f6c48..d8baea8 100644
>> --- a/gdb/top.h
>> +++ b/gdb/top.h
>> @@ -79,6 +79,8 @@ extern int history_expansion_p;
>>   extern int server_command;
>>   extern char *lim_at_start;
>>
>> +extern void gdb_add_history (const char *);
>> +
>>   extern void show_commands (char *args, int from_tty);
>>
>>   extern void set_history (char *, int);
>>
>
>
> Thanks,
> Pedro Alves

Re: [PATCH] Append to input history file instead of overwriting it

[Pedro Alves]

On 12/05/2014 02:11 PM, Patrick Palka wrote:
> On Fri, Dec 5, 2014 at 5:45 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 12/05/2014 12:19 AM, Patrick Palka wrote:
>>> On Thu, 4 Dec 2014, Pedro Alves wrote:
>>>
>>>> On 11/29/2014 02:01 AM, Patrick Palka wrote:
>>>>> This patch makes readline append new history lines to the GDB history
>>>>> file on exit instead of overwriting the entire history file on exit.
>>>>> This change allows us to run multiple simultaneous GDB sessions without
>>>>> having each session overwrite the added history of each other session on
>>>>> exit.  It is particularly helpful when debugging GDB with GDB.
>>>>>
>>>>> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.
>>>>
>>>> Does this mean the history file will keep growing forever, rather than
>>>> be trimmed by the history size?
>>>
>>> That it does... my .gdb_history is up to 2200 lines already!
>>>
>>> Looks like we have to explicitly truncate the history file on exit after
>>> appending to it.  Here's v2 of the patch that initializes the static
>>> variable command_count, and calls history_truncate_file() appropriately.
>>> Does it look OK?
>>
>> I'd like to hear how does bash (the canonical readline/history user)
>> handle this scenario, if at all.
> 
> It looks like bash truncates the history file size whenever the
> $HISTFILESIZE variable is changed (which is usually at startup when it
> reads ~/.bashrc), not on exit like this patch does.  It doesn't do any
> kind of file-level locking for the truncation operation or for
> write_history() or append_history().  It writes directly to $HISTFILE.

Bah.

Is it that hard to do though?  How about temporarily renaming the
history file to something that includes gdb's PID (and would not a
file name a user would use in practice) while we append
to it, and then (atomically) rename it back?  Something like:

 #1 - move $HISTFILE -> $HISTFILE-gdb$PID~
 #2 - write/append history to $HISTFILE-gdb$PID~
 #3 - move $HISTFILE-gdb$PID~ -> $HISTFILE

We can then use non-atomic file access at will in #2, as no
other process will be writing to that file (unless the user
does evil thinks with "set history filename2, but then he deserves
what he gets).

This way, if two GDB's race writing to the file, then we'll lose the
history of one of them, but that's better than ending up with a
corrupted file, IMO.

> 
>> It seems like we're opening ourselves
>> up for more chances of history file corruption, considering that e.g.,
>> you may be quitting several gdb's simultaneously (e.g., when SIGTERM
>> is sent to all processes).  A single write call with O_APPEND should
>> be atomic, but history_do_write uses mmap if available.  And then
>> seems like the truncation operation could end up with a broken hist
>> file as well.
>> ISTM that if we go this path, we should be doing an atomic update:
>> create a new file under a parallel name, and then move to final destination.
> 
> history_truncate_file() is definitely not atomic and does not look
> obviously thread-safe, but if bash gets away with not doing any kind
> of file-level locking, then can't we? :)
> 
>>
>>> This patch makes readline append new history lines to the GDB history
>>> file on exit instead of overwriting the entire history file on exit.
>>
>>> This change allows us to run multiple simultaneous GDB sessions without
>>> having each session overwrite the added history of each other session on
>>> exit.
>>
>>> It is particularly helpful when debugging GDB with GDB.
>>
>> I'd like to have the description of how this helps that use case
>> expanded.  I mean, why would you want the history of the top
>> level gdb mixed with the inferior gdb's history?  Like, in:
> 
> I don't necessarily want to, but I think such behavior is more useful
> than not retaining the inferior gdb's history at all.  Besides I don't
> debug GDB that way.
> 
> In one shell I open "gdb", in another I open "gdb -p $(pidof gdb)" and
> I execute commands in both processes from time to time.  In such a
> scenario, the contents of the history file depends on which gdb
> process I close first.  I would rather like to have the history file
> retain the commands of both processes.

That still sounds odd to me -- the commands issued in the inferior gdb
should be of no use to the other gdb, IMO.

> This problem is not peculiar
> to debugging GDB with GDB, rather it's encountered whenever there are
> multiple GDB processes running simultaneously.  

Yes, that makes more sense.

> So I would rather
> remove that remark from the commit message ("It is particularly
> helpful when debugging GDB with GDB.") because it's not really true.

Alright.

Thanks,
Pedro Alves

Re: [PATCH] Append to input history file instead of overwriting it

[Eli Zaretskii]

> Date: Wed, 10 Dec 2014 16:54:13 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> Is it that hard to do though?  How about temporarily renaming the
> history file to something that includes gdb's PID (and would not a
> file name a user would use in practice) while we append
> to it, and then (atomically) rename it back?  Something like:
> 
>  #1 - move $HISTFILE -> $HISTFILE-gdb$PID~
>  #2 - write/append history to $HISTFILE-gdb$PID~
>  #3 - move $HISTFILE-gdb$PID~ -> $HISTFILE

You cannot portably rename a file someone writes to, unless you are
willing to limit this to Posix filesystems.

Re: [PATCH] Append to input history file instead of overwriting it

[Pedro Alves]

On 12/10/2014 05:12 PM, Eli Zaretskii wrote:
>> Date: Wed, 10 Dec 2014 16:54:13 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: gdb-patches@sourceware.org
>>
>> Is it that hard to do though?  How about temporarily renaming the
>> history file to something that includes gdb's PID (and would not a
>> file name a user would use in practice) while we append
>> to it, and then (atomically) rename it back?  Something like:
>>
>>  #1 - move $HISTFILE -> $HISTFILE-gdb$PID~
>>  #2 - write/append history to $HISTFILE-gdb$PID~
>>  #3 - move $HISTFILE-gdb$PID~ -> $HISTFILE
> 
> You cannot portably rename a file someone writes to, unless you are
> willing to limit this to Posix filesystems.

The way I've proposed, no gdb would be actually writing directly
to $HISTFILE, only reading.  Is it a problem still?

Not considering someone manually opening the file for writing, but
in that case the system that would fail the rename would fail the
write-in-place too.

Thanks,
Pedro Alves

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

On Wed, Dec 10, 2014 at 11:54 AM, Pedro Alves <palves@redhat.com> wrote:
> On 12/05/2014 02:11 PM, Patrick Palka wrote:
>> On Fri, Dec 5, 2014 at 5:45 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 12/05/2014 12:19 AM, Patrick Palka wrote:
>>>> On Thu, 4 Dec 2014, Pedro Alves wrote:
>>>>
>>>>> On 11/29/2014 02:01 AM, Patrick Palka wrote:
>>>>>> This patch makes readline append new history lines to the GDB history
>>>>>> file on exit instead of overwriting the entire history file on exit.
>>>>>> This change allows us to run multiple simultaneous GDB sessions without
>>>>>> having each session overwrite the added history of each other session on
>>>>>> exit.  It is particularly helpful when debugging GDB with GDB.
>>>>>>
>>>>>> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.
>>>>>
>>>>> Does this mean the history file will keep growing forever, rather than
>>>>> be trimmed by the history size?
>>>>
>>>> That it does... my .gdb_history is up to 2200 lines already!
>>>>
>>>> Looks like we have to explicitly truncate the history file on exit after
>>>> appending to it.  Here's v2 of the patch that initializes the static
>>>> variable command_count, and calls history_truncate_file() appropriately.
>>>> Does it look OK?
>>>
>>> I'd like to hear how does bash (the canonical readline/history user)
>>> handle this scenario, if at all.
>>
>> It looks like bash truncates the history file size whenever the
>> $HISTFILESIZE variable is changed (which is usually at startup when it
>> reads ~/.bashrc), not on exit like this patch does.  It doesn't do any
>> kind of file-level locking for the truncation operation or for
>> write_history() or append_history().  It writes directly to $HISTFILE.
>
> Bah.
>
> Is it that hard to do though?  How about temporarily renaming the
> history file to something that includes gdb's PID (and would not a
> file name a user would use in practice) while we append
> to it, and then (atomically) rename it back?  Something like:
>
>  #1 - move $HISTFILE -> $HISTFILE-gdb$PID~
>  #2 - write/append history to $HISTFILE-gdb$PID~
>  #3 - move $HISTFILE-gdb$PID~ -> $HISTFILE
>
> We can then use non-atomic file access at will in #2, as no
> other process will be writing to that file (unless the user
> does evil thinks with "set history filename2, but then he deserves
> what he gets).
>
> This way, if two GDB's race writing to the file, then we'll lose the
> history of one of them, but that's better than ending up with a
> corrupted file, IMO.

With your renaming method, what to do if the user has no .gdb_history
file to begin with?  How would you tell the difference between the
case of having no .gdb_history and the case of another process in the
middle of writing to the history file (and thus having temporarily
renamed it)? In either case it looks like the .gdb_history file
doesn't exist.  But in the first case we want to write a history file
anyway, and in the second case we want to give up on writing to the
history file.  But it doesn't seem possible to tell the difference
between these two cases with your proposed method.

>
>>
>>> It seems like we're opening ourselves
>>> up for more chances of history file corruption, considering that e.g.,
>>> you may be quitting several gdb's simultaneously (e.g., when SIGTERM
>>> is sent to all processes).  A single write call with O_APPEND should
>>> be atomic, but history_do_write uses mmap if available.  And then
>>> seems like the truncation operation could end up with a broken hist
>>> file as well.
>>> ISTM that if we go this path, we should be doing an atomic update:
>>> create a new file under a parallel name, and then move to final destination.
>>
>> history_truncate_file() is definitely not atomic and does not look
>> obviously thread-safe, but if bash gets away with not doing any kind
>> of file-level locking, then can't we? :)
>>
>>>
>>>> This patch makes readline append new history lines to the GDB history
>>>> file on exit instead of overwriting the entire history file on exit.
>>>
>>>> This change allows us to run multiple simultaneous GDB sessions without
>>>> having each session overwrite the added history of each other session on
>>>> exit.
>>>
>>>> It is particularly helpful when debugging GDB with GDB.
>>>
>>> I'd like to have the description of how this helps that use case
>>> expanded.  I mean, why would you want the history of the top
>>> level gdb mixed with the inferior gdb's history?  Like, in:
>>
>> I don't necessarily want to, but I think such behavior is more useful
>> than not retaining the inferior gdb's history at all.  Besides I don't
>> debug GDB that way.
>>
>> In one shell I open "gdb", in another I open "gdb -p $(pidof gdb)" and
>> I execute commands in both processes from time to time.  In such a
>> scenario, the contents of the history file depends on which gdb
>> process I close first.  I would rather like to have the history file
>> retain the commands of both processes.
>
> That still sounds odd to me -- the commands issued in the inferior gdb
> should be of no use to the other gdb, IMO.
>
>> This problem is not peculiar
>> to debugging GDB with GDB, rather it's encountered whenever there are
>> multiple GDB processes running simultaneously.
>
> Yes, that makes more sense.
>
>> So I would rather
>> remove that remark from the commit message ("It is particularly
>> helpful when debugging GDB with GDB.") because it's not really true.
>
> Alright.
>
> Thanks,
> Pedro Alves

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

On Sat, Jan 10, 2015 at 9:09 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Wed, Dec 10, 2014 at 11:54 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 12/05/2014 02:11 PM, Patrick Palka wrote:
>>> On Fri, Dec 5, 2014 at 5:45 AM, Pedro Alves <palves@redhat.com> wrote:
>>>> On 12/05/2014 12:19 AM, Patrick Palka wrote:
>>>>> On Thu, 4 Dec 2014, Pedro Alves wrote:
>>>>>
>>>>>> On 11/29/2014 02:01 AM, Patrick Palka wrote:
>>>>>>> This patch makes readline append new history lines to the GDB history
>>>>>>> file on exit instead of overwriting the entire history file on exit.
>>>>>>> This change allows us to run multiple simultaneous GDB sessions without
>>>>>>> having each session overwrite the added history of each other session on
>>>>>>> exit.  It is particularly helpful when debugging GDB with GDB.
>>>>>>>
>>>>>>> Does this look OK to commit?  Tested on x86_64-unknown-linux-gnu.
>>>>>>
>>>>>> Does this mean the history file will keep growing forever, rather than
>>>>>> be trimmed by the history size?
>>>>>
>>>>> That it does... my .gdb_history is up to 2200 lines already!
>>>>>
>>>>> Looks like we have to explicitly truncate the history file on exit after
>>>>> appending to it.  Here's v2 of the patch that initializes the static
>>>>> variable command_count, and calls history_truncate_file() appropriately.
>>>>> Does it look OK?
>>>>
>>>> I'd like to hear how does bash (the canonical readline/history user)
>>>> handle this scenario, if at all.
>>>
>>> It looks like bash truncates the history file size whenever the
>>> $HISTFILESIZE variable is changed (which is usually at startup when it
>>> reads ~/.bashrc), not on exit like this patch does.  It doesn't do any
>>> kind of file-level locking for the truncation operation or for
>>> write_history() or append_history().  It writes directly to $HISTFILE.
>>
>> Bah.
>>
>> Is it that hard to do though?  How about temporarily renaming the
>> history file to something that includes gdb's PID (and would not a
>> file name a user would use in practice) while we append
>> to it, and then (atomically) rename it back?  Something like:
>>
>>  #1 - move $HISTFILE -> $HISTFILE-gdb$PID~
>>  #2 - write/append history to $HISTFILE-gdb$PID~
>>  #3 - move $HISTFILE-gdb$PID~ -> $HISTFILE
>>
>> We can then use non-atomic file access at will in #2, as no
>> other process will be writing to that file (unless the user
>> does evil thinks with "set history filename2, but then he deserves
>> what he gets).
>>
>> This way, if two GDB's race writing to the file, then we'll lose the
>> history of one of them, but that's better than ending up with a
>> corrupted file, IMO.
>
> With your renaming method, what to do if the user has no .gdb_history
> file to begin with?  How would you tell the difference between the
> case of having no .gdb_history and the case of another process in the
> middle of writing to the history file (and thus having temporarily
> renamed it)? In either case it looks like the .gdb_history file
> doesn't exist.  But in the first case we want to write a history file
> anyway, and in the second case we want to give up on writing to the
> history file.  But it doesn't seem possible to tell the difference
> between these two cases with your proposed method.

.... therefore we must conservatively assume case #1, that the history
file does not exist, and to write (not append) the process's known
history to the local history file and try to rename it back anyway.

>
>>
>>>
>>>> It seems like we're opening ourselves
>>>> up for more chances of history file corruption, considering that e.g.,
>>>> you may be quitting several gdb's simultaneously (e.g., when SIGTERM
>>>> is sent to all processes).  A single write call with O_APPEND should
>>>> be atomic, but history_do_write uses mmap if available.  And then
>>>> seems like the truncation operation could end up with a broken hist
>>>> file as well.
>>>> ISTM that if we go this path, we should be doing an atomic update:
>>>> create a new file under a parallel name, and then move to final destination.
>>>
>>> history_truncate_file() is definitely not atomic and does not look
>>> obviously thread-safe, but if bash gets away with not doing any kind
>>> of file-level locking, then can't we? :)
>>>
>>>>
>>>>> This patch makes readline append new history lines to the GDB history
>>>>> file on exit instead of overwriting the entire history file on exit.
>>>>
>>>>> This change allows us to run multiple simultaneous GDB sessions without
>>>>> having each session overwrite the added history of each other session on
>>>>> exit.
>>>>
>>>>> It is particularly helpful when debugging GDB with GDB.
>>>>
>>>> I'd like to have the description of how this helps that use case
>>>> expanded.  I mean, why would you want the history of the top
>>>> level gdb mixed with the inferior gdb's history?  Like, in:
>>>
>>> I don't necessarily want to, but I think such behavior is more useful
>>> than not retaining the inferior gdb's history at all.  Besides I don't
>>> debug GDB that way.
>>>
>>> In one shell I open "gdb", in another I open "gdb -p $(pidof gdb)" and
>>> I execute commands in both processes from time to time.  In such a
>>> scenario, the contents of the history file depends on which gdb
>>> process I close first.  I would rather like to have the history file
>>> retain the commands of both processes.
>>
>> That still sounds odd to me -- the commands issued in the inferior gdb
>> should be of no use to the other gdb, IMO.
>>
>>> This problem is not peculiar
>>> to debugging GDB with GDB, rather it's encountered whenever there are
>>> multiple GDB processes running simultaneously.
>>
>> Yes, that makes more sense.
>>
>>> So I would rather
>>> remove that remark from the commit message ("It is particularly
>>> helpful when debugging GDB with GDB.") because it's not really true.
>>
>> Alright.
>>
>> Thanks,
>> Pedro Alves

[PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

This patch makes readline append new history lines to the GDB history
file on exit instead of overwriting the entire history file on exit.
This change allows us to run multiple simultaneous GDB sessions without
having each session overwrite the added history of each other session on
exit.

Care must be taken to ensure that the history file doesn't get corrupted
when multiple GDB processes are trying to simultaneously append to and
then truncate it.  Safety is achieved in such a situation by using an
intermediate local history file to mutually exclude multiple processes
from simultaneoeusly performing write operations on the global history
file.

gdb/ChangeLog:

	* top.h (gdb_add_history): Declare.
	* top.c (command_count): New variable.
	(gdb_add_history): New function.
	(gdb_safe_append_history): New static function.
	(quit_force): Call it.
	(command_line_input): Use gdb_add_history instead of
	add_history.
	* event-top.c (command_line_handler): Likewise.
---
 gdb/event-top.c |  2 +-
 gdb/top.c       | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 gdb/top.h       |  2 ++
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 13ddee2..bbda5dc 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -667,7 +667,7 @@ command_line_handler (char *rl)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Note: lines consisting solely of comments are added to the command
      history.  This is useful when you type a command, and then
diff --git a/gdb/top.c b/gdb/top.c
index b85ea1a..19fc1d2 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -895,7 +895,60 @@ gdb_rl_operate_and_get_next (int count, int key)
 
   return rl_newline (1, key);
 }
-\f
+
+/* Number of user commands executed during this session.  */
+
+static int command_count = 0;
+
+/* Add the user command COMMAND to the input history list.  */
+
+void
+gdb_add_history (const char *command)
+{
+  add_history (command);
+  command_count++;
+}
+
+/* Safely append new history entries to the history file in a corruption-free
+   way using an intermediate local history file.  */
+
+static void
+gdb_safe_append_history (void)
+{
+  int ret, saved_errno;
+  char *local_history_filename;
+  struct cleanup *old_chain;
+
+  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());
+  old_chain = make_cleanup (xfree, local_history_filename);
+
+  ret = rename (history_filename, local_history_filename);
+  if (ret < 0)
+    {
+      /* If the rename failed then either the global history file never existed
+         in the first place or another GDB process is currently appending to it
+         (and has thus temporarily renamed it).  Since we can't distinguish
+         between these two cases, we have to conservatively assume the first
+         case and therefore must write out (not append) our known history to
+         our local history file and try to move it back anyway.  Otherwise a
+         global history file would never get created!  */
+      write_history (local_history_filename);
+    }
+  else
+    {
+      append_history (command_count, local_history_filename);
+      history_truncate_file (local_history_filename, history_max_entries);
+    }
+
+  ret = rename (local_history_filename, history_filename);
+  saved_errno = errno;
+  if (ret < 0)
+    warning (_("Could not rename %s to %s: error %d"),
+	     local_history_filename, history_filename, saved_errno);
+
+  do_cleanups (old_chain);
+}
+
 /* Read one line from the command input stream `instream'
    into the local static buffer `linebuffer' (whose current length
    is `linelength').
@@ -1094,7 +1147,7 @@ command_line_input (const char *prompt_arg, int repeat, char *annotation_suffix)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Save into global buffer if appropriate.  */
   if (repeat)
@@ -1445,7 +1498,7 @@ quit_force (char *args, int from_tty)
     {
       if (write_history_p && history_filename
 	  && input_from_terminal_p ())
-	write_history (history_filename);
+	gdb_safe_append_history ();
     }
   DO_PRINT_EX;
 
diff --git a/gdb/top.h b/gdb/top.h
index b68e896..987279b 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -79,6 +79,8 @@ extern int history_expansion_p;
 extern int server_command;
 extern char *lim_at_start;
 
+extern void gdb_add_history (const char *);
+
 extern void show_commands (char *args, int from_tty);
 
 extern void set_history (char *, int);
-- 
2.2.1.212.gc5b9256

Re: [PATCH] Append to input history file instead of overwriting it

[Eli Zaretskii]

> From: Patrick Palka <patrick@parcs.ath.cx>
> Cc: Patrick Palka <patrick@parcs.ath.cx>
> Date: Sat, 10 Jan 2015 10:18:28 -0500
> 
> +  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());
> +  old_chain = make_cleanup (xfree, local_history_filename);
> +
> +  ret = rename (history_filename, local_history_filename);
> +  if (ret < 0)
> +    {
> +      /* If the rename failed then either the global history file never existed
> +         in the first place or another GDB process is currently appending to it
> +         (and has thus temporarily renamed it).  Since we can't distinguish
> +         between these two cases, we have to conservatively assume the first
> +         case and therefore must write out (not append) our known history to
> +         our local history file and try to move it back anyway.  Otherwise a
> +         global history file would never get created!  */
> +      write_history (local_history_filename);
> +    }
> +  else
> +    {
> +      append_history (command_count, local_history_filename);
> +      history_truncate_file (local_history_filename, history_max_entries);
> +    }
> +
> +  ret = rename (local_history_filename, history_filename);
> +  saved_errno = errno;
> +  if (ret < 0)
> +    warning (_("Could not rename %s to %s: error %d"),
> +	     local_history_filename, history_filename, saved_errno);
> +
> +  do_cleanups (old_chain);

On Windows, a call to 'rename' fails if the destination already
exists.  Does the logic here cope with that?

Thanks.

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

On Sat, Jan 10, 2015 at 10:39 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Patrick Palka <patrick@parcs.ath.cx>
>> Cc: Patrick Palka <patrick@parcs.ath.cx>
>> Date: Sat, 10 Jan 2015 10:18:28 -0500
>>
>> +  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());
>> +  old_chain = make_cleanup (xfree, local_history_filename);
>> +
>> +  ret = rename (history_filename, local_history_filename);
>> +  if (ret < 0)
>> +    {
>> +      /* If the rename failed then either the global history file never existed
>> +         in the first place or another GDB process is currently appending to it
>> +         (and has thus temporarily renamed it).  Since we can't distinguish
>> +         between these two cases, we have to conservatively assume the first
>> +         case and therefore must write out (not append) our known history to
>> +         our local history file and try to move it back anyway.  Otherwise a
>> +         global history file would never get created!  */
>> +      write_history (local_history_filename);
>> +    }
>> +  else
>> +    {
>> +      append_history (command_count, local_history_filename);
>> +      history_truncate_file (local_history_filename, history_max_entries);
>> +    }
>> +
>> +  ret = rename (local_history_filename, history_filename);
>> +  saved_errno = errno;
>> +  if (ret < 0)
>> +    warning (_("Could not rename %s to %s: error %d"),
>> +          local_history_filename, history_filename, saved_errno);
>> +
>> +  do_cleanups (old_chain);
>
> On Windows, a call to 'rename' fails if the destination already
> exists.  Does the logic here cope with that?

Hmm, the logic does not really cope with Windows' behavior here,
because the above warning should only get emitted for unexpected
failures.  So I suppose we should only emit the above warning if errno
!= EBUSY (perhaps only on Windows systems)?

>
> Thanks.

Re: [PATCH] Append to input history file instead of overwriting it

[Eli Zaretskii]

> From: Patrick Palka <patrick@parcs.ath.cx>
> Date: Sat, 10 Jan 2015 10:48:03 -0500
> Cc: gdb-patches@sourceware.org
> 
> > On Windows, a call to 'rename' fails if the destination already
> > exists.  Does the logic here cope with that?
> 
> Hmm, the logic does not really cope with Windows' behavior here,
> because the above warning should only get emitted for unexpected
> failures.  So I suppose we should only emit the above warning if errno
> != EBUSY (perhaps only on Windows systems)?

Why EBUSY?

We could also explicitly remove the target before the rename call (and
ignore any errors from that), which will make it work like on Posix
systems.

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

On Sat, Jan 10, 2015 at 11:03 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Patrick Palka <patrick@parcs.ath.cx>
>> Date: Sat, 10 Jan 2015 10:48:03 -0500
>> Cc: gdb-patches@sourceware.org
>>
>> > On Windows, a call to 'rename' fails if the destination already
>> > exists.  Does the logic here cope with that?
>>
>> Hmm, the logic does not really cope with Windows' behavior here,
>> because the above warning should only get emitted for unexpected
>> failures.  So I suppose we should only emit the above warning if errno
>> != EBUSY (perhaps only on Windows systems)?
>
> Why EBUSY?

Just a wild guess.  What would be the correct error code to check for?
 Looks like it would be EACCES..

>
> We could also explicitly remove the target before the rename call (and
> ignore any errors from that), which will make it work like on Posix
> systems.

I don't think that would be sufficient.  In a hypothetical but
plausible scenario, process GDB1 would call unlink(), process GDB2
would then call unlink(), process GDB1 would then call rename()
successfully, process GDB2 would then call rename() and fail on
Windows despite calling unlink() on the destination.

Re: [PATCH] Append to input history file instead of overwriting it

[Eli Zaretskii]

> From: Patrick Palka <patrick@parcs.ath.cx>
> Date: Sat, 10 Jan 2015 11:17:56 -0500
> Cc: gdb-patches@sourceware.org
> 
> On Sat, Jan 10, 2015 at 11:03 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> From: Patrick Palka <patrick@parcs.ath.cx>
> >> Date: Sat, 10 Jan 2015 10:48:03 -0500
> >> Cc: gdb-patches@sourceware.org
> >>
> >> > On Windows, a call to 'rename' fails if the destination already
> >> > exists.  Does the logic here cope with that?
> >>
> >> Hmm, the logic does not really cope with Windows' behavior here,
> >> because the above warning should only get emitted for unexpected
> >> failures.  So I suppose we should only emit the above warning if errno
> >> != EBUSY (perhaps only on Windows systems)?
> >
> > Why EBUSY?
> 
> Just a wild guess.  What would be the correct error code to check for?
>  Looks like it would be EACCES..

According to my testing, it's EEXIST.

> > We could also explicitly remove the target before the rename call (and
> > ignore any errors from that), which will make it work like on Posix
> > systems.
> 
> I don't think that would be sufficient.  In a hypothetical but
> plausible scenario, process GDB1 would call unlink(), process GDB2
> would then call unlink(), process GDB1 would then call rename()
> successfully, process GDB2 would then call rename() and fail on
> Windows despite calling unlink() on the destination.

What would you suggest that GDB2 does instead?  I see no solution here
that would not fail in some way.  Do you?

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

On Sat, Jan 10, 2015 at 11:41 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Patrick Palka <patrick@parcs.ath.cx>
>> Date: Sat, 10 Jan 2015 11:17:56 -0500
>> Cc: gdb-patches@sourceware.org
>>
>> On Sat, Jan 10, 2015 at 11:03 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> >> From: Patrick Palka <patrick@parcs.ath.cx>
>> >> Date: Sat, 10 Jan 2015 10:48:03 -0500
>> >> Cc: gdb-patches@sourceware.org
>> >>
>> >> > On Windows, a call to 'rename' fails if the destination already
>> >> > exists.  Does the logic here cope with that?
>> >>
>> >> Hmm, the logic does not really cope with Windows' behavior here,
>> >> because the above warning should only get emitted for unexpected
>> >> failures.  So I suppose we should only emit the above warning if errno
>> >> != EBUSY (perhaps only on Windows systems)?
>> >
>> > Why EBUSY?
>>
>> Just a wild guess.  What would be the correct error code to check for?
>>  Looks like it would be EACCES..
>
> According to my testing, it's EEXIST.

Ah OK.

>
>> > We could also explicitly remove the target before the rename call (and
>> > ignore any errors from that), which will make it work like on Posix
>> > systems.
>>
>> I don't think that would be sufficient.  In a hypothetical but
>> plausible scenario, process GDB1 would call unlink(), process GDB2
>> would then call unlink(), process GDB1 would then call rename()
>> successfully, process GDB2 would then call rename() and fail on
>> Windows despite calling unlink() on the destination.
>
> What would you suggest that GDB2 does instead?  I see no solution here
> that would not fail in some way.  Do you?

I would just let it fail.  It's no no big deal, just an
inconsequential change in semantics: on POSIX, the last process to
perform the rename wins the race; on Windows, the first process to
perform the rename wins the race.  Yet in either case we end up with a
more-or-less up-to-date uncorrupted history file.

[PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

v2: Emit a warning if the first call to rename() fails with errno !=
      ENOENT.
    Don't emit a warning if the second call to rename() fails with
      EEXIST.

This patch makes readline append new history lines to the GDB history
file on exit instead of overwriting the entire history file on exit.
This change allows us to run multiple simultaneous GDB sessions without
having each session overwrite the added history of each other session on
exit.

Care must be taken to ensure that the history file doesn't get corrupted
when multiple GDB processes are trying to simultaneously append to and
then truncate it.  Safety is achieved in such a situation by using an
intermediate local history file to mutually exclude multiple processes
from simultaneously performing write operations on the global history
file.

gdb/ChangeLog:

	* top.h (gdb_add_history): Declare.
	* top.c (command_count): New variable.
	(gdb_add_history): New function.
	(gdb_safe_append_history): New static function.
	(quit_force): Call it.
	(command_line_input): Use gdb_add_history instead of
	add_history.
	* event-top.c (command_line_handler): Likewise.
---
 gdb/event-top.c |  2 +-
 gdb/top.c       | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 gdb/top.h       |  2 ++
 3 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 13ddee2..bbda5dc 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -667,7 +667,7 @@ command_line_handler (char *rl)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Note: lines consisting solely of comments are added to the command
      history.  This is useful when you type a command, and then
diff --git a/gdb/top.c b/gdb/top.c
index b85ea1a..3d50aba 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -895,7 +895,66 @@ gdb_rl_operate_and_get_next (int count, int key)
 
   return rl_newline (1, key);
 }
-\f
+
+/* Number of user commands executed during this session.  */
+
+static int command_count = 0;
+
+/* Add the user command COMMAND to the input history list.  */
+
+void
+gdb_add_history (const char *command)
+{
+  add_history (command);
+  command_count++;
+}
+
+/* Safely append new history entries to the history file in a corruption-free
+   way using an intermediate local history file.  */
+
+static void
+gdb_safe_append_history (void)
+{
+  int ret, saved_errno;
+  char *local_history_filename;
+
+  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());
+
+  ret = rename (history_filename, local_history_filename);
+  saved_errno = errno;
+  if (ret < 0 && saved_errno == ENOENT)
+    {
+      /* If the rename failed with ENOENT then either the global history file
+	 never existed in the first place or another GDB process is currently
+	 appending to it (and has thus temporarily renamed it).  Since we can't
+	 distinguish between these two cases, we have to conservatively assume
+	 the first case and therefore must write out (not append) our known
+	 history to our local history file and try to move it back anyway.
+	 Otherwise a global history file would never get created!  */
+      write_history (local_history_filename);
+    }
+  else if (ret < 0)
+    {
+      warning (_("Could not rename %s to %s: %s"),
+	       history_filename, local_history_filename, strerror (saved_errno));
+      goto out;
+    }
+  else
+    {
+      append_history (command_count, local_history_filename);
+      history_truncate_file (local_history_filename, history_max_entries);
+    }
+
+  ret = rename (local_history_filename, history_filename);
+  saved_errno = errno;
+  if (ret < 0 && saved_errno != EEXIST)
+    warning (_("Could not rename %s to %s: %s"),
+	     local_history_filename, history_filename, strerror (saved_errno));
+
+out:
+  xfree (local_history_filename);
+}
+
 /* Read one line from the command input stream `instream'
    into the local static buffer `linebuffer' (whose current length
    is `linelength').
@@ -1094,7 +1153,7 @@ command_line_input (const char *prompt_arg, int repeat, char *annotation_suffix)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Save into global buffer if appropriate.  */
   if (repeat)
@@ -1445,7 +1504,7 @@ quit_force (char *args, int from_tty)
     {
       if (write_history_p && history_filename
 	  && input_from_terminal_p ())
-	write_history (history_filename);
+	gdb_safe_append_history ();
     }
   DO_PRINT_EX;
 
diff --git a/gdb/top.h b/gdb/top.h
index b68e896..987279b 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -79,6 +79,8 @@ extern int history_expansion_p;
 extern int server_command;
 extern char *lim_at_start;
 
+extern void gdb_add_history (const char *);
+
 extern void show_commands (char *args, int from_tty);
 
 extern void set_history (char *, int);
-- 
2.2.1.212.gc5b9256

Re: [PATCH] Append to input history file instead of overwriting it

[Pedro Alves]

On 01/10/2015 06:46 PM, Patrick Palka wrote:

> +/* Safely append new history entries to the history file in a corruption-free
> +   way using an intermediate local history file.  */
> +
> +static void
> +gdb_safe_append_history (void)
> +{
> +  int ret, saved_errno;
> +  char *local_history_filename;
> +
> +  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());

IMO just appending a number is not sufficiently distinct
from what a user might reasonably want to name alternate history
files.  How about picking a more obscure name, like what
I had originally suggested:

  local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());

?

> +
> +  ret = rename (history_filename, local_history_filename);
> +  saved_errno = errno;
> +  if (ret < 0 && saved_errno == ENOENT)
> +    {
> +      /* If the rename failed with ENOENT then either the global history file
> +	 never existed in the first place or another GDB process is currently
> +	 appending to it (and has thus temporarily renamed it).  Since we can't
> +	 distinguish between these two cases, we have to conservatively assume
> +	 the first case and therefore must write out (not append) our known
> +	 history to our local history file and try to move it back anyway.
> +	 Otherwise a global history file would never get created!  */
> +      write_history (local_history_filename);
> +    }
> +  else if (ret < 0)
> +    {
> +      warning (_("Could not rename %s to %s: %s"),
> +	       history_filename, local_history_filename, strerror (saved_errno));

use safe_strerror.  Watch out for line too long.


> +      goto out;

Let's avoid gotos when simple enough.  In this case, seems to
me that to skip the second rename call, we only need to move
the "else if" and "else" within a parent "else" branch.

Also, please use a cleanup instead of the xfree at the end:

  local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());
  old_chain = make_cleanup (xfree, local_history_filename);
  ...
  do_cleanups (old_chain);

> +    }
> +  else
> +    {
> +      append_history (command_count, local_history_filename);
> +      history_truncate_file (local_history_filename, history_max_entries);
> +    }
> +
> +  ret = rename (local_history_filename, history_filename);
> +  saved_errno = errno;
> +  if (ret < 0 && saved_errno != EEXIST)
> +    warning (_("Could not rename %s to %s: %s"),
> +	     local_history_filename, history_filename, strerror (saved_errno));

safe_strerror.

> +
> +out:
> +  xfree (local_history_filename);
> +}
> +

This is OK with these changes.

Thanks!

-- 
Pedro Alves

Re: [PATCH] Append to input history file instead of overwriting it

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 2975 bytes --]

On Mon, Jan 12, 2015 at 2:05 PM, Pedro Alves <palves@redhat.com> wrote:
> On 01/10/2015 06:46 PM, Patrick Palka wrote:
>
>> +/* Safely append new history entries to the history file in a corruption-free
>> +   way using an intermediate local history file.  */
>> +
>> +static void
>> +gdb_safe_append_history (void)
>> +{
>> +  int ret, saved_errno;
>> +  char *local_history_filename;
>> +
>> +  local_history_filename = xstrprintf ("%s.%d", history_filename, getpid ());
>
> IMO just appending a number is not sufficiently distinct
> from what a user might reasonably want to name alternate history
> files.  How about picking a more obscure name, like what
> I had originally suggested:
>
>   local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());
>
> ?
>
>> +
>> +  ret = rename (history_filename, local_history_filename);
>> +  saved_errno = errno;
>> +  if (ret < 0 && saved_errno == ENOENT)
>> +    {
>> +      /* If the rename failed with ENOENT then either the global history file
>> +      never existed in the first place or another GDB process is currently
>> +      appending to it (and has thus temporarily renamed it).  Since we can't
>> +      distinguish between these two cases, we have to conservatively assume
>> +      the first case and therefore must write out (not append) our known
>> +      history to our local history file and try to move it back anyway.
>> +      Otherwise a global history file would never get created!  */
>> +      write_history (local_history_filename);
>> +    }
>> +  else if (ret < 0)
>> +    {
>> +      warning (_("Could not rename %s to %s: %s"),
>> +            history_filename, local_history_filename, strerror (saved_errno));
>
> use safe_strerror.  Watch out for line too long.
>
>
>> +      goto out;
>
> Let's avoid gotos when simple enough.  In this case, seems to
> me that to skip the second rename call, we only need to move
> the "else if" and "else" within a parent "else" branch.
>
> Also, please use a cleanup instead of the xfree at the end:
>
>   local_history_filename = xstrprintf ("%s-gdb%d~", history_filename, getpid ());
>   old_chain = make_cleanup (xfree, local_history_filename);
>   ...
>   do_cleanups (old_chain);
>
>> +    }
>> +  else
>> +    {
>> +      append_history (command_count, local_history_filename);
>> +      history_truncate_file (local_history_filename, history_max_entries);
>> +    }
>> +
>> +  ret = rename (local_history_filename, history_filename);
>> +  saved_errno = errno;
>> +  if (ret < 0 && saved_errno != EEXIST)
>> +    warning (_("Could not rename %s to %s: %s"),
>> +          local_history_filename, history_filename, strerror (saved_errno));
>
> safe_strerror.
>
>> +
>> +out:
>> +  xfree (local_history_filename);
>> +}
>> +
>
> This is OK with these changes.

I have made the changes you requested and committed the attached
patch.  I hope I haven't made any oversights.

Thanks for reviewing!

>
> Thanks!
>
> --
> Pedro Alves
>

[-- Attachment #2: 0001-Append-to-input-history-file-instead-of-overwriting-.patch --]
[-- Type: application/octet-stream, Size: 5249 bytes --]

From 6086d5e70163c62bedf3228200c378a04af23646 Mon Sep 17 00:00:00 2001
From: Patrick Palka <patrick@parcs.ath.cx>
Date: Fri, 28 Nov 2014 20:22:45 -0500
Subject: [PATCH] Append to input history file instead of overwriting it

This patch makes readline append new history lines to the GDB history
file on exit instead of overwriting the entire history file on exit.
This change allows us to run multiple simultaneous GDB sessions without
having each session overwrite the added history of each other session on
exit.

Care must be taken to ensure that the history file doesn't get corrupted
when multiple GDB processes are trying to simultaneously append to and
then truncate it.  Safety is achieved in such a situation by using an
intermediate local history file to mutually exclude multiple processes
from simultaneously performing write operations on the global history
file.

gdb/ChangeLog:

	* top.h (gdb_add_history): Declare.
	* top.c (command_count): New variable.
	(gdb_add_history): New function.
	(gdb_safe_append_history): New static function.
	(quit_force): Call it.
	(command_line_input): Use gdb_add_history instead of
	add_history.
	* event-top.c (command_line_handler): Likewise.
---
 gdb/event-top.c |  2 +-
 gdb/top.c       | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 gdb/top.h       |  2 ++
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 13ddee2..bbda5dc 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -667,7 +667,7 @@ command_line_handler (char *rl)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Note: lines consisting solely of comments are added to the command
      history.  This is useful when you type a command, and then
diff --git a/gdb/top.c b/gdb/top.c
index b85ea1a..a1462a0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -895,7 +895,74 @@ gdb_rl_operate_and_get_next (int count, int key)
 
   return rl_newline (1, key);
 }
-\f
+
+/* Number of user commands executed during this session.  */
+
+static int command_count = 0;
+
+/* Add the user command COMMAND to the input history list.  */
+
+void
+gdb_add_history (const char *command)
+{
+  add_history (command);
+  command_count++;
+}
+
+/* Safely append new history entries to the history file in a corruption-free
+   way using an intermediate local history file.  */
+
+static void
+gdb_safe_append_history (void)
+{
+  int ret, saved_errno;
+  char *local_history_filename;
+  struct cleanup *old_chain;
+
+  local_history_filename
+    = xstrprintf ("%s-gdb%d~", history_filename, getpid ());
+  old_chain = make_cleanup (xfree, local_history_filename);
+
+  ret = rename (history_filename, local_history_filename);
+  saved_errno = errno;
+  if (ret < 0 && saved_errno != ENOENT)
+    {
+      warning (_("Could not rename %s to %s: %s"),
+	       history_filename, local_history_filename,
+	       safe_strerror (saved_errno));
+    }
+  else
+    {
+      if (ret < 0)
+	{
+	  /* If the rename failed with ENOENT then either the global history
+	     file never existed in the first place or another GDB process is
+	     currently appending to it (and has thus temporarily renamed it).
+	     Since we can't distinguish between these two cases, we have to
+	     conservatively assume the first case and therefore must write out
+	     (not append) our known history to our local history file and try
+	     to move it back anyway.  Otherwise a global history file would
+	     never get created!  */
+	   gdb_assert (saved_errno == ENOENT);
+	   write_history (local_history_filename);
+	}
+      else
+	{
+	  append_history (command_count, local_history_filename);
+	  history_truncate_file (local_history_filename, history_max_entries);
+	}
+
+      ret = rename (local_history_filename, history_filename);
+      saved_errno = errno;
+      if (ret < 0 && saved_errno != EEXIST)
+        warning (_("Could not rename %s to %s: %s"),
+		 local_history_filename, history_filename,
+		 safe_strerror (saved_errno));
+    }
+
+  do_cleanups (old_chain);
+}
+
 /* Read one line from the command input stream `instream'
    into the local static buffer `linebuffer' (whose current length
    is `linelength').
@@ -1094,7 +1161,7 @@ command_line_input (const char *prompt_arg, int repeat, char *annotation_suffix)
 
   /* Add line to history if appropriate.  */
   if (*linebuffer && input_from_terminal_p ())
-    add_history (linebuffer);
+    gdb_add_history (linebuffer);
 
   /* Save into global buffer if appropriate.  */
   if (repeat)
@@ -1445,7 +1512,7 @@ quit_force (char *args, int from_tty)
     {
       if (write_history_p && history_filename
 	  && input_from_terminal_p ())
-	write_history (history_filename);
+	gdb_safe_append_history ();
     }
   DO_PRINT_EX;
 
diff --git a/gdb/top.h b/gdb/top.h
index b68e896..987279b 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -79,6 +79,8 @@ extern int history_expansion_p;
 extern int server_command;
 extern char *lim_at_start;
 
+extern void gdb_add_history (const char *);
+
 extern void show_commands (char *args, int from_tty);
 
 extern void set_history (char *, int);
-- 
2.2.1.212.gc5b9256