[PATCH] PR 17520 -- structure offset wrong when 1/4 GB or greater

[Joel Brobecker]

On Thu, Dec 18, 2014 at 08:53:50PM -0500, David Taylor wrote:
> This patch addresses PR 17520 -- structure offset wrong when 1/4 GB or
> greater.
> 
> When calculating the offset of a structure member, GDB did the
> calculation using 32 bit signed integers.  Because GDB uses this
> same machinery for bit fields and for non bit fields, it does the
> offset calcuation first in bits and then, if a byte offset is desired
> it divides by 8.
> 
> This meant that if the offset was 1/4 G bytes or greater, the returned
> value would be wrong.  Such large offsets were possible on 32-bit
> architectures and even more so on 64-bit ones.
> 
> With this change GDB uses 64-bit integers.

FTR, increasing type size of field bitpos here does NOT increase
the overall size of struct main_type, because it's part of a union
where other fields are already a LONGEST. So, memory-allocation-wise,
we should be OK.

> Tested on x86-64 GNU/Linux with no regressions.
> 
>     gdb/ChangeLog:
> 
> 	    * c-lang.h (cp_print_value_fields, cp_print_value_fields_rtti):
> 	    Modify prototypes to match new function definitions.
[...]
> 	    match new definitions.
> 
>     gdb/testsuite/ChangeLog:
> 
> 	    * gdb.base/Makefile.in (EXECUTABLES): Add offsets to the list.
> 	    * gdb.base/offsets.exp: New file.  Test large member offsets.
> 	    * gdb.base/offsets.c: New file.  Used in testing large member
> 	    offsets.

A general comment regarding ChangeLog formatting: when you modify
multiple functions in a given file, and the description of the change
for each function is different, you correctly put the next function's
name between '(' and ')', but you also need to do it on the next line.
Eg, instead of...

        * gnu-v3-abi.c (gnuv3_rtti_type): Change type of arg top_p from
        int * to LONGEST *.  (gnuv3_baseclass_offset): Change type of arg

... write...

        * gnu-v3-abi.c (gnuv3_rtti_type): Change type of arg top_p from
        int * to LONGEST *.
        (gnuv3_baseclass_offset): Change type of arg [...]

Also, do not provide any info about "why" you make a change. If you
feel the need to provide that info, this needs to be in the code
rather than in the ChangeLog. One example (moot for this patch, but
a good example nonetheless):

        * gdbtypes.c (recursive_dump_type): In printfi_filtered call
        change format for bitpos from %d to %lld and cast arg to long long
        (arg is a LONGEST which could be either a long or a long long).

Similarly, for the testsuite/ChangeLog entry, you can just say "New
file.".

But, for your patch in particular, enumerating every single change
that is a direct consequence of the change in struct main_type just
makes for a fairly useless ChangeLog entry because it is nearly
unreadable. For cases like this, we are allowed to simplify a bit
the ChangeLog entry. Perhaps you could use something like:

        * cp-abi.c: Changeg all parameters and variables used as struct
        field offsets from int to LONGEST.
        * cp-abi.h, cp-valprint.c, extension.c, extension.h,
        otherfile.h, otherfile.c, something.h, something.c,
        etc.c, [...]: Likewise.

For those files where you did make more than this mechanical change,
just document them separately. For instance:

**** FIXME ****

