diff mbox

[v2,17/19] Fix bit-/byte-offset mismatch in parameter to read_value_memory

Message ID 1494352015-10465-18-git-send-email-arnez@linux.vnet.ibm.com
State New
Headers show

Commit Message

Andreas Arnez May 9, 2017, 5:46 p.m. UTC
The function read_value_memory accepts a parameter embedded_offset and
expects it to represent the byte offset into the given value.  However,
the only invocation with a possibly non-zero embedded_offset happens in
read_pieced_value, where a bit offset is passed instead.

Adjust the implementation of read_value_memory to meet the caller's
expectation.  This implicitly fixes the invocation in read_pieced_value.

gdb/ChangeLog:

	* valops.c (read_value_memory): Change embedded_offset to
	represent a bit offset instead of a byte offset.
	* value.h (read_value_memory): Adjust comment.
---
 gdb/valops.c | 7 ++++---
 gdb/value.h  | 9 ++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

Comments

Simon Marchi May 30, 2017, 7:59 p.m. UTC | #1
On 2017-05-09 19:46, Andreas Arnez wrote:
> @@ -984,8 +984,9 @@ read_value_memory (struct value *val, LONGEST
> embedded_offset,
>        if (status == TARGET_XFER_OK)
>  	/* nothing */;
>        else if (status == TARGET_XFER_UNAVAILABLE)
> -	mark_value_bytes_unavailable (val, embedded_offset + xfered_total,
> -				      xfered_partial);
> +	mark_value_bits_unavailable (val, (xfered_total * HOST_CHAR_BIT
> +					   + bit_offset),
> +				     xfered_partial * HOST_CHAR_BIT);

Since it's readily available, please use the unit_size variable here 
instead of HOST_CHAR_BIT.

LGTM with that change.
Andreas Arnez May 31, 2017, 2:02 p.m. UTC | #2
On Tue, May 30 2017, Simon Marchi wrote:

> On 2017-05-09 19:46, Andreas Arnez wrote:
>> @@ -984,8 +984,9 @@ read_value_memory (struct value *val, LONGEST
>> embedded_offset,
>>        if (status == TARGET_XFER_OK)
>>  	/* nothing */;
>>        else if (status == TARGET_XFER_UNAVAILABLE)
>> -	mark_value_bytes_unavailable (val, embedded_offset + xfered_total,
>> -				      xfered_partial);
>> +	mark_value_bits_unavailable (val, (xfered_total * HOST_CHAR_BIT
>> +					   + bit_offset),
>> +				     xfered_partial * HOST_CHAR_BIT);
>
> Since it's readily available, please use the unit_size variable here
> instead of HOST_CHAR_BIT.

Hm, I thought unit_size represents the number of *bytes* stored at the
same target address.  But we need an offset in *bits* here.  Maybe you
mean that xfered_total should have been multiplied by unit_size even
before my patch?  (Is it even correct that xfered_partial measures
addressable target units?  Note that to_xfer_partial is documented to
"[...] transfer up to LEN 8-bit bytes of the target's OBJECT.")

>
> LGTM with that change.

Thanks!
Simon Marchi May 31, 2017, 2:30 p.m. UTC | #3
On 2017-05-31 16:02, Andreas Arnez wrote:
> On Tue, May 30 2017, Simon Marchi wrote:
> 
>> On 2017-05-09 19:46, Andreas Arnez wrote:
>>> @@ -984,8 +984,9 @@ read_value_memory (struct value *val, LONGEST
>>> embedded_offset,
>>>        if (status == TARGET_XFER_OK)
>>>  	/* nothing */;
>>>        else if (status == TARGET_XFER_UNAVAILABLE)
>>> -	mark_value_bytes_unavailable (val, embedded_offset + xfered_total,
>>> -				      xfered_partial);
>>> +	mark_value_bits_unavailable (val, (xfered_total * HOST_CHAR_BIT
>>> +					   + bit_offset),
>>> +				     xfered_partial * HOST_CHAR_BIT);
>> 
>> Since it's readily available, please use the unit_size variable here
>> instead of HOST_CHAR_BIT.
> 
> Hm, I thought unit_size represents the number of *bytes* stored at the
> same target address.  But we need an offset in *bits* here.  Maybe you
> mean that xfered_total should have been multiplied by unit_size even
> before my patch?

Ah yes sorry, it should be "* unit_size * 8", since the unit size is 
specifically the number of 8-bit units.

And yes, the multiplication with unit_size "should" have been there 
before.  In our GDB port that uses 16-bit bytes, we don't really deal 
with unavailable bytes, so we haven't adapted that particular code.  In 
the end, I don't really mind if you keep "* HOST_CHAR_BIT": if we ever 
need to deal with unavailable data on a non 8-bit bytes platform, we'll 
need to adjust many places, so it doesn't really matter if this one uses 
the unit size or not right now.  You can forget about this comment :).

> (Is it even correct that xfered_partial measures
> addressable target units?  Note that to_xfer_partial is documented to
> "[...] transfer up to LEN 8-bit bytes of the target's OBJECT.")

Yes that's the intent, and that's how it is in our GDB port.  As for the 
documentation of to_xfer_partial, that's probably an oversight.  
target_read and target_write have the right doc.

Simon
diff mbox

Patch

diff --git a/gdb/valops.c b/gdb/valops.c
index 93ae6cf..8675e6c 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -958,7 +958,7 @@  value_at_lazy (struct type *type, CORE_ADDR addr)
 }
 
 void
-read_value_memory (struct value *val, LONGEST embedded_offset,
+read_value_memory (struct value *val, LONGEST bit_offset,
 		   int stack, CORE_ADDR memaddr,
 		   gdb_byte *buffer, size_t length)
 {
@@ -984,8 +984,9 @@  read_value_memory (struct value *val, LONGEST embedded_offset,
       if (status == TARGET_XFER_OK)
 	/* nothing */;
       else if (status == TARGET_XFER_UNAVAILABLE)
-	mark_value_bytes_unavailable (val, embedded_offset + xfered_total,
-				      xfered_partial);
+	mark_value_bits_unavailable (val, (xfered_total * HOST_CHAR_BIT
+					   + bit_offset),
+				     xfered_partial * HOST_CHAR_BIT);
       else if (status == TARGET_XFER_EOF)
 	memory_error (TARGET_XFER_E_IO, memaddr + xfered_total);
       else
diff --git a/gdb/value.h b/gdb/value.h
index a1d1609..fb7f13d 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -581,12 +581,11 @@  extern int value_contents_eq (const struct value *val1, LONGEST offset1,
 
 /* Read LENGTH addressable memory units starting at MEMADDR into BUFFER,
    which is (or will be copied to) VAL's contents buffer offset by
-   EMBEDDED_OFFSET (that is, to &VAL->contents[EMBEDDED_OFFSET]).
-   Marks value contents ranges as unavailable if the corresponding
-   memory is likewise unavailable.  STACK indicates whether the memory
-   is known to be stack memory.  */
+   BIT_OFFSET bits.  Marks value contents ranges as unavailable if
+   the corresponding memory is likewise unavailable.  STACK indicates
+   whether the memory is known to be stack memory.  */
 
-extern void read_value_memory (struct value *val, LONGEST embedded_offset,
+extern void read_value_memory (struct value *val, LONGEST bit_offset,
 			       int stack, CORE_ADDR memaddr,
 			       gdb_byte *buffer, size_t length);