[PATCH 1/3] gdb: refactor amd64_analyze_prologue
Andrew Burgess
aburgess@redhat.com
Fri Jun 13 15:08:49 GMT 2025
Paweł Kupczak <pawel.kupczak@intel.com> writes:
> From: Pawel Kupczak <pawel.kupczak@intel.com>
>
> Refactor amd64_analyze_prologue so it clearly reflects what is the order
> of operations in the prologue that we expect to encounter, as is the
> case for i386's implementation.
Looks good. I think the new layout is easier to follow. I had one tiny
comment, which isn't really a requirement...
> ---
> gdb/amd64-tdep.c | 125 +++++++++++++++++++++++++++--------------------
> 1 file changed, 73 insertions(+), 52 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index e4957783502..5b23cf58e88 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2129,6 +2129,30 @@ amd64_alloc_frame_cache (void)
> return cache;
> }
>
> +/* Check whether PC is at "endbr64" instruction. If so, return PC past it.
> + Otherwise, return PC passed to this function. */
> +
> +static CORE_ADDR
> +amd64_skip_endbr (gdbarch *gdbarch, CORE_ADDR pc)
> +{
> + static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
> +
> + bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> + gdb_byte buf[3];
> + gdb_byte op = read_code_unsigned_integer (pc, 1, byte_order);
> +
> + /* Check for the `endbr64` instruction, skip it if found. */
> + if (op == endbr64[0])
> + {
> + read_code (pc + 1, buf, 3);
> +
> + if (memcmp (buf, &endbr64[1], 3) == 0)
> + return pc + 4;
> + }
> +
> + return pc;
> +}
> +
> /* GCC 4.4 and later, can put code in the prologue to realign the
> stack pointer. Check whether PC points to such code, and update
> CACHE accordingly. Return the first instruction after the code
> @@ -2466,35 +2490,15 @@ amd64_x32_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
> return std::min (pc + offset + 2, current_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.
> -
> - We will handle only functions beginning with:
> -
> - pushq %rbp 0x55
> - movq %rsp, %rbp 0x48 0x89 0xe5 (or 0x48 0x8b 0xec)
> -
> - or (for the X32 ABI):
> -
> - pushq %rbp 0x55
> - movl %esp, %ebp 0x89 0xe5 (or 0x8b 0xec)
> -
> - The `endbr64` instruction can be found before these sequences, and will be
> - skipped if found.
> -
> - Any function that doesn't start with one of these sequences will be
> - assumed to have no prologue and thus no valid frame pointer in
> - %rbp. */
> +/* Check whether PC points at a code setting up a base pointer based frame.
> + If so, update CACHE and return pc past the sequence that sets up the frame
> + or CURRENT_PC, whichever is smaller. If there's no frame setup, return
> + either PC, or PC + 1 if there happened to be just 'push %rbp'. */
I had to read this a few times before I understood this. Ideally it
would be nice if the supported code sequences were mentioned, but that
would duplicate the comment from amd64_analyze_prologue, so I wonder if
just referencing that function would do, at least then it is obvious
that the comment is not supposed to be read in isolation.
Maybe this comment is clearer?
/* Analyze frame setup instructions at PC on behalf of amd64_analyze_prologue
and update CACHE accordingly. Bail out early if CURRENT_PC is reached.
Return the address where the analysis stopped.
See comment on amd64_analyze_prologue for the sequences handled. The
movq/movl after the push of %rbp is considered optional, and the
`endbr64` handling is not done here, that's in amd64_analyze_prologue. */
Anyway, this is just a suggestion, not a requirement. Figuring it out
didn't take that long, so either way is fine with me.
Approved-By: Andrew Burgess <aburgess@redhat.com>
Thanks,
Andrew
>
> static CORE_ADDR
> -amd64_analyze_prologue (struct gdbarch *gdbarch,
> - CORE_ADDR pc, CORE_ADDR current_pc,
> - struct amd64_frame_cache *cache)
> +amd64_analyze_frame_setup (gdbarch *gdbarch, CORE_ADDR pc,
> + CORE_ADDR current_pc, amd64_frame_cache *cache)
> {
> - enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> - /* The `endbr64` instruction. */
> - static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
> /* There are two variations of movq %rsp, %rbp. */
> static const gdb_byte mov_rsp_rbp_1[3] = { 0x48, 0x89, 0xe5 };
> static const gdb_byte mov_rsp_rbp_2[3] = { 0x48, 0x8b, 0xec };
> @@ -2502,34 +2506,11 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
> static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
> static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
>
> + bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> gdb_byte buf[3];
> - gdb_byte op;
> -
> - if (current_pc <= pc)
> - return current_pc;
> -
> - if (gdbarch_ptr_bit (gdbarch) == 32)
> - pc = amd64_x32_analyze_stack_align (pc, current_pc, cache);
> - else
> - pc = amd64_analyze_stack_align (pc, current_pc, cache);
> -
> - op = read_code_unsigned_integer (pc, 1, byte_order);
> + gdb_byte op = read_code_unsigned_integer (pc, 1, byte_order);
>
> - /* Check for the `endbr64` instruction, skip it if found. */
> - if (op == endbr64[0])
> - {
> - read_code (pc + 1, buf, 3);
> -
> - if (memcmp (buf, &endbr64[1], 3) == 0)
> - pc += 4;
> -
> - op = read_code_unsigned_integer (pc, 1, byte_order);
> - }
> -
> - if (current_pc <= pc)
> - return current_pc;
> -
> - if (op == 0x55) /* pushq %rbp */
> + if (op == 0x55) /* pushq %rbp. */
> {
> /* Take into account that we've executed the `pushq %rbp' that
> starts this instruction sequence. */
> @@ -2569,6 +2550,46 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
> 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.
> +
> + We will handle only functions beginning with:
> +
> + pushq %rbp 0x55
> + movq %rsp, %rbp 0x48 0x89 0xe5 (or 0x48 0x8b 0xec)
> +
> + or (for the X32 ABI):
> +
> + pushq %rbp 0x55
> + movl %esp, %ebp 0x89 0xe5 (or 0x8b 0xec)
> +
> + The `endbr64` instruction can be found before these sequences, and will be
> + skipped if found.
> +
> + Any function that doesn't start with one of these sequences will be
> + assumed to have no prologue and thus no valid frame pointer in
> + %rbp. */
> +
> +static CORE_ADDR
> +amd64_analyze_prologue (gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR current_pc,
> + amd64_frame_cache *cache)
> +{
> + if (current_pc <= pc)
> + return current_pc;
> +
> + if (gdbarch_ptr_bit (gdbarch) == 32)
> + pc = amd64_x32_analyze_stack_align (pc, current_pc, cache);
> + else
> + pc = amd64_analyze_stack_align (pc, current_pc, cache);
> +
> + pc = amd64_skip_endbr (gdbarch, pc);
> + if (current_pc <= pc)
> + return current_pc;
> +
> + return amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache);
> +}
> +
> /* Work around false termination of prologue - GCC PR debug/48827.
>
> START_PC is the first instruction of a function, PC is its minimal already
> --
> 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.
More information about the Gdb-patches
mailing list