From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18920 invoked by alias); 7 Oct 2010 19:25:22 -0000 Received: (qmail 18893 invoked by uid 22791); 7 Oct 2010 19:25:20 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Oct 2010 19:25:14 +0000 Received: (qmail 4077 invoked from network); 7 Oct 2010 19:25:12 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 7 Oct 2010 19:25:12 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA] valprint.c / *-valprint.c: Don't lose `embedded_offset' Date: Thu, 07 Oct 2010 19:25:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.33-29-realtime; KDE/4.4.2; x86_64; ; ) Cc: Tom Tromey , Joel Brobecker References: <201010070207.02695.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201010072025.07762.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2010-10/txt/msg00127.txt.bz2 On Thursday 07 October 2010 19:13:10, Tom Tromey wrote: > >>>>> "Pedro" == Pedro Alves writes: > Pedro> Trouble is in languages other than C/C++ where the > Pedro> advance-embedded_offset-don't-touch-valaddr-or-address contract > Pedro> isn't compromised in many places. > > I think the embedded_offset is very confusing. Every time I have to fix > a bug in it, I have to re-learn what it really means. I used to find it very confusing as well, but I think that with this patch, it became much clearer. In all the changes that I have been doing on top, I never again got confused. :-) > Perhaps next time > I will plan ahead and fix the commentary in value.c to be more clear (to > me at least). I was planning doing that too, along with extending the comment around val_print. :-) > Pedro> All in all, I think it's just better to be consistent throughout. > Pedro> I know I got mighty confused with some functions taking adjusted > Pedro> `valaddr' and `address', while others taking `embedded_offset' > Pedro> into account. Maybe some day we will not allow passing around > Pedro> a NULL `val', and thus we will be able to get rid of `valaddr' > Pedro> and `address' parameters throughout, and instead get those from > Pedro> the value directly. I don't plan to actually do that, but > Pedro> this seems like a step in that direction. > > I was going to do this last time I had to add a parameter to the whole > val_print hierarchy, but then blinked. > > I think we should just get rid of val_print entirely, and only have > value_print, passing around values. If that is not efficient enough > (too much copying or something), we can change struct value to make it > efficient. > > What do you think of that? Should be possible. Actually, I did go one step further, because it occured to me that I might as well add an assertion to val_print that valaddr is in fact always equal to value->contents. See patch below that applies on top of yesterday's. I don't know why that didn't occur to me sooner. :-) This passes regression testing as well. So, the steps I guess would be: - apply yesterday's and this patch. - add an assertion to val_print forbidding a NULL struct value, and fix all callers to make sure to construct a value. Not sure how many there are, might not be that many. I now that "info reg" is one case. - get rid of valaddr and address from all the val_print methods, getting at the contents of the passed in value instead. It's also nice to get rid of the `address' parameter, because not all values actually _have_ a notion of value. Currently, passing around an address is an abstraction violation. - investigate whether passing an offset around is cool, or whether we need something like a new value type that provides a view into another value, and pass that around instead? > I did not read the patch extremely closely. What I did read looks > reasonable. Unfortunately the nature of a patch like this is that it is > very hard to know whether some spot was missed :-( Yeah... -- Pedro Alves 2010-10-07 Pedro Alves * ada-valprint.c (val_print_packed_array_elements): Pass the correct struct value to val_print. (ada_val_print_1): Ditto. * valprint.c (val_print): Add assertions. * value.c (value_contents_for_printing_const): New. * value.h (value_contents_for_printing_const): Declare. --- gdb/ada-valprint.c | 11 ++++++----- gdb/valprint.c | 5 +++++ gdb/value.c | 7 +++++++ gdb/value.h | 2 ++ 4 files changed, 20 insertions(+), 5 deletions(-) Index: src/gdb/ada-valprint.c =================================================================== --- src.orig/gdb/ada-valprint.c 2010-10-07 11:53:48.000000000 +0100 +++ src/gdb/ada-valprint.c 2010-10-07 11:58:08.000000000 +0100 @@ -211,7 +211,7 @@ val_print_packed_array_elements (struct opts.deref_ref = 0; val_print (elttype, value_contents_for_printing (v0), value_embedded_offset (v0), 0, stream, - recurse + 1, val, &opts, current_language); + recurse + 1, v0, &opts, current_language); annotate_elt_rep (i - i0); fprintf_filtered (stream, _(" "), i - i0); annotate_elt_rep_end (); @@ -242,7 +242,7 @@ val_print_packed_array_elements (struct } val_print (elttype, value_contents_for_printing (v0), value_embedded_offset (v0), 0, stream, - recurse + 1, val, &opts, current_language); + recurse + 1, v0, &opts, current_language); annotate_elt (); } } @@ -765,12 +765,13 @@ ada_val_print_1 (struct type *type, cons return ada_val_print_1 (target_type, value_contents_for_printing (v), value_embedded_offset (v), 0, - stream, recurse + 1, NULL, options); + stream, recurse + 1, v, options); } else return ada_val_print_1 (TYPE_TARGET_TYPE (type), valaddr, offset, - address, stream, recurse, original_value, options); + address, stream, recurse, + original_value, options); } else { @@ -909,7 +910,7 @@ ada_val_print_1 (struct type *type, cons value_contents_for_printing (deref_val), value_embedded_offset (deref_val), value_address (deref_val), stream, recurse + 1, - original_value, options, current_language); + deref_val, options, current_language); } else fputs_filtered ("(null)", stream); Index: src/gdb/valprint.c =================================================================== --- src.orig/gdb/valprint.c 2010-10-07 11:53:48.000000000 +0100 +++ src/gdb/valprint.c 2010-10-07 20:04:35.000000000 +0100 @@ -305,6 +305,11 @@ val_print (struct type *type, const gdb_ struct value_print_options local_opts = *options; struct type *real_type = check_typedef (type); + gdb_assert (valaddr != NULL); + gdb_assert (embedded_offset >= 0); + gdb_assert (val == NULL + || (value_contents_for_printing_const (val) == valaddr)); + if (local_opts.pretty == Val_pretty_default) local_opts.pretty = (local_opts.prettyprint_structs ? Val_prettyprint : Val_no_prettyprint); Index: src/gdb/value.c =================================================================== --- src.orig/gdb/value.c 2010-10-07 11:53:48.000000000 +0100 +++ src/gdb/value.c 2010-10-07 20:23:01.000000000 +0100 @@ -434,6 +434,13 @@ value_contents_for_printing (struct valu } const gdb_byte * +value_contents_for_printing_const (const struct value *value) +{ + gdb_assert (!value->lazy); + return value->contents; +} + +const gdb_byte * value_contents_all (struct value *value) { const gdb_byte *result = value_contents_for_printing (value); Index: src/gdb/value.h =================================================================== --- src.orig/gdb/value.h 2010-10-07 11:53:48.000000000 +0100 +++ src/gdb/value.h 2010-10-07 20:22:55.000000000 +0100 @@ -262,6 +262,8 @@ extern const gdb_byte *value_contents_al plan to check the validity manually. */ extern const gdb_byte *value_contents_for_printing (struct value *value); +extern const gdb_byte *value_contents_for_printing_const (const struct value *value); + extern int value_fetch_lazy (struct value *val); extern int value_contents_equal (struct value *val1, struct value *val2);