[PATCH] Partially available/unavailable data in requested range

[PATCH] Partially available/unavailable data in requested range

[Yao Qi]

I am testing again my pending patches "Visit varobj available children only in MI"
and find a regression in mi-available-children-only.exp, in which we
collect part of the fields of struct simple,

struct simple
{
  int a; /* Collected by both.  */
  int b; /* Collected by action 2.  */
  struct
  {
    struct
    {
      int g; /* Collected by action 1.  */
      int h; /* Collected by action 2.  */
    } s3;
    int d; /* Collected by action 1.  */
  } s1;

  struct
  {
    int e;
    int f; /* Collected by action 1.  */
  } s2;
};

When GDB examine traceframe collected by action 1, some collected fields
are unavailable, which is wrong.

(gdb) p s
$1 = {a = 0, b = <unavailable>, s1 = {s3 = {g = <unavailable>, h = <unavailable>}, d = <unavailable>}, s2 = {e = <unavailable>, f = <unavailable>}}

  1. When GDB reads struct 's', it will request to read memory at address S
of length 28.  Since s.a is collected while s.b isn't, tfile_xfer_partial
will return TARGET_XFER_OK and *xfered_len is set to 4 (length of s.a).
  2. Then, GDB request to read memory at address S + 4 of length 24.  No block
includes the first part of the desired range, so tfile_xfer_partial returns
TARGET_XFER_UNAVAILABLE and GDB thinks the range [S + 4, S + 28) is unavailable.

In order to fix this problem, in the iteration to 'M' blocks, we record the
minimal address of blocks within the request range.  If it has, the requested
range isn't unavailable completely.  This applies to ctf too.  With this patch
applied, the result looks good and fails in mi-available-children-only.exp are
fixed as a result.

(gdb) p s
$1 = {a = 0, b = <unavailable>, s1 = {s3 = {g = 0, h = <unavailable>}, d = 0}, s2 = {e = <unavailable>, f = 0}}

I don't add a test case for it because mi-available-children-only.exp
will cover it.

gdb:

2014-04-20  Yao Qi  <yao@codesourcery.com>

	* tracefile-tfile.c (tfile_xfer_partial): Record the minimal
	address of blocks and return TARGET_XFER_UNAVAILABLE if it
	is within the requested range.
	* ctf.c (ctf_xfer_partial): Likewise.
---
 gdb/ctf.c             | 18 ++++++++++++++++++
 gdb/tracefile-tfile.c | 18 ++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/gdb/ctf.c b/gdb/ctf.c
index bac7c28..253548d 100644
--- a/gdb/ctf.c
+++ b/gdb/ctf.c
@@ -1328,6 +1328,7 @@ ctf_xfer_partial (struct target_ops *ops, enum target_object object,
       struct bt_iter_pos *pos;
       int i = 0;
       enum target_xfer_status res;
+      ULONGEST min_addr_available = 0;
 
       gdb_assert (ctf_iter != NULL);
       /* Save the current position.  */
@@ -1410,6 +1411,13 @@ ctf_xfer_partial (struct target_ops *ops, enum target_object object,
 		}
 	    }
 
+	  /* If the block is within the desired range, the desired
+	     range isn't fully unavailable.  Record the minimal address
+	     of blocks.  */
+	  if (offset < maddr && maddr < (offset + len))
+	    if (min_addr_available == 0 || min_addr_available > maddr)
+	      min_addr_available = maddr;
+
 	  if (bt_iter_next (bt_ctf_get_iter (ctf_iter)) < 0)
 	    break;
 	}
