RE: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes

["Schimpe, Christina" <christina.schimpe@intel.com>]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Friday, July 18, 2025 5:24 PM
> To: Guinevere Larsen <guinevere@redhat.com>; Kupczak, Pawel
> <pawel.kupczak@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to
> skip register pushes
> 
> Guinevere Larsen <guinevere@redhat.com> writes:
> 
> > Hello! Thanks for working on this!
> >
> > I am not an authority on i386, but I did notice a few things that gave
> > me pause so I'd like to ask them to make sure I understand the changes.
> >
> > Skipping the commit message:
> > On 7/1/25 7:47 AM, Pawel Kupczak wrote:
> >> ---
> >>   gdb/amd64-tdep.c                              | 53 +++++++++++-
> >>   .../amd64-extended-prologue-analysis.c        | 49 +++++++++++
> >>   .../amd64-extended-prologue-analysis.exp      | 86 +++++++++++++++++++
> >>   3 files changed, 187 insertions(+), 1 deletion(-)
> >>   mode change 100644 => 100755 gdb/amd64-tdep.c
> >>   create mode 100644 gdb/testsuite/gdb.arch/amd64-extended-prologue-
> analysis.c
> >>   create mode 100644
> >> gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp
> >>
> >> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c old mode 100644 new
> >> mode 100755 index 82dd1e07cf3..863b29a8c27
> >> --- a/gdb/amd64-tdep.c
> >> +++ b/gdb/amd64-tdep.c
> >> @@ -2553,6 +2553,56 @@ amd64_analyze_frame_setup (gdbarch
> *gdbarch, CORE_ADDR pc,
> >>     return pc;
> >>   }
> >>
> >> +/* Check whether PC points at code pushing registers onto the stack.  If
> so,
> >> +   update CACHE and return pc after those pushes or CURRENT_PC,
> whichever is
> >> +   smaller.  Otherwise, return PC passed to this function.  */
> >> +
> >> +static CORE_ADDR
> >> +amd64_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
> >> +			      amd64_frame_cache *cache)
> >> +{
> >> +  gdb_byte op;
> >> +  int offset = 0;
> >> +
> >> +  /* There are at most 16 registers that would be pushed in the
> >> + prologue.  */  for (int i = 0; i < 16 && pc < current_pc; i++)
> >> +    {
> >> +      int reg = 0;
> >> +      int pc_offset = 0;
> >> +
> >> +      if (target_read_code (pc, &op, 1) == -1)
> >> +	return pc;
> >> +
> >> +      /* %r8 - %r15 prefix.  */
> >> +      if (op == 0x41)
> >
> > Looking over on the disassembler for record-full, I see that all 0x4-
> > are considered prefixes, and (in 64 bit targets) they all have this effect.
> >
> > Is this something that could affect this prologue analyzer? ie, that
> > some prefix like 0x40 is used to mean "push a register larger than 7",
> > which would cause us to not skip that instruction?
> >
> >> +	{
> >> +	  reg += 8;
> >> +	  pc_offset = 1;
> >> +
> >> +	  if (target_read_code (pc + 1, &op, 1) == -1)
> >> +	    return pc;
> >> +	}
> >> +
> >> +      /* push %rax|%rcx|%rdx|%rbx|%rsp|%rbp|%rsi|%rdi
> >> +
> >> +	 or with 0x41 prefix:
> >> +	 push %r8|%r9|%r10|%r11|%r12|%r13|%r14|%r15.  */
> >> +      if (op < 0x50 || op > 0x57)
> >> +	break;
> >
> > This code also ignores the "push lz" and "push lb" instructions. Could
> > they be used as prologue instructions? (again, this comes from the
> > recording disassembler)
> >
> > If so, I would think this is worth explicity saying it in the comment
> > at the top of the function, otherwise it could confuse us in the future.
> >
> >> +
> >> +      reg += op - 0x50;
> >> +      offset -= 8;
> >> +
> >> +      int regnum = amd64_arch_reg_to_regnum (reg);
> >> +      cache->saved_regs[regnum] = offset;
> >> +      cache->sp_offset += 8;
> >> +
> >> +      pc += 1 + pc_offset;
> >> +    }
> >> +
> >> +  return pc;
> >> +}
> >> +
> >>   /* Do a limited analysis of the prologue at PC and update CACHE
> >>      accordingly.  Bail out early if CURRENT_PC is reached.  Return the
> >>      address where the analysis stopped.
> >> @@ -2594,7 +2644,8 @@ amd64_analyze_prologue (gdbarch *gdbarch,
> CORE_ADDR pc, CORE_ADDR current_pc,
> >>     if (current_pc <= pc)
> >>       return current_pc;
> >>
> >> -  return amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
> >> +  pc = amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
> >> + return amd64_analyze_register_saves (pc, current_pc, cache);
> >>   }
> >>
> >>   /* Work around false termination of prologue - GCC PR debug/48827.
> >> diff --git
> >> a/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c
> >> b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c
> >> new file mode 100644
> >> index 00000000000..7d777d23236
> >> --- /dev/null
> >> +++ b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c
> >> @@ -0,0 +1,49 @@
> >> +/* This testcase is part of GDB, the GNU debugger.
> >> +
> >> +   Copyright 2025 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/>.  */
> >> +
> >> +int __attribute__ ((noinline))
> >> +bar (int x)
> >> +{
> >> +  return x + x;
> >> +}
> >> +
> >> +/* This function should generate a prologue in shape of:
> >> +    push  %rbp
> >> +    .cfi_def_cfa_offset 16
> >> +    .cfi_offset %rbp, -16
> >> +    mov   %rsp, %rbp
> >> +    .cfi_def_cfa_register %rbp
> >> +    push  %reg1
> >> +    push  %reg2
> >> +    .cfi_offset %reg2, 32
> >> +    .cfi_offset %reg1, 24
> >> +
> >> +    So to be able to unwind a register, GDB needs to skip prologue past
> >> +    register pushes (to access .cfi directives).  */
> >
> > In general, if possible, I think it's preferred that we actually use
> > assembly instead of relying on compiler behavior. You should be able
> > to use asm statements to create a fake function, and either the dwarf
> > assembler to actually label it as such... Maybe even using global
> > labels to do that more easily (not sure if GDB would use the prologue
> > skip in that case, though).
> 
> I think there's definitely scope for both types of test in cases like this.  On the
> one hand, we are interested in debugging actual code, so if a future compiler
> does something unexpected, then it would be nice to see a test break.
> 
> But as Gwen says, having tests that use fixed assembler sequences does
> mean that GDB is guaranteed to always support a particular series of
> instructions.
> 
> The good thing in this case is that the test is 100% amd64 specific, so it
> should be easy enough to just grab the disassembly from the current test
> binary, and build a test around that.  You might even be able to fold the two
> tests into one, something like:
> 
>   standard_testfile .c .S
> 
> then you can compile $srcfile and $srcfile2 in turn and run the exact same
> test proc against the compiled binary.  Though this isn't a hard requirement,
> if its easier to do it some other way, then that's fine.

Just my 2 cents on this, as I wondered about this some time ago and am happy that we have this discussion here. 😊
I also thought we should prefer using compiler based tests to see potential breaks with future compilers (as Andrew states) and in this way we can also easily test different compilers.
For handwritten assembly we have to make sure that we follow the ABI which can be tricky if we not directly grab the assembly from the compiled binary, but I also understand Guinevere's argument.

Having both handwritten-assembly and compiler based tests seems ideal I think.
But if I had to decide on one option I would avoid having assembly-based tests only actually to make sure we support future compiler's behaviour in GDB.

Kind Regards,
Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928