From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14954 invoked by alias); 5 Dec 2014 14:11:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 14942 invoked by uid 89); 5 Dec 2014 14:11:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-pa0-f41.google.com Received: from mail-pa0-f41.google.com (HELO mail-pa0-f41.google.com) (209.85.220.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 05 Dec 2014 14:11:33 +0000 Received: by mail-pa0-f41.google.com with SMTP id rd3so756015pab.28 for ; Fri, 05 Dec 2014 06:11:31 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=GMu1ZAoztJYesBfnlPtBMOuP8N/4SijsENtQxPZo5SU=; b=exQx+f5txMhRm7S9Bw/B/4PepuUukK7cxeojFz6mZ8LHIjX7kKqrKb7F39z4mSmtyI 9w3sYyt6dtiuguYOXVX8hfLas6qg1TGzyj/fZSQVkB1ZcmYL/F7NDFhdNtt/wEpur5E5 xq5GS4+Q8rCottK635n4jML2cHSUC4eXzAErAM6oFFvuZKMjocoovIoVw7Mf+GcWtA3o RZSpUP9lzWk6XW+8KHx/WMM8vAWkAn2MZ8VSBh8J/7aWk84OWVQ0PsYoK0YBHHYIRlbg Ji4R7ljv2+tDYmasWQkW68ZSYam5enisjv522q4eQnNgWmAyMQPz4VnnxSxhnsiGA/Qv iXkg== X-Gm-Message-State: ALoCoQkaZ31AMhwpBq6hvq6PgiGFjR3CQzNZoK1+rp3el2ZG4O02gk3njXZ36mDnTBA8uORCklbX X-Received: by 10.70.130.174 with SMTP id of14mr29080413pdb.90.1417788691472; Fri, 05 Dec 2014 06:11:31 -0800 (PST) MIME-Version: 1.0 Received: by 10.70.31.100 with HTTP; Fri, 5 Dec 2014 06:11:11 -0800 (PST) In-Reply-To: <54818CCE.5010701@redhat.com> References: <1417226462-11254-1-git-send-email-patrick@parcs.ath.cx> <54808956.9070507@redhat.com> <54818CCE.5010701@redhat.com> From: Patrick Palka Date: Fri, 05 Dec 2014 14:11:00 -0000 Message-ID: Subject: Re: [PATCH] Append to input history file instead of overwriting it To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-12/txt/msg00137.txt.bz2 On Fri, Dec 5, 2014 at 5:45 AM, Pedro Alves 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