From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id C6895385AC23 for ; Thu, 6 Jan 2022 09:33:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C6895385AC23 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-27-Pb8ckKq-OPO2rOU7KGMt3w-1; Thu, 06 Jan 2022 04:33:25 -0500 X-MC-Unique: Pb8ckKq-OPO2rOU7KGMt3w-1 Received: by mail-wr1-f70.google.com with SMTP id w25-20020adf8bd9000000b001a255212b7cso988136wra.18 for ; Thu, 06 Jan 2022 01:33:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=53vk5UpeUCmQ6fPRYxnwko9GNV8X/EEwwQThZKtluz8=; b=ApkbIO0lMtts2az/Hn2SbTNPkzP+ZC3wdMSBaMcmYPcLTSpWaLRwkVy7YPgIfyq0WL Vza3ZRDcKqohvnajUTWM5CxFa//pJNFuiKID+D3K9VPo/j7TkzYfkqh0BOXjtbzasXs9 z14pK7tx8AsU0y2B0djw/cCy+1rQRneH27VN2sixhkxwoMsWS0aB/Yifquy7iJVnGN5l Z0JR62+cy+SflcuLkLhH0EAjruvW3tsbCR47n+gbmAUnSkgERSTw3f1Rw+K9KpLqtA4P 4cu4md4Q/T5yIe9MnrAYxCGlLCj2GvhcPlTKymNvTu/vCoi9XD3wNQl2bdRGE3B64DnZ y28A== X-Gm-Message-State: AOAM530309W8Sgi8LArOYXeICzCevUfkTDaM+FChFmpWzZilR2A/msWd tSimokAvjsKOVEZF2ECFHEzgdGrSdahXOgb9ag4FXMQn128XuGySS9SO5kaV/92jvvuFyMme3B6 26Sjh4xXcKT+jp1rGLcTQjA== X-Received: by 2002:a5d:4408:: with SMTP id z8mr1278999wrq.89.1641461604381; Thu, 06 Jan 2022 01:33:24 -0800 (PST) X-Google-Smtp-Source: ABdhPJz/gJNyAyvLGMBS5xNdlma+M2wf3G5f0OlQAOOIQmM2OTsZs5z//4HCWkuN6sxhaGjn03vVOw== X-Received: by 2002:a5d:4408:: with SMTP id z8mr1278989wrq.89.1641461604140; Thu, 06 Jan 2022 01:33:24 -0800 (PST) Received: from localhost (host109-154-163-67.range109-154.btcentralplus.com. [109.154.163.67]) by smtp.gmail.com with ESMTPSA id k31sm1275980wms.21.2022.01.06.01.33.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Jan 2022 01:33:23 -0800 (PST) Date: Thu, 6 Jan 2022 09:33:22 +0000 From: Andrew Burgess To: Simon Sobisch Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2] gdb: split array and string limiting options Message-ID: <20220106093322.GD828155@redhat.com> References: <111e4bb6-d781-ff80-f64b-125ad665f502@web.de> MIME-Version: 1.0 In-Reply-To: <111e4bb6-d781-ff80-f64b-125ad665f502@web.de> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 09:18:34 up 4 days, 18:12, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jan 2022 09:33:30 -0000 * Simon Sobisch via Gdb-patches [2022-01-06 08:58:38 +0100]: > > From: Andrew Burgess > > > > This commit splits the set/show print elements options into two. We > > retain set/show print elements for controlling how many elements of an > > array we print, but a new set/show print characters is added which is > > used for controlling how many characters of a string are printed. > > [...] > Reference to the full patch: > https://sourceware.org/pipermail/gdb-patches/2021-December/184467.html > > > I really like the general idea and would like to see this added, but I > think the patch goes too far and will break both old and new usage. I don't understand what "new usage" means here. > > In my opinion there should be only 1 new test for the new setting and > the other should be left unchanged while still working, because then > existing usages will still work, too. > > To do so the new parameter cannot be directly used as it is in the patch > but must be fowarded to a new function or duplicated. > > As an example this would mean instead of : > > --- src.orig/gdb/c-valprint.c > +++ src/gdb/c-valprint.c > @@ -271,7 +271,7 @@ c_value_print_array (struct value *val, > > for (temp_len = 0; > (temp_len < len > - && temp_len < options->print_max > + && temp_len < options->print_max_chars > && extract_unsigned_integer (valaddr + temp_len * eltlen, > eltlen, byte_order) != 0); > ++temp_len) > > have the following > > > + const int print_max_chars = (options->print_max_chars != -1) ? > options->print_max_chars : options->print_max; > for (temp_len = 0; > (temp_len < len > - && temp_len < options->print_max > + && temp_len < print_max_chars > && extract_unsigned_integer (valaddr + temp_len * eltlen, > eltlen, byte_order) != 0); > ++temp_len) What follows is just my thoughts on what you propose. This work was being done for a client of Embecosm's, and Maciej is now driving this patch, so what he does is up to him. That said... I did consider this when I originally wrote the patch. But decided against it in the end. I agree that maintaining backwards compatibility is important. But I think we should be careful about going over the top to avoid breaking changes. IMHO GDB script is not equivalent to something like C, C++, Python, etc, where I would certainly expect complete backwards compatibility. Instead I think we should aim to strike a balance between maintaining backward compatibility, and keeping a simple, easy to understand interface. For me, changing the behaviour of this option didn't seem that crazy, it's pretty easy to restore the old behaviour by also setting the new option. Maybe the help text could do a better job of cross referencing the other option, to make discovery easier. Anyway, just me thoughts. I'm certainly not going to block the patch if it is rewritten inline with your suggestion. > > > This effectively means that until you explicit set the new "print > characters" the existing "print elements" setting will be used, and if > you "set print characters off" (or similar, that would internally > resolve to -1) you get the length depending on "print elements" again. > > The parameter type in python for "print elements" is (guessed) > gdb.PARAM_UINTEGER, the type for "print characters" would be > gdb.PARAM_INTEGER with -1 meaning "take the other parameter". > > > If those changes are done there is no breakage whatever which could also > speedup the inclusion s it hopefully lands in GDB 11.2. Maybe I'm not understanding how releases work, but I thought 11.2 was a bug fix release over 11.1, and a future 12 branch would be the new release in which new features would land. Is that not how things work? Thanks, Andrew