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

Pedro Alves palves@redhat.com
Thu Aug 22 10:04:00 GMT 2013


On 08/22/2013 02:24 AM, Yao Qi wrote:
> On 08/21/2013 11:32 PM, Pedro Alves wrote:
>> 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
> 
> Is it in function default_xfer_partial?

Yeah, it's used there, and it seems the actual errno value
is just ignored nowadays.  Only zero vs non-zero is checked.

> 
>> indication, but I didn't look exhaustively.  We should really get rid
>> of that...)
>>
> 
> What do you mean by "that"? using errno for error indication?

I meant deprecated_xfer_memory, which does:

       0 means that we can't handle this.  If errno has been set, it is the
       error which prevented us from doing it (FIXME: What about bfd_error?).

and using errno values as error indication in target.h methods.  E.g.,
target_read_memory:

/* Read LEN bytes of target memory at address MEMADDR, placing the results in
   GDB's memory at MYADDR.  Returns either 0 for success or an errno value
   if any error occurs.

using errno values limits what we can return here.  There's no
errno for <unavailable>, for example.  So, it'd be better if
we returned target_xfer_error instead.  If we needed to indicate
an errno or a bfd error in addition, new TARGET_XFER_E_ERRNO or
TARGET_XFER_E_BFD values could be added to target_xfer_error
(and the caller would then have to consult errno or bfd_error
in addition then).

> 
>> +/* 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.  */
> 
> This line of comment doesn't make much sense in the context of this
> function.

I think it makes some sense if you consider the context that the caller
always has a length handy.  But yeah.  This is a preexisting issue
though (see memory_error, this new function can be looked at as
a refactor).  Maybe what we should do is actually pass in the
length, and show the range to the user instead of just the first
address.  Sometimes that info is useful, as it may not be the first
address in the requested range that actually is inaccessible.

> 
> This patch looks good to me.  Please commit it without the change to
> gdb.trace/entry-values.exp, and I'll update my patch to remove kfail.

OK, done, as below.

> A suggestion here, IWBN to print the string of 'enum target_xfer_error'
> in the debugging message at the end of target_xfer_partial.  We are
> print numbers currently.

Done.  We now get:

internal-error: unhandled target_xfer_error: <unknown> (-3)
internal-error: unhandled target_xfer_error: TARGET_XFER_E_UNAVAILABLE (-2)
internal-error: unhandled target_xfer_error: TARGET_XFER_E_IO (-1)
internal-error: unhandled target_xfer_error: <unknown> (0)
internal-error: unhandled target_xfer_error: <unknown> (1)

-------
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 deprecated_xfer_memory and of passing down errno values as error
indication in target_read & friends methods).

Tested on x86_64 Fedora 17, native and gdbserver.  Fixes the test in
the PR, which will be added to the testsuite later.

gdb/
2013-08-22  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 (target_xfer_error_to_string): New function.
	(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 enum.
	(target_xfer_error_to_string): Declare function.
	(target_xfer_partial): Declare function.
	(struct target_ops) <xfer_partial>: Adjust describing comment.
---
 gdb/corefile.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
 gdb/target.c   | 25 +++++++++++++++++--------
 gdb/target.h   | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index a86f4b3..cb7f14e 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -193,17 +193,39 @@ 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_xfer_error: %s (%s)",
+		      target_xfer_error_to_string (err),
+		      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 +238,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..f18661b 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);
 
@@ -1238,6 +1232,21 @@ target_translate_tls_address (struct objfile *objfile, CORE_ADDR offset)
   return addr;
 }
 
+const char *
+target_xfer_error_to_string (enum target_xfer_error err)
+{
+#define CASE(X) case X: return #X
+  switch (err)
+    {
+      CASE(TARGET_XFER_E_IO);
+      CASE(TARGET_XFER_E_UNAVAILABLE);
+    default:
+      return "<unknown>";
+    }
+#undef CASE
+};
+
+
 #undef	MIN
 #define MIN(A, B) (((A) <= (B)) ? (A) : (B))
 
@@ -1523,7 +1532,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 +1709,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..6959503 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -199,6 +199,26 @@ 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 that 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 = -2,
+
+  /* Keep list in sync with target_xfer_error_to_string.  */
+};
+
+/* Return the string form of ERR.  */
+
+extern const char *target_xfer_error_to_string (enum target_xfer_error err);
+
 /* Enumeration of the kinds of traceframe searches that a target may
    be able to perform.  */
 
@@ -293,6 +313,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,7 +503,8 @@ 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
+       further transfer is possible, and a negative error code (really
+       an 'enum target_xfer_error' value) 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
-- 
1.7.11.7



More information about the Gdb-patches mailing list