From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12759 invoked by alias); 20 Dec 2014 18:27:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 12745 invoked by uid 89); 20 Dec 2014 18:27:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sat, 20 Dec 2014 18:27:28 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id CC1B61163FF; Sat, 20 Dec 2014 13:27:26 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id xFjbxFTt-Epo; Sat, 20 Dec 2014 13:27:26 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id B940E1163FE; Sat, 20 Dec 2014 13:27:25 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9227640164; Sat, 20 Dec 2014 13:27:25 -0500 (EST) Date: Sat, 20 Dec 2014 18:27:00 -0000 From: Joel Brobecker To: David Taylor Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] PR 17520 -- structure offset wrong when 1/4 GB or greater Message-ID: <20141220182725.GI12884@adacore.com> References: <1418954030-16797-1-git-send-email-dtaylor@emc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1418954030-16797-1-git-send-email-dtaylor@emc.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-12/txt/msg00592.txt.bz2 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