Re: [PATCH] Improve intel IBT support

[Simon Marchi <simark@simark.ca>]

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