From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4521 invoked by alias); 21 Apr 2011 20:55:41 -0000 Received: (qmail 4505 invoked by uid 22791); 21 Apr 2011 20:55:40 -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,RFC_ABUSE_POST,TW_EG X-Spam-Check-By: sourceware.org Received: from mail-fx0-f41.google.com (HELO mail-fx0-f41.google.com) (209.85.161.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 21 Apr 2011 20:55:26 +0000 Received: by fxm18 with SMTP id 18so89998fxm.0 for ; Thu, 21 Apr 2011 13:55:24 -0700 (PDT) Received: by 10.223.62.11 with SMTP id v11mr376005fah.122.1303419324113; Thu, 21 Apr 2011 13:55:24 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.98.70 with HTTP; Thu, 21 Apr 2011 13:55:04 -0700 (PDT) In-Reply-To: <335149.24692.qm@web112515.mail.gq1.yahoo.com> 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> From: =?UTF-8?B?UGV0ciBIbHV6w61u?= Date: Thu, 21 Apr 2011 20:55:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : To: paawan oza Cc: gdb@sourceware.org, gdb-patches@sourceware.org Content-Type: text/plain; charset=UTF-8 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-04/txt/msg00402.txt.bz2 On 20 April 2011 21:16, paawan oza wrote: > Hi, > > I am working on phase-3 now. > if anybody could please start reviewing phase-2 patch (as this is > independent of phase-3 and could be checked in independently too) > I may start implementing review comments as and when I get. > In Parallel, test cases are also being worked upon. > following is the phase-2 patch. I took a peak and noticed the first points which looked suspicious to me. Note: I am just a causal mailing-list observer. > + > +struct arm_mem_r > +{ > + =C2=A0 =C2=A0uint32_t len; > + =C2=A0 =C2=A0CORE_ADDR addr; > +}; > + > +typedef struct insn_decode_record_t > +{ > + =C2=A0struct gdbarch *gdbarch; > + =C2=A0struct regcache *regcache; > + =C2=A0CORE_ADDR this_addr; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* address= of the insn being decoded. =C2=A0*/ > + =C2=A0uint32_t arm_insn; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* sh= ould accomodate thumb. =C2=A0*/ > + =C2=A0uint32_t cond; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/* condition code. =C2=A0*/ > + =C2=A0uint32_t id; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0/* type of insn. =C2=A0*/ > + =C2=A0uint32_t opcode; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= /* insn opcode. =C2=A0*/ > + =C2=A0uint32_t decode; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= /* insn decode bits. =C2=A0*/ > + =C2=A0uint32_t *arm_regs; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* registe= rs to be saved for this record. =C2=A0*/ > + =C2=A0struct arm_mem_r *arm_mems; =C2=A0 /* memory to be saved for this= record. =C2=A0*/ This field points to array of 2 or register_count (which itself changes). It might be worth documenting it points to array. (Pointers are usually not used for pointer-arithmetics at my job.) It is quite difficult to prove that accesses to arm_mems[] are within allocated bounds. Also some array elements are initialized only partially. (Usually I add a struct field like debug_arm_mems_count and add assertions at appropriate places.) > +static int > +decode_insn (insn_decode_record *arm_record, uint32_t insn_size) > +{ > + > + =C2=A0/* (starting from numerical 0); bits 25, 26, 27 decodes type of a= rm > instruction. =C2=A0*/ > + =C2=A0int (*const arm_handle_insn[NO_OF_TYPE_OF_ARM_INSNS]) (void*) =3D > + =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0arm_handle_data_proc_misc_load_str_insn, =C2=A0/* 0= 00. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0arm_handle_data_proc_imm_insn, =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0/* 001. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0arm_handle_ld_st_imm_offset_insn, =C2=A0 =C2=A0 =C2= =A0 =C2=A0 /* 010. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0arm_handle_ld_st_reg_offset_insn, =C2=A0 =C2=A0 =C2= =A0 =C2=A0 /* 011. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0arm_hamdle_ld_st_multiple_insn, =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 /* 100. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0arm_handle_brn_insn, =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* 101. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0arm_handle_coproc_insn, =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* 110. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0arm_handle_coproc_data_proc_insn =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0/* 111. =C2=A0*/ > + =C2=A0}; > + > + =C2=A0/* (starting from numerical 0); bits 13,14,15 decodes type of thu= mb > instruction. =C2=A0*/ > + =C2=A0int (*const thumb_handle_insn[NO_OF_TYPE_OF_THUMB_INSNS]) (void*)= =3D This array is local to the function. If you mark it as static you avoid its initialization in each invocation of the function. > + =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0thumb_handle_shift_add_sub_insn, =C2=A0 =C2=A0 =C2= =A0 =C2=A0 /* 000. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0thumb_handle_add_sub_cmp_mov_insn, =C2=A0 =C2=A0 = =C2=A0 /* 001. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0thumb_handle_ld_st_reg_offset_insn, =C2=A0 =C2=A0 = =C2=A0/* 010. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0thumb_handle_ld_st_imm_offset_insn, =C2=A0 =C2=A0 = =C2=A0/* 011. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0thumb_hamdle_ld_st_stack_insn, =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 /* 100. =C2=A0*/ Typo, hamdle > + =C2=A0 =C2=A0 =C2=A0thumb_handle_misc_insn, =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* 101. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0thumb_handle_swi_insn, =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* 110. =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0thumb_handle_branch_insn =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* 111. =C2=A0*/ > + =C2=A0}; All the functions are called only from this module (only from this function). If you make their arguments type-safe (not void*), you will get less code. (And type-safety, obviously.) There might be more, I just picked the first thing. I am not familiar with the code base. --=20 Petr Hluzin