Re: [PATCH 5/6] Return target_xfer_status in to_xfer_partial

[Pedro Alves <palves at redhat dot com>]

On 01/31/2014 03:35 AM, Yao Qi wrote:
> This patch does the conversion of to_xfer_partial 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, ULONGEST len);
> 
> to
> 
>     enum target_xfer_status (*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 *xfered_len);
> 
> It changes to_xfer_partial return the transfer status and the transfered
> length by *XFERED_LEN.  Generally, the return status has three stats,
> 
>  - TARGET_XFER_OK,
>  - TARGET_XFER_DONE,
>  - TARGET_XFER_E_XXXX,

I was reading this and thinking "what's the difference
between OK and DONE again?" ...

> See the comments to them in 'enum target_xfer_status'.  Note that
> Pedro suggested not name TARGET_XFER_DONE, as it is confusing,
> compared with "TARGET_XFER_OK".  I still choose "done" because it has
> been used for a while.

Right.  :-)  This DONE status indicates that no further
transfer is possible.  How about we call it TARGET_XFER_END or
TARGET_XFER_EOF ?  EOF is pretty much universally understood
I think.  I believe that'd be much clearer.


>  	  size -= offset;
>  	  if (size > len)
>  	    size = len;
> @@ -714,7 +714,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
>  	      return TARGET_XFER_E_IO;
>  	    }
>  
> -	  return size;
> +	  if (size == 0)
> +	    return TARGET_XFER_DONE;
> +	  else
> +	    {
> +	      *xfered_len = (ULONGEST) size;
> +	      return TARGET_XFER_OK;
> +	    }

If we move this size == 0 check above, we simplify
the code above too.  Like:

	  if (size > len)
	    size = len;

+	  if (size == 0)
+	    return TARGET_XFER_DONE;

-	  if (size > 0
-	      && !bfd_get_section_contents (core_bfd, section, readbuf,
+	  if (!bfd_get_section_contents (core_bfd, section, readbuf,
					    (file_ptr) offset, size))
	    {
	      warning (_("Couldn't read NT_AUXV note in core file."));
	      return TARGET_XFER_E_IO;
	    }

-	  return size;
+	  *xfered_len = (ULONGEST) size;
+	  return TARGET_XFER_OK;

>  	}
>        return TARGET_XFER_E_IO;
>  
> @@ -746,7 +752,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
>  	      return TARGET_XFER_E_IO;
>  	    }
>  
> -	  return size;
> +	  if (size == 0)
> +	    return TARGET_XFER_DONE;
> +	  else
> +	    {
> +	      *xfered_len = (ULONGEST) size;
> +	      return TARGET_XFER_OK;
> +	    }
>  	}

Likewise.

>  
> @@ -793,7 +822,7 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
>  
>  	  size = bfd_section_size (core_bfd, section);
>  	  if (offset >= size)
> -	    return 0;
> +	    return TARGET_XFER_DONE;
>  	  size -= offset;
>  	  if (size > len)
>  	    size = len;
> @@ -805,7 +834,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
>  	      return TARGET_XFER_E_IO;
>  	    }
>  
> -	  return size;
> +	  if (size == 0)
> +	    return TARGET_XFER_DONE;
> +	  else
> +	    {
> +	      *xfered_len = (ULONGEST) size;
> +	      return TARGET_XFER_OK;
> +	    }

Ditto.


