[PATCH] Send qXfer:traceframe-info:read when traceframe is selected.

[PATCH] Send qXfer:traceframe-info:read when traceframe is selected.

[Yao Qi]

When I do 'si', I find many 'qXfer:traceframe-info:read' packets are sent,
which is not necessary.  It slows down the single step.
(gdb) si
Sending packet: $qTStatus#49...Packet received: T0;tnotrun:0;tframes:0;tcreated:0;tfree:500000;tsize:500000;circular:0;disconn:0;starttime:0;stoptime:0;username:;notes::
Sending packet: $Z0,80483c7,1#b4...Packet received: OK
Sending packet: $Z0,4ce5b6b0,1#6e...Packet received: OK
Sending packet: $QPassSignals:e;10;14;17;1a;1b;1c;21;24;25;2c;4c;#5f...Packet received: OK
Sending packet: $vCont;s:p1b15.1b15;c#20...Packet received: T0505:44efffbf;04:44efffbf;08:d1830408;thread:p1b15.1b15;core:3;
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $mbfffef40,40#c0...Packet received: d183040878efffbf2e840408030000000000a040030000000500000070efffbf07000000010000004984040807000000030000000500000000000000b396e84c
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $z0,80483c7,1#d4...Packet received: OK
Sending packet: $z0,4ce5b6b0,1#8e...Packet received: OK
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01
Sending packet: $qXfer:traceframe-info:read::0,fff#0b...Packet received: E01

