[PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

[Pedro Alves <palves at redhat dot com>]

On 08/13/2013 08:39 AM, Yao Qi wrote:
> However, when I run this test, I find j@entry is something like,
> j@entry=<error reading variable: Cannot access memory at address 0x8049788>
> instead of unavailable.
> 
> I don't emit a fail for it because I am not very sure it is expected
> to be "unavailable".  I am fine to kfail it.
> 
> I looked into a little, and looks reading entry value doesn't use
> value availability-aware API.  It is not an easy fix to me.

I looked into this.  Here's a patch.  Let me know what you think.

--------
Subject: [PATCH] PR gdb/15871: Unavailable entry value is not shown correctly

In entry-values.exp, we have a test where the entry value of 'j' is
unavailable, so it is expected that printing j@entry yields
"<unavailable>".  However, the actual output is:

 (gdb) frame
 #0  0x0000000000400540 in foo (i=0, i@entry=2, j=2, j@entry=<error reading variable: Cannot access memory at address 0x6009e8>)

The error is thrown here:

#0  throw_it (reason=RETURN_ERROR, error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s", ap=0x7fffffffc8e8) at ../../src/gdb/exceptions.c:373
#1  0x00000000005e2f9c in throw_error (error=MEMORY_ERROR, fmt=0x8cd550 "Cannot access memory at address %s") at ../../src/gdb/exceptions.c:422
#2  0x0000000000673a5f in memory_error (status=5, memaddr=6293992) at ../../src/gdb/corefile.c:204
#3  0x0000000000673aea in read_memory (memaddr=6293992, myaddr=0x7fffffffca60 "\200\316\377\377\377\177", len=4) at ../../src/gdb/corefile.c:223
#4  0x00000000006784d1 in dwarf_expr_read_mem (baton=0x7fffffffcd50, buf=0x7fffffffca60 "\200\316\377\377\377\177", addr=6293992, len=4) at ../../src/gdb/dwarf2loc.c:334
#5  0x000000000067645e in execute_stack_op (ctx=0x1409480, op_ptr=0x7fffffffce87 "\237<\005@", op_end=0x7fffffffce88 "<\005@") at ../../src/gdb/dwarf2expr.c:1045
#6  0x0000000000674e29 in dwarf_expr_eval (ctx=0x1409480, addr=0x7fffffffce80 "\003\350\t`", len=8) at ../../src/gdb/dwarf2expr.c:364
#7  0x000000000067c5b2 in dwarf2_evaluate_loc_desc_full (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40, byte_offset=0)
    at ../../src/gdb/dwarf2loc.c:2236
#8  0x000000000067cc65 in dwarf2_evaluate_loc_desc (type=0x10876d0, frame=0xd8ecc0, data=0x7fffffffce80 "\003\350\t`", size=8, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:2407
#9  0x000000000067a5d4 in dwarf_entry_parameter_to_value (parameter=0x13a7960, deref_size=18446744073709551615, type=0x10876d0, caller_frame=0xd8ecc0, per_cu=0xf24c40)
    at ../../src/gdb/dwarf2loc.c:1160
#10 0x000000000067a962 in value_of_dwarf_reg_entry (type=0x10876d0, frame=0xd8de70, kind=CALL_SITE_PARAMETER_DWARF_REG, kind_u=...) at ../../src/gdb/dwarf2loc.c:1310
#11 0x000000000067aaca in value_of_dwarf_block_entry (type=0x10876d0, frame=0xd8de70, block=0xf1c2d4 "Q", block_len=1) at ../../src/gdb/dwarf2loc.c:1363
#12 0x000000000067e7c9 in locexpr_read_variable_at_entry (symbol=0x13a7540, frame=0xd8de70) at ../../src/gdb/dwarf2loc.c:3326
#13 0x00000000005daab6 in read_frame_arg (sym=0x13a7540, frame=0xd8de70, argp=0x7fffffffd0e0, entryargp=0x7fffffffd100) at ../../src/gdb/stack.c:362
#14 0x00000000005db384 in print_frame_args (func=0x13a7470, frame=0xd8de70, num=-1, stream=0xea3890) at ../../src/gdb/stack.c:669
#15 0x00000000005dc338 in print_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at ../../src/gdb/stack.c:1199
#16 0x00000000005db8ee in print_frame_info (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC, print_args=1) at ../../src/gdb/stack.c:851
#17 0x00000000005da2bb in print_stack_frame (frame=0xd8de70, print_level=1, print_what=SRC_AND_LOC) at ../../src/gdb/stack.c:169
#18 0x00000000005de236 in frame_command (level_exp=0x0, from_tty=1) at ../../src/gdb/stack.c:2265

dwarf2_evaluate_loc_desc_full (frame #7) knows to handle
NOT_AVAILABLE_ERROR errors, but read_memory always throws
a generic error.

Presently, only the value machinery knows to handle unavailable
memory.  We need to push the awareness down to the target_xfer layer,
making it return a finer grained error indication.  We can only return
a generic -1 nowadays, which leaves the upper layers with no clue on
why the xfer failed.  Use target_xfer_partial directly, rather than
propagating the error through target_read_memory so as to get a better
address to display in the error message.

(target_read_memory & friends build on top of target_read (thus the
target_xfer machinery), but turn all errors to EIO, an errno value.  I
think this is a mistake, and we'd better convert all these to return a
target_xfer_error too, but that can be done separately.  I looked
around a bit over memory_error calls, and the need to handle random
errno values, other than the EIOs gdb itself hardcodes, probably comes
(only) from deprecated_xfer_memory, which uses errno for error
indication, but I didn't look exhaustively.  We should really get rid
of that...)

Tested on x86_64 Fedora 17, native and gdbserver.

gdb/
2013-08-21  Pedro Alves  <palves@redhat.com>

	PR gdb/15871
	* corefile.c (target_xfer_memory_error): New function.
	(memory_error): Defer EIO to target_memory_error.
	(read_memory): Use target_xfer_partial, and handle finer-grained
	target xfer errors.
	* target.c (memory_xfer_partial_1): If memory is known to be
	unavailable, return -TARGET_XFER_E_UNAVAILABLE instead of -1.
	(target_xfer_partial): Make extern.
	* target.h (enum target_xfer_error): New.
	(target_xfer_partial): Declare.
	(struct target_ops) <xfer_partial>: Adjust describing comment.

gdb/testsuite/
2013-08-21  Pedro Alves  <palves@redhat.com>

	PR gdb/15871
	* gdb.trace/entry-values.exp: Remove kfail.
---
 gdb/corefile.c                           | 50 ++++++++++++++++++++++++++------
 gdb/target.c                             | 10 ++-----
 gdb/target.h                             | 33 +++++++++++++++++----
 gdb/testsuite/gdb.trace/entry-values.exp |  1 -
 4 files changed, 71 insertions(+), 23 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a86f4b3..23dfcb8 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -193,17 +193,38 @@ Use the \"file\" or \"exec-file\" command."));
 }
 
 
