From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16322 invoked by alias); 26 Oct 2004 18:19:17 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 16309 invoked from network); 26 Oct 2004 18:19:15 -0000 Received: from unknown (HELO mail.amplepower.com) (216.39.162.139) by sourceware.org with SMTP; 26 Oct 2004 18:19:15 -0000 Received: from [192.168.8.30] (helo=knuth.amplepower.com ident=roth) by mail.amplepower.com with esmtp (Exim 3.36 #1 (Debian)) id 1CMVub-00013t-00; Tue, 26 Oct 2004 11:19:13 -0700 Date: Tue, 26 Oct 2004 18:19:00 -0000 From: "Theodore A. Roth" X-X-Sender: roth@knuth.amplepower.com To: Andrew Cagney cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] initialize err variable in load_section_callback() In-Reply-To: <417D9450.2030401@gnu.org> Message-ID: References: <4176A188.1030904@gnu.org> <417D9450.2030401@gnu.org> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="-1463804408-778998018-1098814752=:17083" X-SW-Source: 2004-10/txt/msg00442.txt.bz2 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. Send mail to mime@docserver.cac.washington.edu for more info. ---1463804408-778998018-1098814752=:17083 Content-Type: TEXT/PLAIN; charset=US-ASCII Content-length: 3875 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 ---1463804408-778998018-1098814752=:17083 Content-Type: TEXT/PLAIN; charset=US-ASCII; name="gdb-initialize-err.diff" Content-Transfer-Encoding: BASE64 Content-ID: Content-Description: Content-Disposition: attachment; filename="gdb-initialize-err.diff" Content-length: 2424 MjAwNC0xMC0yNiAgVGhlb2RvcmUgQS4gUm90aCAgPHRyb3RoQG9wZW5hdnIu b3JnPg0KDQoJKiB0YXJnZXQuYyAodGFyZ2V0X3JlYWRfbWVtb3J5X3BhcnRp YWwpOiBNYWtlIHN1cmUgdGhlIGVyciBpcyBzZXQuDQoJKHRhcmdldF93cml0 ZV9tZW1vcnlfcGFydGlhbCk6IERpdHRvLg0KDQpJbmRleDogdGFyZ2V0LmMN Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0NClJDUyBmaWxlOiAvY3ZzL3NyYy9z cmMvZ2RiL3RhcmdldC5jLHYNCnJldHJpZXZpbmcgcmV2aXNpb24gMS45MA0K ZGlmZiAtdSAtcCAtcCAtcjEuOTAgdGFyZ2V0LmMNCi0tLSB0YXJnZXQuYwk4 IE9jdCAyMDA0IDIwOjI5OjU1IC0wMDAwCTEuOTANCisrKyB0YXJnZXQuYwky NiBPY3QgMjAwNCAxNzozNToxNCAtMDAwMA0KQEAgLTEyMzMsOCArMTIzMywy MCBAQCBpbnQNCiB0YXJnZXRfcmVhZF9tZW1vcnlfcGFydGlhbCAoQ09SRV9B RERSIG1lbWFkZHIsIGNoYXIgKmJ1ZiwgaW50IGxlbiwgaW50ICplcnIpDQog ew0KICAgaWYgKHRhcmdldF94ZmVyX3BhcnRpYWxfcCAoKSkNCi0gICAgcmV0 dXJuIHRhcmdldF94ZmVyX3BhcnRpYWwgKHRhcmdldF9zdGFjaywgVEFSR0VU X09CSkVDVF9NRU1PUlksIE5VTEwsDQotCQkJCWJ1ZiwgTlVMTCwgbWVtYWRk ciwgbGVuKTsNCisgICAgew0KKyAgICAgIGludCByZXR2YWwgPSB0YXJnZXRf eGZlcl9wYXJ0aWFsICh0YXJnZXRfc3RhY2ssIFRBUkdFVF9PQkpFQ1RfTUVN T1JZLA0KKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICBOVUxMLCBidWYsIE5VTEwsIG1lbWFkZHIsIGxlbik7DQorDQorICAgICAg LyogdHJvdGgvMjAwNC0xMC0yNjogQXJlIHdlIGNlcnRhaW4gdGhhdCBlcnJu byB3aWxsIGJlIHNldCBwcm9wZXJseSBpZg0KKyAgICAgICAgIHRoZSBhYm92 ZSBjYWxsIHJldHVybnMgLTE/ICovDQorDQorICAgICAgaWYgKHJldHZhbCA9 PSAtMSkNCisgICAgICAgICplcnIgPSBlcnJubzsNCisgICAgICBlbHNlDQor ICAgICAgICAqZXJyID0gMDsNCisNCisgICAgICByZXR1cm4gcmV0dmFsOw0K KyAgICB9DQogICBlbHNlDQogICAgIHJldHVybiB0YXJnZXRfeGZlcl9tZW1v cnlfcGFydGlhbCAobWVtYWRkciwgYnVmLCBsZW4sIDAsIGVycik7DQogfQ0K QEAgLTEyNDMsOCArMTI1NSwxNyBAQCBpbnQNCiB0YXJnZXRfd3JpdGVfbWVt b3J5X3BhcnRpYWwgKENPUkVfQUREUiBtZW1hZGRyLCBjaGFyICpidWYsIGlu dCBsZW4sIGludCAqZXJyKQ0KIHsNCiAgIGlmICh0YXJnZXRfeGZlcl9wYXJ0 aWFsX3AgKCkpDQotICAgIHJldHVybiB0YXJnZXRfeGZlcl9wYXJ0aWFsICh0 YXJnZXRfc3RhY2ssIFRBUkdFVF9PQkpFQ1RfTUVNT1JZLCBOVUxMLA0KLQkJ CQlOVUxMLCBidWYsIG1lbWFkZHIsIGxlbik7DQorICAgIHsNCisgICAgICBp bnQgcmV0dmFsID0gdGFyZ2V0X3hmZXJfcGFydGlhbCAodGFyZ2V0X3N0YWNr LCBUQVJHRVRfT0JKRUNUX01FTU9SWSwNCisgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgTlVMTCwgTlVMTCwgYnVmLCBtZW1hZGRy LCBsZW4pOw0KKw0KKyAgICAgIGlmIChyZXR2YWwgPT0gLTEpDQorICAgICAg ICAqZXJyID0gZXJybm87DQorICAgICAgZWxzZQ0KKyAgICAgICAgKmVyciA9 IDA7DQorDQorICAgICAgcmV0dXJuIHJldHZhbDsNCisgICAgfQ0KICAgZWxz ZQ0KICAgICByZXR1cm4gdGFyZ2V0X3hmZXJfbWVtb3J5X3BhcnRpYWwgKG1l bWFkZHIsIGJ1ZiwgbGVuLCAxLCBlcnIpOw0KIH0NCg== ---1463804408-778998018-1098814752=:17083--