[PATCH] gdb/testsuite: Work around clang fails in gdb.base/watchpoint.exp

[PATCH] gdb/testsuite: Work around clang fails in gdb.base/watchpoint.exp

[Guinevere Larsen]

as mentioned in commit def86538a46f7ce6fbb215cfb184e23015b5d538, clang
doesn't use the CFA information for variable locations, making it so
software watchpoints get false hits when exiting a function. Differently
to that commit, however, gdb.base/watchpoint.exp also needs to be
explicitly continued so the inferior will remain in sync with what the
exp file expects, so this commit uses gdb_test_multiple to identify that
situation.

I also chose to keep the test passing in that scenario because the GDB
feature being tested, software watchpoints, is working as expected.
---
 gdb/testsuite/gdb.base/watchpoint.exp | 82 +++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 70864655c6d..5d8d6b9cb31 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -30,6 +30,11 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb
      return -1
 }
 
+set using_clang 0
+if {[test_compiler_info "clang-*"]} {
+    set using_clang 1
+}
+
 # True if we're forcing no hardware watchpoints.
 set no_hw 0
 
@@ -486,6 +491,20 @@ proc test_complex_watchpoint {} {
 		}
 		fail $test
 	    }
+	    -re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
+		global no_hw
+		global using_clang
+		# Clang doesn't use the CFA, so software watchpoints get one
+		# false hit here.  Detect if we're in that situation and
+		# ignore the false hit.  For more info, see:
+		# https://github.com/llvm/llvm-project/issues/64390
+		if {$using_clang == 1 && $no_hw == 1} {
+		    send_gdb "cont\n"
+		    exp_continue
+		} else {
+		    fail $gdb_test_name
+		}
+	    }
 	}
 
 	gdb_continue_to_breakpoint "func2 breakpoint here, second time"
@@ -501,8 +520,25 @@ proc test_complex_watchpoint {} {
                  "trigger1 partially local watch"
         gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: local_a . ival5.*" \
                  "trigger2 partially local watch"
-        gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
-                 "self-delete partially local watch"
+        gdb_test_multiple "cont" "self-delete partially local watch" {
+	    -re -wrap "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" {
+		pass $gdb_test_name
+	    }
+	    -re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
+		global no_hw
+		global using_clang
+		# Clang doesn't use the CFA, so software watchpoints get one
+		# false hit here.  Detect if we're in that situation and
+		# ignore the false hit.  For more info, see:
+		# https://github.com/llvm/llvm-project/issues/64390
+		if {$using_clang == 1 && $no_hw == 1} {
+		    send_gdb "cont\n"
+		    exp_continue
+		} else {
+		    fail $gdb_test_name
+		}
+	    }
+	}
 
         # We should be in "func2" again now.  Test a watch of a
         # static (non-stack-based) local.  Since this has scope
@@ -535,8 +571,25 @@ proc test_complex_watchpoint {} {
 		"set local watch in recursive call"
 	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: local_x.*New value = 2.*" \
 		"trigger local watch in recursive call"
-	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
-		"self-delete local watch in recursive call"
+	    gdb_test_multiple "cont" "self-delete local watch in recursive call" {
+		-re -wrap "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" {
+		    pass $gdb_test_name
+		}
+		-re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
+		    global no_hw
+		    global using_clang
+		    # Clang doesn't use the CFA, so software watchpoints get one
+		    # false hit here.  Detect if we're in that situation and
+		    # ignore the false hit.  For more info, see:
+		    # https://github.com/llvm/llvm-project/issues/64390
+		    if {$using_clang == 1 && $no_hw == 1} {
+			send_gdb "cont\n"
+			exp_continue
+		    } else {
+			fail $gdb_test_name
+		    }
+		}
+	    }
 	}
 
         # Repeat the preceding test, but this time use "recurser::local_x" as
@@ -551,8 +604,25 @@ proc test_complex_watchpoint {} {
 		"set local watch in recursive call with explicit scope"
 	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: recurser::local_x.*New value = 2.*" \
 		"trigger local watch with explicit scope in recursive call"
-	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
-		"self-delete local watch with explicit scope in recursive call (2)"
+	    gdb_test_multiple "cont" "self-delete local watch with explicit scope in recursive call (2)" {
+		-re -wrap "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" {
+		    pass $gdb_test_name
+		}
+		-re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
+		    global no_hw
+		    global using_clang
+		    # Clang doesn't use the CFA, so software watchpoints get one
+		    # false hit here.  Detect if we're in that situation and
+		    # ignore the false hit.  For more info, see:
+		    # https://github.com/llvm/llvm-project/issues/64390
+		    if {$using_clang == 1 && $no_hw == 1} {
+			send_gdb "cont\n"
+			exp_continue
+		    } else {
+			fail $gdb_test_name
+		    }
+		}
+	    }
 	}
 
 	# Disable everything so we can finish the program at full speed
