From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12440 invoked by alias); 24 Sep 2008 19:15:45 -0000 Received: (qmail 12338 invoked by uid 22791); 24 Sep 2008 19:15:43 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 24 Sep 2008 19:15:09 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 1CDF62A9696; Wed, 24 Sep 2008 15:15:07 -0400 (EDT) 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 hy5SpQnkG36p; Wed, 24 Sep 2008 15:15:07 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 48A202A9690; Wed, 24 Sep 2008 15:15:06 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 110F5E7ACD; Wed, 24 Sep 2008 12:15:04 -0700 (PDT) Date: Wed, 24 Sep 2008 19:15:00 -0000 From: Joel Brobecker To: Jan Kratochvil Cc: gdb-patches@sourceware.org, Tobias Burnus , Ulrich Weigand , Jim Blandy , jimb@codesourcery.com Subject: Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays] Message-ID: <20080924191504.GA3613@adacore.com> References: <20080818111120.GE16894@adacore.com> <200808181553.m7IFrG3w005270@d12av02.megacenter.de.ibm.com> <48A59B3C.9050801@net-b.de> <20080818111120.GE16894@adacore.com> <20080907115637.GA12939@host0.dyn.jankratochvil.net> <20080919060336.GD3651@adacore.com> <20080922151909.GA12274@host0.dyn.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080922151909.GA12274@host0.dyn.jankratochvil.net> User-Agent: Mutt/1.4.2.2i 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: 2008-09/txt/msg00492.txt.bz2 > Here is the patch. Argh, it took me a long time to break through the patch because I had missed the fact that the upper-bound was indeed set to BOUND_CANNOT_BE_DETERMINED once. I think you were pretty clear about this in your previous message, but my (hopefully then) fried brain didn't register. I'm sorry I missed that somehow, because it changes slightly my point of view. To make it up to you, I will try to help as much as I can. Anyway, so the patch is using the TYPE_FIELD_ARTIFICIAL bit to detect range types where a given bound value cannot be determined. The patch as it is would work, and I wouldn't object to having it checked in as a small step forward (with the agreement from at least one other maintainer). However, I think we can implement this slightly differently to make it more general. struct field has the following two components: /* For a function or member type, this is 1 if the argument is marked artificial. Artificial arguments should not be shown to the user. */ unsigned int artificial : 1; /* This flag is zero for non-static fields, 1 for fields whose location is specified by the label loc.physname, and 2 for fields whose location is specified by loc.physaddr. */ unsigned int static_kind : 2; In practice, these are really what we call "discriminants" in Ada, which means a value that we read in order to know what field to use in the "loc" union. I propose that, instead of re-using the "artificial" flag for undefined bounds, we merge the two components "artificial" and "static_kind" together into a 3-bit field whose value would be an enum that tells us what field to use for our field loc. Because this field would be 3-bit long, it would allow us to have 7 different possible fields, in addition to the case where the bound is undefined or the argument is artificial. That would leave more than enough room to then add a DWARF block as a possible field loc. What does everyone think? Some comments on your patch, that I did review pretty carefully: > 2008-09-22 Jan Kratochvil > > Replace TYPE_ARRAY_{UPPER,LOWER}_BOUND_TYPE by a bit if {un,}defined. > * gdb/c-typeprint.c (c_type_print_varspec_suffix), gdb/m2-typeprint.c > (m2_array), gdb/p-typeprint.c (pascal_type_print_varspec_prefix), > gdb/valops.c (value_cast), gdb/varobj.c (c_number_of_children): Replace > TYPE_ARRAY_UPPER_BOUND_TYPE compared to BOUND_CANNOT_BE_DETERMINED by > TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED. [...] I am not sure about the reason why you listed all the filenames with the gdb/ subdirectory in it, but the gdb/ subdir has its own ChangeLog, so it shouldn't be part of the file name. Perhaps it's because you wanted to make it clear that the changes are in the gdb/ directory. Usually, it's obvious enough that I don't bother. When I make changes to more than one directory such that it will cause more than one ChangeLog to be updated, I sometimes write something like this: gdb/ 2008-09-22 Joel Brobecker <....> [...] gdb/testsuite/ 2008-09-22 Joel Brobecker <...> Most of the time, I don't even bother, as it is clear enough. I do make sure to write each ChangeLog separately as shown above, though. I noticed that you grouped all your changes, including your testsuite changes into one CL entry. > - retcode = f77_get_dynamic_lowerbound (type, &lower_bound); > - > - lower_bound_was_default = 0; > - > - if (retcode == BOUND_FETCH_ERROR) > - fprintf_filtered (stream, "???"); > - else if (lower_bound == 1) /* The default */ > - lower_bound_was_default = 1; > - else > - fprintf_filtered (stream, "%d", lower_bound); > - > - if (lower_bound_was_default) > - lower_bound_was_default = 0; > - else > - fprintf_filtered (stream, ":"); > + lower_bound = f77_get_lowerbound (type); > + if (lower_bound != 1) /* Not the default. */ > + fprintf_filtered (stream, "%d:", lower_bound); Here, it looks like we're slightly modifying the behavior - if the lower bound was undefined, then we're throwing an error when we used not to. I think that would be unfriendly if this issue is caused by bad debug info. Should we check that the lower bound is not undefined first, and print "???" if it is? > @@ -2719,14 +2690,13 @@ recursive_dump_type (struct type *type, > } > puts_filtered ("\n"); > printfi_filtered (spaces, "length %d\n", TYPE_LENGTH (type)); > - printfi_filtered (spaces, "upper_bound_type 0x%x ", > - TYPE_ARRAY_UPPER_BOUND_TYPE (type)); > - print_bound_type (TYPE_ARRAY_UPPER_BOUND_TYPE (type)); > - puts_filtered ("\n"); > - printfi_filtered (spaces, "lower_bound_type 0x%x ", > - TYPE_ARRAY_LOWER_BOUND_TYPE (type)); > - print_bound_type (TYPE_ARRAY_LOWER_BOUND_TYPE (type)); > - puts_filtered ("\n"); > + if (TYPE_CODE (type) == TYPE_CODE_ARRAY) > + { > + printfi_filtered (spaces, "upper bound undefined is %d\n", > + TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED (type)); > + printfi_filtered (spaces, "lower bound undefined is %d\n", > + TYPE_ARRAY_LOWER_BOUND_IS_UNDEFINED (type)); > + } I think this is only repetitive, and shouldn't be displayed. The artificial field value will be displayed a little later, and that should be good enough. No strong objection, though. -- Joel