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 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(-)
  

> 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.
  

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.
  

> 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.
  

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.
  

> 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?
  

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.