Re: [RFA] initialize err variable in load_section_callback()

["Theodore A. Roth" <troth@openavr.org>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3875 bytes --]

On Mon, 25 Oct 2004, Andrew Cagney wrote:

> Theodore A. Roth wrote:
> > On Wed, 20 Oct 2004, Andrew Cagney wrote:
> >
> >
> >>Theodore A. Roth wrote:
> >>
> >>>Hi,
> >>>
> >>>I just encountered a problem with using the "load" command with a remote
> >>>avr target. The first packet would be sent to the remote target and then
> >>>gdb would just give up with this error message:
> >>>
> >>>  (gdb) load
> >>>  Loading section .text, size 0x1f8 lma 0x0
> >>>  Sending packet: $M0,a:0c9446000c9463000c94#d7...Ack
> >>>  Packet received: OK
> >>>  Memory access error while loading section .text.
> >>>
> >>>It looks like load_section_callback() in symfile.c is assuming that a
> >>>call to target_write_memory_partial() will set the err variable.
> >>>Unfortunately, that is not a valid assumption.
> >>>
> >>>The attached patch got things working again, but this feels like a hack
> >>>to me since target_write_memory_partial() should really be setting err
> >>>to a sane value before returning.
> >>>
> >>>Patch is against today's cvs mainline.
> >>
> >>Here's the contract:
> >>/* Make a single attempt at transfering LEN bytes.  On a successful
> >>    transfer, the number of bytes actually transfered is returned and
> >>    ERR is set to 0.  When a transfer fails, -1 is returned (the number
> >>    of bytes actually transfered is not defined) and ERR is set to a
> >>    non-zero error indication.  */
> >>So the bug is further down the target stack.
> >
> >
> > Both target_write_memory_partial() and target_read_memory_partial()
> > break that contract then:
> >
> >   int
> >   target_write_memory_partial (CORE_ADDR memaddr, char *buf, int len, int *err)
> >   {
> >     if (target_xfer_partial_p ())
> >       return target_xfer_partial (target_stack, TARGET_OBJECT_MEMORY, NULL,
> >                                   NULL, buf, memaddr, len);
> >     else
> >       return target_xfer_memory_partial (memaddr, buf, len, 1, err);
> >   }
> >
> > If target_xfer_partial_p() returns true (which the avr port does), then
> > err is never set and the caller will see garbage if it didn't initialize
> > err.
> >
> > Should the return value of the target_xfer_partial() call be checked, or
> > should err just be blindly see to zero?
>
> The result will need to be checked, and *err set accordingly.
>
> Hmm, to_xfer_partial doesn't specify how to handle errors.  We'd better
> pin that down.
>
> Of hand the interface could allow:
>
> - when -1, set *err to errno

Attached patch implements the above case.

> - when -1, set *err to EIO

I dug down the stack to see if there was a guarantee if errno is going
to be set if retval -1. I didn't see that so I'm a bit nervous about my
attached patch. Would it make any sense to set errno to 0 before the
call to target_xfer_partial(), then if retval is -1 also check errno?
I.e. if errno == 0, set *err to EIO, else *err to errno.

> - when -ve, set *err -VE return value

I assume -ve is an error code? Sould I extend my patch to also check for
retval < -1 and if so set *err to retval?

>
> I suspect that it should be the first.  The comments for
> target_read_partial should also be updated to mention this.

You lost me on this one. target_read_partial() with comments currently
reads like this:

  /* Target vector read/write partial wrapper functions.

     NOTE: cagney/2003-10-21: I wonder if having "to_xfer_partial
     (inbuf, outbuf)", instead of separate read/write methods, make life
     easier.  */

  LONGEST
  target_read_partial (struct target_ops *ops,
  		       enum target_object object,
		       const char *annex, void *buf,
		       ULONGEST offset, LONGEST len)
  {
    return target_xfer_partial (ops, object, annex, buf, NULL, offset, len);
  }

Was there some other comment you had in mind?

Thanks for helping me with this.

---
Ted Roth
PGP Key ID: 0x18F846E9
Jabber ID: troth@jabber.org

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1786 bytes --]

2004-10-26  Theodore A. Roth  <troth@openavr.org>

	* target.c (target_read_memory_partial): Make sure the err is set.
	(target_write_memory_partial): Ditto.

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.90
diff -u -p -p -r1.90 target.c
--- target.c	8 Oct 2004 20:29:55 -0000	1.90
+++ target.c	26 Oct 2004 17:35:14 -0000
@@ -1233,8 +1233,20 @@ int
 target_read_memory_partial (CORE_ADDR memaddr, char *buf, int len, int *err)
 {
   if (target_xfer_partial_p ())
-    return target_xfer_partial (target_stack, TARGET_OBJECT_MEMORY, NULL,
-				buf, NULL, memaddr, len);
+    {
+      int retval = target_xfer_partial (target_stack, TARGET_OBJECT_MEMORY,
+                                        NULL, buf, NULL, memaddr, len);
+
+      /* troth/2004-10-26: Are we certain that errno will be set properly if
+         the above call returns -1? */
+
+      if (retval == -1)
+        *err = errno;
+      else
+        *err = 0;
+
+      return retval;
+    }
   else
     return target_xfer_memory_partial (memaddr, buf, len, 0, err);
 }
@@ -1243,8 +1255,17 @@ int
 target_write_memory_partial (CORE_ADDR memaddr, char *buf, int len, int *err)
 {
   if (target_xfer_partial_p ())
-    return target_xfer_partial (target_stack, TARGET_OBJECT_MEMORY, NULL,
-				NULL, buf, memaddr, len);
+    {
+      int retval = target_xfer_partial (target_stack, TARGET_OBJECT_MEMORY,
+                                        NULL, NULL, buf, memaddr, len);
+
+      if (retval == -1)
+        *err = errno;
+      else
+        *err = 0;
+
+      return retval;
+    }
   else
     return target_xfer_memory_partial (memaddr, buf, len, 1, err);
 }