From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 3W4ANN0Aq1+YFwAAWB0awg (envelope-from ) for ; Tue, 10 Nov 2020 16:06:37 -0500 Received: by simark.ca (Postfix, from userid 112) id C92241F08B; Tue, 10 Nov 2020 16:06:37 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI,RDNS_NONE autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 5028B1E58E for ; Tue, 10 Nov 2020 16:06:37 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D880C3A4B822; Tue, 10 Nov 2020 21:06:36 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id AD7703A4B81B for ; Tue, 10 Nov 2020 21:06:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AD7703A4B81B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 438A81E58E; Tue, 10 Nov 2020 16:06:33 -0500 (EST) From: Simon Marchi Subject: Re: [PATCH 5/9] Add support for printing value of DWARF-based fixed-point type objects To: Joel Brobecker , GDB patches References: <1604817017-25807-1-git-send-email-brobecker@adacore.com> <1604817017-25807-6-git-send-email-brobecker@adacore.com> Message-ID: <2d12525a-3a85-3f69-bfea-22166f7fd358@simark.ca> Date: Tue, 10 Nov 2020 16:06:32 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <1604817017-25807-6-git-send-email-brobecker@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" [Sorry Joel, I failed to reply-all, so I am sending it again including gdb-patches] I did not do an in-depth review of the functionality, because I don't have that much time right now (and coming from AdaCore I trust that it's quite well tested), so the comments are mostly coding-style related. > This commit introduces a new kind of type, meant to describe > fixed-point types, using a new code added specifically for > this purpose (TYPE_CODE_FIXED_POINT). > > It then adds handling of fixed-point base types in the DWARF reader. > > And finally, as a first step, this commit adds support for printing > the value of fixed-point type objects. > > Note that this commit has a known issue: Trying to print the value > of a fixed-point object with a format letter (e.g. "print /x NAME") > causes the wrong value to be printed because the scaling factor > is not applied. Since the fix for this issue is isolated, and > this is not a regression, the fix will be made in a pach of its own. > This is meant to simplify review and archeology. > > Also, other functionalities related to fixed-point type handling > (ptype, arithmetics, etc), will be added piecemeal as well, for > the same reasons (faciliate reviews and archeology). Related to this, > the testcase gdb.ada/fixed_cmp.exp is adjusted to compile the test > program with -fgnat-encodings=all, so as to force the use of GNAT > encodings, rather than rely on the compiler's default to use them. > The intent is to enhance this testcase to also test the pure DWARF > approach using -fgnat-encodings=minimal as soon as the corresponding > suport gets added in. Thus, the modification to the testcase is made > in a way that it prepares this testcase to be tested in both modes. Since it's an easy change to do, I'd suggest to change NULL for nullptr in all the new code. Also, declare variable on first use when possible. > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index dbf0a3e..1f5152d 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -18130,6 +18130,157 @@ read_typedef (struct die_info *die, struct dwarf2_cu *cu) > return this_type; > } > > +/* Assuming DIE is a rational DW_TAG_constant, read the DIE's > + numerator and denominator into NUMERATOR and DENOMINATOR (resp). > + > + If the numerator and/or numerator attribute is missing, > + a complaint is filed, and NUMERATOR and DENOMINATOR are left > + untouched. */ > + > +static void > +get_dwarf2_rational_constant (struct die_info *die, struct dwarf2_cu *cu, > + LONGEST *numerator, LONGEST *denominator) > +{ > + struct attribute *num_attr, *denom_attr; > + > + num_attr = dwarf2_attr (die, DW_AT_GNU_numerator, cu); > + if (num_attr == NULL) > + complaint (_("DW_AT_GNU_numerator missing in %s DIE at %s"), > + dwarf_tag_name (die->tag), sect_offset_str (die->sect_off)); > + > + denom_attr = dwarf2_attr (die, DW_AT_GNU_denominator, cu); > + if (denom_attr == NULL) > + complaint (_("DW_AT_GNU_denominator missing in %s DIE at %s"), > + dwarf_tag_name (die->tag), sect_offset_str (die->sect_off)); > + > + if (num_attr == NULL || denom_attr == NULL) > + return; > + > + *numerator = num_attr->constant_value (1); > + *denominator = denom_attr->constant_value (1); > +} > + > +/* Same as get_dwarf2_rational_constant, but extracting an unsigned > + rational constant, rather than a signed one. > + > + If the rational constant is has a negative value, a complaint "is has" > + is filed, and NUMERATOR and DENOMINATOR are left untouched. */ > + > +static void > +get_dwarf2_unsigned_rational_constant (struct die_info *die, > + struct dwarf2_cu *cu, > + ULONGEST *numerator, > + ULONGEST *denominator) > +{ > + LONGEST num = 1, denom = 1; > + > + get_dwarf2_rational_constant (die, cu, &num, &denom); > + if (num < 0 && denom < 0) > + { > + num = -num; > + denom = -denom; > + } > + else if (num < 0) > + { > + complaint (_("unexpected negative value for DW_AT_GNU_numerator" > + " in DIE at %s"), > + sect_offset_str (die->sect_off)); > + return; > + } > + else if (denom < 0) > + { > + complaint (_("unexpected negative value for DW_AT_GNU_denominator" > + " in DIE at %s"), > + sect_offset_str (die->sect_off)); > + return; > + } > + > + *numerator = num; > + *denominator = denom; > +} > + > +/* Assiuming DIE corresponds to a fixed point type, finish the creation "Assiuming" > + of the corresponding TYPE by setting its TYPE_FIXED_POINT_INFO. > + CU is the DIE's CU. */ > + > +static void > +finish_fixed_point_type (struct type *type, struct die_info *die, > + struct dwarf2_cu *cu) Unless there's a good reason not to (coming up in the following patches), I would make this function create the "struct type", instead of having the caller create it and pass it. In other words, this signature: struct type *create_fixed_point_type (die_info *die, dwarf2_cu *cu) That just makes it more obvious that it's a simple "die" to "type" transform. > +{ > + struct attribute *attr; > + /* Numerator and denominator of our fixed-point type's scaling factor. > + The default is a scaling factor of 1, which we use as a fallback > + when we are not able to decode it (problem with the debugging info, > + unsupported forms, bug in GDB, etc...). Using that as the default > + allows us to at least print the unscaled value, which might still > + be useful to a user. */ > + ULONGEST scale_num = 1; > + ULONGEST scale_denom = 1; > + > + gdb_assert (type->code () == TYPE_CODE_FIXED_POINT > + && TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_FIXED_POINT); > + > + attr = dwarf2_attr (die, DW_AT_binary_scale, cu); > + if (!attr) > + attr = dwarf2_attr (die, DW_AT_decimal_scale, cu); > + if (!attr) > + attr = dwarf2_attr (die, DW_AT_small, cu); > + > + if (attr == NULL) > + { > + /* Scaling factor not found. Assume a scaling factor of 1, > + and hope for the best. At least the user will be able to see > + the encoded value. */ > + complaint (_("no scale found for fixed-point type (DIE at %s)"), > + sect_offset_str (die->sect_off)); > + } > + else if (attr->name == DW_AT_binary_scale) > + { > + LONGEST scale_exp = attr->constant_value (0); > + ULONGEST *num_or_denom = scale_exp > 0 ? &scale_num : &scale_denom; > + > + *num_or_denom = 1 << abs (scale_exp); > + } > + else if (attr->name == DW_AT_decimal_scale) > + { > + LONGEST scale_exp = attr->constant_value (0); > + ULONGEST *num_or_denom = scale_exp > 0 ? &scale_num : &scale_denom; > + > + *num_or_denom = uinteger_pow (10, abs (scale_exp)); > + } > + else if (attr->name == DW_AT_small) > + { > + struct die_info *scale_die; > + struct dwarf2_cu *scale_cu = cu; > + > + scale_die = follow_die_ref (die, attr, &scale_cu); > + if (scale_die->tag == DW_TAG_constant) > + get_dwarf2_unsigned_rational_constant (scale_die, scale_cu, > + &scale_num, &scale_denom); > + else > + complaint (_("%s DIE not supported as target of DW_AT_small attribute" > + " (DIE at %s)"), > + dwarf_tag_name (die->tag), sect_offset_str (die->sect_off)); > + } > + else > + { > + complaint (("unsupported scale attribute %s for fixed-point type" > + " (DIE at %s)"), Missing _()? Also, the second line has one space too much. > @@ -1821,6 +1845,9 @@ extern void set_type_vptr_basetype (struct type *, struct type *); > (TYPE_CPLUS_SPECIFIC(thistype)->virtual_field_bits == NULL ? 0 \ > : B_TST(TYPE_CPLUS_SPECIFIC(thistype)->virtual_field_bits, (index))) > > +#define TYPE_FIXED_POINT_INFO(thistype) \ > + (TYPE_MAIN_TYPE(thistype)->type_specific.fixed_point_info) Do you think you can make this macro a method on struct type in the style of the ones that were added recently? It will be one less macro to convert later :). That method should contain an assert that the type code is indeed TYPE_CODE_FIXED_POINT. I think it could return a `fixed_point_info &`, since there's no way it can fail and need to return nullptr (now I see that type::bounds should probably return a `range_bounds &` instead of a `range_bounds *`). > + > #define FIELD_NAME(thisfld) ((thisfld).name) > #define FIELD_LOC_KIND(thisfld) ((thisfld).loc_kind) > #define FIELD_BITPOS_LVAL(thisfld) ((thisfld).loc.bitpos) > @@ -2192,6 +2219,8 @@ extern struct type *init_decfloat_type (struct objfile *, int, const char *); > extern struct type *init_complex_type (const char *, struct type *); > extern struct type *init_pointer_type (struct objfile *, int, const char *, > struct type *); > +extern struct type *init_fixed_point_type (struct objfile *, int, int, > + const char *); > > /* Helper functions to construct architecture-owned types. */ > extern struct type *arch_type (struct gdbarch *, enum type_code, int, > @@ -2529,6 +2558,26 @@ extern int type_not_allocated (const struct type *type); > > extern int type_not_associated (const struct type *type); > > +/* Return True if TYPE is a TYPE_CODE_FIXED_POINT or if TYPE is > + range whose base type is a TYPE_CODE_FIXED_POINT. */ > +extern int is_fixed_point_type (struct type *type); int -> bool > + > +/* Assuming that TYPE is a fixed point type, return its base type. > + > + In other words, this returns the type after having peeled all > + intermediate type layers (such as TYPE_CODE_RANGE, for instance). > + The TYPE_CODE of the type returned is guaranteed to be > + a TYPE_CODE_FIXED_POINT. */ > +extern struct type *fixed_point_type_base_type (struct type *type); I think it would make sense to make this a `struct type` method. > + > +/* Given TYPE, which is a fixed point type, return its scaling factor. */ > +extern const gdb_mpq &fixed_point_scaling_factor (struct type *type); I think this would belong in fixed_point_type_info, so you'd access it using: type->fixed_point_info ().scaling_factor () > diff --git a/gdb/testsuite/gdb.ada/fixed_cmp.exp b/gdb/testsuite/gdb.ada/fixed_cmp.exp > index 38e41c4..10e2c9a 100644 > --- a/gdb/testsuite/gdb.ada/fixed_cmp.exp > +++ b/gdb/testsuite/gdb.ada/fixed_cmp.exp > @@ -19,25 +19,29 @@ if { [skip_ada_tests] } { return -1 } > > standard_ada_testfile fixed > > -if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } { > - return -1 > -} > +foreach_with_prefix scenario {all} { I suggest naming this variable gnat-encodings, just because the resulting test name will be a bit clearer. Simon