Re: [PATCH v2 1/2] Add i386 support for endbr skipping

Simon Marchi
2020-06-21T11:27:39Z

On 2020-06-11 6:54 p.m., Victor Collod via Gdb-patches wrote:
> 2020-06-11 Victor Collod <vcollod@nvidia.com>
>
> * i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
> (i386_analyze_prologue): Call i386_skip_endbr.
> ---
> gdb/i386-tdep.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 9b905c1996a..263a3fd452e 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)
> +{
> + static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
> +
> + gdb_byte buf[sizeof (endbr32)];
> +
> + /* Stop there if we can't read the code */
> + if (target_read_code (pc, buf, sizeof (endbr32)))
> + return pc;
> +
> + /* If the instruction isn't an endbr32, stop */
> + if (memcmp (buf, endbr32, sizeof (endbr32)) != 0)
> + return pc;
> +
> + return pc + sizeof (endbr32);
> +}
>
> /* Check whether PC points to a no-op instruction. */
> static CORE_ADDR
> @@ -1814,6 +1832,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
> CORE_ADDR pc, CORE_ADDR current_pc,
> struct i386_frame_cache *cache)
> {
> + pc = i386_skip_endbr (pc);
> pc = i386_skip_noop (pc);
> pc = i386_follow_jump (gdbarch, pc);
> pc = i386_analyze_struct_return (pc, current_pc, cache);
> --
> 2.20.1

Hi Victor,

I hadn't realized that this instriction also existed in the 32 bit variant.

The patch looks fine, but is just missing a test. You could maybe adapt
gdb.arch/amd64-prologue-skip-cf-protection.exp so that it runs on i386 as
well? If so, I'd rename it from amd64- to i386-, like there are other tests
running on both amd64 and i386 that are prefixed i386. It's not great, but
at least it would be consistent.

Simon