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

[Zander Work <zdw@google.com>]

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 AM Guinevere Larsen <guinevere@redhat.com> 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        # 0x2004
> >     0x0000000000001158 <+15>:    mov    %rax,%rdi
> >     0x000000000000115b <+18>:    mov    $0x0,%eax
> >     0x0000000000001160 <+23>:    call   0x1050 <printf@plt>
> >     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        # 0x2004
> >     0x0000000000001158 <+0xf>:   mov    %rax,%rdi
> >     0x000000000000115b <+0x12>:  mov    $0x0,%eax
> >     0x0000000000001160 <+0x17>:  call   0x1050 <printf@plt>
> >     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 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'.  */
> > +     if (m_uiout->is_mi_like_p ())
> > +       {
> > +         if (offset >= 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 = 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, 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..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 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>
> > +
> > +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 "<main\\+$offset_re>:\[^\r\n\]*\r\n(?:\[^\r\n\]*\r\n)*$gdb_prompt $" {
>
> 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 = (offset < 0) ? "-" : "+";
> > +  ULONGEST uoffset = (offset < 0) ? -(ULONGEST) offset : (ULONGEST) offset;
> > +
> > +  std::string body = 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 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
>