Re: [PATCH 19/43] Add new memory access interface to expr.c

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-03-01 9:45 a.m., Zoran Zaric via Gdb-patches wrote:
> DWARF expression evaluator is currently using a few different
> interfaces for memory access: write_memory_with_notification,
> read_value_memory, read_memory.
> 
> They all seem incosistent, while some of them even need a struct
> value typed argument to be present.
> 
> This patch is simplifying that interface by replacing it with two new
> low level functions: read_from_memory and write_to_memory.
> 
> The advantage of this new interface is that it behaves in the same way
> as the register access interface from the previous patch. Both of these
> have the same error returning policy, which will be usefull for the
> following patches.
> 
> 	* dwarf2/expr.c (xfer_memory):  New function.
> 	(read_from_memory): New function.
> 	(write_to_memory): New function.
> 	(rw_pieced_value): Now calls the read_from_memory and
> 	write_to_memory functions.
> ---
>  gdb/dwarf2/expr.c | 187 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 144 insertions(+), 43 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index 5a1fd5b941f..1abece7d173 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -32,6 +32,8 @@
>  #include "frame.h"
>  #include "gdbsupport/underlying.h"
>  #include "gdbarch.h"
> +#include "inferior.h"
> +#include "observable.h"
>  
>  /* Cookie for gdbarch data.  */
>  
> @@ -196,6 +198,85 @@ write_to_register (struct frame_info *frame, int regnum,
>    return;
>  }
>  
> +/* Helper for read_from_memory and write_to_memory.  */
> +
> +static void
> +xfer_memory (CORE_ADDR address, gdb_byte *readbuf,
> +	     const gdb_byte *writebuf,
> +	     size_t length, bool stack, int *unavailable)

unavailable should probably be bool (end-to-end).

