From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25601 invoked by alias); 25 Apr 2011 19:57:40 -0000 Received: (qmail 25548 invoked by uid 22791); 25 Apr 2011 19:57:36 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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; Mon, 25 Apr 2011 19:57:21 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p3PJvIs6017894 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 25 Apr 2011 15:57:19 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p3PJvIxK023178; Mon, 25 Apr 2011 15:57:18 -0400 Received: from opsy.redhat.com (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 p3PJvHAY001112; Mon, 25 Apr 2011 15:57:18 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 7BE663781C7; Mon, 25 Apr 2011 13:57:17 -0600 (MDT) From: Tom Tromey To: paawan oza Cc: Petr =?utf-8?Q?Hluz=C3=ADn?= , gdb@sourceware.org, gdb-patches@sourceware.org Subject: Re: [PATCH] arm reversible : References: <341905.10459.qm@web112513.mail.gq1.yahoo.com> <208397.95006.qm@web112517.mail.gq1.yahoo.com> <4DA27006.1080607@codesourcery.com> <763549.92092.qm@web112506.mail.gq1.yahoo.com> <335149.24692.qm@web112515.mail.gq1.yahoo.com> <592215.58786.qm@web112508.mail.gq1.yahoo.com> <172713.29831.qm__351.089161313389$1303740245$gmane$org@web112503.mail.gq1.yahoo.com> Date: Mon, 25 Apr 2011 19:57:00 -0000 In-Reply-To: <172713.29831.qm__351.089161313389$1303740245$gmane$org@web112503.mail.gq1.yahoo.com> (paawan oza's message of "Mon, 25 Apr 2011 07:03:12 -0700 (PDT)") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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-04/txt/msg00457.txt.bz2 >>>>> "Oza" == paawan oza writes: Oza> PS: I have added REG_ALLOC and MEM_ALLOC macors to accumulate Oza> xmallocs, but I am not sure whether to go for that macros or leave Oza> the implementation of reg/mem allcoation scattered across code Oza> calling xmalloc. I saw one use of REG_ALLOC and none of MEM_ALLOC. If you intend to just have a single use it is probably better to just drop the macros. But, maybe you intend to use these universally through the code. That would be fine. Oza> + /* Enable process record */ Period plus two spaces at the end of a comment. Oza> + set_gdbarch_process_record(gdbarch, arm_process_record); Space before an open paren. There are a lot of small formatting problems in this patch. Oza> #include "features/arm-with-m.c" Oza> + Oza> static int arm_debug; Adding a blank line for no reason. Oza> +/* arm-reversible process reacord data structures. */ Surely it should be "ARM". Oza> +struct arm_mem_r Oza> +{ Oza> + uint32_t len; Oza> + CORE_ADDR addr; Oza> +}; Wrong formatting. The struct needs documentation and so do its fields. Oza> + Oza> +static int Oza> +SBO_SBZ (uint32_t insn, uint32_t bit_num, uint32_t len, uint32_t sbo) A new function needs a documentation comment. Uppercase function names need special pleading. Oza> + uint32_t ONES = bits (insn, bit_num - 1, (bit_num -1) + (len - 1)); Likewise locals. Oza> +static int Oza> +arm_handle_data_proc_misc_load_str_insn \ Oza> +(insn_decode_record *arm_insn_r) Bad formatting. Oza> + union Oza> + { Oza> + uint32_t s_word; Oza> + gdb_byte buf[4]; Oza> + } u_buf[2]; There are lots of these little unions in the code. >From what I can tell, this is mostly to read target memory into the array, then convert to a scalar like: Oza> + GET_REG_VAL (reg_cache, reg_src1, &u_buf[0].buf[0]); [...] Oza> + arm_insn_r->arm_mems[1].addr = u_buf[0].s_word; It seems to me that extract_typed_address or something similar would be better. I'm not very familiar with the target record stuff; but if it is possible to cross-debug and use it, then this code does not seem to account for host/target endian differences. Oza> + /* SPSR is going to be changed. */ Oza> + /* Oza: FIX ME ? how to read SPSR value? */ There are a few comments in this style. First, don't put your name into comments. Second, I think instead of new "FIXME" comments, new code should either just work, or call error and have an explanatory comment. I can't really comment on the details of the implementation. I don't know much about either ARM or process record. It seems basically reasonable; I prefer a somewhat more symbolic style, but I understand that the x86 process record code is also written with many manifest constants. Tom