From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id OTHzLwxr/mnwgyQAWB0awg (envelope-from ) for ; Fri, 08 May 2026 19:00:28 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20251104 header.b=UUCNQO3l; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id BC7921E067; Fri, 08 May 2026 19:00:28 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-10.9 required=5.0 tests=ARC_SIGNED,ARC_VALID, BAYES_00,DKIMWL_WL_MED,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED,RCVD_IN_VALIDITY_RPBL_BLOCKED, RCVD_IN_VALIDITY_SAFE_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 39A5B1E067 for ; Fri, 08 May 2026 19:00:27 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id EAE3B4BA2E05 for ; Fri, 8 May 2026 23:00:25 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org EAE3B4BA2E05 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20251104 header.b=UUCNQO3l Received: from mail-qt1-x82e.google.com (mail-qt1-x82e.google.com [IPv6:2607:f8b0:4864:20::82e]) by sourceware.org (Postfix) with ESMTPS id 6D0F04BA2E05 for ; Fri, 8 May 2026 22:59:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6D0F04BA2E05 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=google.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6D0F04BA2E05 Authentication-Results: sourceware.org; arc=pass smtp.remote-ip=2607:f8b0:4864:20::82e ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1778281172; cv=pass; b=Y0bLfSSRfkP0+Yl1t1xh5DDN5dohC1gm7Nt97uGKvu4hExoR08/AyD2b6mjpG/lcjRxCkyDQz8BEy7Ri3epKdOFbSHXwHbdmLvf1XKNJpUqeRM0gj9vbveTzZIVF5LJ8+VCH3eKdDxQiuAyyoAxL1OzpokIKzYSojmxSn0YxEug= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1778281172; c=relaxed/simple; bh=liRGpK6VEkq5ZsSlk4JWoVGvYey0fV/QtRzMJgUWkZo=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=gCkkLYKF4LDO3Fng0X3FVpCb+I0VaAjOVnU5ox4QmocUOrG36vFUfLZ9lK5vd6KL7PL9jZn4qfiNafaHsJKdDmAjv1J2iELdn1WkFM0Vg9aRbLcT9JXvdaOy8D4LvFU0RfhyClbIvHe/vahzHOiwDapDDIB8nPOwfPYz/slDvac= ARC-Authentication-Results: i=2; sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20251104 header.b=UUCNQO3l DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6D0F04BA2E05 Received: by mail-qt1-x82e.google.com with SMTP id d75a77b69052e-50d836552daso351cf.0 for ; Fri, 08 May 2026 15:59:32 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1778281172; cv=none; d=google.com; s=arc-20240605; b=FYE3030OHIGp/YeMeWtOSQr0h5FE/Wad7cUbt56Ic53OYjnbTN2kCO0Oo8uR34NuJx fuxEJaSO7f1bnYB51FDBciVu93rBInTfz5dLsyd14ig+J1Ha8Q0JZQVKZtjO4oa5LfCi 9qGctoqgQs7YvxwkiAmmrQJ1KrKtfDdGmWtzEfZWaO38op4uHZKkzsSRYq4DSuiy6O4P 74+29xFZxaDbJaZgiQDjB3AJMwmfXRlTCQGCR3yn17ex9XxqTRr95uJ8OOtvrR6cNzFu T8MvIkmWaaG8CrIqWwB6CL+1bdys4KPgP8ZznnSBNRSv7SrMyKIdpoVmuT8O264JV2Cu nL1w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=4IpXIUOUR/CgXjwWsJvZYvNEdMW/F8RC4GUOYwjw1LE=; fh=P6aByBDHUSJA7ql08umElq+UpS69E5XMNq4GyJQawj4=; b=eirUlj79zmC+AZFQwugBrwsz4URnWKuT1x6prxPlt+vZJGlFONX4wn/y4+vSF8rgVF 4/p2uW+ZwpO8bVDOQwHgRWmey47VkG5Ps23jJmeZEuEbgPbloeBfRzO52APNHNPFfwy2 QPsnwJ17XN6PkxtcMqfXEiuGH/rKaTS64detTGau7XIO2+scTUFkvUraNf/Ayt6l3Kqv 7QVCa0S5xjIP8v7LYvYtRKJKHXgq8cuM0/fSD171wpHvXz49va/0OYp6KO6Amt43RH6a 94jegUWV41s/E4D/vPOHAeMq43quLOp+254rpHFrKfZMVLftr9Sjber+2pzXjhXqOxcv Su0w==; darn=sourceware.org ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1778281172; x=1778885972; darn=sourceware.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=4IpXIUOUR/CgXjwWsJvZYvNEdMW/F8RC4GUOYwjw1LE=; b=UUCNQO3ltvA3DLMXNuGFGxHF0hK1/WEtOyLLghdh5nxG/qzZvDiemu1hZKbOGpB+Qn 64jMoCiyNtG7yPVcNBIkUU/Ym5SZfGNTFx7UANnz0ha08TD7awuZz+ka1a7C1eUhD+Y3 wQCIXKUzScmvXpiVYxpIe5+LTPlQ/+ftZfcjDIVUx4BCrsK0ZBS3nxDKQG2Lu35HH5hq 7VpaWUSVHcvqVVA00PuO2MMegZV+uJvv2Wya3QXvkDEKgEqymIrrXlvd8RHpYnYIMSGS GiUUSO8zBIVYTHCEi3bj4C2BXK4h/OwdBFKZRwLu3jNj4JssHR8kR8FDUOA8C976zW3r 1FTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778281172; x=1778885972; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=4IpXIUOUR/CgXjwWsJvZYvNEdMW/F8RC4GUOYwjw1LE=; b=VwbrbgC9Z0rxd9OTR2lLYEjVER8lNM+KzKJkbxKHyiltsy6zNdzelNDxooSTMhKV7L QESKS/MmJp3SfuwCWkCQrBGQafo2kTRGry6ZV+T69jMFsMNCysuEASQ5z5vtql+/WWJG usBg66G6fwcyMIycJ4uns9t0xIxDEPJAkI/qgX9u62Jy+isAiz4lUfa70sWx2etoWFa7 pV2rsZ6PT8gGTVsVvNU/eRvUx74InwGPxoaDgOO5kO7AITRsXtC/qUUOa6EAPiaFHx+I pxRaeRqWYy9u7467B85aAfIaqXe+FE1YQkkoM6ylI8TVBFEsEYtG/8Y4QQ3xqKLIKdY0 505Q== X-Gm-Message-State: AOJu0YzP+USHe2lwhLKlMytgp+t7F+Rh5UwXPXbtNRhmjQVUNCGudFqe MxaYs0axtI4W6GtFqB6ktW1AmlwlgHgsfFBloAqYs+KuPG/9Y5NUmbYsGlyCkFRtwIsn+BvBwHD flopuS0ZEH9L+bSx3j8UD41fyz7zu3DPnJ0uONIDQ9ztJWvUt5Y2qOlekwhc= X-Gm-Gg: AeBDievACJJpqnwO337DKSl6/9QPLJy+gcSE51Udl+raqNK6X7VW3U0nlTVisgvVib0 qyIk05HAN/P6UE7tN+KLPD+xcKIQJB4WctGN8nLkGhpIhiTAenq5GxNVoVCDi6yS9ihCUCcqzPK hdC3Jne0ZIDvQX230iSqhJVUN8FDCgafYCyK7VLP7ofrAyeTH9t8I3C/JMzzGnxIbcaorhWueZb 6mRydxsMW2ctsWrda0+Z0wkXPjlyf+km9VgbRe0DAW3xv4Q6SXHRrniUP9KBARtT11MKOUpPbTt Uvd5g1ZXh5tovjhI65/h6Y+BFtSktDP350E4fIEacQikOKlqOg== X-Received: by 2002:ac8:588b:0:b0:510:fa1:73c5 with SMTP id d75a77b69052e-5149f331766mr4802151cf.16.1778281171175; Fri, 08 May 2026 15:59:31 -0700 (PDT) MIME-Version: 1.0 References: <20260504213710.209740-1-zdw@google.com> In-Reply-To: From: Zander Work Date: Fri, 8 May 2026 15:59:19 -0700 X-Gm-Features: AVHnY4ISBeUX_4LXvBf6JiadJOoVacAFI3e7Sv0T0nl5--P37EIFqM6s7SzAEAw Message-ID: Subject: Re: [PATCH] Use "output-radix" setting to format function offsets To: Guinevere Larsen Cc: gdb-patches@sourceware.org, Tom Tromey Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Thank you for the review! Replies inline, I will follow up (probably next week) with a v2 of this patch. -Zander On Fri, May 8, 2026 at 6:45=E2=80=AFAM Guinevere Larsen wrote: > > On 5/4/26 6:37 PM, Zander Work wrote: > > This is a patch for a discussion [1] I had previously where it was > > determined that it was a bug for GDB to not use the "output-radix" > > setting for function offsets. > > > > This patch includes updates to the radix.exp tests, and I verified that > > there were no new breakages added when running the full gdb testsuite > > with this patch. > > > > I didn't make any changes to NEWS or a /gdb/ Changelog entry for this, > > if I should please let me know. > > Hi! Thank you for working on this! > > Changelog entries are definitely no longer required, and I don't think > this would need a NEWS entry either. > > I have some minor comments, mostly about styling or wondering about a > few choices, but in general I think this patch is in the right direction! > > > > > I also have not personally completed a FSF copyright assignment form. > Yeah, for a patch this size, it will definitely be necessary. I'm adding > Tom Tromey in CC since I know he knows about the assignment process > (more than me, at least) > > > > Sample output with this patch: > > > > ``` > > (gdb) disas main > > Dump of assembler code for function main: > > 0x0000000000001149 <+0>: endbr64 > > 0x000000000000114d <+4>: push %rbp > > 0x000000000000114e <+5>: mov %rsp,%rbp > > 0x0000000000001151 <+8>: lea 0xeac(%rip),%rax # 0x200= 4 > > 0x0000000000001158 <+15>: mov %rax,%rdi > > 0x000000000000115b <+18>: mov $0x0,%eax > > 0x0000000000001160 <+23>: call 0x1050 > > 0x0000000000001165 <+28>: mov $0x0,%eax > > 0x000000000000116a <+33>: pop %rbp > > 0x000000000000116b <+34>: ret > > End of assembler dump. > > (gdb) set radix 0x10 > > Input and output radices now set to decimal 16, hex 10, octal 20. > > (gdb) disas main > > Dump of assembler code for function main: > > 0x0000000000001149 <+0x0>: endbr64 > > 0x000000000000114d <+0x4>: push %rbp > > 0x000000000000114e <+0x5>: mov %rsp,%rbp > > 0x0000000000001151 <+0x8>: lea 0xeac(%rip),%rax # 0x200= 4 > > 0x0000000000001158 <+0xf>: mov %rax,%rdi > > 0x000000000000115b <+0x12>: mov $0x0,%eax > > 0x0000000000001160 <+0x17>: call 0x1050 > > 0x0000000000001165 <+0x1c>: mov $0x0,%eax > > 0x000000000000116a <+0x21>: pop %rbp > > 0x000000000000116b <+0x22>: ret > > End of assembler dump. > > ``` > > > > [1] https://sourceware.org/pipermail/gdb/2026-April/052170.html > > --- > > gdb/disasm.c | 18 +++++++++++++---- > > gdb/printcmd.c | 2 +- > > gdb/testsuite/gdb.base/radix.c | 31 +++++++++++++++++++++++++++++ > > gdb/testsuite/gdb.base/radix.exp | 34 +++++++++++++++++++++++++++++++= + > > gdb/valprint.c | 10 ++++++++++ > > gdb/valprint.h | 6 ++++++ > > 6 files changed, 96 insertions(+), 5 deletions(-) > > create mode 100644 gdb/testsuite/gdb.base/radix.c > > > > diff --git a/gdb/disasm.c b/gdb/disasm.c > > index 81c466c188a..5d19dea72b6 100644 > > --- a/gdb/disasm.c > > +++ b/gdb/disasm.c > > @@ -375,10 +375,20 @@ gdb_pretty_print_disassembler::pretty_print_insn = (const struct disasm_insn *insn > > m_uiout->field_string ("func-name", name, > > function_name_style.style ()); > > /* For negative offsets, avoid displaying them as +-N; the sign o= f > > - the offset takes the place of the "+" here. */ > > - if (offset >=3D 0) > > - m_uiout->text ("+"); > > - m_uiout->field_signed ("offset", offset); > > + the offset takes the place of the "+" here. For MI consumers, > > + emit the integer value; otherwise, print the formatted offset b= ased > > + on the current 'output-radix'. */ > > + if (m_uiout->is_mi_like_p ()) > > + { > > + if (offset >=3D 0) > > + m_uiout->text ("+"); > > + m_uiout->field_signed ("offset", offset); > > I'm not familiar with the radix functionality, so this is a genuine > quesiton, but shouldn't the MI interface also honor the radix request? > > It seems to me like it would make sense, but if other MI messages don't, > your approach here seems right. My assumption was that a change from an int-type to a string-type may cause compatibility/API contract issues, I'm not familiar enough with how MI is consumed to know for sure. Happy to simplify this to be the same output unconditionally if it makes sense. > > > + } > > + else > > + { > > + std::string s =3D format_pc_offset (offset); > > + m_uiout->field_string ("offset", s.c_str ()); > > + } > > m_uiout->text (">:\t"); > > } > > else > > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > > index ae498395436..5fb8c666447 100644 > > --- a/gdb/printcmd.c > > +++ b/gdb/printcmd.c > > @@ -567,7 +567,7 @@ print_address_symbolic (struct gdbarch *gdbarch, CO= RE_ADDR addr, > > gdb_puts ("<", stream); > > fputs_styled (name.c_str (), function_name_style.style (), stream); > > if (offset !=3D 0) > > - gdb_printf (stream, "%+d", offset); > > + gdb_puts (format_pc_offset (offset).c_str (), stream); > > > > /* Append source filename and line number if desired. Give specifi= c > > line # of this addr, if we have it; else line # of the nearest s= ymbol. */ > > diff --git a/gdb/testsuite/gdb.base/radix.c b/gdb/testsuite/gdb.base/ra= dix.c > > new file mode 100644 > > index 00000000000..191c374d466 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/radix.c > > @@ -0,0 +1,31 @@ > > +/* This testcase is part of GDB, the GNU debugger. > > + > > + Copyright 2013-2026 Free Software Foundation, Inc. > The copyright year can be only 2026. ack > > + > > + This program is free software; you can redistribute it and/or modif= y > > + it under the terms of the GNU General Public License as published b= y > > + the Free Software Foundation; either version 3 of the License, or > > + (at your option) any later version. > > + > > + This program is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + GNU General Public License for more details. > > + > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see . */ > > + > > +#include > > +#include > > + > > +int v; > > + > > +int main() > Testsuite should also follow the coding style, so main should be in > column 0 of the next line and have a space between function name and > parenthesis. ack > > +{ > > + puts("hello world"); > > + /* Don't let the test case run forever. */ > > + alarm (60); > > This feels overly complex for what you want to test. Is there any reason > why a simple > > int > main () > { > return 0; > } > > wouldn't be enough for the test? I just duplicated and modified another test program right next to this one, I will trim it down and update the styling and copyright as requested. Just need to have enough in the function for there to be enough instructions for testing the offsets. > > > + > > + for (;;) > > + ; > > +} > > diff --git a/gdb/testsuite/gdb.base/radix.exp b/gdb/testsuite/gdb.base/= radix.exp > > index 7a4320bbf36..99256b98df3 100644 > > --- a/gdb/testsuite/gdb.base/radix.exp > > +++ b/gdb/testsuite/gdb.base/radix.exp > > @@ -17,6 +17,11 @@ > > # This file was written by Fred Fish. (fnf@cygnus.com) > > # And rewritten by Michael Chastain (mec.gnu@mindspring.com) > > > > +standard_testfile > > + > > +if {[build_executable "failed to prepare" $testfile $srcfile debug]} { > > + return -1 > > +} > > > > # Start with a fresh gdb. > > > > @@ -189,3 +194,32 @@ gdb_test "set radix 7" \ > > gdb_test "show output-radix" \ > > "Default output radix for printing of values is 10\\." \ > > "output radix unchanged after rejection through set radix command= " > > + > > +with_test_prefix "pc offset radix" { > > + clean_restart $testfile > > + > > + if { ![runto_main] } { > > + return -1 > > + } > > + > > + proc test_pc_offset_radix { oradix offset_re } { > > + global gdb_prompt > > + > > + gdb_test "set output-radix $oradix" \ > > + "Output radix now set to decimal $oradix.*\\." > > + > > + set test "x/10i main with output-radix $oradix" > > + gdb_test_multiple "x/10i main" $test { > I wonder if using x/i or x/2i would be enough. I think it's good to > minimize the amount of stuff emitted by GDB, so that we don't fill a > buffer on slow/overworked machines and get unreliable tests I wanted to get enough output for the test to fully verify that the right octal was being used (rather than an (unlikely) formatting issue; ie, offsets > 0n10/0xa). I'll make a tweak here to decrease the buffer size while still validating this behavior. > > + -re ":\[^\r\n\]*\r\n(?:\[^\r\n\]*\r\n)*$gdb_pr= ompt $" { > > This can be simplified in a few ways. First, you can use -wrap to wrap > your regular expression in the stuff that gdb_test adds around it, so > you won't need to add $gdb_prompt at the end and some stuff at the > start, and second, the whole "\[^\r\n\]*\r\n(?:\[^\r\n\]*\r\n)*" is just > "any number of lines with any amount of characters", so there's no > reason to not use a simple ".*" there. We just avoid .* when the exact > amount of lines, or that something is in the same line, is important, > which isn't the case in this test. Ah perfect, thanks. Will adjust. > > > + pass $gdb_test_name > > + } > > + } > > + } > > + > > + test_pc_offset_radix 8 {0[0-7]{2,}} > > + test_pc_offset_radix 10 {[1-9][0-9]+} > This regex fails if the number is exactly 0, but if you use multiple > instructions and -wrap, I don't think it is a big deal.... Yep, I'll make some tweaks here. > > + test_pc_offset_radix 16 {0x[0-9a-f]{2,}} > > + > > + gdb_test "set output-radix 10" "Output radix now set to decimal 10= .*\\." \ > > + "restore output-radix" > > +} > > diff --git a/gdb/valprint.c b/gdb/valprint.c > > index 62b1b33bb66..ea4bece0416 100644 > > --- a/gdb/valprint.c > > +++ b/gdb/valprint.c > > @@ -171,6 +171,16 @@ show_output_radix (struct ui_file *file, int from_= tty, > > value); > > } > > > > There should be a comment here like > > /* See valprint.h. */ ack > > > +std::string > > +format_pc_offset (int offset) > > +{ > > + const char *sign =3D (offset < 0) ? "-" : "+"; > > + ULONGEST uoffset =3D (offset < 0) ? -(ULONGEST) offset : (ULONGEST) = offset; > > + > > + std::string body =3D int_string (uoffset, output_radix, 0, 0, 1); > > since uoffset is ULONGEST, you should use pulongest. However, I don't > even think you need this extra variable, you can just pass (offset < 0) > -offset : offset to the int_string call. > > > + return std::string (sign) + body; > why not declare sign as an std::string? I think it would make things a > little more readable. > > +} > > + > > /* By default we print arrays without printing the index of each elem= ent in > > the array. This behavior can be changed by setting PRINT_ARRAY_IN= DEXES. */ > > > > diff --git a/gdb/valprint.h b/gdb/valprint.h > > index 0ce3e0781f6..5511707cba3 100644 > > --- a/gdb/valprint.h > > +++ b/gdb/valprint.h > > @@ -320,6 +320,12 @@ extern int build_address_symbolic (struct gdbarch = *, > > int *line, > > int *unmapped); > > > > +/* Format OFFSET, the offset portion of a "" display, a= s > > + a string with an explicit sign prefix ("+" or "-"). The numeric > > + portion is rendered using the current "output-radix". */ > > + > > +extern std::string format_pc_offset (int offset); > > + > > /* Check to see if RECURSE is greater than or equal to the allowed > > printing max-depth (see 'set print max-depth'). If it is then pri= nt an > > ellipsis expression to STREAM and return true, otherwise return fa= lse. > > > -- > Cheers, > Guinevere Larsen > It/she >