+/* Report a target xfer memory error by throwing a suitable
+   exception.  */
+
+static void
+target_xfer_memory_error (enum target_xfer_error err, CORE_ADDR memaddr)
+{
+  switch (err)
+    {
+    case TARGET_XFER_E_IO:
+      /* Actually, address between memaddr and memaddr + len was out of
+	 bounds.  */
+      throw_error (MEMORY_ERROR,
+		   _("Cannot access memory at address %s"),
+		   paddress (target_gdbarch (), memaddr));
+    case TARGET_XFER_E_UNAVAILABLE:
+      throw_error (NOT_AVAILABLE_ERROR,
+		   _("Memory at address %s unavailable."),
+		   paddress (target_gdbarch (), memaddr));
+    default:
+      internal_error (__FILE__, __LINE__,
+		      "unhandled target memory error: %s",
+		      plongest (err));
+    }
+}
+
 /* Report a memory error by throwing a MEMORY_ERROR error.  */
 
 void
 memory_error (int status, CORE_ADDR memaddr)
 {
   if (status == EIO)
-    /* Actually, address between memaddr and memaddr + len was out of
-       bounds.  */
-    throw_error (MEMORY_ERROR,
-		 _("Cannot access memory at address %s"),
-		 paddress (target_gdbarch (), memaddr));
+    target_xfer_memory_error (TARGET_XFER_E_IO, memaddr);
   else
     throw_error (MEMORY_ERROR,
 		 _("Error accessing memory address %s: %s."),
@@ -216,11 +237,22 @@ memory_error (int status, CORE_ADDR memaddr)
 void
 read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
-  int status;
+  LONGEST xfered = 0;
 
-  status = target_read_memory (memaddr, myaddr, len);
-  if (status != 0)
-    memory_error (status, memaddr);
+  while (xfered < len)
+    {
+      LONGEST xfer = target_xfer_partial (current_target.beneath,
+					  TARGET_OBJECT_MEMORY, NULL,
+					  myaddr + xfered, NULL,
+					  memaddr + xfered, len - xfered);
+
+      if (xfer == 0)
+	target_xfer_memory_error (TARGET_XFER_E_IO, memaddr + xfered);
+      if (xfer < 0)
+	target_xfer_memory_error (-xfer, memaddr + xfered);
+      xfered += xfer;
+      QUIT;
+    }
 }
 
 /* Same as target_read_stack, but report an error if can't read.  */
diff --git a/gdb/target.c b/gdb/target.c
index 377724d..97c8ce3 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -81,12 +81,6 @@ static LONGEST current_xfer_partial (struct target_ops *ops,
 				     const gdb_byte *writebuf,
 				     ULONGEST offset, LONGEST len);
 
-static LONGEST target_xfer_partial (struct target_ops *ops,
-				    enum target_object object,
-				    const char *annex,
-				    void *readbuf, const void *writebuf,
-				    ULONGEST offset, LONGEST len);
-
 static struct gdbarch *default_thread_architecture (struct target_ops *ops,
 						    ptid_t ptid);
 
@@ -1523,7 +1517,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 
 	      /* No use trying further, we know some memory starting
 		 at MEMADDR isn't available.  */
-	      return -1;
+	      return -TARGET_XFER_E_UNAVAILABLE;
 	    }
 
 	  /* Don't try to read more than how much is available, in
@@ -1700,7 +1694,7 @@ make_show_memory_breakpoints_cleanup (int show)
 
 /* For docs see target.h, to_xfer_partial.  */
 
