From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24909 invoked by alias); 13 Jul 2011 02:04:08 -0000 Received: (qmail 24901 invoked by uid 22791); 13 Jul 2011 02:04:06 -0000 X-SWARE-Spam-Status: No, hits=0.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from nm6-vm0.bullet.mail.ne1.yahoo.com (HELO nm6-vm0.bullet.mail.ne1.yahoo.com) (98.138.91.54) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Wed, 13 Jul 2011 02:03:52 +0000 Received: from [98.138.90.56] by nm6.bullet.mail.ne1.yahoo.com with NNFMP; 13 Jul 2011 02:03:51 -0000 Received: from [98.138.88.237] by tm9.bullet.mail.ne1.yahoo.com with NNFMP; 13 Jul 2011 02:03:51 -0000 Received: from [127.0.0.1] by omp1037.mail.ne1.yahoo.com with NNFMP; 13 Jul 2011 02:03:51 -0000 Received: (qmail 91435 invoked by uid 60001); 13 Jul 2011 02:03:51 -0000 Received: from [1.23.232.189] by web36101.mail.mud.yahoo.com via HTTP; Tue, 12 Jul 2011 19:03:50 PDT Message-ID: <1310522630.48264.YahooMailClassic@web36101.mail.mud.yahoo.com> Date: Wed, 13 Jul 2011 03:17:00 -0000 From: chandra krishnappa Subject: Re: [PATCH] arm reversible : To: paawan oza , Tom Tromey Cc: gdb-patches@sourceware.org, =?iso-8859-1?Q?Petr_Hluz=EDn?= In-Reply-To: MIME-Version: 1.0 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-07/txt/msg00334.txt.bz2 Hi, I am trying to the test if the existing gdb.reverse works with the ARM port= ing done by Oza.. I am a learner, and I am taking more time than a average developer to get d= ocumentation, read, understand and complete the testing of ARM porting of r= eversible.... Thanks & Regards, -Chandra K --- On Wed, 7/13/11, Tom Tromey wrote: > From: Tom Tromey > Subject: Re: [PATCH] arm reversible : > To: "paawan oza" > Cc: gdb-patches@sourceware.org, "Petr Hluz=EDn" > Date: Wednesday, July 13, 2011, 2:29 AM > >>>>> "Oza" =3D=3D paawan > oza > writes: >=20 > Oza> any more comments are welcome make this patch ok, > if ARM person can > Oza> have a look at it it would be great. >=20 > You have submitted this patch many times now and nobody has > commented > on the details of the ARM decoding.=A0 I think we should > proceed on the > theory that this is simply not going to happen. >=20 > Also, I am not as concerned about the correctness of every > detail as I > am about the general maintainability and style of the > code.=A0 I expect > there will be bugs; those can be fixed. >=20 > You need a ChangeLog entry.=A0 A patch of this magnitude > should also have > a NEWS entry. >=20 > Some kind of testing would be good.=A0 Do the existing > tests in > gdb.reverse work with your port?=A0 If so then I think > that is sufficient >=20 > Oza> +=A0 =A0 unsigned int reg_len =3D 0; reg_len =3D > LENGTH; \ >=20 > Just write=A0=A0=A0unsigned int reg_len =3D LENGTH; >=20 > Oza> +=A0 =A0 =A0 =A0 REGS =3D (uint32_t*) > xmalloc (sizeof(uint32_t) * (reg_len)); \ >=20 > Mind the spaces and parens.=A0 Better, use XNEWVEC: >=20 > =A0 =A0 REGS =3D XNEWVEC (uint32_t, reg_len); >=20 > Oza> +=A0 =A0 =A0 =A0 while (reg_len) \ > Oza> +=A0 =A0 =A0 =A0 =A0 { \ > Oza> +=A0 =A0 =A0 =A0 =A0 =A0 > REGS[reg_len - 1] =3D RECORD_BUF[reg_len - 1];=A0 \ > Oza> +=A0 =A0 =A0 =A0 =A0 =A0 > reg_len--;=A0 \ > Oza> +=A0 =A0 =A0 =A0 =A0 } \ >=20 > Just use memcpy. >=20 > Oza> +#define MEM_ALLOC(MEMS,LENGTH,RECORD_BUF) \ >=20 > The same comments apply for this macro. >=20 > Oza> +/* ARM instruction record contains opcode of > current insn and execution state=20 > Oza> (before entry to=20 >=20 > Oza> +decode_insn() ), contains list of to-be-modified > registers and memory blocks=20 > Oza> (on return from=20 >=20 > Your email got corrupted.=A0 Usually this is some bad > MUA setting. >=20 > Oza> +=A0 uint32_t mem_rec_count;=A0 =A0 > =A0=A0=A0/* No of mem recors */ >=20 > Typo, "recors" >=20 > Oza> +/* Checks ARM SBZ and SBO mendatory fields.=A0 > */ >=20 > Typo, should be "mandatory". >=20 > Oza> +=A0 if(!sbo) >=20 > Spacing. >=20 > Oza> +=A0 if ((3 =3D=3D opcode1) && (bit > (arm_insn_r->arm_insn, 4))) >=20 > Over-parenthesizing makes the code harder to read.=A0 > Please fix this.=A0 I > noticed it in many places.=A0 This specific case should > read: >=20 > =A0 if (3 =3D=3D opcode1 && bit > (arm_insn_r->arm_insn, 4)) >=20 > Oza> +=A0 memset(&u_buf, 0, sizeof (u_buf)); >=20 > Spacing.=A0 Just go through the entire patch and fix all > the spacing > issues. >=20 > I feel like I have mentioned this before. >=20 > Oza> +=A0 =A0 =A0 regcache_raw_read_unsigned > (reg_cache, reg_src1 > Oza> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 > =A0 =A0 , &u_buf[0].unsigned_regval); >=20 > What if this does not return REG_VALID? > There are multiple instances of this. >=20 > Oza> +=A0 =A0 =A0 gdb_assert_not_reached ("no > decoding pattern found"); >=20 > It seems wrong to use an assert in this code.=A0 At > least, it is not > obvious to me that this represents a logic error in gdb as > opposed to a > merely unrecognized instruction.=A0 An unrecognized > instruction can occur > for many reasons, e.g., a bad jump. >=20 > Oza> +=A0 =A0 =A0 if ((8 =3D=3D > arm_insn_r->opcode) || (10 =3D=3D > arm_insn_r->opcode)=A0 =A0=A0=A0 > Oza> +=A0 =A0 =A0 || (12 =3D=3D > arm_insn_r->opcode) || (14 =3D=3D arm_insn_r->opcode) > Oza> +=A0 =A0 =A0 || (9 =3D=3D > arm_insn_r->opcode) || (11 =3D=3D > arm_insn_r->opcode)=A0 =A0=A0=A0 > Oza> +=A0 =A0 =A0 || (13 =3D=3D > arm_insn_r->opcode) || (15 =3D=3D > arm_insn_r->opcode)=A0 =A0 =A0 > =A0=A0=A0 > Oza> +=A0 =A0 =A0 || (0 =3D=3D > arm_insn_r->opcode) || (2 =3D=3D arm_insn_r->opcode)=A0 > =A0=A0=A0 > Oza> +=A0 =A0 =A0 || (4 =3D=3D > arm_insn_r->opcode) || (6 =3D=3D arm_insn_r->opcode) > Oza> +=A0 =A0 =A0 || (1 =3D=3D > arm_insn_r->opcode) || (3 =3D=3D arm_insn_r->opcode) > Oza> +=A0 =A0 =A0 || (5 =3D=3D > arm_insn_r->opcode) || (7 =3D=3D arm_insn_r->opcode)) >=20 > This reads very oddly.=A0 Is there a particular reason > behind the ordering > (if so -- document).=A0 If not, isn't this: >=20 > =A0 if (arm_insn_r->opcode >=3D 0 && > arm_insn_r->opcode <=3D 15) >=20 > There are other odd-looking conditions like this. >=20 > Oza> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 > default: > Oza> +=A0 =A0 =A0 =A0 =A0 =A0 =A0 > =A0 gdb_assert_not_reached ("Invalid addressing mode for > insn"); >=20 > Again, assert seems wrong. >=20 > I'm afraid I ran out of steam here.=A0 Please fix all > the issues already > noted and look at the rest of the patch with a critical eye > to see what > else should be cleaned up.=A0 I want this patch to go > in, but first it > must comply to the usual gdb standards. >=20 > Tom >