This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: "pcarroll\@codesourcery.com" <pcarroll at codesourcery dot com>
- Cc: "gdb-patches\@sourceware.org" <gdb-patches at sourceware dot org>
- Date: Thu, 02 Nov 2017 23:11:01 -0400
- Subject: Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7842E80F90
- References: <1155839491.1748621.1509663923992.ref@mail.yahoo.com> <1155839491.1748621.1509663923992@mail.yahoo.com>
On Thursday, November 02 2017, pcarroll@codesourcery.com wrote:
> We have a customer who is using a Corelis gdb server to connect to gdb.
> Occasionally, the gdb server will send a 0-byte block of memory for a read.
> When this happens, gdb gives an assertion from target.c:
>
> internal-error: target_xfer_partial: Assertion `*xfered_len > 0' failed.
>
> This problem is almost identical to that fixed in https://sourceware.org/ml/gdb-patches/2014-02/msg00636.html
>
> In this case, remote.c needs to be modified to return TARGET_XFER_EOF instead of TARGET_XFER_OK or TARGET_XFER_UNAVAILABLE when 0 bytes are transferred.
Thanks for the patch. A few comments below. BTW, your mail client
seems to have messed up with the formatting of the patch, removing TABs
and replacing them with whitespaces. Could you please send the patch
again, but using a client that handles patches better?
> The proposed fix would be:
>
> diff -rup fsf/gdb/ChangeLog fix/gdb/ChangeLog
> --- fsf/gdb/ChangeLog 2017-11-02 16:13:19.188615000 -0500
> +++ fix/gdb/ChangeLog 2017-11-02 16:13:21.626754500 -0500
> @@ -1,3 +1,10 @@
> +2017-11-02 Paul Carroll <pcarroll@codesourcery.com>
> +
> + PR gdb/22388
> + * remote.c (remote_write_bytes_aux, remote_read_bytes_1,
> + remote_read_bytes, remote_write_qxfer, remote_xfer_partial):
> + Return TARGET_XFER_EOF if size of returned data is 0.
> +
The ChangeLog entries need to be formatted with TABs instead of
whitespaces. But the text looks great.
> 2017-11-02 Yao Qi <yao.qi@linaro.org>
>
> * frame.c (do_frame_register_read): Remove aspace.
> diff -rup fsf/gdb/remote.c fix/gdb/remote.c
> --- fsf/gdb/remote.c 2017-11-02 16:06:15.979408800 -0500
> +++ fix/gdb/remote.c 2017-11-02 15:24:35.536391700 -0500
> @@ -8264,7 +8264,7 @@ remote_write_bytes_aux (const char *head
> /* Return UNITS_WRITTEN, not TODO_UNITS, in case escape chars caused us to
> send fewer units than we'd planned. */
> *xfered_len_units = (ULONGEST) units_written;
> - return TARGET_XFER_OK;
> + return (*xfered_len_units != 0) ? TARGET_XFER_OK : TARGET_XFER_EOF;
> }
This one looks correct to me.
>
> /* Write memory data directly to the remote machine.
> @@ -8358,7 +8358,7 @@ remote_read_bytes_1 (CORE_ADDR memaddr,
> decoded_bytes = hex2bin (p, myaddr, todo_units * unit_size);
> /* Return what we have. Let higher layers handle partial reads. */
> *xfered_len_units = (ULONGEST) (decoded_bytes / unit_size);
> - return TARGET_XFER_OK;
> + return (*xfered_len_units != 0) ? TARGET_XFER_OK : TARGET_XFER_EOF;
> }
Likewise.
>
> /* Using the set of read-only target sections of remote, read live
> @@ -8455,13 +8455,14 @@ remote_read_bytes (struct target_ops *op
> res = remote_xfer_live_readonly_partial (ops, myaddr, memaddr,
> len, unit_size, xfered_len);
> if (res == TARGET_XFER_OK)
> - return TARGET_XFER_OK;
> + return (*xfered_len != 0) ? TARGET_XFER_OK : TARGET_XFER_EOF;
remote_xfer_live_readonly_partial can only return TARGET_XFER_EOF or
what remote_read_bytes_1 return (which already takes care of the EOF
part), so I don't think you need this part here, although keeping it
wouldn't hurt.
> else
> {
> /* No use trying further, we know some memory starting
> at MEMADDR isn't available. */
> *xfered_len = len;
> - return TARGET_XFER_UNAVAILABLE;
> + return (*xfered_len != 0) ?
> + TARGET_XFER_UNAVAILABLE : TARGET_XFER_EOF;
> }
> }
>
> @@ -10386,7 +10387,7 @@ remote_write_qxfer (struct target_ops *o
> unpack_varlen_hex (rs->buf, &n);
>
> *xfered_len = n;
> - return TARGET_XFER_OK;
> + return (*xfered_len != 0) ? TARGET_XFER_OK : TARGET_XFER_EOF;
> }
>
> /* Read OBJECT_NAME/ANNEX from the remote target using a qXfer packet.
> @@ -10687,7 +10688,7 @@ remote_xfer_partial (struct target_ops *
> strcpy ((char *) readbuf, rs->buf);
>
> *xfered_len = strlen ((char *) readbuf);
> - return TARGET_XFER_OK;
> + return (*xfered_len != 0) ? TARGET_XFER_OK : TARGET_XFER_EOF;
> }
>
> /* Implementation of to_get_memory_xfer_limit. */
The rest looks sane to me, but I'm not a global maintainer and cannot
approve your patch. Meanwhile I'd recommend resending the patch to fix
the formatting errors.
Cheers,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/