-- 
2.41.0

Re: [PATCH] gdb/testsuite: Work around clang fails in gdb.base/watchpoint.exp

[Andrew Burgess]

Guinevere Larsen <blarsen@redhat.com> writes:

> as mentioned in commit def86538a46f7ce6fbb215cfb184e23015b5d538, clang
> doesn't use the CFA information for variable locations, making it so
> software watchpoints get false hits when exiting a function. Differently
> to that commit, however, gdb.base/watchpoint.exp also needs to be
> explicitly continued so the inferior will remain in sync with what the
> exp file expects, so this commit uses gdb_test_multiple to identify that
> situation.
>
> I also chose to keep the test passing in that scenario because the GDB
> feature being tested, software watchpoints, is working as expected.

I'm not sure I 100% agree with this conclusion.  The test is placing on
a watchpoint that isn't expected to change ... yet we see the watchpoint
trigger.  I'd say the test is failing.

I wonder if we could just set a flag when we see an unexpected stop, and
then xfail rather than pass once the watchpoint goes out of scope?

I think the right fix would be to implement the
gdbarch_stack_frame_destroyed_p method for amd64.  Looking in
amd64-tdep.c I noticed we actually already have
amd64_stack_frame_destroyed_p, we just don't register it with the
gdbarch ... I guess there's some history here.

Anyway, the patch below (untested except for watchpoint.exp) seems to
remove the need for any changes to watchpoint.exp, what are your
thoughts?


> ---
>  gdb/testsuite/gdb.base/watchpoint.exp | 82 +++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
> index 70864655c6d..5d8d6b9cb31 100644
> --- a/gdb/testsuite/gdb.base/watchpoint.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint.exp
> @@ -30,6 +30,11 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {deb
>       return -1
>  }
>  
> +set using_clang 0
> +if {[test_compiler_info "clang-*"]} {
> +    set using_clang 1
> +}

You can actually use 'true' and 'false' here, given this is a boolean.

> +
>  # True if we're forcing no hardware watchpoints.
>  set no_hw 0
>  
> @@ -486,6 +491,20 @@ proc test_complex_watchpoint {} {
>  		}
>  		fail $test
>  	    }
> +	    -re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
> +		global no_hw
> +		global using_clang
> +		# Clang doesn't use the CFA, so software watchpoints get one
> +		# false hit here.  Detect if we're in that situation and
> +		# ignore the false hit.  For more info, see:
> +		# https://github.com/llvm/llvm-project/issues/64390
> +		if {$using_clang == 1 && $no_hw == 1} {

And here (and elsewhere) you can write:

  if {$using_clang && $no_hw} {

which better reflects the boolean nature of these flags.

> +		    send_gdb "cont\n"
> +		    exp_continue
> +		} else {
> +		    fail $gdb_test_name
> +		}
> +	    }
>  	}
>  
>  	gdb_continue_to_breakpoint "func2 breakpoint here, second time"
> @@ -501,8 +520,25 @@ proc test_complex_watchpoint {} {
>                   "trigger1 partially local watch"
>          gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: local_a . ival5.*" \
>                   "trigger2 partially local watch"
> -        gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
> -                 "self-delete partially local watch"
> +        gdb_test_multiple "cont" "self-delete partially local watch" {
> +	    -re -wrap "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" {
> +		pass $gdb_test_name
> +	    }
> +	    -re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
> +		global no_hw
> +		global using_clang
> +		# Clang doesn't use the CFA, so software watchpoints get one
> +		# false hit here.  Detect if we're in that situation and
> +		# ignore the false hit.  For more info, see:
> +		# https://github.com/llvm/llvm-project/issues/64390
> +		if {$using_clang == 1 && $no_hw == 1} {
> +		    send_gdb "cont\n"
> +		    exp_continue
> +		} else {
> +		    fail $gdb_test_name
> +		}
> +	    }
> +	}
>  
>          # We should be in "func2" again now.  Test a watch of a
>          # static (non-stack-based) local.  Since this has scope
> @@ -535,8 +571,25 @@ proc test_complex_watchpoint {} {
>  		"set local watch in recursive call"
>  	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: local_x.*New value = 2.*" \
>  		"trigger local watch in recursive call"
> -	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
> -		"self-delete local watch in recursive call"
> +	    gdb_test_multiple "cont" "self-delete local watch in recursive call" {
> +		-re -wrap "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" {
> +		    pass $gdb_test_name
> +		}
> +		-re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
> +		    global no_hw
> +		    global using_clang
> +		    # Clang doesn't use the CFA, so software watchpoints get one
> +		    # false hit here.  Detect if we're in that situation and
> +		    # ignore the false hit.  For more info, see:
> +		    # https://github.com/llvm/llvm-project/issues/64390
> +		    if {$using_clang == 1 && $no_hw == 1} {
> +			send_gdb "cont\n"
> +			exp_continue
> +		    } else {
> +			fail $gdb_test_name
> +		    }
> +		}
> +	    }
>  	}
>  
>          # Repeat the preceding test, but this time use "recurser::local_x" as
> @@ -551,8 +604,25 @@ proc test_complex_watchpoint {} {
>  		"set local watch in recursive call with explicit scope"
>  	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .*: recurser::local_x.*New value = 2.*" \
>  		"trigger local watch with explicit scope in recursive call"
> -	    gdb_test "cont" "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" \
> -		"self-delete local watch with explicit scope in recursive call (2)"
> +	    gdb_test_multiple "cont" "self-delete local watch with explicit scope in recursive call (2)" {
> +		-re -wrap "Continuing.*\[Ww\]atchpoint .* deleted because the program has left the block in.*which its expression is valid.*" {
> +		    pass $gdb_test_name
> +		}
> +		-re -wrap ".*Continuing.*\[Ww\]atchpoint.*" {
> +		    global no_hw
> +		    global using_clang
> +		    # Clang doesn't use the CFA, so software watchpoints get one
> +		    # false hit here.  Detect if we're in that situation and
> +		    # ignore the false hit.  For more info, see:
> +		    # https://github.com/llvm/llvm-project/issues/64390
> +		    if {$using_clang == 1 && $no_hw == 1} {
> +			send_gdb "cont\n"
> +			exp_continue
> +		    } else {
> +			fail $gdb_test_name
> +		    }
> +		}
> +	    }
>  	}
>  
>  	# Disable everything so we can finish the program at full speed
> -- 
> 2.41.0