> -static LONGEST
> +static enum target_xfer_status
>  linux_nat_xfer_osdata (struct target_ops *ops, enum target_object object,
>  		       const char *annex, gdb_byte *readbuf,
> -		       const gdb_byte *writebuf, ULONGEST offset, ULONGEST len)
> +		       const gdb_byte *writebuf, ULONGEST offset, ULONGEST len,
> +		       ULONGEST *xfered_len)
>  {
>    gdb_assert (object == TARGET_OBJECT_OSDATA);
>  
> -  return linux_common_xfer_osdata (annex, readbuf, offset, len);
> +  *xfered_len = linux_common_xfer_osdata (annex, readbuf, offset, len);
> +  if (*xfered_len== 0)

Missing space.



> -static LONGEST
> +static enum target_xfer_status
>  target_read_live_memory (enum target_object object,
> -			 ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len)
> +			 ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len,
> +			 ULONGEST *xfered_len)
>  {
> -  LONGEST ret;
> +  enum target_xfer_status ret;
>    struct cleanup *cleanup;
>  
>    /* Switch momentarily out of tfind mode so to access live memory.
> @@ -1326,8 +1327,8 @@ target_read_live_memory (enum target_object object,
>    cleanup = make_cleanup_restore_traceframe_number ();
>    set_traceframe_number (-1);
>  
> -  ret = target_read (current_target.beneath, object, NULL,
> -		     myaddr, memaddr, len);
> +  ret = target_xfer_partial (current_target.beneath, object, NULL,
> +			     myaddr, NULL, memaddr, len, xfered_len);

This doesn't seem equivalent?

>  
>        if (readbuf)
>  	myaddr = readbuf;
>        if (writebuf)
>  	myaddr = writebuf;
> -      if (retval > 0 && myaddr != NULL)
> +      if (retval == TARGET_XFER_OK && myaddr != NULL)
>  	{
>  	  int i;
>  
>  	  fputs_unfiltered (", bytes =", gdb_stdlog);
> -	  for (i = 0; i < retval; i++)
> +	  for (i = 0; i < *xfered_len; i++)
>  	    {
>  	      if ((((intptr_t) &(myaddr[i])) & 0xf) == 0)
>  		{
> @@ -1763,12 +1786,19 @@ target_xfer_partial (struct target_ops *ops,
>  
>        fputc_unfiltered ('\n', gdb_stdlog);
>      }
> +
> +  /* Check implementations of to_xfer_partial update *XFERED_LEN
> +     properly.  Do assertion after printing debug messages, so that we
> +     can find more clues on assertion failure from debugging messages.  */
> +  if (retval == TARGET_XFER_OK || retval == TARGET_XFER_E_UNAVAILABLE)
> +      gdb_assert (*xfered_len > 0);

Indentation looks a little odd here.


>  /* Wrappers to perform the full transfer.  */
> @@ -2058,16 +2098,22 @@ target_read (struct target_ops *ops,
>  
>    while (xfered < len)
>      {
> -      LONGEST xfer = target_read_partial (ops, object, annex,
> -					  (gdb_byte *) buf + xfered,
> -					  offset + xfered, len - xfered);
> +      ULONGEST xfered_len;
> +      enum target_xfer_status status;
> +
> +      status = target_read_partial (ops, object, annex,
> +				    (gdb_byte *) buf + xfered,
> +				    offset + xfered, len - xfered,
> +				    &xfered_len);
>  
>        /* Call an observer, notifying them of the xfer progress?  */
> -      if (xfer == 0)
> +      if (status == TARGET_XFER_DONE)
>  	return xfered;
> -      if (xfer < 0)
> +      if (TARGET_XFER_STATUS_ERROR_P (status))
>  	return -1;
> -      xfered += xfer;
> +
> +      gdb_assert (status == TARGET_XFER_OK);
> +      xfered += xfered_len;

I think you can avoid all these TARGET_XFER_SATUS_ERROR_P uses by
simply reversing the checks.  E.g.,:

      if (status == TARGET_XFER_DONE)
 	return xfered;
      else if (status == TARGET_XFER_OK)
       {
         xfered += xfered_len;
         QUIT;
       }
      else
	return -1;

>      }
>    return len;
> @@ -2104,6 +2150,7 @@ read_whatever_is_readable (struct target_ops *ops,
>    ULONGEST current_end = end;
>    int forward;
>    memory_read_result_s r;
> +  ULONGEST xfered_len;
>  
>    /* If we previously failed to read 1 byte, nothing can be done here.  */
>    if (end - begin <= 1)
> @@ -2116,13 +2163,16 @@ read_whatever_is_readable (struct target_ops *ops,
>       if not.  This heuristic is meant to permit reading accessible memory
>       at the boundary of accessible region.  */
>    if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
> -			   buf, begin, 1) == 1)
> +			   buf, begin, 1, &xfered_len) == TARGET_XFER_OK
> +      && xfered_len == 1)

Didn't we assert that TARGET_XFER_OK transfers more than 0 ?  Seems like
this "xfered_len == 1" check is redundant.

>      {
>        forward = 1;
>        ++current_begin;
>      }
>    else if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
> -				buf + (end-begin) - 1, end - 1, 1) == 1)
> +				buf + (end-begin) - 1, end - 1, 1,
> +				&xfered_len) == TARGET_XFER_OK
> +	   && xfered_len == 1)

Likewise.

>  
> -/* Possible error codes returned by target_xfer_partial, etc.  */
> +/* Possible values returned by target_xfer_partial, etc.  */
>  
> -enum target_xfer_error
> +enum target_xfer_status
>  {
> +  /* Some bytes are transferred.  */
> +  TARGET_XFER_OK = 1,
> +
> +  /* No further transfer is possible.  */
> +  TARGET_XFER_DONE = 0,
> +
>    /* Generic I/O error.  Note that it's important that this is '-1',
>       as we still have target_xfer-related code returning hardcoded
>       '-1' on error.  */
> @@ -219,9 +225,11 @@ enum target_xfer_error
>    /* Keep list in sync with target_xfer_error_to_string.  */
>  };
>  
> +#define TARGET_XFER_STATUS_ERROR_P(STATUS) (STATUS) < TARGET_XFER_DONE

Wrap in ()'s:

  #define TARGET_XFER_STATUS_ERROR_P(STATUS) ((STATUS) < TARGET_XFER_DONE)


>  /* Request that OPS transfer up to LEN 8-bit bytes of the target's
>     OBJECT.  The OFFSET, for a seekable object, specifies the
> @@ -518,10 +527,11 @@ struct target_ops
>         starting point.  The ANNEX can be used to provide additional
>         data-specific information to the target.
>  
> -       Return the number of bytes actually transfered, zero when no
> -       further transfer is possible, and a negative error code (really
> -       an 'enum target_xfer_error' value) when the transfer is not
> -       supported.  Return of a positive value smaller than LEN does
> +       Return the transferred status, error or OK (an
> +       'enum target_xfer_status' value).  Save the number of bytes
> +       actually transferred in *XFERED_LEN if transfer is successful
> +       (TARGET_XFER_OK) or the requested data is unavailable
> +       (TARGET_XFER_E_UNAVAILABLE).  *XFERED_LEN smaller than LEN does

       ...  Save the number of bytes
       actually transferred in *XFERED_LEN if transfer is successful
       (TARGET_XFER_OK) or the number of unavailable bytes if the requested data
       is unavailable (TARGET_XFER_E_UNAVAILABLE).  ...


Otherwise looks good to me.

Thanks a lot for doing this!

-- 
Pedro Alves