From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22775 invoked by alias); 28 Apr 2015 01:06:00 -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 22762 invoked by uid 89); 28 Apr 2015 01:05:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: mail-pa0-f51.google.com Received: from mail-pa0-f51.google.com (HELO mail-pa0-f51.google.com) (209.85.220.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 28 Apr 2015 01:05:57 +0000 Received: by pacwv17 with SMTP id wv17so125111563pac.0 for ; Mon, 27 Apr 2015 18:05:56 -0700 (PDT) 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=Cvqsq3IQ+EU9OFbF6QOaMK/6ODh/JkPe80LLK3OfyhY=; b=ea+BkkbylBKFVASm8ecBF7PRUracAd1Si7GZyq4RKUI1aRcSSR0w4qf2dv1sXBHHE6 agynGrqLoWsWnj5qWCQEzZZubgPG4XeEJ0fykshXyFcp33QGeIpPaSRyIAGzZEbfCmNz FOUbBGyhGiWbR7nVpCaEMuEdjJf4ykg5mG3j7utzq13x4Aoz3U1LM2xjXsb0noKlgYSe JyqeoM+06XvFaV1InVzOn0CLvTe8Aef2/TQcvit1MzsxIj3ThXek5GesOGpE9EUyl4eV gBHGYcG0QtbmLge43YJzH7FHVBYPPM9E69f00xJefQcy6Yu/c7gttufPM5MPrK16qykv nivg== X-Gm-Message-State: ALoCoQlWULb2/rFWOqyLAnDXuzr8OQIiuGT3JwPV3AsyUPunBNTJ9l7XS61ckN2AhgGP5ImYBHVu X-Received: by 10.68.232.194 with SMTP id tq2mr27336811pbc.90.1430183155899; Mon, 27 Apr 2015 18:05:55 -0700 (PDT) MIME-Version: 1.0 Received: by 10.70.102.99 with HTTP; Mon, 27 Apr 2015 18:05:35 -0700 (PDT) In-Reply-To: <553E826E.70300@redhat.com> References: <1430073669-31059-1-git-send-email-patrick@parcs.ath.cx> <553E826E.70300@redhat.com> From: Patrick Palka Date: Tue, 28 Apr 2015 01:54:00 -0000 Message-ID: Subject: Re: [PATCH] Fix PR gdb/17820 To: Pedro Alves Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2015-04/txt/msg01024.txt.bz2 On Mon, Apr 27, 2015 at 2:39 PM, Pedro Alves wrote: > On 04/26/2015 07:41 PM, Patrick Palka wrote: >> This patch is a comprehensive fix for PR 17820 which reports that >> using "set history size unlimited" inside one's gdbinit file doesn't >> really work. >> >> There are three small changes in this patch. The most important change >> this patch makes is to decode the argument of the "size" subcommand >> using add_setshow_zuinteger_unlimited_cmd() instead of using >> add_setshow_uinteger_cmd(). The new decoder takes an int * and maps >> unlimited to -1 whereas the old decoder takes an unsigned int * and maps >> unlimited to UINT_MAX. Using the new decoder simplifies our handling of >> unlimited and makes it easier to interface with readline which itself >> expects a signed-int history size. >> >> The second change is the factoring of the [stifle|unstifle]_history logic >> into a common function which is now used by both init_history() and >> set_history_size_command(). This is technically the change that fixes >> the PR itself. >> >> Thirdly, this patch initializes history_size_setshow_var to -2 to mean >> that the variable has not been set yet. Now init_history() tests for -2 >> instead of 0 to determine whether to give the variable a default value. >> This means that having "set history size 0" in one's gdbinit file will >> actually keep the history size at 0 and not reset it to 256. > > Please see also: > > https://sourceware.org/ml/gdb-patches/2013-03/msg00962.html > > for background. I too think the variable should be signed if only because it simplifies the code. > > Darn. So around that time, we added support for explicit "unlimited" > to a bunch of commands. Since "set history size 0" already meant > unlimited before, and since this is the sort of command that users > put in .gdbinit instead of issuing manually, it was reasonable > to assume if we changed the "unlimited" representation, we'd > break user scripts. So the idea was that we'd let a few years/releases > go by, and then we'd change "size=0" to really mean 0, and we'd > tell users to use "set history size unlimited" instead. > > But now we see that "set history size unlimited" in .gdbinit never > really worked. Bummer. So this means that users must have kept > using "set history size 0" instead... "set history size 0" (in the .gdbinit) doesn't set history to unlimited either -- it sets it to 256! So users currently have no choice but to use "set history size BIGINT" for approximately-unlimited history. > > So if we change this now, there's no way to have a single > "set history size FOO" setting that means unlimited with > both gdb <= 7.9 and the next gdb release. :-/ Oh well. Maybe we should > just tell users to do "set history size BIGINT" instead? But currently, neither "set history size 0" nor "set history size unlimited" mean unlimited (in the .gdbinit). So users today cannot set an unlimited history size at all. Changing this now will now at least make "set history size unlimited". So whether or not we change this now, there will be no way to have a single "set history size FOO" setting that means unlimited across current and future GDB versions. Am I misunderstanding? > > I'd definitely like to make "set history size 0" really > disable history. > > So I think that if goes forward, it'd be good to have a NEWS entry. I can think of two things to mention. One is that "set history size 0" now really disables history. The other is that "set history size unlimited" now really does not truncate history. Do you have anything else in mind? > > What do you (and others) think? > >> [Alternatively I can just initialize the variable to 256 in the first >> place. Would that be better?] > > -2 is fine with me. > >> >> gdb/ChangeLog: >> >> PR gdb/17820 >> * top.c (history_size_setshow_var): Change type to signed. >> Initialize to -2. Update documentation. >> (set_readline_history_size): Define. >> (set_history_size_command): Use it. Remove logic for handling >> out-of-range sizes. >> (init_history): Use set_readline_history_size(). Test for a >> value of -2 instead of 0 when determining whether to set a >> default history size. >> (init_main): Decode the argument of the "size" command as a >> zuinteger_unlimited. > > Looks good to me. > > Adding a testcase would be ideal, but I'll not make it a requirement. > I think we should be able to write one making use of GDBFLAGSs. (and > IWBN to test the GDBHISTSIZE/HISTSIZE environment variables too, which > we can do with "set env(HISTSIZE)", etc.) Do you have in mind a test that creates a dummy .gdbinit file to be read by GDB? Or is there another way to test this code path? > > Thanks, > Pedro Alves >