Re: [PATCH] Improve intel IBT support

Simon Marchi
2020-06-11T03:18:51Z

On 2020-06-05 7:23 p.m., Victor Collod via Gdb-patches wrote:
> Refactor amd64_analyze_prologue to be more linear, add i386 support for endbr32.

Hi Victor,

Thanks for the patch. You mentioned this is your first submission; I see you've
used git-send-email, that's a very good start!

> 2020-03-12 Victor Collod <vcollod@nvidia.com>
>
> * i386-tdep.c (i386_skip_endbr): add a helper function to skip endbr
> instructions.
> (i386_analyze_prologue): call i386_skip_endbr.
> * amd64-tdep.c (amd64_analyze_prologue): make the function more linear

If I understand correctly, you are doing two orthogonal changes in this patch:

1- Change amd64_analyze_prologue to make it clearer / more readable (that's what I
understand by "more linear")
2. Add support for skipping another instruction

If that's the case, I think that can be a two patches series, such that each patch
has only one concern. This way, it's easier to convince ourself that each is correct.
Also, if a bug is introduced by one of the patches, it's easier to bisect and find
the culprit.

> ---
> gdb/amd64-tdep.c | 76 +++++++++++++++++++++++-------------------------
> gdb/i386-tdep.c | 19 ++++++++++++
> 2 files changed, 56 insertions(+), 39 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index f96a9868259..06d0fe9a194 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
> CORE_ADDR pc, CORE_ADDR current_pc,
> struct 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. */
> @@ -2384,8 +2383,7 @@ 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 };
>
> - gdb_byte buf[3];
> - gdb_byte op;
> + gdb_byte buf[4];
>
> if (current_pc <= pc)
> return current_pc;
> @@ -2395,57 +2393,57 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
> else
> pc = amd64_analyze_stack_align (pc, current_pc, cache);
>
> - op = read_code_unsigned_integer (pc, 1, byte_order);
> + read_code (pc, buf, 4);

Just guessing, but I thought that the purpose of reading just one byte here
is so that if we're right at the end of a readable memory region, we won't
read too far. If read_code can't read the whole 4 bytes, it will throw an
exception.

> /* Check for the `endbr64` instruction, skip it if found. */
> - if (op == endbr64[0])
> + if (memcmp (buf, endbr64, sizeof(endbr64)) == 0)

Space after `sizeof`, happens a few times.

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index e87d7f36356..f7670a7febb 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
> { 0 }
> };
>
> +/* Check whether PC points to an endbr32 instruction. */
> +static CORE_ADDR
> +i386_skip_endbr(CORE_ADDR pc)

Space before parenthesis.

Simon