From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9849 invoked by alias); 21 Feb 2014 09:21:29 -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 9837 invoked by uid 89); 21 Feb 2014 09:21:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 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; Fri, 21 Feb 2014 09:21:27 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 16F06116569; Fri, 21 Feb 2014 04:21:25 -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 ukcHG8PNjAPf; Fri, 21 Feb 2014 04:21:25 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id E5CBC1164F2; Fri, 21 Feb 2014 04:21:24 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9E7DBE0833; Fri, 21 Feb 2014 10:21:23 +0100 (CET) Date: Fri, 21 Feb 2014 09:21:00 -0000 From: Joel Brobecker To: Mark Wielaard Cc: gdb-patches@sourceware.org Subject: Re: [RFA/DWARF] Set enum type "flag_enum" and "unsigned" flags at type creation. Message-ID: <20140221092123.GA4720@adacore.com> References: <1390796357-3739-1-git-send-email-brobecker@adacore.com> <1392820455.21975.235.camel@bordewijk.wildebeest.org> <1392823115.21975.238.camel@bordewijk.wildebeest.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1392823115.21975.238.camel@bordewijk.wildebeest.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-02/txt/msg00684.txt.bz2 Hi Mark, > > This looks suspicious. I think the explicit sign-extension is in general > > wrong. It seems to assume that the DWARF producer encoded the > > DW_AT_upper_bound wrongly (not as a signed value, but as an unsigned > > value that needs to be sign-extended). Something like that might happen > > in theory if the producer used DW_FORM_data[1248] because DWARF doesn't > > define how to encode signed values in that case. But in practice this > > seems to have been settled by interpreting these values as zero-extended > > values (not sign-extended) and by the producer using either > > DW_FORM_sdata or DW_FORM_udata to remove any ambiguity (like in your > > testcase). Thanks again for looking at the patch, and for your comments. Interestingly, I had thought at the time that the only way to express signedness of the underlying value was by using a base type as a subtype (IIRC) which, of course, if a lot of data for one small attribute. The use of the form seems like a much more efficient way to achieve that goal. But the above explains why I interpreted the current code as a way to work around the fact that compilers might not be emitting the required subtype for enums with negative values. Not sure how correct or not that interpretation was... > > Does anything break if you just remove the sign-extension part? > > If not, then you don't have to go through the whole > > update_enumeration_type_from_children. Or do you need that for anything > > else? > > So, this patch doesn't show any regressions in the testsuite: I will verify your fix against my testcase as well, but I like the idea of removing code that should normally not be there. But to answer your question above (do I also need my patch?), I think the need might become less obvious once your patch is in. But I also think it would probably be cleaner to have a complete type right from the get-go, especially since I don't think the patch actually complexifies the code (maybe even the opposite). That being said, I'm not strongly attached to it, as long as GDB does TRT :-). Thanks again for your feedback! > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 54c538a..0b5de99 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -14303,7 +14303,6 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu) > LONGEST low, high; > int low_default_is_valid; > const char *name; > - LONGEST negative_mask; > > orig_base_type = die_type (die, cu); > /* If ORIG_BASE_TYPE is a typedef, it will not be TYPE_UNSIGNED, > @@ -14433,13 +14432,6 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu) > } > } > > - negative_mask = > - (LONGEST) -1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1); > - if (!TYPE_UNSIGNED (base_type) && (low & negative_mask)) > - low |= negative_mask; > - if (!TYPE_UNSIGNED (base_type) && (high & negative_mask)) > - high |= negative_mask; > - > range_type = create_range_type (NULL, orig_base_type, low, high); > > /* Mark arrays with dynamic length at least as an array of unspecified > > > So, my hope is that sign extension hack really isn't needed. > Of course it could be that there is some case where it was really needed > and there just isn't a test case for it. Does anybody know/remember? > > Thanks, > > Mark -- Joel