@@ -1417,6 +1425,16 @@ ctf_xfer_partial (struct target_ops *ops, enum target_object object,
       /* Restore the position.  */
       bt_iter_set_pos (bt_ctf_get_iter (ctf_iter), pos);
 
+      /* There should be at least one block within desired range, and
+	 range [OFFSET, MIN_ADDR_AVAILABLE) is unavailable.  Tell
+	 caller about it and caller will request memory from
+	 MIN_ADDR_AVAILABLE.  */
+      if (offset < min_addr_available)
+	{
+	  *xfered_len = min_addr_available - offset;
+	  return TARGET_XFER_UNAVAILABLE;
+	}
+
       /* Requested memory is unavailable in the context of traceframes,
 	 and this address falls within a read-only section, fallback
 	 to reading from executable.  */
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index efa69b2..472f387 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -853,6 +853,7 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
     {
       int pos = 0;
       enum target_xfer_status res;
+      ULONGEST min_addr_available = 0;
 
       /* Iterate through the traceframe's blocks, looking for
 	 memory.  */
@@ -886,10 +887,27 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
 	      return TARGET_XFER_OK;
 	    }
 
+	  /* If the block is within the desired range, the desired
+	     range isn't fully unavailable.  Record the minimal address
+	     of blocks.  */
+	  if (offset < maddr && maddr < (offset + len))
+	    if (min_addr_available == 0 || min_addr_available > maddr)
+	      min_addr_available = maddr;
+
 	  /* Skip over this block.  */
 	  pos += (8 + 2 + mlen);
 	}
 
+      /* There should be at least one block within desired range, and
+	 range [OFFSET, MIN_ADDR_AVAILABLE) is unavailable.  Tell
+	 caller about it and caller will request memory from
+	 MIN_ADDR_AVAILABLE.  */
+      if (offset < min_addr_available)
+	{
+	  *xfered_len = min_addr_available - offset;
+	  return TARGET_XFER_UNAVAILABLE;
+	}
+
       /* Requested memory is unavailable in the context of traceframes,
 	 and this address falls within a read-only section, fallback
 	 to reading from executable.  */
-- 
1.9.0

Re: [PATCH] Partially available/unavailable data in requested range

[Pedro Alves]

On 04/20/2014 01:48 PM, Yao Qi wrote:

> (gdb) p s
> $1 = {a = 0, b = <unavailable>, s1 = {s3 = {g = 0, h = <unavailable>}, d = 0}, s2 = {e = <unavailable>, f = 0}}
> 
> I don't add a test case for it because mi-available-children-only.exp
> will cover it.

It'd be best if unavailable.cc/unavailable.exp covers this scenario
as well.  It's meant to be complete in this sort of partial
collection stuff.  Could you do that, please ?

> +      /* There should be at least one block within desired range, and
> +	 range [OFFSET, MIN_ADDR_AVAILABLE) is unavailable.  Tell
> +	 caller about it and caller will request memory from
> +	 MIN_ADDR_AVAILABLE.  */
> +      if (offset < min_addr_available)
> +	{
> +	  *xfered_len = min_addr_available - offset;
> +	  return TARGET_XFER_UNAVAILABLE;
> +	}

I find the comment above confusing and hard to grok.  :-/

- "There should be" sounds like an assertion, which this is not.

- Comments that assume the condition is true are better place
_within_ the then block.  Comments that appear before
the condition usually are more naturally of the 

/* if $human-understandable-version-of-the-condition then
   do something.  */

form.

- A few "the"'s are missing.

So I think this would be much clearer:

      if (offset < min_addr_available)
	{
	  /* There's at least one block containing the desired range
	     but the range [OFFSET, MIN_ADDR_AVAILABLE) is
	     unavailable.  Return that and GDB will re-request memory
	     starting at MIN_ADDR_AVAILABLE.  */
	  *xfered_len = min_addr_available - offset;
	  return TARGET_XFER_UNAVAILABLE;
	}


But, looking deeper, I don't think the patch is correct, actually.

Even if the range [OFFSET, MIN_ADDR_AVAILABLE) is
not unavailable in the context of traceframes, that
range might fall within a read-only section, so we should
still try falling back to reading from the executable file.

So it seems to me what we need to do is trim LEN up to
the first available address.  Then if reading from the
executable still yields nothing, the 

      else
	{
	  /* No use trying further, we know some memory starting
	     at MEMADDR isn't available.  */
	  *xfered_len = len;
	  return TARGET_XFER_UNAVAILABLE;
	}

