[PING][PATCH v3] gdb: register frame_destroyed function for amd64 gdbarch

[Guinevere Larsen <blarsen@redhat.com>]

Ping!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

On 08/11/2023 15:24, Guinevere Larsen wrote:
> gdbarches usually register functions to check when a frame is destroyed
> which is used with software watchpoints, since the expression of the
> watchpoint is no longer vlaid at this point.  On amd64, this wasn't done
> anymore because GCC started using CFA for variable locations instead.
>
> However, clang doesn't use the CFA and instead relies on specifying when
> an epilogue has started, meaning software watchpoints get a spurious hit
> when a frame is destroyed. This patch re-adds the code to register the
> function that detects when a frame is destroyed, but only uses this when
> the producer is LLVM, so gcc code isn't affected. The logic that
> identifies the epilogue has been factored out into the new function
> amd64_stack_frame_destroyed_p_1, so the frame sniffer can call it
> directly, and its behavior isn't changed.
>
> This can also remove the XFAIL added to gdb.python/pq-watchpoint tests
> that handled this exact flaw in clang
>
> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
> ---
>   gdb/amd64-tdep.c                           | 32 ++++++++++++++++------
>   gdb/testsuite/gdb.python/py-watchpoint.exp | 17 +-----------
>   2 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index e6feee677b3..2e101b4fca1 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2886,6 +2886,22 @@ static const struct frame_base amd64_frame_base =
>     amd64_frame_base_address
>   };
>   
> +/* Implement core of the stack_frame_destroyed_p gdbarch method.  */
> +
> +static int
> +amd64_stack_frame_destroyed_p_1 (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  gdb_byte insn;
> +
> +  if (target_read_memory (pc, &insn, 1))
> +    return 0;   /* Can't read memory at pc.  */
> +
> +  if (insn != 0xc3)     /* 'ret' instruction.  */
> +    return 0;
> +
> +  return 1;
> +}
> +
>   /* Normal frames, but in a function epilogue.  */
>   
>   /* Implement the stack_frame_destroyed_p gdbarch method.
> @@ -2897,15 +2913,13 @@ static const struct frame_base amd64_frame_base =
>   static int
>   amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>   {
> -  gdb_byte insn;
> +  struct compunit_symtab *cust = find_pc_compunit_symtab (pc);
>   
> -  if (target_read_memory (pc, &insn, 1))
> -    return 0;   /* Can't read memory at pc.  */
> +  if (cust != nullptr && cust->producer () != nullptr
> +      && producer_is_llvm (cust->producer ()))
> +    return amd64_stack_frame_destroyed_p_1 (gdbarch, pc);
>   
> -  if (insn != 0xc3)     /* 'ret' instruction.  */
> -    return 0;
> -
> -  return 1;
> +  return 0;
>   }
>   
>   static int
> @@ -2938,7 +2952,7 @@ amd64_epilogue_frame_sniffer_1 (const struct frame_unwind *self,
>       }
>   
>     /* Check whether we're in an epilogue.  */
> -  return amd64_stack_frame_destroyed_p (gdbarch, pc);
> +  return amd64_stack_frame_destroyed_p_1 (gdbarch, pc);
>   }
>   
>   static int
> @@ -3310,6 +3324,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
>   
>     set_gdbarch_gen_return_address (gdbarch, amd64_gen_return_address);
>   
> +  set_gdbarch_stack_frame_destroyed_p (gdbarch, amd64_stack_frame_destroyed_p);
> +
>     /* SystemTap variables and functions.  */
>     set_gdbarch_stap_integer_prefixes (gdbarch, stap_integer_prefixes);
>     set_gdbarch_stap_register_prefixes (gdbarch, stap_register_prefixes);
> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.exp b/gdb/testsuite/gdb.python/py-watchpoint.exp
> index 5ff61285979..9a6ef447572 100644
> --- a/gdb/testsuite/gdb.python/py-watchpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
> @@ -42,20 +42,5 @@ gdb_test "source $pyfile" ".*Python script imported.*" \
>       "import python scripts"
>   gdb_test "python print(len(gdb.breakpoints()))" "2" "check modified BP count"
>   gdb_test "continue" ".*" "run until program stops"
> -# Clang doesn't use CFA location information for variables (despite generating
> -# them), meaning when the instruction "pop rbp" happens, we get a false hit
> -# on the watchpoint. for more details, see:
> -# https://github.com/llvm/llvm-project/issues/64390
> -gdb_test_multiple "python print(bpt.n)" "check watchpoint hits" {
> -    -re -wrap "5" {
> -	pass $gdb_test_name
> -    }
> -    -re -wrap "6" {
> -	if {[test_compiler_info "clang-*"]} {
> -	    xfail "$gdb_test_name (clang issue 64390)"
> -	} else {
> -	    fail $gdb_test_name
> -	}
> -    }
> -}
> +gdb_test "python print(bpt.n)" "5" "check watchpoint hits"
>   gdb_test "python print(len(gdb.breakpoints()))" "1" "check BP count"