diff mbox series

[PATCHv3,05/13] gdbserver: allow agent expressions to fail with invalid memory access

Message ID e2527787cc78d90e121ca7dcaba236934a0229b3.1675185990.git.aburgess@redhat.com
State New
Headers
Series Infcalls from B/P conditions in multi-threaded inferiors |

Commit Message

Andrew Burgess Jan. 31, 2023, 5:27 p.m. UTC 
  This commit extends gdbserver to take account of a failed memory
access from agent_mem_read, and to return a new eval_result_type
expr_eval_invalid_memory_access.

I have only updated the agent_mem_read calls related directly to
reading memory, I have not updated any of the calls related to
tracepoint data collection.  This is just because I'm not familiar
with that area of gdb/gdbserver, and I don't want to break anything,
so leaving the existing behaviour as it is seems like the safest
approach.

I've then updated gdb.base/bp-cond-failure.exp to test evaluating the
breakpoints on the target, and have also extended the test so that it
checks for different sizes of memory access.
---
 gdb/testsuite/gdb.base/bp-cond-failure.exp | 21 +++++++--------------
 gdbserver/ax.cc                            | 12 ++++++++----
 gdbserver/ax.h                             |  3 ++-
 3 files changed, 17 insertions(+), 19 deletions(-)

Comments

Terekhov, Mikhail via Gdb-patches Feb. 16, 2023, 10:29 a.m. UTC | #1 
On Tuesday, January 31, 2023 6:27 PM, Andrew Burgess wrote:
> This commit extends gdbserver to take account of a failed memory
> access from agent_mem_read, and to return a new eval_result_type
> expr_eval_invalid_memory_access.
> 
> I have only updated the agent_mem_read calls related directly to
> reading memory, I have not updated any of the calls related to
> tracepoint data collection.  This is just because I'm not familiar
> with that area of gdb/gdbserver, and I don't want to break anything,
> so leaving the existing behaviour as it is seems like the safest

"it is" -> "it"

