From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7419 invoked by alias); 27 Apr 2015 18:39:50 -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 7358 invoked by uid 89); 27 Apr 2015 18:39:49 -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,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 27 Apr 2015 18:39:47 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3RIdi0L029323 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 27 Apr 2015 14:39:44 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3RIdgO1007993; Mon, 27 Apr 2015 14:39:43 -0400 Message-ID: <553E826E.70300@redhat.com> Date: Mon, 27 Apr 2015 18:45:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Patrick Palka , gdb-patches@sourceware.org Subject: Re: [PATCH] Fix PR gdb/17820 References: <1430073669-31059-1-git-send-email-patrick@parcs.ath.cx> In-Reply-To: <1430073669-31059-1-git-send-email-patrick@parcs.ath.cx> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg01009.txt.bz2 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. 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... 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? 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. 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.) Thanks, Pedro Alves