This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Joel Brobecker <brobecker@adacore.com> wrote:

> On Thu, Dec 18, 2014 at 08:53:50PM -0500, David Taylor wrote:

> >     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 [...]

Okay.

> 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.

I've used ditto / likewise in the past, but only when saying,
effectively, what was done with this function is the same as what was
done with that previous function, never at the file level.  In fact, I
thought that listing all the affected functions was a hard requirement.
I also, and I wrote it, found the ChangeLog to be pretty much
unreadable.

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

I think that 99% or close to it of the changes fall into one of the
variants of:

    Change { function parameter | local variable } <foo> from
    { int to LONGEST | unsigned int to ULONGEST | int * to LONGEST * }

The two exceptions that come to mind (without scanning the diff for
verification) are:

    . the printing related changes
    . the testsuite related changes

> >       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!).

Okay, will do.

> >  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).

I think I've caught all of these.  Should be fixed in the next posting.

> > 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.

Done.

The file is short enough that I'm not sure that it really is
copyrightable -- but, the lawyers can argue that one, I don't care.  I
noticed that most of the testsuite *.c files, at least in gdb.base do
not have copyright notices.

> > +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).

I found an example that did something similar to what I wanted and
copied it.  It had the timeout within the gdb_test_multiple, so that is
what I did, too.

I try to do as little as possible in Tcl as every time I do anything
significant, by the time I'm done I'm remembering just how much I hate
writing Tcl code.

I'll modify it to remove the timeouts.

> > +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.

I'll look into it.

> > --- 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.

Will do.

> > -      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?

I think I've caught them all.

[...]

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

Will do.

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

Good to hear.  Thanks for taking the time to review it.  I expect to
post an updated version later this week, possibly later today, but I
won't promise the latter.

> --
> Joel

David


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]