From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9520 invoked by alias); 3 Mar 2011 18:32:58 -0000 Received: (qmail 9512 invoked by uid 22791); 3 Mar 2011 18:32:57 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_EG,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; Thu, 03 Mar 2011 18:32:48 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p23IWldn013163 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 3 Mar 2011 13:32:47 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p23IWkQB030817; Thu, 3 Mar 2011 13:32:46 -0500 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 p23IWjpi013104; Thu, 3 Mar 2011 13:32:45 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id 6C6C33785EB; Thu, 3 Mar 2011 11:32:45 -0700 (MST) From: Tom Tromey To: paawan oza Cc: "gdb\@sourceware.org" Subject: Re: [PATCH] arm reversible : progress References: <341905.10459.qm@web112513.mail.gq1.yahoo.com> Date: Thu, 03 Mar 2011 18:32:00 -0000 In-Reply-To: <341905.10459.qm@web112513.mail.gq1.yahoo.com> (paawan oza's message of "Wed, 2 Mar 2011 20:08:27 -0800 (PST)") 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-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-owner@sourceware.org X-SW-Source: 2011-03/txt/msg00038.txt.bz2 >>>>> ">" == paawan oza writes: >> I am done with the deisng for both thumb and arm framework. Thanks for your work on this. >> having some doubts as of now. I can't help with the ARM-specific stuff, but I have a few more general comments. >> diff -urN arm_new/arm-linux-tdep.c arm_orig/arm-linux-tdep.c The patch is reversed. >> - /* Enable process record */ >> - set_gdbarch_process_record(gdbarch, arm_process_record); There are a bunch of minor coding style problems in the patch. You'll have to fix them all before submitting. In the quoted lines: the comment must end with a period and two spaces, and there is a space missing before the open paren. That is, the code should look like: /* Enable process record. */ set_gdbarch_process_record (gdbarch, arm_process_record); These two problems appear frequently. >> -int arm_handle_data_proc_misc_load_str_insn (void*); I did not read closely, but I think most or maybe all of the functions declared here should be static. This applies to some other objects as well, like arm_handle_insn. Actually, in most cases, I think the code could be rearranged so that you don't need forward declarations at all. >> - else if(ARM_INSN_SIZE_BYTES == insn_size) Space after the "if" -- this happens a lot. >> - struct gdbarch_tdep *tdep = gdbarch_tdep ((struct gdbarch*) arm_insn_r-> gdbarch); I think you don't need the cast here. I think this occurs in a few places. >> - struct regcache *reg_cache = (struct regcache*) arm_insn_r->regcache; Or here. But if you needed something like this, there would have to be a space before the "*". >> - /* data processing insn /multiply sinsn */ >> - if(9 == arm_insn_r->decode) One thing I dislike about the x86 record code is the huge number of constants. I think this sort of thing is better if you use some symbolic name instead. One idea would be to share things with opcodes/arm-dis.c somehow. I don't know if that is practical or not. Actually, I have never understood why the process record stuff doesn't share more code with the simulators. Maybe that is just too much work somehow. And, whatever happened to the QEMU-based replay approach? That seemed promising to me. >> - arm_insn_r->arm_regs = (uint32_t*)xmalloc (sizeof(uint32_t)*3); I think it is a little clearer to use the libiberty macros, e.g.: arm_insn_r->arm_regs = XNEWVEC (uint32_t, 3); >> - uint32_t reg_val1=0,reg_val2=0; Spaces around the "=" signs. This occurs in a few places. Tom