This problem was introduced by this patch
(https://sourceware.org/ml/gdb-patches/2013-04/msg00000.html), in
which get_traceframe_number is not checked before calling
traceframe_available_memory.  This patch moves the check to
remote_traceframe_info, say, if GDB doesn't have traceframe selected, GDB
doesn't need to send qXfer:traceframe-info:read packets.

With this patch applied, there is no qXfer:traceframe-info:read sent
out and single step is speed up a little bit.

Here is the experiment I did:

	   Num of single step	Original	Patched

single-step cpu_time 10000	8.08		7.57
single-step cpu_time 20000	16.23		14.23
single-step cpu_time 30000	24.19		21.59
single-step cpu_time 40000	32.49		28.0
single-step wall_time 10000	14.1974210739	13.2641420364
single-step wall_time 20000	28.5278921127	25.0541369915
single-step wall_time 30000	42.5864038467	38.0038759708
single-step wall_time 40000	57.2107698917	49.2350611687
single-step vmsize 10000	16128		16388
single-step vmsize 20000	16128		16388
single-step vmsize 30000	16260		16520
single-step vmsize 40000	16444		16704

The patch is tested on x86_64-linux.

gdb:

2013-10-24  Yao Qi  <yao@codesourcery.com>

	* remote.c (remote_traceframe_info): Return early if
	traceframe is not selected.
---
 gdb/remote.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 83acec3..3694973 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -11210,6 +11210,11 @@ remote_traceframe_info (void)
 {
   char *text;
 
+  /* If current traceframe is not selected, don't bother the remote
+     stub.  */
+  if (get_traceframe_number () < 0)
+    return NULL;
+
   text = target_read_stralloc (&current_target,
 			       TARGET_OBJECT_TRACEFRAME_INFO, NULL);
   if (text != NULL)
-- 
1.7.7.6

Re: [PATCH] Send qXfer:traceframe-info:read when traceframe is selected.

[Pedro Alves]

On 10/24/2013 02:57 PM, Yao Qi wrote:

> gdb:
> 
> 2013-10-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* remote.c (remote_traceframe_info): Return early if
> 	traceframe is not selected.

This is OK.

Thanks,
-- 
Pedro Alves

supporting all kinds of partially-<unavailable> enum target_object types (was: Re: [PATCH] Send qXfer:traceframe-info:read when traceframe is selected.)

[Pedro Alves]

(Sending this as separate email, as I'm not objecting to the patch.)

Compared to when the tfile target itself handled reading from
the file's RO sections itself, IMO, the end result is more confusing,
with strange interfaces -- "why is traceframe_info being called if
there's no traceframe selected in the first place" immediately comes
to mind.  It seems to me we traded one little wrinkle for a
larger one.  :-)

I sent this to Sergio offlist in the context of supporting
partially <unavailable> $_siginfo, but I think it's appropriate
here too.  So this was about supporting <unavailable> target
objects other than memory (in that case, TARGET_OBJECT_SIGNAL_INFO),
but it'd apply to memory just as well.

I think a clean solution would entail extending the
target_xfer interface.  Consider an object/value like this:

  0          100      150        200           512
  DDDDDDDDDDDxxxxxxxxxDDDDDD...DDIIIIIIIIIIII..III

where D is valid data, and xxx is unavailable data, and I is beyond
the end of the object (Invalid).  Currently, if we start the
xfer at 0, requesting, say 512 bytes, we'll first get back 100 bytes.
The xfer machinery then retries fetching [100,512), and gets back
TARGET_XFER_E_UNAVAILABLE.  That's sufficient when you're either
interested in either having the whole of the 512 bytes available,
or erroring out.  But, in this scenario, we're interested in
the data at [150,512).  The problem is that the last
TARGET_XFER_E_UNAVAILABLE gives us no indication where to
start the read next.  We'd need something like:

get me [0,512) >>>
           <<< here's [0,100)

get me [100,512)  >>>
           <<< [100,150) is unavailable    (**1)

get me [150,512) >>>
           <<< here's [150,200)

get me [200,512) >>>
           <<< no more data

We could do that by changing target_xfer_partial's
interface from:

    LONGEST (*to_xfer_partial) (struct target_ops *ops,
				enum target_object object, const char *annex,
				gdb_byte *readbuf, const gdb_byte *writebuf,
				ULONGEST offset, LONGEST len);

to:

    int (*to_xfer_partial) (struct target_ops *ops,
				enum target_object object, const char *annex,
				gdb_byte *readbuf, const gdb_byte *writebuf,
				ULONGEST offset, ULONGEST len, ULONGEST *read_count);

Where the target implementation would write how many bytes were read
to *READ_COUNT, instead of returning it. (The int return would then be just
be success/error, instead of the read length.)

When returning TARGET_XFER_E_UNAVAILABLE, we'd return in
*READ_COUNT the number of unavailable bytes "read", or IOW, where
the next non-<unavailable> byte is, or LEN, if that comes before.

So the (**1) case above would return TARGET_XFER_E_UNAVAILABLE
with *READ_COUNT set to 50.

target_read_memory would be likewise adjusted, or a variant added.
And then, in specific case of values, read_value_memory would not have that
available_memory vector or any traceframe_number check, but instead
would call target_read_memory in a loop, and if TARGET_XFER_E_UNAVAILABLE
comes out, it'd mark READ_COUNT value bytes unavailable starting at
the requested address, and then continue reading from the previous
addr + READ_COUNT, rinse repeat, until the whole value was read in.

This naturally implies pushing down the decision of whether
to return TARGET_XFER_E_UNAVAILABLE or something else
down to the target.  (Which kinds of leads back to tfile
itself reading from RO memory from file (though we could
export a function in exec.c for that that tfile delegates to,
instead of re-adding the old code).

-- 
Pedro Alves

Re: [PATCH] Send qXfer:traceframe-info:read when traceframe is selected.

[Yao Qi]

On 10/24/2013 11:55 PM, Pedro Alves wrote:
> This is OK.

Patch is committed.  I start to read and think about your mail
"supporting all kinds of partially-<unavailable> enum target_object 
types", and will give comments in a few days.

-- 
Yao (齐尧)

Re: supporting all kinds of partially-<unavailable> enum target_object types

[Yao Qi]

On 10/25/2013 12:15 AM, Pedro Alves wrote:
> We could do that by changing target_xfer_partial's
> interface from:
>
>      LONGEST (*to_xfer_partial) (struct target_ops *ops,
> 				enum target_object object, const char *annex,
> 				gdb_byte *readbuf, const gdb_byte *writebuf,
> 				ULONGEST offset, LONGEST len);
>
> to:
>
>      int (*to_xfer_partial) (struct target_ops *ops,
> 				enum target_object object, const char *annex,
> 				gdb_byte *readbuf, const gdb_byte *writebuf,
> 				ULONGEST offset, ULONGEST len, ULONGEST *read_count);
>
> Where the target implementation would write how many bytes were read
> to *READ_COUNT, instead of returning it. (The int return would then be just
> be success/error, instead of the read length.)

How about "XFERED_COUT" or "XFERED_LEN"? since it may be used by write.

Current to_xfer_partial returns one of three states, read length (> 0), 
done (0), and error (< 0).  to_xfer_partial could return an enum to 
represent these status, e.g.,

enum target_xfer_status
{
   TARGET_XFER_OK = 1,
   TARGET_XFER_DONE = 0,

   TARGET_XFER_ERROR_IO = -1,
   TARGET_XFER_ERROR_UNAVAILABLE = -2,
};

When TARGET_XFER_OK is returned, *XFERED_COUNT is the number of bytes 
read from target.  When TARGET_XFER_DONE is returned, to_xfer_partial 
can't handle this request.  When TARGET_XFER_ERROR_XXX are returned, 
*XFERED_COUNT is set properly.

What do you think about the interface like this?

>
> When returning TARGET_XFER_E_UNAVAILABLE, we'd return in
> *READ_COUNT the number of unavailable bytes "read", or IOW, where
> the next non-<unavailable> byte is, or LEN, if that comes before.
>
> So the (**1) case above would return TARGET_XFER_E_UNAVAILABLE
> with *READ_COUNT set to 50.
>
> target_read_memory would be likewise adjusted, or a variant added.
> And then, in specific case of values, read_value_memory would not have that
> available_memory vector or any traceframe_number check, but instead
> would call target_read_memory in a loop, and if TARGET_XFER_E_UNAVAILABLE
> comes out, it'd mark READ_COUNT value bytes unavailable starting at
> the requested address, and then continue reading from the previous
> addr + READ_COUNT, rinse repeat, until the whole value was read in.
>
> This naturally implies pushing down the decision of whether
> to return TARGET_XFER_E_UNAVAILABLE or something else
> down to the target.  (Which kinds of leads back to tfile
> itself reading from RO memory from file (though we could

IIUC, each target (tfile and remote, for example) should fetch 
traceframe_info in its to_xfer_partial implementation, to decide whether 
to return TARGET_XFER_E_UNAVAILABLE, right?

-- 
Yao (齐尧)

Re: supporting all kinds of partially-<unavailable> enum target_object types

[Pedro Alves]

On 11/07/2013 01:03 AM, Yao Qi wrote:
> On 10/25/2013 12:15 AM, Pedro Alves wrote:
>> We could do that by changing target_xfer_partial's
>> interface from:
>>
>>      LONGEST (*to_xfer_partial) (struct target_ops *ops,
>> 				enum target_object object, const char *annex,
>> 				gdb_byte *readbuf, const gdb_byte *writebuf,
>> 				ULONGEST offset, LONGEST len);
>>
>> to:
>>
>>      int (*to_xfer_partial) (struct target_ops *ops,
>> 				enum target_object object, const char *annex,
>> 				gdb_byte *readbuf, const gdb_byte *writebuf,
>> 				ULONGEST offset, ULONGEST len, ULONGEST *read_count);
>>
>> Where the target implementation would write how many bytes were read
>> to *READ_COUNT, instead of returning it. (The int return would then be just
>> be success/error, instead of the read length.)
> 
> How about "XFERED_COUT" or "XFERED_LEN"? since it may be used by write.

Yeah.  I guess XFERED_LEN to go with LEN.

> 
> Current to_xfer_partial returns one of three states, read length (> 0), 
> done (0), and error (< 0).  to_xfer_partial could return an enum to 
> represent these status, e.g.,
> 
> enum target_xfer_status
> {
>    TARGET_XFER_OK = 1,
>    TARGET_XFER_DONE = 0,
> 
>    TARGET_XFER_ERROR_IO = -1,
>    TARGET_XFER_ERROR_UNAVAILABLE = -2,
> };
> 
> When TARGET_XFER_OK is returned, *XFERED_COUNT is the number of bytes 
> read from target.  When TARGET_XFER_DONE is returned, to_xfer_partial 
> can't handle this request.  When TARGET_XFER_ERROR_XXX are returned, 
> *XFERED_COUNT is set properly.
> 
> What do you think about the interface like this?

Yes, something like that was what I was going for, when I added
enum target_xfer_error.  I think you've chosen 0 for
TARGET_XFER_DONE to try to avoid changing current code
unnecessarily.  IIRC, I mentioned back then that I didn't
encode an enum for success because different places treat "0"
differently.  If possible, cleaning that all up would be nice,
though I'm not sure we'd really be able to distinguish
TARGET_XFER_ERROR_IO from TARGET_XFER_DONE.

> When TARGET_XFER_DONE is returned, to_xfer_partial can't handle
> this request.

I think we need to pick a word other than "done" here.  "done"
sounds ambiguous with "ok", hence confusing, and it's actually
indicative that no transfer was done/possible.  Perhaps
TARGET_XFER_ERROR_NOT_SUPPORTED.

> 
>>
>> When returning TARGET_XFER_E_UNAVAILABLE, we'd return in
>> *READ_COUNT the number of unavailable bytes "read", or IOW, where
>> the next non-<unavailable> byte is, or LEN, if that comes before.
>>
>> So the (**1) case above would return TARGET_XFER_E_UNAVAILABLE
>> with *READ_COUNT set to 50.
>>
>> target_read_memory would be likewise adjusted, or a variant added.
>> And then, in specific case of values, read_value_memory would not have that
>> available_memory vector or any traceframe_number check, but instead
>> would call target_read_memory in a loop, and if TARGET_XFER_E_UNAVAILABLE
>> comes out, it'd mark READ_COUNT value bytes unavailable starting at
>> the requested address, and then continue reading from the previous
>> addr + READ_COUNT, rinse repeat, until the whole value was read in.
>>
>> This naturally implies pushing down the decision of whether
>> to return TARGET_XFER_E_UNAVAILABLE or something else
>> down to the target.  (Which kinds of leads back to tfile
>> itself reading from RO memory from file (though we could
> 
> IIUC, each target (tfile and remote, for example) should fetch 
> traceframe_info in its to_xfer_partial implementation, to decide whether 
> to return TARGET_XFER_E_UNAVAILABLE, right?

Yeah, for things that are in the traceframe info.  For remote,
we might end up extending qXfer similarly, to consult the target
directly avoiding having to list all kinds of objects in
traceframe info.

Thanks,
-- 
Pedro Alves