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

[Simon Marchi <simark@simark.ca>]

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