-static LONGEST
+LONGEST
 target_xfer_partial (struct target_ops *ops,
 		     enum target_object object, const char *annex,
 		     void *readbuf, const void *writebuf,
diff --git a/gdb/target.h b/gdb/target.h
index d538e02..a3a4cfd 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -199,6 +199,20 @@ enum target_object
   /* Possible future objects: TARGET_OBJECT_FILE, ...  */
 };
 
+/* Possible error codes returned by target_xfer_partial, etc.  */
+
+enum target_xfer_error
+{
+  /* Generic I/O error.  Note that it's important this is '1', as we
+     still have target_xfer-related code returning hardcoded "-1" on
+     error.  */
+  TARGET_XFER_E_IO = 1,
+
+  /* Transfer failed because the piece of the object requested is
+     unavailable.  */
+  TARGET_XFER_E_UNAVAILABLE
+};
+
 /* Enumeration of the kinds of traceframe searches that a target may
    be able to perform.  */
 
@@ -293,6 +307,14 @@ extern char *target_read_stralloc (struct target_ops *ops,
 				   enum target_object object,
 				   const char *annex);
 
+/* See target_ops->to_xfer_partial.  */
+
+extern LONGEST target_xfer_partial (struct target_ops *ops,
+				    enum target_object object,
+				    const char *annex,
+				    void *readbuf, const void *writebuf,
+				    ULONGEST offset, LONGEST len);
+
 /* Wrappers to target read/write that perform memory transfers.  They
    throw an error if the memory transfer fails.
 
@@ -475,11 +497,12 @@ struct target_ops
        data-specific information to the target.
 
        Return the number of bytes actually transfered, zero when no
-       further transfer is possible, and -1 when the transfer is not
-       supported.  Return of a positive value smaller than LEN does
-       not indicate the end of the object, only the end of the
-       transfer; higher level code should continue transferring if
-       desired.  This is handled in target.c.
+       further transfer is possible, and a negative error code
+       (-TARGET_XFER_ERROR) when the transfer is not supported.
+       Return of a positive value smaller than LEN does not indicate
+       the end of the object, only the end of the transfer; higher
+       level code should continue transferring if desired.  This is
+       handled in target.c.
 
        The interface does not support a "retry" mechanism.  Instead it
        assumes that at least one byte will be transfered on each
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
index bfa4f5c..20d06ce 100644
--- a/gdb/testsuite/gdb.trace/entry-values.exp
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -265,7 +265,6 @@ gdb_test_no_output "tstop"
 gdb_test "tfind" "Found trace frame 0, .*" "tfind start"
 
 # Since 'global2' is not collected, j@entry is expected to be 'unavailable'.
-setup_kfail "gdb/15871" *-*-*
 gdb_test "bt 1" "#0 .* foo \\(i=\[-\]?$decimal, i@entry=2, j=\[-\]?$decimal, j@entry=<unavailable>\\).*"
 
 gdb_test "tfind" "Target failed to find requested trace frame\..*"
-- 
1.7.11.7