From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10093 invoked by alias); 31 Dec 2008 20:46:26 -0000 Received: (qmail 10084 invoked by uid 22791); 31 Dec 2008 20:46:25 -0000 X-SWARE-Spam-Status: No, hits=-0.3 required=5.0 tests=AWL,BAYES_05,J_CHICKENPOX_64,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from rn-out-0910.google.com (HELO rn-out-0910.google.com) (64.233.170.185) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 31 Dec 2008 20:45:48 +0000 Received: by rn-out-0910.google.com with SMTP id k40so3916982rnd.0 for ; Wed, 31 Dec 2008 12:45:44 -0800 (PST) Received: by 10.150.135.2 with SMTP id i2mr30978161ybd.229.1230756344375; Wed, 31 Dec 2008 12:45:44 -0800 (PST) Received: by 10.150.226.17 with HTTP; Wed, 31 Dec 2008 12:45:44 -0800 (PST) Message-ID: Date: Wed, 31 Dec 2008 20:46:00 -0000 From: "Sean D'Epagnier" To: tromey@redhat.com Subject: Re: [patch] add support for debugging fixed-point numbers Cc: gdb-patches@sourceware.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: 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: 2008-12/txt/msg00475.txt.bz2 Tom, Thanks so much for reviewing my patch. I am sorry for all the inconsistencies, I will do my best to correct them, more comments below. On 12/30/08, Tom Tromey wrote: >>>>>> "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 would be glad to fill anything out needed for copyright assignment. > 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. > Yes, I thought about adding this support, but I decided to wait until a later patch since it would require a bunch more changes. > 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. Ok, I will triple check everything for formatting issues. > > 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. > Ok, I will undo it. > 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'. > The largest fixed point type I know of is "long long _Accum" which is by default 16 bytes, however targets are free to redefine it to be larger (none have done this yet) It is a good point you made, LONGEST might not be large enough in some of the cases I have used it. If I don't convert to double anymore, then hopefully this will be corrected. > 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? > Yes, that is what I intended. > Sean> + error (_("Failed to convert to fixed-point.")); > Sean> + return 0; > > This return is redundant; error does not return. > Ok, I will take note of that. > 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. > My mistake, I believe it is only a warning which is why I missed it. > 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. > Ok. > > 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? > If the type is _Sat, then it is a saturating type. I did not have any support for this, however there is currently no way for me to know if a variable is a saturating type (nothing in the dwarf format specifying it) So gdb might be inconsistent in the sense that if you add a number to a saturating variable ie: "p x+.5" and x saturates, then gdb won't know to saturate it, but unless we add more fields to dwarf specifying this. What do you suggest we do about this? At least you can examine saturating values correctly. > 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. > Ok, I was hoping to avoid this since it is so much simpler to just convert to double to do all the operations. I will have to implement operations using only integer math and shifts to make it all work correctly. I should do this in the updated patch. I also noticed what seems to be a quirk in gdb. Maybe you have some insight. If you launch gdb and type something like: (gdb) p ((unsigned _Accum)- 1) $7 = -1 (gdb) p ((unsigned int)- 1) $8 = 65535 It gets it incorrectly for _Accum, and unsigned accum should not be negative. This is because gdb gets the definition for the type from the stabs data, not the dwarf data in this case (casting) I'm not sure why it does this.. but currently gcc outputs stabs data for fixed-point which gdb parses as a float which is wrong, for example: p ((_Fract) .1) Unrecognized 16-bit floating-point type. I added support for the decimal location in dwarf output, but not stabs, because I'm not sure how to do it correctly. I'm not sure which is best, to try to add fixed-point support to stabs format too, or to make gdb read the types from the dwarf format in this case (or maybe both) I will try to get you an updated patch soon. Thanks, Sean > Tom >