Re: [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf

[Simon Marchi <simark@simark.ca>]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> Both variables were used to store function code.

I expressed concerns earlier that the code might be written in this way on purpose,
to make sure we don't read too much, in case we are close to the end of a readable
region.  I don't know if this is something that can actually happen in pracptice,
but still tt makes sense to me that we read a single byte, check what it is, and
read more if needed.  Any performance concerns should be taken care of by some cache
at the lower level (each call to read_code won't necessarily cause a target read to
happen).


> 2020-06-23  Victor Collod  <vcollod@nvidia.com>
> 
> 	* amd64-tdep.c (amd64_analyze_prologue): Merge op and buf.
> ---
>  gdb/amd64-tdep.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index ff12cb874b8..c1a9b553e20 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];
>  
>    /* Analysis must not go past current_pc.  */
>    if (pc >= current_pc)
> @@ -2396,24 +2394,20 @@ 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);
> -
> -  /* Check for the `endbr64` instruction, skip it if found.  */
> -  if (op == endbr64[0])
> +  read_code (pc, buf, 4);
> +  /* Check for the `endbr64' instruction and skip it if found.  */
> +  if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
>      {
> -      read_code (pc + 1, buf, 3);
> -
> -      if (memcmp (buf, &endbr64[1], 3) == 0)
> -	pc += 4;
> +      pc += sizeof (endbr64);
> +      /* If we went past the allowed bound, stop.  */
> +      if (pc >= current_pc)
> +	return current_pc;
>  
> -      op = read_code_unsigned_integer (pc, 1, byte_order);
> +      /* Update the code buffer, as pc changed.  */
> +      read_code (pc, buf, 1);
>      }
>  
> -  /* If we went past the allowed bound, stop.  */
> -  if (pc >= current_pc)
> -    return current_pc;
> -
> -  if (op == 0x55)		/* pushq %rbp */
> +  if (buf[0] == 0x55)           /* pushq %rbp */
>      {
>        /* Take into account that we've executed the `pushq %rbp' that
>           starts this instruction sequence.  */

If it's ok to read 4 bytes from the start, then why not merge remove the
following read (not shown in the snippet):

    read_code (pc + 1, buf, 3);

and make the read above (which happens if the endbr64 instruction is found) read
4 bytes directly?

Simon