From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9272 invoked by alias); 24 Oct 2011 00:13:04 -0000 Received: (qmail 9217 invoked by uid 22791); 24 Oct 2011 00:12:57 -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-bw0-f41.google.com (HELO mail-bw0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 24 Oct 2011 00:12:41 +0000 Received: by bkbzu5 with SMTP id zu5so8537788bkb.0 for ; Sun, 23 Oct 2011 17:12:39 -0700 (PDT) Received: by 10.223.76.201 with SMTP id d9mr39579717fak.12.1319415159096; Sun, 23 Oct 2011 17:12:39 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.89.11 with HTTP; Sun, 23 Oct 2011 17:12:19 -0700 (PDT) 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: Mon, 24 Oct 2011 07:43:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : To: oza Pawandeep Cc: Tom Tromey , "gdb-patches@sourceware.org" , chandra krishnappa Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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-10/txt/msg00618.txt.bz2 Hi Oza, hi folks On 22 October 2011 17:41, oza Pawandeep wrote: > @Chandra: please take this patch and run your tests on this. (it is > integrated with gdb7.3) You probably meant to say the patch should be applied to gdb-7.3. It is not there yet. > + > +#define INSN_RECORDED(ARM_RECORD) \ > + =C2=A0((ARM_RECORD->arm_regs || ARM_RECORD->arm_mems)) Is it supposed to return a boolean-like value? If yes then it would be more descriptive to use INSN_IS_RECORDED. However I do not know if GDB uses this convention. Tom? Also there are extra parentheses. There should be parentheses around ARM_RECORD argument. Consider adding `=3D=3D NULL' to the two tests to make it clear the fields are pointers, not integers or booleans, and that the || is supposed to be logical, not bit-wise. (I have heard few people have tools good enough to tell them the types.) In arm_record_strx: > + =C2=A0 =C2=A0 =C2=A0if (ARM_RECORD_STRH =3D=3D str_type) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(record_buf_mem) =3D 2; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(record_buf_mem + 1) =3D tgt_mem_add= r; Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syntax? > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0arm_insn_r->mem_rec_count =3D 1; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0 =C2=A0else if (ARM_RECORD_STRD =3D=3D str_type) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(record_buf_mem) =3D 4; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(record_buf_mem + 1) =3D tgt_mem_add= r; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(record_buf_mem + 2) =3D 4; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(record_buf_mem + 3) =3D tgt_mem_add= r + 4; The same here and several more places in this function. Search for "*(" In arm_record_extension_space: > + > + =C2=A0opcode1 =3D bits (arm_insn_r->arm_insn, 20, 27); > + =C2=A0opcode1 =3D bits (arm_insn_r->arm_insn, 4, 7); > + =C2=A0if (arm_insn_r->cond) > + =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0/* PLD has no affect on architectural state, it jus= t affects > the caches. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0if (5 =3D=3D ((opcode1 & 0xE0) >> 5)) This condition is never true because `opcode1 =3D bits (arm_insn_r->arm_insn, 4, 7)' is value in range 0..15 therefore `opcode1 & 0xE0' is always zero. I think. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* BLX(1) */ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0record_buf[0] =3D ARM_PS_REGNUM; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0record_buf[1] =3D ARM_LR_REGNUM; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0arm_insn_r->reg_rec_count =3D 2; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0 =C2=A0/* STC2, LDC2, MCR2, MRC2, CDP2: , cprocessor = insn. =C2=A0*/ Typo cprocessor. > + > + =C2=A0opcode1 =3D bits (arm_insn_r->arm_insn, 23, 27); > + =C2=A0if ((24 =3D=3D opcode1) && bit (arm_insn_r->arm_insn, 21) && > + =C2=A0 =C2=A0!(INSN_RECORDED(arm_insn_r))) Operators should go to beginning of a new line, not at the end of previous = line. I.e. if ((24 =3D=3D opcode1) && bit (arm_insn_r->arm_insn, 21) && !(INSN_RECORDED(arm_insn_r))) It is on multiple places involving the macro. > + =C2=A0 =C2=A0 =C2=A0if (!INSN_RECORDED(arm_record)) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0arm_handle_insn[insn_id] (arm_record); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0} > + =C2=A0else if (THUMB_INSN_SIZE_BYTES =3D=3D insn_size) > + =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0/* As thumb does not have condition codes, we set n= egatiive. =C2=A0*/ Typo negatiive. In function arm_record_extension_space: The function stores some values in local variable `uint32_t record_buf[8]' but these values are never read. I suspect you forgot to add calls to: + REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count, record_buf); + MEM_ALLOC (arm_insn_r->arm_mems, arm_insn_r->mem_rec_count, record_buf_m= em); In function arm_record_extension_space: The calls to INSN_RECORDED(arm_insn_r) will always return false, because the fields referenced by the macro are always 0 at entry to arm_record_extension_space() and the function does not modify them. The missing calls to REG_ALLOC() & MEM_ALLOC() would modify them in the end but it would be late anyway. I guess you will have to either add a local flag variable or alter control flow - either way the INSN_RECORDED seems like going to be unused. (Function arm_record_strx() works differently and is not affected.) In function arm_record_extension_space: Consider adding `gdb_assert (!INSN_RECORDED(arm_insn_r))' at the beginning of the function. That way it will be easy to understand which part of code does change the state. In function decode_insn: Consider adding `gdb_assert (!INSN_RECORDED(arm_insn_r))' just before call to arm_record_extension_space. That way it will be easy to understand which part of code does change the state. Or use return value from arm_record_extension_space() - this would make the understanding even faster. In function decode_insn: Consider adding these assertions at the exit: + gdb_assert (arm_record->mem_rec_count =3D=3D 0 || arm_record->arm_mems != =3D NULL); + gdb_assert (arm_record->reg_rec_count =3D=3D 0 || arm_record->arm_regs != =3D NULL); This would catch the missing REG_ALLOC calls in arm_record_extension_space(= ). Issues remaining since the previous revision: Tom Tromey or someone should decide whether to use gdb_assert or not (in the situation described in [1]). Probably turn field `decode' & friends in insn_decode_record_t to local variables. (Phase 3?) [1] http://sourceware.org/ml/gdb-patches/2011-10/msg00449.html --=20 Petr Hluzin