> approach.
> 
> I've then updated gdb.base/bp-cond-failure.exp to test evaluating the
> breakpoints on the target, and have also extended the test so that it
> checks for different sizes of memory access.
> ---
>  gdb/testsuite/gdb.base/bp-cond-failure.exp | 21 +++++++--------------
>  gdbserver/ax.cc                            | 12 ++++++++----
>  gdbserver/ax.h                             |  3 ++-
>  3 files changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.exp b/gdb/testsuite/gdb.base/bp-cond-
> failure.exp
> index 9388b8cf582..d67c68c46a1 100644
> --- a/gdb/testsuite/gdb.base/bp-cond-failure.exp
> +++ b/gdb/testsuite/gdb.base/bp-cond-failure.exp
> @@ -48,7 +48,7 @@ if { [is_address_zero_readable] } {
>  # Where the breakpoint will be placed.
>  set bp_line [gdb_get_line_number "Breakpoint here"]
> 
> -proc run_test { cond_eval } {
> +proc run_test { cond_eval access_type } {
>      clean_restart ${::binfile}
> 
>      if { ![runto_main] } {
> @@ -61,7 +61,7 @@ proc run_test { cond_eval } {
>      }
> 
>      # Setup the conditional breakpoint and record its number.
> -    gdb_breakpoint "${::srcfile}:${::bp_line} if (*(int *) 0) == 0"
> +    gdb_breakpoint "${::srcfile}:${::bp_line} if (*(${access_type} *) 0) == 0"
>      set bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*"]
> 
>      gdb_test "continue" \
> @@ -90,16 +90,7 @@ set cond_eval_modes { "auto" }
> 
>  gdb_test_multiple "show breakpoint condition-evaluation" "" {
>      -re -wrap "Breakpoint condition evaluation mode is auto \\(currently target\\)\\." {
> -
> -	## NOTE: Instead of testing with "auto" and "host" in this
> -	## case we only test with "host".  This is because a GDB bug
> -	## prevents the "auto" (a.k.a. target) mode from working.
> -	##
> -	## Don't worry, this will be fixed in a later commit, and this
> -	## comment will be removed at that time.
> -	##
> -	## lappend cond_eval_modes "host"
> -
> +	lappend cond_eval_modes "host"
>  	set cond_eval_modes { "host" }

We do lappend but then set the variable.  Did you mean to remove the second line?

Regards
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
diff mbox series

Patch

diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.exp b/gdb/testsuite/gdb.base/bp-cond-failure.exp
index 9388b8cf582..d67c68c46a1 100644
--- a/gdb/testsuite/gdb.base/bp-cond-failure.exp
+++ b/gdb/testsuite/gdb.base/bp-cond-failure.exp
@@ -48,7 +48,7 @@  if { [is_address_zero_readable] } {
 # Where the breakpoint will be placed.
 set bp_line [gdb_get_line_number "Breakpoint here"]
 
-proc run_test { cond_eval } {
+proc run_test { cond_eval access_type } {
     clean_restart ${::binfile}
 
     if { ![runto_main] } {
@@ -61,7 +61,7 @@  proc run_test { cond_eval } {
     }
 
     # Setup the conditional breakpoint and record its number.
-    gdb_breakpoint "${::srcfile}:${::bp_line} if (*(int *) 0) == 0"
+    gdb_breakpoint "${::srcfile}:${::bp_line} if (*(${access_type} *) 0) == 0"
     set bp_num [get_integer_valueof "\$bpnum" "*UNKNOWN*"]
 
     gdb_test "continue" \
@@ -90,16 +90,7 @@  set cond_eval_modes { "auto" }
 
 gdb_test_multiple "show breakpoint condition-evaluation" "" {
     -re -wrap "Breakpoint condition evaluation mode is auto \\(currently target\\)\\." {
-
-	## NOTE: Instead of testing with "auto" and "host" in this
-	## case we only test with "host".  This is because a GDB bug
-	## prevents the "auto" (a.k.a. target) mode from working.
-	##
-	## Don't worry, this will be fixed in a later commit, and this
-	## comment will be removed at that time.
-	##
-	## lappend cond_eval_modes "host"
-
+	lappend cond_eval_modes "host"
 	set cond_eval_modes { "host" }
 	pass $gdb_test_name
     }
@@ -109,6 +100,8 @@  gdb_test_multiple "show breakpoint condition-evaluation" "" {
     }
 }
 
-foreach_with_prefix cond_eval $cond_eval_modes {
-    run_test $cond_eval
+foreach_with_prefix access_type { "char" "short" "int" "long long" } {
+    foreach_with_prefix cond_eval $cond_eval_modes {
+	run_test $cond_eval $access_type
+    }
 }
diff --git a/gdbserver/ax.cc b/gdbserver/ax.cc
index 38ebfbbd750..fba5b4ad0fc 100644
--- a/gdbserver/ax.cc
+++ b/gdbserver/ax.cc
@@ -1112,22 +1112,26 @@  gdb_eval_agent_expr (struct eval_agent_expr_context *ctx,
 	  break;
 
 	case gdb_agent_op_ref8:
-	  agent_mem_read (ctx, cnv.u8.bytes, (CORE_ADDR) top, 1);
+	  if (agent_mem_read (ctx, cnv.u8.bytes, (CORE_ADDR) top, 1) != 0)
+	    return expr_eval_invalid_memory_access;
 	  top = cnv.u8.val;
 	  break;
 
 	case gdb_agent_op_ref16:
-	  agent_mem_read (ctx, cnv.u16.bytes, (CORE_ADDR) top, 2);
+	  if (agent_mem_read (ctx, cnv.u16.bytes, (CORE_ADDR) top, 2) != 0)
+	    return expr_eval_invalid_memory_access;
 	  top = cnv.u16.val;
 	  break;
 
 	case gdb_agent_op_ref32:
-	  agent_mem_read (ctx, cnv.u32.bytes, (CORE_ADDR) top, 4);
+	  if (agent_mem_read (ctx, cnv.u32.bytes, (CORE_ADDR) top, 4) != 0)
+	    return expr_eval_invalid_memory_access;
 	  top = cnv.u32.val;
 	  break;
 
 	case gdb_agent_op_ref64:
-	  agent_mem_read (ctx, cnv.u64.bytes, (CORE_ADDR) top, 8);
+	  if (agent_mem_read (ctx, cnv.u64.bytes, (CORE_ADDR) top, 8) != 0)
+	    return expr_eval_invalid_memory_access;
 	  top = cnv.u64.val;
 	  break;
 
diff --git a/gdbserver/ax.h b/gdbserver/ax.h
index 8e64a7a593e..c98e36a83c6 100644
--- a/gdbserver/ax.h
+++ b/gdbserver/ax.h
@@ -41,7 +41,8 @@  enum eval_result_type
     expr_eval_unhandled_opcode,
     expr_eval_unrecognized_opcode,
     expr_eval_divide_by_zero,
-    expr_eval_invalid_goto
+    expr_eval_invalid_goto,
+    expr_eval_invalid_memory_access
   };
 
 struct agent_expr