part returns the corrected LEN.  That is, I think the below
would be both simpler, and more correct.  (I also think
first_addr_available is clearer than min_addr_available").

Completely untested, but should give you the idea.

--------------
diff --git c/gdb/tracefile-tfile.c w/gdb/tracefile-tfile.c
index efa69b2..e570b10 100644
--- c/gdb/tracefile-tfile.c
+++ w/gdb/tracefile-tfile.c
@@ -853,6 +853,8 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
     {
       int pos = 0;
       enum target_xfer_status res;
+      /* Records the first available address of all blocks.  */
+      ULONGEST first_addr_available = 0;
 
       /* Iterate through the traceframe's blocks, looking for
 	 memory.  */
@@ -886,13 +888,18 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
 	      return TARGET_XFER_OK;
 	    }
 
+	  if (first_addr_available == 0 || maddr < first_addr_available)
+	    first_addr_available = maddr;
+
 	  /* Skip over this block.  */
 	  pos += (8 + 2 + mlen);
 	}
 
       /* Requested memory is unavailable in the context of traceframes,
 	 and this address falls within a read-only section, fallback
-	 to reading from executable.  */
+	 to reading from executable, up to FIRST_ADDR_AVAILABLE.  */
+      if (offset < first_addr_available)
+	len = min (len, first_addr_available - offset);
       res = exec_read_partial_read_only (readbuf, offset, len, xfered_len);
 
       if (res == TARGET_XFER_OK)

Re: [PATCH] Partially available/unavailable data in requested range

[Yao Qi]

On 04/25/2014 12:19 AM, Pedro Alves wrote:
> It'd be best if unavailable.cc/unavailable.exp covers this scenario
> as well.  It's meant to be complete in this sort of partial
> collection stuff.  Could you do that, please ?
> 

Sure.  I didn't add a test case for it because
mi-available-children-only.exp can cover it, but looks the review to
"available-children-only" series is slow-moving.  I'll add test to
unavailable.exp.

unavailable.exp test is only for live target, while the bug this patch
tries to fix is about trace file targets.  I'll factor
unavailable.exp out for tfile and ctf targets first.

>> > +      /* There should be at least one block within desired range, and
>> > +	 range [OFFSET, MIN_ADDR_AVAILABLE) is unavailable.  Tell
>> > +	 caller about it and caller will request memory from
>> > +	 MIN_ADDR_AVAILABLE.  */
>> > +      if (offset < min_addr_available)
>> > +	{
>> > +	  *xfered_len = min_addr_available - offset;
>> > +	  return TARGET_XFER_UNAVAILABLE;
>> > +	}
> I find the comment above confusing and hard to grok.  :-/
> 
> - "There should be" sounds like an assertion, which this is not.
> 
> - Comments that assume the condition is true are better place
> _within_ the then block.  Comments that appear before
> the condition usually are more naturally of the 
> 
> /* if $human-understandable-version-of-the-condition then
>    do something.  */
> 
> form.
> 
> - A few "the"'s are missing.
> 
> So I think this would be much clearer:
> 
>       if (offset < min_addr_available)
> 	{
> 	  /* There's at least one block containing the desired range
> 	     but the range [OFFSET, MIN_ADDR_AVAILABLE) is
> 	     unavailable.  Return that and GDB will re-request memory
> 	     starting at MIN_ADDR_AVAILABLE.  */
> 	  *xfered_len = min_addr_available - offset;
> 	  return TARGET_XFER_UNAVAILABLE;
> 	}
> 
> 
> But, looking deeper, I don't think the patch is correct, actually.
> 
> Even if the range [OFFSET, MIN_ADDR_AVAILABLE) is
> not unavailable in the context of traceframes, that
> range might fall within a read-only section, so we should
> still try falling back to reading from the executable file.
> 

I thought of this when I wrote this patch.  I was unable to figure
out your trim-LEN-up trick, and get the code complicated.  Given
"requesting memory across sections" is a rare case, I didn't go on
this track further.

