[v2] gdb: register frame_destroyed function for amd64 gdbarch

  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(-)
  

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