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

[Andrew Burgess <aburgess@redhat.com>]

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.

Thanks,
Andrew