> So it seems to me what we need to do is trim LEN up to
> the first available address.  Then if reading from the
> executable still yields nothing, the 
> 
>       else
> 	{
> 	  /* No use trying further, we know some memory starting
> 	     at MEMADDR isn't available.  */
> 	  *xfered_len = len;
> 	  return TARGET_XFER_UNAVAILABLE;
> 	}
> 
> part returns the corrected LEN.  That is, I think the below
> would be both simpler, and more correct.  (I also think

Yes, that is much simpler.

> first_addr_available is clearer than min_addr_available").

I don't think "first" is clearer than "min".  There are multiple 'M'
blocks in a traceframe, and the address of some of them are within the
desired range [OFFSET, OFFSET + LEN).  We are looking for the minimal
address within the range, instead of the first address within the range.
For example, supposing we have three 'M' blocks, M1 (0x01 0x02),
M2 (0x07, 0x08) and M3 (0x4, 0x05), and the requested range is
[0x03, 0x09), the first 'M' block within this range is M2, while the
minimal address of 'M' block is M3.  M3 is what we are looking for.

> 
> Completely untested, but should give you the idea.
> 
> --------------
> diff --git c/gdb/tracefile-tfile.c w/gdb/tracefile-tfile.c
> index efa69b2..e570b10 100644
> --- c/gdb/tracefile-tfile.c
> +++ w/gdb/tracefile-tfile.c
> @@ -853,6 +853,8 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
>      {
>        int pos = 0;
>        enum target_xfer_status res;
> +      /* Records the first available address of all blocks.  */
> +      ULONGEST first_addr_available = 0;
>  
>        /* Iterate through the traceframe's blocks, looking for
>  	 memory.  */
> @@ -886,13 +888,18 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
>  	      return TARGET_XFER_OK;
>  	    }
>  
> +	  if (first_addr_available == 0 || maddr < first_addr_available)
> +	    first_addr_available = maddr;
> +

In my patch, there is one more condition check

	  if (offset < maddr && maddr < (offset + len))
	  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	    if (min_addr_available == 0 || min_addr_available > maddr)
	      min_addr_available = maddr;

to avoid recoding minimal address *outside* of requested range.  For
example, we have three 'M' blocks, M1 (0x01 0x02), M2 (0x07, 0x08) and
M3 (0x04, 0x05), and the requested range is [0x03, 0x09).

          OFFSET
          |--- requested ---|
  |-M1-|    |-M2-|   |-M3-|

The MIN_ADDR_AVAILABLE is expected to be 0x04 instead of 0x01.  I'll
post a patch soon.

-- 
Yao (齐尧)

Re: [PATCH] Partially available/unavailable data in requested range

[Pedro Alves]

On 04/25/2014 09:48 AM, Yao Qi wrote:

> I thought of this when I wrote this patch.  I was unable to figure
> out your trim-LEN-up trick, and get the code complicated.  Given
> "requesting memory across sections" is a rare case, I didn't go on
> this track further.

Well...

On 04/25/2014 09:48 AM, Yao Qi wrote:
> On 04/25/2014 12:19 AM, Pedro Alves wrote:

>> first_addr_available is clearer than min_addr_available").
> 
> I don't think "first" is clearer than "min".  There are multiple 'M'
> blocks in a traceframe, and the address of some of them are within the
> desired range [OFFSET, OFFSET + LEN).  We are looking for the minimal
> address within the range, instead of the first address within the range.
> For example, supposing we have three 'M' blocks, M1 (0x01 0x02),
> M2 (0x07, 0x08) and M3 (0x4, 0x05), and the requested range is
> [0x03, 0x09), the first 'M' block within this range is M2, while the
> minimal address of 'M' block is M3.  M3 is what we are looking for.

It confused me, because I read "minimal" as in "only barely adequate".
And then we have "maddr", starting with "m", and "M" blocks.  That's a
lot of unrelated 'm's.  I can see now how "first" might be confusing
as well, as leading to think that it's the first we encounter.  Maybe
"low" would be even better:

      /* Records the lowest available address of all blocks that
	 intersects the requested range.  */
      ULONGEST low_addr_available = 0;

> In my patch, there is one more condition check
> 
> 	  if (offset < maddr && maddr < (offset + len))
> 	  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 	    if (min_addr_available == 0 || min_addr_available > maddr)
> 	      min_addr_available = maddr;
> 
> to avoid recoding minimal address *outside* of requested range.  For
> example, we have three 'M' blocks, M1 (0x01 0x02), M2 (0x07, 0x08) and
> M3 (0x04, 0x05), and the requested range is [0x03, 0x09).
> 
>           OFFSET
>           |--- requested ---|
>   |-M1-|    |-M2-|   |-M3-|
> 
> The MIN_ADDR_AVAILABLE is expected to be 0x04 instead of 0x01.

Ah, indeed.

Thanks,
-- 
Pedro Alves