Thanks,
Andrew

---

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index e6feee677b3..15120d55976 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2888,7 +2888,7 @@ static const struct frame_base amd64_frame_base =
 
 /* Normal frames, but in a function epilogue.  */
 
-/* Implement the stack_frame_destroyed_p gdbarch method.
+/* Implement core of the stack_frame_destroyed_p gdbarch method.
 
    The epilogue is defined here as the 'ret' instruction, which will
    follow any instruction such as 'leave' or 'pop %ebp' that destroys
@@ -2908,6 +2908,33 @@ amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
   return 1;
 }
 
+/* Implement the gdbarch_stack_frame_destroyed_p method.
+
+   This wrapper delegates to amd64_stack_frame_destroyed_p_1 for compilers
+   that we know get the debug information for stack local variables wrong
+   (i.e. Clang) during the function epilogue.
+
+   For other compilers (i.e. not Clang) we trust the debug information.  */
+
+static int
+amd64_stack_frame_destroyed_p_1 (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  struct compunit_symtab *cust = find_pc_compunit_symtab (pc);
+
+  /* LLVM backend (Clang/Flang) doesn't use CFA based locations for stack
+     local variables.  As a consequence, when we enter the function
+     epilogue GDB will calculate the wrong location for stack local
+     variables, and watchpoints will trigger.  */
+  if (cust != nullptr
+      && cust->producer () != nullptr
+      && producer_is_llvm (cust->producer ()))
+    return amd64_stack_frame_destroyed_p_1 (gdbarch, pc);
+
+  /* For other producers, trust the debug information.  */
+  return 0;
+}
+
+
 static int
 amd64_epilogue_frame_sniffer_1 (const struct frame_unwind *self,
 				frame_info_ptr this_frame,
@@ -2938,7 +2965,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 +3337,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);

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

[Guinevere Larsen]

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);
 }
 
 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

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

[Andrew Burgess]

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

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

[Guinevere Larsen]

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"
-- 
2.41.0

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

[Guinevere Larsen]

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"

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

[Guinevere Larsen]

Ping!
On 07/12/2023 18:36, Guinevere Larsen wrote:
> 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" 


-- 
Cheers,
Guinevere Larsen
She/Her/Hers

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

[Andrew Burgess]

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. 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

Missing a '.' on the last sentence of your commit message :)

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks for fixing this.

Andrew


>
> 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"
> -- 
> 2.41.0