* [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
2025-07-01 10:47 [PATCH 0/3] extending the amd64 prologue analyzer Pawel Kupczak
@ 2025-07-01 10:47 ` Pawel Kupczak
2025-07-18 13:43 ` Guinevere Larsen
2025-07-01 10:47 ` [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached Pawel Kupczak
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Pawel Kupczak @ 2025-07-01 10:47 UTC (permalink / raw)
To: gdb-patches
A typical function's prologue can consist of setting up a frame pointer,
pushing registers onto the stack and allocating space on the stack.
Current amd64 prologue analyzer would stop after the frame setup.
This patch allows GDB to skip past register pushes, while also improving
unwinding pushed registers, for functions with a frame pointer, without
debug info and .cfi directives found in .eh_frame section that are used
for unwinding. Skipping register pushes was also present for i386
targets before - the proposed changes are based on i386 implementation.
It also improves the unwinding even if .cfi directives are present,
because GDB can only unwind a register if it has reached a corresponding
.cfi directive, which won't be there before the pushes.
Additionally, at least gcc 11.4 and later by default doesn't emit
necessary debug info, which GDB would try to use to find prologue's end.
In that case, extended prologue analyzer would take effect.
Using C source listed below as an example, compiled with gcc 11.4.0:
```
int __attribute__ ((noinline))
bar (int a)
{
return a + a;
}
int __attribute__ ((noinline))
foo (int a, int b, int c, int d, int e)
{
int x = bar (a) + bar (b) + bar (c) + bar (d) + bar (e);
return x;
}
int
main (int argc, char **argv)
{
return foo (1, 2, 3, 4, 5);
}
```
Compiling with "gcc -O1 -fno-omit-frame-pointer
-fno-asynchronous-unwind-tables", we get:
```
(gdb) b foo
Breakpoint 1 at 0x1139
(gdb) r
...
Breakpoint 1, 0x0000555555555139 in foo ()
(gdb) disassemble
Dump of assembler code for function foo:
0x0000555555555131 <+0>: endbr64
0x0000555555555135 <+4>: push %rbp
0x0000555555555136 <+5>: mov %rsp,%rbp
=> 0x0000555555555139 <+8>: push %r15
0x000055555555513b <+10>: push %r14
0x000055555555513d <+12>: push %r13
0x000055555555513f <+14>: push %r12
0x0000555555555141 <+16>: push %rbx
0x0000555555555142 <+17>: sub $0x8,%rsp
0x0000555555555146 <+21>: mov %esi,%r15d
...
(gdb) ni
0x000055555555513b in foo ()
(gdb) p $r15
$1 = 140737354125376
(gdb) p $r15=1234
$2 = 1234
(gdb) p $r15
$3 = 1234
(gdb) up
#1 0x00005555555551b7 in main ()
(gdb) p $r15
$4 = 1234
```
With the proposed changes, breakpoint gets past those register pushes:
```
(gdb) b foo
Breakpoint 1 at 0x1142
(gdb) r
...
Breakpoint 1, 0x0000555555555142 in foo ()
(gdb) disassemble
Dump of assembler code for function foo:
0x0000555555555131 <+0>: endbr64
0x0000555555555135 <+4>: push %rbp
0x0000555555555136 <+5>: mov %rsp,%rbp
0x0000555555555139 <+8>: push %r15
0x000055555555513b <+10>: push %r14
0x000055555555513d <+12>: push %r13
0x000055555555513f <+14>: push %r12
0x0000555555555141 <+16>: push %rbx
=> 0x0000555555555142 <+17>: sub $0x8,%rsp
0x0000555555555146 <+21>: mov %esi,%r15d
...
```
Also, unwinding pushed registers now works:
```
...
Breakpoint 1, 0x0000555555555142 in foo ()
(gdb) disassemble
Dump of assembler code for function foo:
0x0000555555555131 <+0>: endbr64
0x0000555555555135 <+4>: push %rbp
0x0000555555555136 <+5>: mov %rsp,%rbp
0x0000555555555139 <+8>: push %r15
0x0000555555555139 <+8>: push %r15
0x000055555555513b <+10>: push %r14
0x000055555555513d <+12>: push %r13
0x000055555555513f <+14>: push %r12
0x0000555555555141 <+16>: push %rbx
=> 0x0000555555555142 <+17>: sub $0x8,%rsp
0x0000555555555146 <+21>: mov %esi,%r15d
...
(gdb) p $r15
$1 = 140737354125376
(gdb) p $r15=1234
$2 = 1234
(gdb) p $r15
$3 = 1234
(gdb) up
#1 0x00005555555551b7 in main ()
(gdb) p $r15
$4 = 140737354125376
```
Additionally a new test was added to verify this behavior.
---
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)
+ {
+ 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;
+
+ 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). */
+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;
+}
diff --git a/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp
new file mode 100644
index 00000000000..49e03357995
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp
@@ -0,0 +1,86 @@
+# 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/>.
+#
+# This test verifies that when placing a breakpoint on a function with a frame
+# pointer, instructions that push callee-saved registers in the prologue are
+# skipped, without debug info. When stopped on such breakpoint, the pushed
+# registers should be able to be immediately unwound. With debug info present,
+# GDB would try to use prologue-end markers found in the line table to
+# determine where the prologue ends.
+#
+# It is also tested both with and without .eh_frame's .cfi directives - with
+# them, GDB can only unwind a register once stopped after .cfi directive for
+# that register's push.
+
+require is_x86_64_m64_target
+standard_testfile .c
+
+# Place a breakpoint at foo, go to it and test if %r12 can be unwound.
+proc test_run {} {
+ gdb_breakpoint "foo"
+ gdb_continue_to_breakpoint "Continue to foo"
+
+ gdb_test "backtrace" "#0 $::hex in foo\[^s\]+#1 $::hex in main\[^s\]+" \
+ "Verify backtrace output in foo"
+
+ set main_r12 [get_integer_valueof "\$r12" "null"]
+ gdb_assert { $main_r12 ne "null" }
+
+ set foo_r12 [expr "$main_r12 + 2"]
+ gdb_test "print \$r12=$foo_r12" "$foo_r12" "Set foo's %r12=$foo_r12"
+ gdb_test "up" ".*main.*" "Go up a frame from foo"
+ gdb_test "print \$r12" "$main_r12" "foo's %r12 unwound"
+ gdb_test "down" ".*foo.*" "Go back down a frame to foo"
+ gdb_test "print \$r12" "$foo_r12" "foo's %r12 unwound back"
+}
+
+with_test_prefix "w/o .cfi directives" {
+ # -fno-asynchronous-unwind-tables is needed to get rid of .cfi directives in
+ # .eh_frame section.
+ if { [gdb_can_simple_compile fno-asynchronous-unwind-tables \
+ { void foo () { } } object -fno-asynchronous-unwind-tables] != 0 } {
+
+ # For at least gcc 11.4/clang 14.0.0 and later, -O1 makes them more
+ # eager to use registers in the prologue.
+ #
+ # At least gcc 11.4 and later by default does not generate full debug
+ # info, "nodebug" is there for other compilers.
+ if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
+ { optimize=-O1 nodebug
+ additional_flags=-fno-asynchronous-unwind-tables
+ additional_flags=-fno-omit-frame-pointer}] } {
+ return -1
+ }
+
+ if { ![runto_main] } {
+ return -1
+ }
+
+ test_run
+ }
+}
+
+with_test_prefix "with .cfi directives" {
+ if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
+ { optimize=-O1 nodebug additional_flags=-fno-omit-frame-pointer}] } {
+ return -1
+ }
+
+ if { ![runto_main] } {
+ return -1
+ }
+
+ test_run
+}
--
2.34.1
---------------------------------------------------------------------
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
2025-07-01 10:47 ` [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes Pawel Kupczak
@ 2025-07-18 13:43 ` Guinevere Larsen
2025-07-18 15:15 ` Kupczak, Pawel
2025-07-18 15:23 ` Andrew Burgess
0 siblings, 2 replies; 15+ messages in thread
From: Guinevere Larsen @ 2025-07-18 13:43 UTC (permalink / raw)
To: Pawel Kupczak, gdb-patches
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).
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.
> +}
(Skipping exp file, since I have no comments about it specifically)
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
2025-07-18 13:43 ` Guinevere Larsen
@ 2025-07-18 15:15 ` Kupczak, Pawel
2025-07-23 10:34 ` Kupczak, Pawel
2025-07-18 15:23 ` Andrew Burgess
1 sibling, 1 reply; 15+ messages in thread
From: Kupczak, Pawel @ 2025-07-18 15:15 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
2025-07-18 15:15 ` Kupczak, Pawel
@ 2025-07-23 10:34 ` Kupczak, Pawel
2025-07-23 16:07 ` Guinevere Larsen
0 siblings, 1 reply; 15+ messages in thread
From: Kupczak, Pawel @ 2025-07-23 10:34 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches; +Cc: Schimpe, Christina
> -----Original Message-----
> From: Kupczak, Pawel <pawel.kupczak@intel.com>
> Sent: Friday, July 18, 2025 5:15 PM
> To: Guinevere Larsen <guinevere@redhat.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to
> skip register pushes
>
> 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).
Correction on that - first, that 0x48 never actually happened (I was looking
at the wrong thing).
Second, regarding REX values. I think for those register pushes, 0x41 is the
only value relevant here. Looking at instruction set and reading about the
prefix at https://dn720004.ca.archive.org/0/items/os-dev-manuals/amd%20amd64%20sdm%20instructions.pdf,
it mentions in section 1.4.4 the meanings of the 4 bits. I think in this case
REX.B is the only one that's gonna be set:
'push' will always take 64 bit operand here so REX.W would not be set.
'push' also doesn't use ModRM AFAIK so REX.R shouldn't be applicable.
REX.X seems completely irrelevant.
Based on that I think REX.B is the only bit that will be set in such cases,
which maps to REX == 0x41.
Let me know your thoughts :D
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
2025-07-23 10:34 ` Kupczak, Pawel
@ 2025-07-23 16:07 ` Guinevere Larsen
0 siblings, 0 replies; 15+ messages in thread
From: Guinevere Larsen @ 2025-07-23 16:07 UTC (permalink / raw)
To: Kupczak, Pawel, gdb-patches; +Cc: Schimpe, Christina
On 7/23/25 7:34 AM, Kupczak, Pawel wrote:
>> -----Original Message-----
>> From: Kupczak, Pawel <pawel.kupczak@intel.com>
>> Sent: Friday, July 18, 2025 5:15 PM
>> To: Guinevere Larsen <guinevere@redhat.com>; gdb-
>> patches@sourceware.org
>> Subject: RE: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to
>> skip register pushes
>>
>> 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
Skipped for brevity
>>>> + 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).
> Correction on that - first, that 0x48 never actually happened (I was looking
> at the wrong thing).
>
> Second, regarding REX values. I think for those register pushes, 0x41 is the
> only value relevant here. Looking at instruction set and reading about the
> prefix at https://dn720004.ca.archive.org/0/items/os-dev-manuals/amd%20amd64%20sdm%20instructions.pdf,
> it mentions in section 1.4.4 the meanings of the 4 bits. I think in this case
> REX.B is the only one that's gonna be set:
> 'push' will always take 64 bit operand here so REX.W would not be set.
> 'push' also doesn't use ModRM AFAIK so REX.R shouldn't be applicable.
> REX.X seems completely irrelevant.
>
> Based on that I think REX.B is the only bit that will be set in such cases,
> which maps to REX == 0x41.
>
> Let me know your thoughts :D
well, I think you probably know a lot more of me :D
If I was writing it, I'd say "if ((op & 0xf1) == 0x41)", but that's
because I'm overly cautious about this stuff since I don't know how
compilers behave when "bit will be ignored". I'm fine with whichever
option you prefer in this case.
--
Cheers,
Guinevere Larsen
She/Her/Hers
>
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
2025-07-18 13:43 ` Guinevere Larsen
2025-07-18 15:15 ` Kupczak, Pawel
@ 2025-07-18 15:23 ` Andrew Burgess
2025-07-18 16:04 ` Schimpe, Christina
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Burgess @ 2025-07-18 15:23 UTC (permalink / raw)
To: Guinevere Larsen, Pawel Kupczak, gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
2025-07-18 15:23 ` Andrew Burgess
@ 2025-07-18 16:04 ` Schimpe, Christina
0 siblings, 0 replies; 15+ messages in thread
From: Schimpe, Christina @ 2025-07-18 16:04 UTC (permalink / raw)
To: Andrew Burgess, Guinevere Larsen, Kupczak, Pawel, gdb-patches
> -----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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached
2025-07-01 10:47 [PATCH 0/3] extending the amd64 prologue analyzer Pawel Kupczak
2025-07-01 10:47 ` [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes Pawel Kupczak
@ 2025-07-01 10:47 ` Pawel Kupczak
2025-07-18 13:46 ` Guinevere Larsen
2025-07-18 14:46 ` Andrew Burgess
2025-07-01 10:47 ` [PATCH 3/3] gdb, amd64: extend the amd64 prologue analyzer to skip stack alloc Pawel Kupczak
2025-07-15 7:37 ` [PING] [PATCH 0/3] extending the amd64 prologue analyzer Kupczak, Pawel
3 siblings, 2 replies; 15+ messages in thread
From: Pawel Kupczak @ 2025-07-01 10:47 UTC (permalink / raw)
To: gdb-patches
Make sure the function bails out early if CURRENT_PC is reached, to
avoid the call to amd64_analyze_register_saves.
---
gdb/amd64-tdep.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 863b29a8c27..e4eb076a55a 100755
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2645,6 +2645,9 @@ amd64_analyze_prologue (gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR current_pc,
return current_pc;
pc = amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
+ if (current_pc <= pc)
+ return current_pc;
+
return amd64_analyze_register_saves (pc, current_pc, cache);
}
--
2.34.1
---------------------------------------------------------------------
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached
2025-07-01 10:47 ` [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached Pawel Kupczak
@ 2025-07-18 13:46 ` Guinevere Larsen
2025-07-18 15:19 ` Kupczak, Pawel
2025-07-18 14:46 ` Andrew Burgess
1 sibling, 1 reply; 15+ messages in thread
From: Guinevere Larsen @ 2025-07-18 13:46 UTC (permalink / raw)
To: Pawel Kupczak, gdb-patches
On 7/1/25 7:47 AM, Pawel Kupczak wrote:
> Make sure the function bails out early if CURRENT_PC is reached, to
> avoid the call to amd64_analyze_register_saves.
This patch is simple enough that I personally don't think it needs to be
separate, it could be sent with the previous one. If you (or someone
else who ends up reviewing this series) would prefer it to be separate,
though, feel free to add my tag
Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
--
Cheers,
Guinevere Larsen
She/Her/Hers
> ---
> gdb/amd64-tdep.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 863b29a8c27..e4eb076a55a 100755
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2645,6 +2645,9 @@ amd64_analyze_prologue (gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR current_pc,
> return current_pc;
>
> pc = amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
> + if (current_pc <= pc)
> + return current_pc;
> +
> return amd64_analyze_register_saves (pc, current_pc, cache);
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached
2025-07-18 13:46 ` Guinevere Larsen
@ 2025-07-18 15:19 ` Kupczak, Pawel
0 siblings, 0 replies; 15+ messages in thread
From: Kupczak, Pawel @ 2025-07-18 15:19 UTC (permalink / raw)
To: Guinevere Larsen, gdb-patches
> -----Original Message-----
> From: Guinevere Larsen <guinevere@redhat.com>
> Sent: Friday, July 18, 2025 3:47 PM
> To: Kupczak, Pawel <pawel.kupczak@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 2/3] gdb, amd64: return after
> amd64_analyze_frame_setup if current_pc reached
>
> On 7/1/25 7:47 AM, Pawel Kupczak wrote:
> > Make sure the function bails out early if CURRENT_PC is reached, to
> > avoid the call to amd64_analyze_register_saves.
>
> This patch is simple enough that I personally don't think it needs to be
> separate, it could be sent with the previous one. If you (or someone
> else who ends up reviewing this series) would prefer it to be separate,
> though, feel free to add my tag
>
> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
Personally, I don't mind either. Though in the meantime, Andrew approved this
so I'll stick with it. Thanks anyway!
>
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers
>
> > ---
> > gdb/amd64-tdep.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> > index 863b29a8c27..e4eb076a55a 100755
> > --- a/gdb/amd64-tdep.c
> > +++ b/gdb/amd64-tdep.c
> > @@ -2645,6 +2645,9 @@ amd64_analyze_prologue (gdbarch *gdbarch,
> CORE_ADDR pc, CORE_ADDR current_pc,
> > return current_pc;
> >
> > pc = amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
> > + if (current_pc <= pc)
> > + return current_pc;
> > +
> > return amd64_analyze_register_saves (pc, current_pc, cache);
> > }
> >
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached
2025-07-01 10:47 ` [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached Pawel Kupczak
2025-07-18 13:46 ` Guinevere Larsen
@ 2025-07-18 14:46 ` Andrew Burgess
2025-07-18 15:21 ` Kupczak, Pawel
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Burgess @ 2025-07-18 14:46 UTC (permalink / raw)
To: Pawel Kupczak, gdb-patches
Pawel Kupczak <pawel.kupczak@intel.com> writes:
> Make sure the function bails out early if CURRENT_PC is reached, to
> avoid the call to amd64_analyze_register_saves.
Thanks for splitting this out. I much prefer seeing smaller patches
like this!
Approved-By: Andrew Burgess <aburgess@redhat.com>
Thanks,
Andrew
> ---
> gdb/amd64-tdep.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 863b29a8c27..e4eb076a55a 100755
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2645,6 +2645,9 @@ amd64_analyze_prologue (gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR current_pc,
> return current_pc;
>
> pc = amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
> + if (current_pc <= pc)
> + return current_pc;
> +
> return amd64_analyze_register_saves (pc, current_pc, cache);
> }
>
> --
> 2.34.1
>
> ---------------------------------------------------------------------
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached
2025-07-18 14:46 ` Andrew Burgess
@ 2025-07-18 15:21 ` Kupczak, Pawel
0 siblings, 0 replies; 15+ messages in thread
From: Kupczak, Pawel @ 2025-07-18 15:21 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Thanks a lot!
With regards,
Paweł
> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Friday, July 18, 2025 4:47 PM
> To: Kupczak, Pawel <pawel.kupczak@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 2/3] gdb, amd64: return after
> amd64_analyze_frame_setup if current_pc reached
>
> Pawel Kupczak <pawel.kupczak@intel.com> writes:
>
> > Make sure the function bails out early if CURRENT_PC is reached, to
> > avoid the call to amd64_analyze_register_saves.
>
> Thanks for splitting this out. I much prefer seeing smaller patches
> like this!
>
> Approved-By: Andrew Burgess <aburgess@redhat.com>
>
> Thanks,
> Andrew
>
>
> > ---
> > gdb/amd64-tdep.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> > index 863b29a8c27..e4eb076a55a 100755
> > --- a/gdb/amd64-tdep.c
> > +++ b/gdb/amd64-tdep.c
> > @@ -2645,6 +2645,9 @@ amd64_analyze_prologue (gdbarch *gdbarch,
> CORE_ADDR pc, CORE_ADDR current_pc,
> > return current_pc;
> >
> > pc = amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
> > + if (current_pc <= pc)
> > + return current_pc;
> > +
> > return amd64_analyze_register_saves (pc, current_pc, cache);
> > }
> >
> > --
> > 2.34.1
> >
> > ---------------------------------------------------------------------
> > 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.
---------------------------------------------------------------------
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] gdb, amd64: extend the amd64 prologue analyzer to skip stack alloc
2025-07-01 10:47 [PATCH 0/3] extending the amd64 prologue analyzer Pawel Kupczak
2025-07-01 10:47 ` [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes Pawel Kupczak
2025-07-01 10:47 ` [PATCH 2/3] gdb, amd64: return after amd64_analyze_frame_setup if current_pc reached Pawel Kupczak
@ 2025-07-01 10:47 ` Pawel Kupczak
2025-07-15 7:37 ` [PING] [PATCH 0/3] extending the amd64 prologue analyzer Kupczak, Pawel
3 siblings, 0 replies; 15+ messages in thread
From: Pawel Kupczak @ 2025-07-01 10:47 UTC (permalink / raw)
To: gdb-patches
Following the previous patch (gdb, amd64: extend the amd64 prologue
analyzer to skip register pushes), this patch extends the analyzer
further to be able to skip stack space allocation as the next prologue
part, for functions with a frame pointer. Implementation was based
on the i386 counterpart, which already had that functionality.
As of now, the stack allocation is not skipped. Examples below use C
source listed below, compiled with gcc 11.4.0.
```
int foo (int n)
{
int ns[] = { 1, 4, 9, 16, 25 };
return ns[n];
}
int
main (int argc, char **argv)
{
return foo (argc);
}
```
Compiling with "gcc -O0 -fno-omit-frame-pointer" we get:
```
(gdb) b foo
Breakpoint 1 at 0x1151
(gdb) r
...
Breakpoint 1, 0x0000555555555151 in foo ()
(gdb) disassemble
Dump of assembler code for function foo:
0x0000555555555149 <+0>: endbr64
0x000055555555514d <+4>: push %rbp
0x000055555555514e <+5>: mov %rsp,%rbp
=> 0x0000555555555151 <+8>: sub $0x30,%rsp
0x0000555555555155 <+12>: mov %edi,-0x24(%rbp)
...
```
With this patch, it gets skipped the same way register pushes are:
```
(gdb) b foo
Breakpoint 1 at 0x1155
(gdb) r
...
Breakpoint 1, 0x0000555555555155 in foo ()
(gdb) disassemble
Dump of assembler code for function foo:
0x0000555555555149 <+0>: endbr64
0x000055555555514d <+4>: push %rbp
0x000055555555514e <+5>: mov %rsp,%rbp
0x0000555555555151 <+8>: sub $0x30,%rsp
=> 0x0000555555555155 <+12>: mov %edi,-0x24(%rbp)
...
```
---
gdb/amd64-tdep.c | 70 ++++++++++++++++++-
.../amd64-extended-prologue-analysis.c | 11 ++-
.../amd64-extended-prologue-analysis.exp | 10 +--
3 files changed, 83 insertions(+), 8 deletions(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index e4eb076a55a..a147d07d9b6 100755
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2603,6 +2603,70 @@ amd64_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
return pc;
}
+/* Check whether PC points at code allocating space on the stack.
+ If so, update CACHE and return pc past it or CURRENT_PC, whichever is
+ smaller. Otherwise, return PC passed to this function. */
+
+static CORE_ADDR
+amd64_analyze_stack_alloc (gdbarch *arch, CORE_ADDR pc, CORE_ADDR current_pc,
+ amd64_frame_cache *cache)
+{
+ static const gdb_byte sub_imm8_rsp[] = { 0x83, 0xec };
+ static const gdb_byte sub_imm32_rsp[] = { 0x81, 0xec };
+ static const gdb_byte lea_disp_rsp[] = { 0x8D, 0x64 };
+
+ bfd_endian byte_order = gdbarch_byte_order (arch);
+ const CORE_ADDR start_pc = pc;
+
+ gdb_byte op;
+ if (target_read_code (pc, &op, 1) == -1)
+ return pc;
+
+ /* Check for REX.W. */
+ if (op == 0x48)
+ pc++;
+
+ if (current_pc <= pc)
+ return current_pc;
+
+ gdb_byte buf[2];
+ read_code (pc, buf, 2);
+
+ /* Check for instruction allocating space on the stack, which looks like
+ sub imm8/32, %rsp
+ or
+ lea -imm (%rsp), %rsp
+
+ and forward pc past it + update cache. */
+
+ /* sub imm8, %rsp. */
+ if (memcmp (buf, sub_imm8_rsp, 2) == 0)
+ {
+ /* Instruction is 3 bytes long. The imm8 arg is the 3rd, single
+ byte. */
+ cache->sp_offset += read_code_integer (pc + 2, 1, byte_order);
+ return pc + 3;
+ }
+ /* sub imm32, %rsp. */
+ else if (memcmp (buf, sub_imm32_rsp, 2) == 0)
+ {
+ /* Instruction is 6 bytes long. The imm32 arg is stored in 4 bytes,
+ starting from 3rd one. */
+ cache->sp_offset += read_code_integer (pc + 2, 4, byte_order);
+ return pc + 6;
+ }
+ /* lea -imm (%rsp), %rsp. */
+ else if (memcmp (buf, lea_disp_rsp, 2) == 0)
+ {
+ /* Instruction is 4 bytes long. The imm arg is the 4th, single
+ byte. */
+ cache->sp_offset += -1 * read_code_integer (pc + 3, 1, byte_order);
+ return pc + 4;
+ }
+
+ return start_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.
@@ -2648,7 +2712,11 @@ amd64_analyze_prologue (gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR current_pc,
if (current_pc <= pc)
return current_pc;
- return amd64_analyze_register_saves (pc, current_pc, cache);
+ pc = amd64_analyze_register_saves (pc, current_pc, cache);
+ if (current_pc <= pc)
+ return current_pc;
+
+ return amd64_analyze_stack_alloc (gdbarch, 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
index 7d777d23236..e4805ec6f55 100644
--- a/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c
+++ b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c
@@ -29,15 +29,22 @@ bar (int x)
.cfi_def_cfa_register %rbp
push %reg1
push %reg2
+ sub $XXX, %rsp
.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). */
+ register pushes and stack allocation (to access .cfi directives). */
int __attribute__ ((noinline))
foo (int a, int b, int c, int d)
{
- a += bar (a) + bar (b) + bar (c);
+ /* "volatile" alone isn't enough for clang to not optimize it out and
+ allocate space on the stack. */
+ volatile char s[256];
+ for (int i = 0; i < 256; i++)
+ s[i] = (char) (a + i);
+
+ a += bar (a) + bar (b) + bar (c) + bar (d);
return a;
}
diff --git a/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp
index 49e03357995..5aeff028a6c 100644
--- a/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp
+++ b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp
@@ -14,11 +14,11 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
# This test verifies that when placing a breakpoint on a function with a frame
-# pointer, instructions that push callee-saved registers in the prologue are
-# skipped, without debug info. When stopped on such breakpoint, the pushed
-# registers should be able to be immediately unwound. With debug info present,
-# GDB would try to use prologue-end markers found in the line table to
-# determine where the prologue ends.
+# pointer, instructions that push callee-saved registers and stack allocation
+# in the prologue are skipped, without debug info. When stopped on such
+# breakpoint, the pushed registers should be able to be immediately unwound.
+# With debug info present, GDB would try to use prologue-end markers found in
+# the line table to determine where the prologue ends.
#
# It is also tested both with and without .eh_frame's .cfi directives - with
# them, GDB can only unwind a register once stopped after .cfi directive for
--
2.34.1
---------------------------------------------------------------------
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PING] [PATCH 0/3] extending the amd64 prologue analyzer
2025-07-01 10:47 [PATCH 0/3] extending the amd64 prologue analyzer Pawel Kupczak
` (2 preceding siblings ...)
2025-07-01 10:47 ` [PATCH 3/3] gdb, amd64: extend the amd64 prologue analyzer to skip stack alloc Pawel Kupczak
@ 2025-07-15 7:37 ` Kupczak, Pawel
3 siblings, 0 replies; 15+ messages in thread
From: Kupczak, Pawel @ 2025-07-15 7:37 UTC (permalink / raw)
To: gdb-patches
Kindly pinging.
With regards,
Paweł
> -----Original Message-----
> From: Pawel Kupczak <pawel.kupczak@intel.com>
> Sent: Tuesday, July 1, 2025 12:48 PM
> To: gdb-patches@sourceware.org
> Subject: [PATCH 0/3] extending the amd64 prologue analyzer
>
> Hi all,
>
> These 3 patches aim to extend the amd64 prologue analyzer, somewhat in
> parallel to how i386 has been doing it already. This should improve
> debugging experience without debug information present.
>
> Looking forward to your feedback!
>
> With regards,
> Paweł
>
> Pawel Kupczak (3):
> gdb, amd64: extend the amd64 prologue analyzer to skip register pushes
> gdb, amd64: return after amd64_analyze_frame_setup if current_pc
> reached
> gdb, amd64: extend the amd64 prologue analyzer to skip stack alloc
>
> gdb/amd64-tdep.c | 124 +++++++++++++++++-
> .../amd64-extended-prologue-analysis.c | 56 ++++++++
> .../amd64-extended-prologue-analysis.exp | 86 ++++++++++++
> 3 files changed, 265 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
>
> --
> 2.34.1
>
> ---------------------------------------------------------------------
> 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.
---------------------------------------------------------------------
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.
^ permalink raw reply [flat|nested] 15+ messages in thread