Re: [PATCH v2 2/2] Refactor amd64_analyze_prologue

Simon Marchi
2020-06-21T11:38:43Z

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