Re: [PATCH v2] gdb: register frame_destroyed function for amd64 gdbarch

[Andrew Burgess <aburgess@redhat.com>]

Guinevere Larsen <blarsen@redhat.com> writes:

> 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.
>
> 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                           | 16 +++++++++++++++-
>  gdb/testsuite/gdb.python/py-watchpoint.exp | 17 +----------------
>  2 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index e6feee677b3..362151f227c 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2908,6 +2908,18 @@ amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>    return 1;
>  }
>  
> +static int
> +amd64_stack_frame_destroyed_p_1 (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  struct compunit_symtab *cust = find_pc_compunit_symtab (pc);
> +
> +  if (cust != nullptr && cust->producer () != nullptr
> +      && producer_is_llvm (cust->producer ()))
> +    return amd64_stack_frame_destroyed_p (gdbarch, pc);
> +
> +  return 0;
> +}
> +
>  static int
>  amd64_epilogue_frame_sniffer_1 (const struct frame_unwind *self,
>  				frame_info_ptr this_frame,
> @@ -2938,7 +2950,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);

Hi Gwen!

I can see you took this change from the patch I posted, so it's not
really your fault .... but this isn't really correct.

Previously amd64_epilogue_frame_sniffer_1 was calling
amd64_stack_frame_destroyed_p, which unconditionally did the instruction
check.  Now, we only do the instruction check for llvm.  I don't think
this is a good change.

Here's what I think I meant, but got wrong...

  1. The existing amd64_stack_frame_destroyed_p should be renamed to
     amd64_stack_frame_destroyed_p_1, this is the "inner" helper
     function,

  2. amd64_epilogue_frame_sniffer_1 should indeed be updated to call
     amd64_stack_frame_destroyed_p_1, but this is now the unconditional
     instruction check, thus amd64_epilogue_frame_sniffer_1 is unchanged
     (in functionality),

  3. The new function being added, which is
     amd64_stack_frame_destroyed_p_1 in your (and my original) patch,
     should actually be called amd64_stack_frame_destroyed_p,

  4. And the set_gdbarch_stack_frame_destroyed_p call below should be
     registering amd64_stack_frame_destroyed_p, not the _1 version.

Also, the new static function (amd64_stack_frame_destroyed_p in the new
plan) will need a comment.  I'm biased, but I can recommend the comment
from my original patch, but whatever you feel is appropriate.

Thanks,
Andrew



>  }
>  
>  static int
> @@ -3310,6 +3322,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_1);
> +
>    /* 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"
> -- 
> 2.41.0