This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFA] initialize err variable in load_section_callback()
- From: "Theodore A. Roth" <troth at openavr dot org>
- Cc: Andrew Cagney <cagney at gnu dot org>, gdb-patches at sources dot redhat dot com
- Date: Mon, 27 Dec 2004 18:33:48 -0800 (PST)
- Subject: Re: [RFA] initialize err variable in load_section_callback()
- References: <Pine.LNX.4.53.0410191307030.17223@knuth.amplepower.com><4176A188.1030904@gnu.org> <Pine.LNX.4.53.0410201058360.19953@knuth.amplepower.com><417D9450.2030401@gnu.org> <Pine.LNX.4.53.0410261100140.17083@knuth.amplepower.com>
This patch got left hanging. Is it a lost cause or can I get approval to
commit it?
Thanks.
On Tue, 26 Oct 2004, Theodore A. Roth wrote:
> 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
---
Ted Roth
PGP Key ID: 0x18F846E9
Jabber ID: troth@jabber.org