From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5519 invoked by alias); 3 Dec 2011 16:32:42 -0000 Received: (qmail 5510 invoked by uid 22791); 3 Dec 2011 16:32:38 -0000 X-SWARE-Spam-Status: No, hits=-2.3 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 16:32:18 +0000 Received: by wgbds11 with SMTP id ds11so2979406wgb.12 for ; Sat, 03 Dec 2011 08:32:17 -0800 (PST) Received: by 10.227.206.4 with SMTP id fs4mr6446785wbb.21.1322929936237; Sat, 03 Dec 2011 08:32:16 -0800 (PST) MIME-Version: 1.0 Received: by 10.223.97.14 with HTTP; Sat, 3 Dec 2011 08:31:55 -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> From: =?UTF-8?B?UGV0ciBIbHV6w61u?= Date: Sat, 03 Dec 2011 16:32:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : To: oza Pawandeep Cc: Tom Tromey , paawan oza , "gdb-patches@sourceware.org" , chandra krishnappa Content-Type: text/plain; charset=UTF-8 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/msg00071.txt.bz2 I did not review the 2011-11-19 nor 2011-11-09 patch. Tom: > Yeah, it is a bit of a pain. I 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 = 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: + arm_record->arm_insn = (uint32_t) extract_unsigned_integer (&u_buf.buf[0], + THUMB_INSN_SIZE_BYTES, gdbarch_byte_order (arm_record->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 this: (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]' syntax? > > 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