Re: [PATCH v2 2/2] Refactor amd64_analyze_prologue

[Simon Marchi <simark@simark.ca>]

On 2020-06-11 6:54 p.m., Victor Collod via Gdb-patches wrote:
> * merge op and the buf array, which were both used for storing code
> * invert conditions to avoid long nested ifs
> * use target_read_code instead of read_code to gracefully handle errors
> * `if (current_pc <= pc)' felt backwards, as current_pc doesn't change,
>   and the test could be described as "stop if pc went past current_pc"

I'd appreciate if you could do one patch per change here.  They'd each be
more trivial to review and ensure they are ok.

> 2020-06-11  Victor Collod  <vcollod@nvidia.com>
> 
> 	* amd64-tdep.c (amd64_analyze_prologue): Make the function more readable.
> ---
>  gdb/amd64-tdep.c | 93 ++++++++++++++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 43 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 0ce9fbc2997..6c1a4a138de 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,10 +2383,10 @@ 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)
> +  /* Analysis must not go past current_pc */

Take the habit of starting each comment with a capital letter, finishing with a period and two
spaces (unless it's not really a sentence, like `/* no-op */`).

> +  if (pc >= current_pc)
>      return current_pc;
>  
>    if (gdbarch_ptr_bit (gdbarch) == 32)
> @@ -2395,57 +2394,65 @@ 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])
> +  /* Try to read enough bytes to check for `endbr64'  */
> +  if (target_read_code (pc, buf, 4) != 0)
> +    {
> +      /* If it fails, read just enough data for `pushq %rbp' */
> +      if (target_read_code (pc, buf, 1) != 0)
> +	return pc;
> +    }
> +  /* If reading succeeded, check for the `endbr64' instruction and skip it if found.  */
> +  else if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
>      {
> -      read_code (pc + 1, buf, 3);
> +      pc += sizeof (endbr64);
> +      /* If we went past the allowed bound, stop */
> +      if (pc >= current_pc)
> +	return current_pc;

For readability, please add an empty line after return statements, when there
is another statement after it.

Thanks,

Simon