From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16095 invoked by alias); 30 Dec 2008 20:01:11 -0000 Received: (qmail 15416 invoked by uid 22791); 30 Dec 2008 20:01:09 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,J_CHICKENPOX_64,KAM_MX,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 30 Dec 2008 20:00:34 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id mBUK0Uu1016620; Tue, 30 Dec 2008 15:00:30 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id mBUK0TdJ002982; Tue, 30 Dec 2008 15:00:30 -0500 Received: from opsy.redhat.com (vpn-12-37.rdu.redhat.com [10.11.12.37]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id mBUK0SRw028501; Tue, 30 Dec 2008 15:00:29 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 37983C880EC; Tue, 30 Dec 2008 13:00:28 -0700 (MST) To: "Sean D'Epagnier" Cc: gdb-patches@sourceware.org Subject: Re: [patch] add support for debugging fixed-point numbers References: From: Tom Tromey Reply-To: tromey@redhat.com Date: Tue, 30 Dec 2008 20:01:00 -0000 In-Reply-To: (Sean D'Epagnier's message of "Mon\, 29 Dec 2008 20\:40\:23 -0500") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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-12/txt/msg00463.txt.bz2 >>>>> "Sean" == Sean D'Epagnier writes: Sean> Now I am able to debug applications with fixed point variables, and Sean> all the usual features work, but I had to patch gcc also to provide Sean> the right info in the dwarf output so that gdb knows where the decimal Sean> place is (patch also attached) Thanks for the patch. Sean> Let me know if there is anything else I need to do to get this applied. A patch this size will require a copyright assignment to the FSF. One of the gdb maintainers should be able to send you the paperwork to get you started. I read through the patch. There are a few things I did not understand, comments below. Also, there are some formatting nits. In addition, patches need a ChangeLog entry (you'll want this for GCC as well). The GNU coding standards describe how to write one. Some test cases would be nice. Looking at GCC's extend.texi, I see there is new syntax to write fixed-point literal constants, and some new keywords. It would be nice to have support for these in gdb as well -- but IMO, this is not a requirement for this patch and if you want you can just file it as a feature request in bugzilla. Sean> + case TYPE_CODE_FIXED: Sean> + /* convert to double and print that */ Style nit: GNU-style comments are full sentences, so start with a capital and end with a period and two spaces. There are a few instances of this. Sean> + if (code == TYPE_CODE_FIXED) { Braces go on their own lines in the GNU style. There are a few instances of this. Sean> + /* First extension operator. Individual language modules define Sean> + extra operators they need as constants with values Sean> + OP_LANGUAGE_SPECIFIC0 + k, for k >= 0, using a separate Sean> + enumerated type definition: Sean> + enum foo_extension_operator { Sean> + BINOP_MOGRIFY = OP_EXTENDED0, Sean> + BINOP_FROB, Sean> + ... Sean> + }; */ Sean> OP_EXTENDED0, It appears that this comment was inadvertently reformatted. Sean> + /* convert fixed to double */ Sean> + if(TYPE_CODE (type1) == TYPE_CODE_FIXED) Sean> + arg1 = value_cast(builtin_type_ieee_double, arg1); Sean> + if(TYPE_CODE (type2) == TYPE_CODE_FIXED) Sean> + arg2 = value_cast(builtin_type_ieee_double, arg2); The GNU style puts spaces before parens. There are a few instances of this. Sean> + else if (TYPE_CODE (type) == TYPE_CODE_FIXED) Sean> + { Sean> + LONGEST unshiftedval = -extract_signed_integer (value_contents (arg1), Sean> + TYPE_LENGTH (type)); Sean> + return value_from_fixed (type, (gdb_byte*)&unshiftedval); I don't understand something here. >From the expression.h change: Sean> + gdb_byte fixedconst[16]; So, I assume that at least in some case, a fixed-point value will require 16 bytes. However, I don't think LONGEST is guaranteed to be that big. It may be as small as 'long'. Basically I'm concerned about the case where sizeof(LONGEST) and TYPE_LENGTH(type) are not the same, and I'm confused about the choice of '16' as the size of the buffer. Sean> + if(ieee_double) { Sean> + LONGEST lvalue; Sean> + DOUBLEST value = value_as_double (ieee_double); [...] Sean> + lvalue = value; Sean> + Sean> + return value_from_fixed (type, (gdb_byte*)&lvalue); The "lvalue = value" assignment looks fishy to me. This will do ordinary C double->integral conversion; is that what you intend? Sean> + error (_("Failed to convert to fixed-point.")); Sean> + return 0; This return is redundant; error does not return. Sean> struct value * Sean> value_one (struct type *type, enum lval_type lv) Sean> { Sean> - struct type *type1 = check_typedef (type); Sean> + CHECK_TYPEDEF(type); Sean> struct value *val = NULL; /* avoid -Wall warning */ Unless I'm misreading the patch, this looks like a declaration after a statement. The project's coding style doesn't allow that; I thought the default compiler settings for gdb would error about this, but I am not certain. Sean> - *invp = 0; /* Assume valid. */ Sean> + if(invp) Sean> + *invp = 0; Please update the function's introductory comment to explain that INVP may now be NULL. I am not really that familiar with the fixed-point extension to C. My understanding is that some of the types saturate -- but I didn't see any code here related to saturation. Am I missing something? The reason I ask about this is that a goal of mine is to have gdb's C expression evaluator be faithful to the C programming language. IMO, some extensions are ok, but giving different results in defined cases is not. Given this, I think the valarith.c change is probably not correct. Instead it ought to implement the rules defined by the language extension. I don't have this document but I assume that converting to floating point is not the way to go. Tom