From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28661 invoked by alias); 3 Dec 2011 18:46:39 -0000 Received: (qmail 28653 invoked by uid 22791); 3 Dec 2011 18:46:38 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-ww0-f43.google.com (HELO mail-ww0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 03 Dec 2011 18:46:24 +0000 Received: by wgbds11 with SMTP id ds11so3154973wgb.12 for ; Sat, 03 Dec 2011 10:46:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.227.207.133 with SMTP id fy5mr2999076wbb.23.1322937981679; Sat, 03 Dec 2011 10:46:21 -0800 (PST) Received: by 10.180.96.72 with HTTP; Sat, 3 Dec 2011 10:46:21 -0800 (PST) In-Reply-To: References: <998639.46560.qm@web112516.mail.gq1.yahoo.com> <321260.58442.qm@web112504.mail.gq1.yahoo.com> <1316327455.23344.YahooMailNeo@web112509.mail.gq1.yahoo.com> <1316404058.27177.YahooMailNeo@web112502.mail.gq1.yahoo.com> <1318650316.91503.YahooMailNeo@web112508.mail.gq1.yahoo.com> Date: Sat, 03 Dec 2011 18:46:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : From: oza Pawandeep To: =?ISO-8859-1?Q?Petr_Hluz=EDn?= Cc: Tom Tromey , paawan oza , "gdb-patches@sourceware.org" , chandra krishnappa Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: 2011-12/txt/msg00079.txt.bz2 Hi Petr, I have updated the patch with your comments; please find the explanation be= low. > In arm_process_record(): > Expression `insn_id =3D bits (arm_record.arm_insn, 11, 15);' uses value > of `arm_record.arm_insn' > which is always zero. When you moved the test from decode_insn() I > guess forgot to copy these lines: > + =A0 =A0 =A0arm_record->arm_insn =3D (uint32_t) extract_unsigned_integer > (&u_buf.buf[0], > + =A0 =A0 =A0 =A0 =A0 THUMB_INSN_SIZE_BYTES, gdbarch_byte_order (arm_reco= rd->gdbarch)); > > Oza : done, corrected, thanks for this comment. > Macro INSN_RECORDED: > I suggested INSN_IS_RECORDED to make it clear it is boolean, Tom > Tromey did not comment on that, Oza left the name as is. Well, it is > not a big deal. > However arguments of macros should be surrounded by parentheses - like th= is: > (ARM_RECORD)->reg_rec_count > This is necessary if INSN_RECORDED() is passed more complex expression > when the order of evaluation matters. This is a common C issue and > customary solution, it is not GDB specific. Oza: have taken care of parenthesis stuff and added; and comment is also added to make it more clear. >> Petr >>In arm_record_strx() >> Petr >>Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syn= tax? >> >> because calling function takes care of REG_ALLOC routine calls. >> I did not want ALLOC calls to be scattered into any other function >> other than main decoding functions. oza: Ahhhh.....I got you now; I have changed, but personally my coding style have never let me think that pointer style is unusual as pointers make the things more clear in my mind. because it always reminds me when i look at the code after a long time that I am playing with address : ) [of course your array syntax is also nothing but address, but probably its just my mind thinks that way) I have changed it as you suggested. > > Have you considered adding the assertions I suggested in [2]? They are > not necessary but they improve understanding of code. Oza: yes I have added assert as you suggested. arm_record_extension_space already returns positive/negative value and decode_insn already catching the return value, and one more point is some insns may not be supported so in that case the code must reach back to process_record and its caller to throw correct record error. PS: next mail will contain the latest patch Regards, Oza. On Sat, Dec 3, 2011 at 10:01 PM, Petr Hluz=EDn wrot= e: > I did not review the 2011-11-19 nor 2011-11-09 patch. > > Tom: >> Yeah, it is a bit of a pain. =A0I wish we had a tool that could reformat >> the code according to our standards. > > I guess 'indent' could do that. Its default settings formats using GNU > guidelines. > Some editors can be configured to automatically format new lines and > have a button to reformat existing lines. I suppose no editor can > autodetect the style from file yet. > > > Review of the 2011-11-03 patch: > > In arm_process_record(): > Expression `insn_id =3D bits (arm_record.arm_insn, 11, 15);' uses value > of `arm_record.arm_insn' > which is always zero. When you moved the test from decode_insn() I > guess forgot to copy these lines: > + =A0 =A0 =A0arm_record->arm_insn =3D (uint32_t) extract_unsigned_integer > (&u_buf.buf[0], > + =A0 =A0 =A0 =A0 =A0 THUMB_INSN_SIZE_BYTES, gdbarch_byte_order (arm_reco= rd->gdbarch)); > > > Remaining from previous reviews: > > Macro INSN_RECORDED: > I suggested INSN_IS_RECORDED to make it clear it is boolean, Tom > Tromey did not comment on that, Oza left the name as is. Well, it is > not a big deal. > However arguments of macros should be surrounded by parentheses - like th= is: > (ARM_RECORD)->reg_rec_count > This is necessary if INSN_RECORDED() is passed more complex expression > when the order of evaluation matters. This is a common C issue and > customary solution, it is not GDB specific. > >> Petr >>In arm_record_strx() >> Petr >>Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syn= tax? >> >> because calling function takes care of REG_ALLOC routine calls. >> I did not want ALLOC calls to be scattered into any other function >> other than main decoding functions. > > The lines can be converted from `*(record_buf_mem + 1)' to > `record_buf_mem[1]' with no changes to REG_ALLOC() calls. The > allocation calls would remain unscattered in main decoding functions. > The code is correct, but I do not understand why you use the unusual > syntax. > > Have you considered adding the assertions I suggested in [2]? They are > not necessary but they improve understanding of code. > > [2] http://sourceware.org/ml/gdb-patches/2011-10/msg00617.html > > -- > Petr Hluzin