From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2843 invoked by alias); 22 Apr 2011 05:55:04 -0000 Received: (qmail 2828 invoked by uid 22791); 22 Apr 2011 05:55:03 -0000 X-SWARE-Spam-Status: No, hits=-1.4 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-pz0-f41.google.com (HELO mail-pz0-f41.google.com) (209.85.210.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 22 Apr 2011 05:54:46 +0000 Received: by pzk32 with SMTP id 32so301666pzk.0 for ; Thu, 21 Apr 2011 22:54:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.142.238.3 with SMTP id l3mr423013wfh.446.1303451685841; Thu, 21 Apr 2011 22:54:45 -0700 (PDT) Received: by 10.142.155.11 with HTTP; Thu, 21 Apr 2011 22:54:45 -0700 (PDT) In-Reply-To: <592215.58786.qm@web112508.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> <592215.58786.qm@web112508.mail.gq1.yahoo.com> Date: Fri, 22 Apr 2011 05:55:00 -0000 Message-ID: Subject: Re: [PATCH] arm reversible : From: oza Pawandeep To: =?ISO-8859-1?Q?Petr_Hluz=EDn?= Cc: gdb@sourceware.org, gdb-patches@sourceware.org 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-04/txt/msg00413.txt.bz2 Hi Petr, If you could look at the rest of the code and provide your comments it would be great too. Note: I am mallocing regs and mems scattered in the code, planning to organise it too in some way. Regards, Oza. On Fri, Apr 22, 2011 at 11:19 AM, paawan oza wrote: > Hi Petr, > > Thanks for your comments. > > 1) This array is local to the function. If you mark it as static you > avoid its initialization in each invocation of the function. > > Oza: I shall change it. > > 2) 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.) > > Oza: this was leftover; as when I designed initially, I felt I would need= any > type of data. > but now it is uniform and I can certainly change to arm_record type. > thanks for pointing that out. > > 3) 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. > > oza: basically both arm_mems and arm_regs point to an array. could be doc= uments > in code. > what we do is: > first find out the registers and memories (where we require length also) = in the > beginning. > and at the end in process_record just go for recording. > in both arm_regs and arm_mems; =A0first array field provides only informa= tion > about how many records are there. > > 4) (Usually I add a struct field like debug_arm_mems_count and add > assertions at appropriate places.) > > Oza: can you please elaborate on that as I am not sure on what condition = to > assert ? > > Regards, > Oza. > > > > > ----- Original Message ---- > From: Petr Hluz=EDn > To: paawan oza > Cc: gdb@sourceware.org; gdb-patches@sourceware.org > Sent: Fri, April 22, 2011 2:25:04 AM > Subject: Re: [PATCH] arm reversible : > > 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 >> +{ >> + =A0 =A0uint32_t len; >> + =A0 =A0CORE_ADDR addr; >> +}; >> + >> +typedef struct insn_decode_record_t >> +{ >> + =A0struct gdbarch *gdbarch; >> + =A0struct regcache *regcache; >> + =A0CORE_ADDR this_addr; =A0 =A0 =A0 =A0 =A0/* address of the insn bein= g decoded. =A0*/ >> + =A0uint32_t arm_insn; =A0 =A0 =A0 =A0 =A0 =A0/* should accomodate thum= b. =A0*/ >> + =A0uint32_t cond; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* condition code. = =A0*/ >> + =A0uint32_t id; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* type of insn. = =A0*/ >> + =A0uint32_t opcode; =A0 =A0 =A0 =A0 =A0 =A0 =A0/* insn opcode. =A0*/ >> + =A0uint32_t decode; =A0 =A0 =A0 =A0 =A0 =A0 =A0/* insn decode bits. = =A0*/ >> + =A0uint32_t *arm_regs; =A0 =A0 =A0 =A0 =A0 /* registers to be saved fo= r this record. =A0*/ >> + =A0struct arm_mem_r *arm_mems; =A0 /* memory to be saved for this reco= rd. =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) >> +{ >> + >> + =A0/* (starting from numerical 0); bits 25, 26, 27 decodes type of arm >> instruction. =A0*/ >> + =A0int (*const arm_handle_insn[NO_OF_TYPE_OF_ARM_INSNS]) (void*) =3D >> + =A0{ >> + =A0 =A0 =A0arm_handle_data_proc_misc_load_str_insn, =A0/* 000. =A0*/ >> + =A0 =A0 =A0arm_handle_data_proc_imm_insn, =A0 =A0 =A0 =A0 =A0 =A0/* 00= 1. =A0*/ >> + =A0 =A0 =A0arm_handle_ld_st_imm_offset_insn, =A0 =A0 =A0 =A0 /* 010. = =A0*/ >> + =A0 =A0 =A0arm_handle_ld_st_reg_offset_insn, =A0 =A0 =A0 =A0 /* 011. = =A0*/ >> + =A0 =A0 =A0arm_hamdle_ld_st_multiple_insn, =A0 =A0 =A0 =A0 =A0 /* 100.= =A0*/ >> + =A0 =A0 =A0arm_handle_brn_insn, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0/* 101. =A0*/ >> + =A0 =A0 =A0arm_handle_coproc_insn, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= /* 110. =A0*/ >> + =A0 =A0 =A0arm_handle_coproc_data_proc_insn =A0 =A0 =A0 =A0 =A0/* 111.= =A0*/ >> + =A0}; >> + >> + =A0/* (starting from numerical 0); bits 13,14,15 decodes type of thumb >> instruction. =A0*/ >> + =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. > >> + =A0{ >> + =A0 =A0 =A0thumb_handle_shift_add_sub_insn, =A0 =A0 =A0 =A0 /* 000. = =A0*/ >> + =A0 =A0 =A0thumb_handle_add_sub_cmp_mov_insn, =A0 =A0 =A0 /* 001. =A0*/ >> + =A0 =A0 =A0thumb_handle_ld_st_reg_offset_insn, =A0 =A0 =A0/* 010. =A0*/ >> + =A0 =A0 =A0thumb_handle_ld_st_imm_offset_insn, =A0 =A0 =A0/* 011. =A0*/ >> + =A0 =A0 =A0thumb_hamdle_ld_st_stack_insn, =A0 =A0 =A0 =A0 =A0 /* 100. = =A0*/ > > Typo, hamdle > >> + =A0 =A0 =A0thumb_handle_misc_insn, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= /* 101. =A0*/ >> + =A0 =A0 =A0thumb_handle_swi_insn, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = /* 110. =A0*/ >> + =A0 =A0 =A0thumb_handle_branch_insn =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*= 111. =A0*/ >> + =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. > > -- > Petr Hluzin > >