public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Assertion 'xfered>0' in target.c for remote connection
       [not found] <1155839491.1748621.1509663923992.ref@mail.yahoo.com>
@ 2017-11-02 23:05 ` pcarroll
  2017-11-03  3:11   ` Sergio Durigan Junior
  0 siblings, 1 reply; 8+ messages in thread
From: pcarroll @ 2017-11-02 23:05 UTC (permalink / raw)
  To: gdb-patches

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. 

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.
+
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;
}

/* 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;
}

/* 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;
              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.  */


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
  2017-11-02 23:05 ` [PATCH] Assertion 'xfered>0' in target.c for remote connection pcarroll
@ 2017-11-03  3:11   ` Sergio Durigan Junior
  2017-11-14 15:03     ` Paul Carroll
  0 siblings, 1 reply; 8+ messages in thread
From: Sergio Durigan Junior @ 2017-11-03  3:11 UTC (permalink / raw)
  To: pcarroll; +Cc: gdb-patches

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
  2017-11-03  3:11   ` Sergio Durigan Junior
@ 2017-11-14 15:03     ` Paul Carroll
  2017-11-14 21:38       ` Simon Marchi
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Carroll @ 2017-11-14 15:03 UTC (permalink / raw)
  To: Sergio Durigan Junior; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 6057 bytes --]

(Resending, since my previous response lost threading information in the 
mailing list)


On 11/2/2017 10:11 PM, Sergio Durigan Junior wrote:
> 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?
Yep, resending here.  I don't think I've sent email to the mailing lists 
for years, so I needed to use a different mail client.

>> 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.
Taken care of in the new patch
>> 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.
I got rid of this in the new patch, since it doesn't add anything, as 
you noted.
>>                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,
>


[-- Attachment #2: gdb_0_mem_read_patch --]
[-- Type: text/plain, Size: 2419 bytes --]

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.
+
 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-03 13:01:04.953135100 -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;
 }
 
 /* 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;
 }
 
 /* Using the set of read-only target sections of remote, read live
@@ -8461,7 +8461,8 @@ remote_read_bytes (struct target_ops *op
 		  /* 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.  */

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
  2017-11-14 15:03     ` Paul Carroll
@ 2017-11-14 21:38       ` Simon Marchi
  2017-11-14 22:08         ` Yao Qi
  2017-11-14 22:30         ` Paul Carroll
  0 siblings, 2 replies; 8+ messages in thread
From: Simon Marchi @ 2017-11-14 21:38 UTC (permalink / raw)
  To: Paul Carroll, Sergio Durigan Junior; +Cc: gdb-patches

On 2017-11-14 10:02 AM, Paul Carroll 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.

The patch look good to me, given that all other implementations do this.
It is small enough that it doesn't require a copyright assignment I think.
I see you have contributed to binutils in the past.  Do you already have
push access to the binutils-gdb repo?  If not we can push it for you, or
we can get you an account if you plan on contributing regularly.

Thanks,

Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
  2017-11-14 21:38       ` Simon Marchi
@ 2017-11-14 22:08         ` Yao Qi
  2017-11-14 22:30         ` Paul Carroll
  1 sibling, 0 replies; 8+ messages in thread
From: Yao Qi @ 2017-11-14 22:08 UTC (permalink / raw)
  To: Simon Marchi; +Cc: Paul Carroll, Sergio Durigan Junior, gdb-patches

On Tue, Nov 14, 2017 at 9:38 PM, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> It is small enough that it doesn't require a copyright assignment I think.
>

Mentor Graphics had the copyright assignment for all @codesourcery.com
email addresses.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
  2017-11-14 21:38       ` Simon Marchi
  2017-11-14 22:08         ` Yao Qi
@ 2017-11-14 22:30         ` Paul Carroll
  2017-11-14 22:41           ` Simon Marchi
  1 sibling, 1 reply; 8+ messages in thread
From: Paul Carroll @ 2017-11-14 22:30 UTC (permalink / raw)
  To: Simon Marchi, Sergio Durigan Junior; +Cc: gdb-patches

On 11/14/2017 4:38 PM, Simon Marchi wrote:
> On 2017-11-14 10:02 AM, Paul Carroll 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.
> The patch look good to me, given that all other implementations do this.
> It is small enough that it doesn't require a copyright assignment I think.
> I see you have contributed to binutils in the past.  Do you already have
> push access to the binutils-gdb repo?  If not we can push it for you, or
> we can get you an account if you plan on contributing regularly.
>
Please feel free to push the patch for me.
Hard to say how regularly I will be patching things.
The copyright assignment, as Yao noted, is covered by Mentor Graphics 
(and Siemens).

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
  2017-11-14 22:30         ` Paul Carroll
@ 2017-11-14 22:41           ` Simon Marchi
  0 siblings, 0 replies; 8+ messages in thread
From: Simon Marchi @ 2017-11-14 22:41 UTC (permalink / raw)
  To: Paul Carroll, Sergio Durigan Junior; +Cc: gdb-patches

On 2017-11-14 05:30 PM, Paul Carroll wrote:
> On 11/14/2017 4:38 PM, Simon Marchi wrote:
>> On 2017-11-14 10:02 AM, Paul Carroll 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.
>> The patch look good to me, given that all other implementations do this.
>> It is small enough that it doesn't require a copyright assignment I think.
>> I see you have contributed to binutils in the past.  Do you already have
>> push access to the binutils-gdb repo?  If not we can push it for you, or
>> we can get you an account if you plan on contributing regularly.
>>
> Please feel free to push the patch for me.
> Hard to say how regularly I will be patching things.
> The copyright assignment, as Yao noted, is covered by Mentor Graphics (and Siemens).

It is pushed, thanks again for the patch.

Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Assertion 'xfered>0' in target.c for remote connection
@ 2017-11-03 20:36 Carroll, Paul
  0 siblings, 0 replies; 8+ messages in thread
From: Carroll, Paul @ 2017-11-03 20:36 UTC (permalink / raw)
  To: Sergio Durigan Junior; +Cc: gdb-patches

Thanks for your comments, Sergio.
Sadly, I had to manually put the tabs into my patch here, but it looks correct now.
I also removed the TARGET_XFER_OK/TARGET_XFER_EOF test in remote_read_bytes, for the reasons you noted.
The TARGET_XFER_UNAVAILABLE/TARGET_XFER_EOF test is still necessary in case 0 bytes is returned by remote_read_bytes_1.
Some of the added tests are for writing blocks, rather than reading blocks.
It is possible that these returns are not needed, but it seems safer to just cover all of the returns.

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.
+
 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-03 13:01:04.953135100 -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;
 }

 /* 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;
 }

 /* Using the set of read-only target sections of remote, read live
@@ -8461,7 +8461,8 @@ remote_read_bytes (struct target_ops *op
 		  /* 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.  */

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-11-14 22:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1155839491.1748621.1509663923992.ref@mail.yahoo.com>
2017-11-02 23:05 ` [PATCH] Assertion 'xfered>0' in target.c for remote connection pcarroll
2017-11-03  3:11   ` Sergio Durigan Junior
2017-11-14 15:03     ` Paul Carroll
2017-11-14 21:38       ` Simon Marchi
2017-11-14 22:08         ` Yao Qi
2017-11-14 22:30         ` Paul Carroll
2017-11-14 22:41           ` Simon Marchi
2017-11-03 20:36 Carroll, Paul

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).