Re: [PATCH 06/24] gdb: fix bugs in {get,put}_frame_register_bytes

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simon.marchi@polymtl.ca> writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> I found this only by inspection: the myaddr pointer in
> {get,put}_frame_register_bytes is reset to `buffer.data ()` at each
> iteration.  This means that we will always use the bytes at the
> beginning of `buffer` to read or write to the registers, instead of
> progressing in `buffer`.
>
> Fix this by re-writing the functions to chip away the beginning of the
> buffer array_view as we progress in reading or writing the data.
>
> These bugs was introduced almost 3 years ago [1], and yet nobody
> complained.  I'm wondering which architecture relies on that register
> "overflow" behavior (reading or writing multiple consecutive registers
> with one {get,put}_frame_register_bytes calls), and in which situation.
> I find these functions a bit dangerous, if a caller mis-calculates
> things, it could end up silently reading or writing to the next
> register, even if it's not the intent.
>
> If I could change it, I would prefer to have functions specifically made
> for that ({get,put}_frame_register_bytes_consecutive or something like
> that) and make {get,put}_frame_register_bytes only able to write within
> a single register (which I presume represents most of the use cases of
> the current {get,put}_frame_register_bytes).  If a caller mis-calculates
> things and an overflow occurs while calling
> {get,put}_frame_register_bytes, it would hit an assert.  The problem is
> knowing which callers rely on the overflow behavior and which don't.

I agree that this overflow behaviour sucks.

I have a memory of being told (years ago now) that this was a result of
some older compilers not emitting correct DWARF for compound value
locations, instead the compiler would just emit a single register
location, and assume that the debugger would read from consecutive DWARF
registers.  Note, this code assumes that the DWARF register numbering is
the same as GDB's register numbering, which is not always the case.

Personally, I'd love for GDB to be more aggressive about removing some
of this legacy behaviour.  What I'd like to do is move things like this
behind a switch, say 'set maintenance deprecated-features on|off', which
would be off by default, but when it is turned on we'd print a message
saying:

  The feature you turned this on for is deprecated, and will be removed
  from future versions of GDB.  To avoid this feature removed, please
  file a bug report here <url> describing the deprecated feature that
  you are using.

Then if nobody complains after a couple of years, we can start deleting
things.

Anyway... just putting my thoughts down.  I think this patch is fine.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> [1] https://gitlab.com/gnutools/binutils-gdb/-/commit/bdec2917b1e94c7198ba39919f45060067952f43
>
> Change-Id: I43bd4a9f7fa8419d388a2b20bdc57d652688ddf8
> ---
>  gdb/frame.c | 63 +++++++++++++++++++----------------------------------
>  1 file changed, 23 insertions(+), 40 deletions(-)
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index afadb8ac4e73..b3d99163b4dc 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1482,9 +1482,6 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  			  int *optimizedp, int *unavailablep)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
> -  int i;
> -  int maxsize;
> -  int numregs;
>  
>    /* Skip registers wholly inside of OFFSET.  */
>    while (offset >= register_size (gdbarch, regnum))
> @@ -1495,9 +1492,9 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  
>    /* Ensure that we will not read beyond the end of the register file.
>       This can only ever happen if the debug information is bad.  */
> -  maxsize = -offset;
> -  numregs = gdbarch_num_cooked_regs (gdbarch);
> -  for (i = regnum; i < numregs; i++)
> +  int maxsize = -offset;
> +  int numregs = gdbarch_num_cooked_regs (gdbarch);
> +  for (int i = regnum; i < numregs; i++)
>      {
>        int thissize = register_size (gdbarch, i);
>  
> @@ -1506,20 +1503,15 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>        maxsize += thissize;
>      }
>  
> -  int len = buffer.size ();
> -  if (len > maxsize)
> +  if (buffer.size () > maxsize)
>      error (_("Bad debug information detected: "
> -	     "Attempt to read %d bytes from registers."), len);
> +	     "Attempt to read %zu bytes from registers."), buffer.size ());
>  
>    /* Copy the data.  */
> -  while (len > 0)
> +  while (!buffer.empty ())
>      {
> -      int curr_len = register_size (gdbarch, regnum) - offset;
> -
> -      if (curr_len > len)
> -	curr_len = len;
> -
> -      gdb_byte *myaddr = buffer.data ();
> +      int curr_len = std::min<int> (register_size (gdbarch, regnum) - offset,
> +				    buffer.size ());
>  
>        if (curr_len == register_size (gdbarch, regnum))
>  	{
> @@ -1527,8 +1519,8 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  	  CORE_ADDR addr;
>  	  int realnum;
>  
> -	  frame_register (frame, regnum, optimizedp, unavailablep,
> -			  &lval, &addr, &realnum, myaddr);
> +	  frame_register (frame, regnum, optimizedp, unavailablep, &lval,
> +			  &addr, &realnum, buffer.data ());
>  	  if (*optimizedp || *unavailablep)
>  	    return false;
>  	}
> @@ -1547,13 +1539,12 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  	      return false;
>  	    }
>  
> -	  memcpy (myaddr, value->contents_all ().data () + offset,
> -		  curr_len);
> +	  copy (value->contents_all ().slice (offset, curr_len),
> +		buffer.slice (0, curr_len));
>  	  release_value (value);
>  	}
>  
> -      myaddr += curr_len;
> -      len -= curr_len;
> +      buffer = buffer.slice (curr_len);
>        offset = 0;
>        regnum++;
>      }
> @@ -1578,36 +1569,28 @@ put_frame_register_bytes (frame_info_ptr frame, int regnum,
>        regnum++;
>      }
>  
> -  int len = buffer.size ();
>    /* Copy the data.  */
> -  while (len > 0)
> +  while (!buffer.empty ())
>      {
> -      int curr_len = register_size (gdbarch, regnum) - offset;
> +      int curr_len = std::min<int> (register_size (gdbarch, regnum) - offset,
> +				    buffer.size ());
>  
> -      if (curr_len > len)
> -	curr_len = len;
> -
> -      const gdb_byte *myaddr = buffer.data ();
>        if (curr_len == register_size (gdbarch, regnum))
> -	{
> -	  put_frame_register (frame, regnum, myaddr);
> -	}
> +	put_frame_register (frame, regnum, buffer.data ());
>        else
>  	{
> -	  struct value *value
> +	  value *value
>  	    = frame_unwind_register_value (frame_info_ptr (frame->next),
>  					   regnum);
> -	  gdb_assert (value != NULL);
> +	  gdb_assert (value != nullptr);
>  
> -	  memcpy ((char *) value->contents_writeable ().data () + offset,
> -		  myaddr, curr_len);
> -	  put_frame_register (frame, regnum,
> -			      value->contents_raw ().data ());
> +	  copy (buffer.slice (0, curr_len),
> +		value->contents_writeable ().slice (offset, curr_len));
> +	  put_frame_register (frame, regnum, value->contents_raw ().data ());
>  	  release_value (value);
>  	}
>  
> -      myaddr += curr_len;
> -      len -= curr_len;
> +      buffer = buffer.slice (curr_len);
>        offset = 0;
>        regnum++;
>      }
> -- 
> 2.42.1