Re: [PATCH v2] Use "output-radix" setting to format function offsets

[Guinevere Larsen <guinevere@redhat.com>]

On 5/9/26 1:45 AM, Zander Work wrote:
> Updates since v1:
>
> * Addressed feedback in the test suite by more robustly handling the
>    GDB I/O and improved the regex patterns used to match on the offset
>    values.
> * Updated the accompanying test program to have a minimum level of
>    complexity to avoid compiler optimizations trimming out too much
>    function body, and fixed the formatting and copyright date
> * Added a comment on the `format_pc_offset()` implementation and
>    addressed comments in that function.
>
> I believe the only open items are:
>
> * Should MI consumers have the string radix-formatted offset value, or
>    continue having an int value (this is the current impl in the patch)?
> * I still need to do an FSF copyright assignment.
>
> Please let me know if I missed anything else to address. Thanks!

Hi Zander!

Thanks for the quick v2 for this patch. When sending another version of 
a patch, we also add the commit message that will be in the git repo, so 
that it can receive comments without needing to find the v1.

For other reviewers' convenience, here's the original email: 
https://inbox.sourceware.org/gdb-patches/CAB3ousCKmytOaWJMNC4kBm8oBqmUWOywg_bEnTutmm9M6vty_Q@mail.gmail.com/T/#m20c344a2b51c35bcb4f678a209f5c5eb986fbb1b

I only have one formatting nit I didn't notice on v1, but there's no 
need to send a v3 just for that. Feel free to add my review tag to the 
end of the commit message!

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

This isn't enough to push the commit, I hope a global maintainer 
approves this patch soon.

> ---
>   gdb/disasm.c                     | 18 ++++++++++---
>   gdb/printcmd.c                   |  2 +-
>   gdb/testsuite/gdb.base/radix.c   | 35 ++++++++++++++++++++++++++
>   gdb/testsuite/gdb.base/radix.exp | 43 ++++++++++++++++++++++++++++++++
>   gdb/valprint.c                   | 13 ++++++++++
>   gdb/valprint.h                   |  6 +++++
>   6 files changed, 112 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 of
> -	   the offset takes the place of the "+" here.  */
> -	if (offset >= 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 based
> +       on the current 'output-radix'.  */
Small nit that I didn't notice in the first version, but this is not 
lined up correctly, it should be 1 tab and 3 spaces.
> +	if (m_uiout->is_mi_like_p ())
> +	  {
> +	    if (offset >= 0)
> +	      m_uiout->text ("+");
> +	    m_uiout->field_signed ("offset", offset);
> +	  }
> +	else
> +	  {
> +	    std::string s = format_pc_offset (offset);
> +	    m_uiout->field_string ("offset", s.c_str ());
I'm going to add this for reference for other reviewers: It seems that 
radix is respected in other commands, like print. So I think it would be 
reasonable to make it respected in the disas command as well, but since 
I don't know how MI consumers work, I also don't know if something would 
break here... so yeah, I think it might be fine, don't really know 
enough but the possibility of MI consumers breaking makes me cautious.
> +	  }
>   	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, CORE_ADDR addr,
>       gdb_puts ("<", stream);
>     fputs_styled (name.c_str (), function_name_style.style (), stream);
>     if (offset != 0)
> -    gdb_printf (stream, "%+d", offset);
> +    gdb_puts (format_pc_offset (offset).c_str (), stream);
>   
>     /* Append source filename and line number if desired.  Give specific
>        line # of this addr, if we have it; else line # of the nearest symbol.  */
> diff --git a/gdb/testsuite/gdb.base/radix.c b/gdb/testsuite/gdb.base/radix.c
> new file mode 100644
> index 00000000000..8a4d1287208
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/radix.c
> @@ -0,0 +1,35 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2026 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   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 <http://www.gnu.org/licenses/>.  */
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +
> +static int v;
> +
> +int
> +main (void)
> +{
> +  v = 0;
> +
> +  puts ("hello world");
> +
> +  printf ("this is another string\n");
> +
> +  v += 3;
> +
> +  return v;
> +}
> diff --git a/gdb/testsuite/gdb.base/radix.exp b/gdb/testsuite/gdb.base/radix.exp
> index 7a4320bbf36..4b8e2d74b49 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,41 @@ 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/i with output-radix $oradix"
> +
> +      gdb_test_multiple "x/i \$pc" "$test 1" {
> +	-re -wrap "<main\\+$offset_re>:.*" {
> +	   pass $gdb_test_name
> +	}
> +      }
> +
> +      gdb_test "ni 3" "\[0-9\]+.*" "Next instruction for radix $oradix"
> +
> +      gdb_test_multiple "x/i \$pc" "$test 2" {
> +	-re -wrap "<main\\+$offset_re>:.*" {
> +	   pass $gdb_test_name
> +	}
> +      }
> +    }
> +
> +    test_pc_offset_radix 8  {0[0-7]+}
> +    test_pc_offset_radix 10 {[1-9][0-9]*}
> +    test_pc_offset_radix 16 {0x[0-9a-f]+}
> +
> +    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..d0d0472ca7a 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -171,6 +171,19 @@ show_output_radix (struct ui_file *file, int from_tty,
>   	      value);
>   }
>   
> +/* See valprint.h.  */
> +
> +std::string
> +format_pc_offset (int offset)
> +{
> +  std::string sign = (offset < 0) ? "-" : "+";
> +
> +  std::string body = int_string (offset < 0 ? -offset : offset, output_radix,
> +                                 0, 0, 1);
> +
> +  return sign + body;
> +}
> +
>   /* By default we print arrays without printing the index of each element in
>      the array.  This behavior can be changed by setting PRINT_ARRAY_INDEXES.  */
>   
> 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 "<symbol+offset>" display, as
> +   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 print an
>      ellipsis expression to STREAM and return true, otherwise return false.


-- 
Cheers,
Guinevere Larsen
It/she