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

[Pedro Alves <palves@redhat.com>]

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)