From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27600 invoked by alias); 17 Nov 2011 20:40:41 -0000 Received: (qmail 27590 invoked by uid 22791); 17 Nov 2011 20:40:40 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_EG X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 17 Nov 2011 20:40:27 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pAHKePoM026969 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 17 Nov 2011 15:40:25 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id pAHKePB6021642; Thu, 17 Nov 2011 15:40:25 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id pAHKeNQe018951; Thu, 17 Nov 2011 15:40:23 -0500 From: Tom Tromey To: oza Pawandeep Cc: Petr =?utf-8?Q?Hluz=C3=ADn?= , paawan oza , "gdb-patches\@sourceware.org" , chandra krishnappa Subject: Re: [PATCH] arm reversible : 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: Thu, 17 Nov 2011 20:40:00 -0000 In-Reply-To: (oza Pawandeep's message of "Wed, 9 Nov 2011 11:37:27 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00486.txt.bz2 >>>>> "oza" == oza Pawandeep writes: oza> please find the latest patch below. No ChangeLog entry. No NEWS entry. Patch got mangled by your mailer again. oza> + memcpy(®S[0],&RECORD_BUF[0],sizeof(uint32_t)*LENGTH); \ Wrong spacing. oza> + memcpy(&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_buf, oza> + uint32_t *record_buf_mem, arm_record_strx_t str_type) Indentation. oza> + if ((14 == arm_insn_r->opcode) || (10 == arm_insn_r->opcode)) Too many parens. oza> + else if ((12 == arm_insn_r->opcode) || (8 == arm_insn_r->opcode)) Again. oza> + else if ((11 == arm_insn_r->opcode) || (15 == arm_insn_r->opcode) oza> + || (2 == arm_insn_r->opcode) || (6 == arm_insn_r->opcode)) Again, plus indentation. oza> + regcache_raw_read_unsigned (reg_cache, reg_src1 oza> + , &u_regval[0]); Formatting -- "," is on the wrong line. I see most of these issues multiple times. Please go over the entire patch and fix them all. I feel like I asked this before. oza> + /* FIX ME: How to read SPSR value? */ 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. 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. But shouldn't it abort the operation in some other way? That is, is emitting a message really sufficient? oza> +/* Decode arm/thumb insn depending on condition cods and opcodes; and oza> dispatch it. */ oza> + oza> +static int oza> +decode_insn (insn_decode_record *arm_record, record_type_t record_type, oza> + uint32_t insn_size) E.g., here the return value should be documented. oza> + static int (*const arm_handle_insn[8]) oza> + (insn_decode_record*) = Formatting looks weird. A typedef for the function type would probably make this look less strange. oza> + /* if this insn has fallen into extension space then we need oza> not decode it anymore. */ oza> + if (!INSN_RECORDED(arm_record)) oza> + { oza> + arm_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> + if (record_arch_list_add_mem \ No need for this backslash. oza> + ((CORE_ADDR)arm_record.arm_mems[no_of_rec].addr, oza> + arm_record.arm_mems[no_of_rec].len)) Really messed up formatting. 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> + /* 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. Tom