> +{
> +  *unavailable = 0;
> +
> +  enum target_object object
> +    = stack ? TARGET_OBJECT_STACK_MEMORY : TARGET_OBJECT_MEMORY;
> +
> +  ULONGEST xfered_total = 0;
> +
> +  while (xfered_total < length)
> +    {
> +      ULONGEST xfered_partial;
> +
> +      enum target_xfer_status status
> +	= target_xfer_partial (current_top_target (), object, NULL,
> +			       (readbuf != nullptr
> +				? readbuf + xfered_total
> +				: nullptr),
> +			       (writebuf != nullptr
> +				? writebuf + xfered_total
> +				: nullptr),
> +			       address + xfered_total, length - xfered_total,
> +			       &xfered_partial);
> +
> +      if (status == TARGET_XFER_OK)
> +	{
> +	  xfered_total += xfered_partial;
> +	  QUIT;
> +	}
> +      else if (status == TARGET_XFER_UNAVAILABLE)
> +	{
> +	  *unavailable = 1;
> +	  return;
> +	}
> +      else if (status == TARGET_XFER_EOF)
> +	memory_error (TARGET_XFER_E_IO, address + xfered_total);
> +      else
> +	memory_error (status, address + xfered_total);
> +    }
> +}
> +
> +/* Read LENGTH bytes of memory contents starting at ADDRESS.
> +
> +   The data read is copied to a caller-managed buffer BUF.  STACK

BUF -> BUFFER.

> +   indicates whether the memory range specified belongs to a stack
> +   memory region.
> +
> +   If the memory is unavailable, the UNAVAILABLE output is set.  */
> +
> +static void
> +read_from_memory (CORE_ADDR address, gdb_byte *buffer,
> +		  size_t length, bool stack, int *unavailable)
> +{
> +  xfer_memory (address, buffer, nullptr, length, stack, unavailable);
> +}
> +
> +/* Write LENGTH bytes of memory contents starting at ADDRESS.
> +
> +   The data written is copied from a caller-managed buffer buf.  STACK

buf -> BUFFER

> +   indicates whether the memory range specified belongs to a stack
> +   memory region.
> +
> +   If the memory is unavailable, the UNAVAILABLE output is set.  */
> +
> +static void
> +write_to_memory (CORE_ADDR address, const gdb_byte *buffer,
> +		 size_t length, bool stack, int *unavailable)
> +{
> +  xfer_memory (address, nullptr, buffer, length, stack, unavailable);
> +
> +  gdb::observers::memory_changed.notify (current_inferior (), address,
> +					 length, buffer);
> +}
> +
>  struct piece_closure
>  {
>    /* Reference count.  */
> @@ -381,66 +462,86 @@ rw_pieced_value (struct value *v, struct value *from)
>  	    bits_to_skip += p->offset;
>  
>  	    CORE_ADDR start_addr = p->v.mem.addr + bits_to_skip / 8;
> +	    bool in_stack_memory = p->v.mem.in_stack_memory;
> +	    int unavail = 0;
>  
>  	    if (bits_to_skip % 8 == 0 && this_size_bits % 8 == 0
>  		&& offset % 8 == 0)
>  	      {
>  		/* Everything is byte-aligned; no buffer needed.  */
>  		if (from != NULL)
> -		  write_memory_with_notification (start_addr,
> -						  (from_contents
> -						   + offset / 8),
> -						  this_size_bits / 8);
> +		  write_to_memory (start_addr, (from_contents + offset / 8),
> +				   this_size_bits / 8, in_stack_memory,
> +				   &unavail);
>  		else
> -		  read_value_memory (v, offset,
> -				     p->v.mem.in_stack_memory,
> -				     p->v.mem.addr + bits_to_skip / 8,
> -				     v_contents + offset / 8,
> -				     this_size_bits / 8);
> -		break;
> -	      }
> -
> -	    this_size = bits_to_bytes (bits_to_skip, this_size_bits);
> -	    buffer.resize (this_size);
> -
> -	    if (from == NULL)
> -	      {
> -		/* Read mode.  */
> -		read_value_memory (v, offset,
> -				   p->v.mem.in_stack_memory,
> -				   p->v.mem.addr + bits_to_skip / 8,
> -				   buffer.data (), this_size);
> -		copy_bitwise (v_contents, offset,
> -			      buffer.data (), bits_to_skip % 8,
> -			      this_size_bits, bits_big_endian);
> +		  read_from_memory (start_addr, (v_contents + offset / 8),
> +				    this_size_bits / 8, in_stack_memory,
> +				    &unavail);
>  	      }
>  	    else
>  	      {
> -		/* Write mode.  */
> -		if (bits_to_skip % 8 != 0 || this_size_bits % 8 != 0)
> +		this_size = bits_to_bytes (bits_to_skip, this_size_bits);
> +		buffer.resize (this_size);
> +
> +		if (from == NULL)
>  		  {
> -		    if (this_size <= 8)
> +		    /* Read mode.  */
> +		    read_from_memory (start_addr, buffer.data (),
> +				      this_size, in_stack_memory,
> +				      &unavail);
> +		    if (!unavail)
> +		      copy_bitwise (v_contents, offset,
> +				    buffer.data (), bits_to_skip % 8,
> +				    this_size_bits, bits_big_endian);
> +		  }
> +		else
> +		  {
> +		    /* Write mode.  */
> +		    if (bits_to_skip % 8 != 0 || this_size_bits % 8 != 0)
>  		      {
> -			/* Perform a single read for small sizes.  */
> -			read_memory (start_addr, buffer.data (),
> -				     this_size);
> +			if (this_size <= 8)
> +			  {
> +			    /* Perform a single read for small sizes.  */
> +			    read_from_memory (start_addr, buffer.data (),
> +					      this_size, in_stack_memory,
> +					      &unavail);
> +			  }
> +			else
> +			  {
> +			    /* Only the first and last bytes can possibly have
> +			       any bits reused.  */
> +			    read_from_memory (start_addr, buffer.data (),
> +					      1, in_stack_memory,
> +					      &unavail);
> +			    if (!unavail)
> +			      read_from_memory (start_addr + this_size - 1,
> +						&buffer[this_size - 1], 1,
> +						in_stack_memory, &unavail);
> +			  }
>  		      }
> -		    else
> +
> +		    if (!unavail)
>  		      {
> -			/* Only the first and last bytes can possibly have
> -			   any bits reused.  */
> -			read_memory (start_addr, buffer.data (), 1);
> -			read_memory (start_addr + this_size - 1,
> -				     &buffer[this_size - 1], 1);
> +			copy_bitwise (buffer.data (), bits_to_skip % 8,
> +				      from_contents, offset,
> +				      this_size_bits, bits_big_endian);
> +			write_to_memory (start_addr, buffer.data (),
> +					 this_size, in_stack_memory,
> +					 &unavail);
>  		      }
>  		  }
> +	      }
>  
> -		copy_bitwise (buffer.data (), bits_to_skip % 8,
> -			      from_contents, offset,
> -			      this_size_bits, bits_big_endian);
> -		write_memory_with_notification (start_addr,
> -						buffer.data (),
> -						this_size);
> +	    if (unavail)
> +	      {
> +		if (from == NULL)
> +		  mark_value_bits_unavailable (v, (offset + bits_to_skip % 8),
> +					       this_size_bits);
> +		else
> +		  throw_error (NOT_AVAILABLE_ERROR,
> +			       _("Can't do read-modify-write to "
> +				 "update bitfield; containing word "
> +				 "is unavailable"));

Did you get inspiration from existing code for this unavailable byte
error handling?  Were you able to test it?

Simon