>  	printfi_filtered (spaces + 2,
> -			  "[%d] bitpos %d bitsize %d type ",
> -			  idx, TYPE_FIELD_BITPOS (type, idx),
> +			  "[%d] bitpos %lld bitsize %d type ",
> +			  idx, (long long int)TYPE_FIELD_BITPOS (type, idx),

As discussed earlier in another email, please use %s and plongest
instead. (I will skip other instances of this issue, but can you
take care of all other instances, please? - thank you!).


>  static struct type *
> -gnuv2_value_rtti_type (struct value *v, int *full, int *top, int *using_enc)
> +gnuv2_value_rtti_type (struct value *v, int *full, LONGEST *top, int *using_enc)

This line is now too long - can you split it? Make sure that you use
tabs + spaces when doing so, not just spaces (not my choice, but the
current convention at the moment).

> diff --git a/gdb/testsuite/gdb.base/offsets.c b/gdb/testsuite/gdb.base/offsets.c
> new file mode 100644
> index 0000000..bf1e7ad
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/offsets.c
> @@ -0,0 +1,11 @@
> +struct big_struct

Can you add a copyright header to that file, please? All files should
have a copyright header unless there is a compelling reason whe cannot
add it.

> +set test "print &big_struct test"
> +gdb_test_multiple "print &big_struct" "$test" {
> +    -re "\\$\[0-9\]* = .* (0x\[0-9a-fA-F\]*) .*\[\r\n\]*$gdb_prompt $" {
> +	set addr1 $expect_out(1,string)
> +	pass "$test ($addr1)"
> +    }
> +    timeout {
> +	fail "$test (timeout)"
> +    }

The "timeout" block is unnecessary. gdb_test_multiple adds it for you
(among many many other things which are the reasons why we ask people
use use either gdb_test or gdb_test_multiple rather than gdb_expect).

> +set test "print &big_struct.second test"
> +gdb_test_multiple "print &big_struct.second" "$test" {
> +    -re "\\$\[0-9\]* = .* (0x\[0-9a-fA-F\]*) .*\[\r\n\]*$gdb_prompt $" {
> +	set addr2 $expect_out(1,string)
> +	pass "$test ($addr2)"
> +    }
> +    timeout {
> +	fail "$test (timeout)"
> +    }

Same here... but see below...

> +}
> +
> +if {[expr $addr2 - $addr1] == [expr 0x10000000 + 16]} {
> +    pass "big offsets"
> +} else {
> +    fail "big offsets"

You can do a little better: Compute the expected address for addr2,
and make that the expected address in the second test's expected
output.  That way, the second test will fail, as it should, if
the feature regresses.

> --- a/gdb/valarith.c
> +++ b/gdb/valarith.c
> @@ -193,7 +193,7 @@ value_subscripted_rvalue (struct value *array, LONGEST index, int lowerbound)
>    struct type *array_type = check_typedef (value_type (array));
>    struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (array_type));
>    unsigned int elt_size = TYPE_LENGTH (elt_type);
> -  unsigned int elt_offs = elt_size * longest_to_int (index - lowerbound);
> +  ULONGEST elt_offs = elt_size * longest_to_int (index - lowerbound);

I think you may have an issue, here, but I might be wrong, as my C foo
is not that good. elt_size is declared as an int, and longest_to_int
also returns an int, so I think the multiplication might be done as
int, and only then is the promotion to ULONGEST performed, I think.
To me, it'd be better to declare elt_size as ULONGEST, and then remove
the call to longest_to_int.

> -      int offset = baseclass_offset (search_type, i, valaddr, embedded_offset,
> -				     address, val);
> +      LONGEST offset = baseclass_offset (search_type, i, valaddr, embedded_offset,
> +					 address, val);

This line is now too long. A number of other functions you changed
in this file have the same problem. Can you fix those as well?

>  int
> -value_bits_available (const struct value *value, int offset, int length)
> +value_bits_available (const struct value *value, LONGEST offset, LONGEST length)

Ditto.

>  int
> -value_bytes_available (const struct value *value, int offset, int length)
> +value_bytes_available (const struct value *value, LONGEST offset, LONGEST length)

Ditto.

>  void
> -mark_value_bits_unavailable (struct value *value, int offset, int length)
> +mark_value_bits_unavailable (struct value *value, LONGEST offset, LONGEST length)

Likewise.

>  void
> -mark_value_bytes_unavailable (struct value *value, int offset, int length)
> +mark_value_bytes_unavailable (struct value *value, LONGEST offset, LONGEST length)

Ditto.

I hope I didn't miss any other case like that, can you double-check me?

>  void
> -set_internalvar_component (struct internalvar *var, int offset, int bitpos,
> -			   int bitsize, struct value *newval)
> +set_internalvar_component (struct internalvar *var, LONGEST offset, LONGEST bitpos,
> +			   LONGEST bitsize, struct value *newval)

One more, different file...

> @@ -3325,7 +3325,7 @@ modify_field (struct type *type, gdb_byte *addr,
>      {
>        /* FIXME: would like to include fieldval in the message, but
>           we don't have a sprintf_longest.  */
> -      warning (_("Value does not fit in %d bits."), bitsize);
> +      warning (_("Value does not fit in %lld bits."), (long long int)bitsize);

Same as before, use %s and plongest...

>  extern void unpack_value_bitfield (struct value *dest_val,
>  				   int bitpos, int bitsize,
> -				   const gdb_byte *valaddr, int embedded_offset,
> +				   const gdb_byte *valaddr, LONGEST embedded_offset,
>  				   const struct value *val);

Line too long...

That's it! I expect the next iteration of the patch will get approved!
-- 
Joel