From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12007 invoked by alias); 13 Jul 2011 03:03:47 -0000 Received: (qmail 11993 invoked by uid 22791); 13 Jul 2011 03:03:46 -0000 X-SWARE-Spam-Status: No, hits=-0.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from nm23-vm0.bullet.mail.sp2.yahoo.com (HELO nm23-vm0.bullet.mail.sp2.yahoo.com) (98.139.91.224) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Wed, 13 Jul 2011 03:03:26 +0000 Received: from [98.139.91.69] by nm23.bullet.mail.sp2.yahoo.com with NNFMP; 13 Jul 2011 03:03:26 -0000 Received: from [98.139.91.8] by tm9.bullet.mail.sp2.yahoo.com with NNFMP; 13 Jul 2011 03:03:26 -0000 Received: from [127.0.0.1] by omp1008.mail.sp2.yahoo.com with NNFMP; 13 Jul 2011 03:03:26 -0000 Received: (qmail 563 invoked by uid 60001); 13 Jul 2011 03:03:25 -0000 Received: from [115.99.18.98] by web112509.mail.gq1.yahoo.com via HTTP; Tue, 12 Jul 2011 20:03:25 PDT References: <1310522630.48264.YahooMailClassic@web36101.mail.mud.yahoo.com> Message-ID: <1310526205.93408.YahooMailRC@web112509.mail.gq1.yahoo.com> Date: Wed, 13 Jul 2011 05:36:00 -0000 From: paawan oza Subject: Re: [PATCH] arm reversible : To: chandra krishnappa , Tom Tromey Cc: gdb-patches@sourceware.org, =?iso-8859-1?Q?Petr_Hluz=EDn?= In-Reply-To: <1310522630.48264.YahooMailClassic@web36101.mail.mud.yahoo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 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-07/txt/msg00335.txt.bz2 I appreciate your effort on this Chandra K; will be sending you updated pat= ch=20 with Tom's comments fixed. thanks for you work and time. Oza. ----- Original Message ---- From: chandra krishnappa To: paawan oza ; Tom Tromey Cc: gdb-patches@sourceware.org; Petr Hluz=EDn Sent: Wed, July 13, 2011 7:33:50 AM Subject: Re: [PATCH] arm reversible : Hi, I am trying to the test if the existing gdb.reverse works with the ARM port= ing=20 done by Oza.. I am a learner, and I am taking more time than a average developer to get=20 documentation, read, understand and complete the testing of ARM porting of= =20 reversible.... 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. 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. I expect > there will be bugs; those can be fixed. >=20 > You need a ChangeLog entry. A patch of this magnitude > should also have > a NEWS entry. >=20 > Some kind of testing would be good. Do the existing > tests in > gdb.reverse work with your port? If so then I think > that is sufficient >=20 > Oza> + unsigned int reg_len =3D 0; reg_len =3D > LENGTH; \ >=20 > Just write unsigned int reg_len =3D LENGTH; >=20 > Oza> + REGS =3D (uint32_t*) > xmalloc (sizeof(uint32_t) * (reg_len)); \ >=20 > Mind the spaces and parens. Better, use XNEWVEC: >=20 > REGS =3D XNEWVEC (uint32_t, reg_len); >=20 > Oza> + while (reg_len) \ > Oza> + { \ > Oza> +=20=20=20=20=20=20=20=20=20=20=20 > REGS[reg_len - 1] =3D RECORD_BUF[reg_len - 1]; \ > Oza> +=20=20=20=20=20=20=20=20=20=20=20 > reg_len--; \ > Oza> + } \ >=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. Usually this is some bad > MUA setting. >=20 > Oza> + uint32_t mem_rec_count;=20=20=20 > /* No of mem recors */ >=20 > Typo, "recors" >=20 > Oza> +/* Checks ARM SBZ and SBO mendatory fields.=20 > */ >=20 > Typo, should be "mandatory". >=20 > Oza> + if(!sbo) >=20 > Spacing. >=20 > Oza> + if ((3 =3D=3D opcode1) && (bit > (arm_insn_r->arm_insn, 4))) >=20 > Over-parenthesizing makes the code harder to read.=20 > Please fix this. I > noticed it in many places. This specific case should > read: >=20 > if (3 =3D=3D opcode1 && bit > (arm_insn_r->arm_insn, 4)) >=20 > Oza> + memset(&u_buf, 0, sizeof (u_buf)); >=20 > Spacing. Just go through the entire patch and fix all > the spacing > issues. >=20 > I feel like I have mentioned this before. >=20 > Oza> + regcache_raw_read_unsigned > (reg_cache, reg_src1 > Oza> +=20=20=20=20=20=20=20=20=20=20=20=20=20 >=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 > , &u_buf[0].unsigned_regval); >=20 > What if this does not return REG_VALID? > There are multiple instances of this. >=20 > Oza> + gdb_assert_not_reached ("no > decoding pattern found"); >=20 > It seems wrong to use an assert in this code. At > least, it is not > obvious to me that this represents a logic error in gdb as > opposed to a > merely unrecognized instruction. An unrecognized > instruction can occur > for many reasons, e.g., a bad jump. >=20 > Oza> + if ((8 =3D=3D > arm_insn_r->opcode) || (10 =3D=3D > arm_insn_r->opcode)=20=20=20=20=20 > Oza> + || (12 =3D=3D > arm_insn_r->opcode) || (14 =3D=3D arm_insn_r->opcode) > Oza> + || (9 =3D=3D > arm_insn_r->opcode) || (11 =3D=3D > arm_insn_r->opcode)=20=20=20=20=20 > Oza> + || (13 =3D=3D > arm_insn_r->opcode) || (15 =3D=3D > arm_insn_r->opcode)=20=20=20=20=20 >=20=20=20=20 > Oza> + || (0 =3D=3D > arm_insn_r->opcode) || (2 =3D=3D arm_insn_r->opcode)=20 >=20=20=20=20 > Oza> + || (4 =3D=3D > arm_insn_r->opcode) || (6 =3D=3D arm_insn_r->opcode) > Oza> + || (1 =3D=3D > arm_insn_r->opcode) || (3 =3D=3D arm_insn_r->opcode) > Oza> + || (5 =3D=3D > arm_insn_r->opcode) || (7 =3D=3D arm_insn_r->opcode)) >=20 > This reads very oddly. Is there a particular reason > behind the ordering > (if so -- document). If not, isn't this: >=20 > if (arm_insn_r->opcode >=3D 0 && > arm_insn_r->opcode <=3D 15) >=20 > There are other odd-looking conditions like this. >=20 > Oza> +=20=20=20=20=20=20=20=20=20=20=20=20=20 > default: > Oza> +=20=20=20=20=20=20=20=20=20=20=20=20=20 > gdb_assert_not_reached ("Invalid addressing mode for > insn"); >=20 > Again, assert seems wrong. >=20 > I'm afraid I ran out of steam here. 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. I want this patch to go > in, but first it > must comply to the usual gdb standards. >=20 > Tom >