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

["Kupczak, Pawel" <pawel.kupczak@intel.com>]

Hi!  Thanks for taking a look.

> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Friday, July 18, 2025 3:44 PM
> To: 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
> 
> 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?
> 

Now that I checked AMD64 manual, it indeed mentions that this prefix
ranges from 0x40 - 0x4F.  I might've tunneled too hard on one value, I
checked compiler explorer now and it uses a different one too (0x48).

> > +	{
> > +	  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.
> 

I checked now and those instructions push immediate values onto the
stack.  AFAIK the part of function prologue that pushes stuff is supposed
to preserve register values for its caller and this is part of the function's
stack frame.  So I don't think these instructions are expected here.
Though a small comment about it would not hurt, true.

> > +
> > +      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).
> 
> Checking the exp file, that is especially important because r12 is
> hardcoded but compilers don't promise to be at all consistent about
> which general purpose registers they'll use; and because you're relying
> on optimizations which can change drastically between compilers and
> versions. Case in point, this test will fail with clang. More below
> 
> > +int __attribute__ ((noinline))
> > +foo (int a, int b, int c, int d)
> > +{
> > +  a += bar (a) + bar (b) + bar (c);
> > +  return a;
> > +}
> > +
> > +int
> > +main (int argc, char **argv)
> > +{
> > +  int a = foo (argc, argc + 1, argc + 2, argc * 2);
> > +  return a;
> 
> Clang seems to perform some tailcall optimization here, since GDB's
> backtrace looks like this:
> 
> I also noticed that the test fails when using clang and getting a
> backtrace, with the following output:
> 
> (gdb) bt
> #0  0x000055555555442b in foo ()
> #1  0x00007ffff7cc45f5 in __libc_start_call_main () from /lib64/libc.so.6
> #2  0x00007ffff7cc46a8 in __libc_start_main_impl () from /lib64/libc.so.6
> #3  0x0000555555554345 in _start ()
> 
> And the disasembly of "main" looks like this:
> 0000000000000460 <main>:
>   460:   55                      push   %rbp
>   461:   48 89 e5                mov    %rsp,%rbp
>   464:   8d 77 01                lea    0x1(%rdi),%esi
>   467:   8d 57 02                lea    0x2(%rdi),%edx
>   46a:   5d                      pop    %rbp
>   46b:   e9 b0 ff ff ff          jmp    420 <foo>
> 
> So clang actually noticed that the 'a' variable isn't really necessary,
> and that the return of 'foo' is the return of main, so does a straight
> jump to foo. Maybe this can be solved by having 'a' be volatile, but I
> would still say that generating a .s file or a series of hand-written
> assembly instructions to test with is much more reliable.
> 

To be fair, I thought it would be preferred to try to avoid hand-written
assembly.  If that's actually not an issue, I'll be happy to redo it.

> > +}
> (Skipping exp file, since I have no comments about it specifically)
> 
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers

Thanks again and with regards,
Paweł

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.