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

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

Commit Message

Andrew Burgess Jan. 18, 2023, 4:18 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 is seems like the safest
approach.

I've then update 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 | 16 +++++++---------
 gdbserver/ax.cc                            | 12 ++++++++----
 gdbserver/ax.h                             |  3 ++-
 3 files changed, 17 insertions(+), 14 deletions(-)
  

Comments

Terekhov, Mikhail via Gdb-patches Jan. 19, 2023, 10:13 a.m. UTC | #1
On Wednesday, January 18, 2023 5:18 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 is seems like the safest

is -> it

> approach.
> 
> I've then update gdb.base/bp-cond-failure.exp to test evaluating the

update -> updated

> 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 | 16 +++++++---------
>  gdbserver/ax.cc                            | 12 ++++++++----
>  gdbserver/ax.h                             |  3 ++-
>  3 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.exp b/gdb/testsuite/gdb.base/bp-cond-
> failure.exp
> index 6f89771d187..aa39b638592 100644
> --- a/gdb/testsuite/gdb.base/bp-cond-failure.exp
> +++ b/gdb/testsuite/gdb.base/bp-cond-failure.exp
> @@ -39,7 +39,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]} {
> @@ -52,7 +52,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" \
> @@ -68,16 +68,14 @@ proc run_test { cond_eval } {
>  # If we're using a remote target then conditions could be evaulated
>  # locally on the host, or on the remote target.  Otherwise, conditions
>  # are always evaluated locally (which is what auto will select).
> -#
> -# NOTE: 'target' is not included here for remote targets as a
> -# gdbserver bug prevents the test from passing.  This will be fixed in
> -# the next commit, and this test updated.
>  if { [gdb_is_remote_or_extended_remote_target] } {
> -    set cond_eval_modes { "host" }
> +    set cond_eval_modes { "host" "target" }

If this test were executed against a gdbserver build that does not support
condition evaluation, would we get failures?  To that regard, would it be safer
to define the modes as { "host" "auto" }?

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
  

Patch

diff --git a/gdb/testsuite/gdb.base/bp-cond-failure.exp b/gdb/testsuite/gdb.base/bp-cond-failure.exp
index 6f89771d187..aa39b638592 100644
--- a/gdb/testsuite/gdb.base/bp-cond-failure.exp
+++ b/gdb/testsuite/gdb.base/bp-cond-failure.exp
@@ -39,7 +39,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]} {
@@ -52,7 +52,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" \
@@ -68,16 +68,14 @@  proc run_test { cond_eval } {
 # If we're using a remote target then conditions could be evaulated
 # locally on the host, or on the remote target.  Otherwise, conditions
 # are always evaluated locally (which is what auto will select).
-#
-# NOTE: 'target' is not included here for remote targets as a
-# gdbserver bug prevents the test from passing.  This will be fixed in
-# the next commit, and this test updated.
 if { [gdb_is_remote_or_extended_remote_target] } {
-    set cond_eval_modes { "host" }
+    set cond_eval_modes { "host" "target" }
 } else {
     set cond_eval_modes { "auto" }
 }
 
-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