From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9781 invoked by alias); 19 Nov 2011 09:43:51 -0000 Received: (qmail 9773 invoked by uid 22791); 19 Nov 2011 09:43:50 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_EG 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, 19 Nov 2011 09:43:34 +0000 Received: by wwp14 with SMTP id 14so5280296wwp.12 for ; Sat, 19 Nov 2011 01:43:33 -0800 (PST) MIME-Version: 1.0 Received: by 10.216.72.145 with SMTP id t17mr934906wed.88.1321695813273; Sat, 19 Nov 2011 01:43:33 -0800 (PST) Received: by 10.180.96.72 with HTTP; Sat, 19 Nov 2011 01:43:33 -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, 19 Nov 2011 09:43:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : From: oza Pawandeep To: Tom Tromey Cc: =?ISO-8859-1?Q?Petr_Hluz=EDn?= , 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-11/txt/msg00531.txt.bz2 Hi Tom, I have fixed most of your comments. as far as formatting is concerned, I am still trying to hard to get it right. but at the end mailer ends up messing it up :( following are the comments. We try not to put FIXME comments into new code. Nobody ever fixes them. You can just write an informative comment instead: "We have no way to read the SPSR value". Assuming that is accurate. Oza >> removed FIX ME comments and put appropriate comments. The enclosing function, arm_record_extension_space, takes care to return a value, but (1) the meaning of the return value is not documented in the function's introductory comment (this problems affects many of the new functions), and (2) the only caller does not check it. oza >> thanks for pointing that out, I have added descirption and caler must be checking return value which I have corrected. What happens if the user hits an instruction which is not handled? Right now a message is printed. But shouldn't it abort the operation in some other way? That is, is emitting a message really sufficient? Oza >> I have chaged the description a bit, the abort message is printed, but at the same time return value is -1, which will cause gdb standard stack unwinding, with error(...) symentics. Formatting looks weird. A typedef for the function type would probably make this look less strange. oza >> converted to typedef defination, it looks simpler now. No need for this backslash. Oza >> removed. oza> + if (arm_record.arm_regs) oza> + xfree (arm_record.arm_regs); oza> + if (arm_record.arm_mems) oza> + xfree (arm_record.arm_mems); This is fine as long as you have proved that nothing in any possible call path here can throw an exception. If anything throws an exception, then you are leaking memory and should instead use a cleanup. Oza >> I have added a new function deallocate_reg_arm.... the design is in such a way, that any function goes for allocation until and unless it supports the insn; otherwise memory is always local/cache, so in case of exception it simply unwinds wihtout affecting dymanic memory. and in case we have allocated memory, we make sure that we come to the end of the process_record funntion and we free them irrespective of success or failure; oza> + /* Parse swi insn args, sycall record. */ oza> + int (*arm_swi_record) (struct regcache *regcache); Since this is only used in your Phase 3 patch, it belongs there. Oza >> In my thinking, it was necessary to add this function pointer because I had already added logic of decoding insn in phase 2, where I am just checking if this fp is NULL. where this fp was initialised to NULL, which will be replaced with valid fp in phase 3. wihtout including this, phase 2 would have missed on insn decoding. Regards, Oza. On Fri, Nov 18, 2011 at 2:10 AM, Tom Tromey wrote: >>>>>> "oza" =3D=3D oza Pawandeep writes: > > oza> please find the latest patch below. > > No ChangeLog entry. > > No NEWS entry. > > Patch got mangled by your mailer again. > > oza> + =A0 =A0 =A0 =A0memcpy(®S[0],&RECORD_BUF[0],sizeof(uint32_t)*LEN= GTH); \ > > Wrong spacing. > > oza> + =A0 =A0 =A0 =A0memcpy(&MEMS->len,&RECORD_BUF[0],sizeof(struct arm_= mem_r) * LENGTH); \ > > Likewise. > > oza> +}arm_record_strx_t; > > Spacing. > > oza> +}record_type_t; > > Again. > > oza> + > oza> +static int > oza> +arm_record_strx (insn_decode_record *arm_insn_r, uint32_t *record_b= uf, > oza> + =A0 =A0uint32_t *record_buf_mem, arm_record_strx_t str_type) > > Indentation. > > oza> + =A0 if ((14 =3D=3D arm_insn_r->opcode) || (10 =3D=3D arm_insn_r->o= pcode)) > > Too many parens. > > oza> + =A0else if ((12 =3D=3D arm_insn_r->opcode) || (8 =3D=3D arm_insn_r= ->opcode)) > > Again. > > oza> + =A0else if ((11 =3D=3D arm_insn_r->opcode) || (15 =3D=3D arm_insn_= r->opcode) > oza> + =A0 =A0|| (2 =3D=3D arm_insn_r->opcode) || (6 =3D=3D arm_insn_r->o= pcode)) > > Again, plus indentation. > > oza> + =A0 =A0 =A0regcache_raw_read_unsigned (reg_cache, reg_src1 > oza> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 , = &u_regval[0]); > > Formatting -- "," is on the wrong line. > > I see most of these issues multiple times. =A0Please go over the entire > patch and fix them all. =A0I feel like I asked this before. > > oza> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* FIX ME: How to read SPSR val= ue? =A0*/ > > We try not to put FIXME comments into new code. > Nobody ever fixes them. > > You can just write an informative comment instead: "We have no way to > read the SPSR value". =A0Assuming that is accurate. > > The enclosing function, arm_record_extension_space, takes care to return > a value, but (1) the meaning of the return value is not documented in > the function's introductory comment (this problems affects many of the > new functions), and (2) the only caller does not check it. > > What happens if the user hits an instruction which is not handled? > Right now a message is printed. =A0But shouldn't it abort the operation in > some other way? =A0That is, is emitting a message really sufficient? > > oza> +/* Decode arm/thumb insn depending on condition cods and opcodes; a= nd > oza> dispatch it. =A0*/ > oza> + > oza> +static int > oza> +decode_insn (insn_decode_record *arm_record, record_type_t record_t= ype, > oza> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uint32_t insn_size) > > E.g., here the return value should be documented. > > oza> + =A0static int (*const arm_handle_insn[8]) > oza> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0(insn_decode_record*) =3D > > Formatting looks weird. > A typedef for the function type would probably make this look less > strange. > > oza> + =A0 =A0 =A0/* if this insn has fallen into extension space then we= need > oza> not decode it anymore. =A0*/ > oza> + =A0 =A0 =A0if (!INSN_RECORDED(arm_record)) > oza> + =A0 =A0 =A0 =A0{ > oza> + =A0 =A0 =A0 =A0 =A0arm_handle_insn[insn_id] (arm_record); > > ... it seems like there should be a check of the return value here. > I don't understand this. > > oza> + =A0 =A0 =A0 =A0 =A0 =A0 =A0if (record_arch_list_add_mem \ > > No need for this backslash. > > oza> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0((CORE_ADDR)arm_record.arm_mems[no_= of_rec].addr, > oza> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0arm_record.arm_mems[no_of_rec].len)) > > Really messed up formatting. > > oza> + =A0if (arm_record.arm_regs) > oza> + =A0 =A0xfree (arm_record.arm_regs); > oza> + =A0if (arm_record.arm_mems) > oza> + =A0 =A0xfree (arm_record.arm_mems); > > This is fine as long as you have proved that nothing in any possible > call path here can throw an exception. =A0If anything throws an exception, > then you are leaking memory and should instead use a cleanup. > > oza> + =A0 /* Parse swi insn args, sycall record. =A0*/ > oza> + =A0int (*arm_swi_record) (struct regcache *regcache); > > Since this is only used in your Phase 3 patch, it belongs there. > > Tom >