From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1017 invoked by alias); 22 Apr 2011 05:49:31 -0000 Received: (qmail 1005 invoked by uid 22791); 22 Apr 2011 05:49:30 -0000 X-SWARE-Spam-Status: No, hits=-0.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RFC_ABUSE_POST,TW_EG,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from nm26-vm2.bullet.mail.ne1.yahoo.com (HELO nm26-vm2.bullet.mail.ne1.yahoo.com) (98.138.91.214) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Fri, 22 Apr 2011 05:49:13 +0000 Received: from [98.138.90.55] by nm26.bullet.mail.ne1.yahoo.com with NNFMP; 22 Apr 2011 05:49:12 -0000 Received: from [98.138.89.251] by tm8.bullet.mail.ne1.yahoo.com with NNFMP; 22 Apr 2011 05:49:12 -0000 Received: from [127.0.0.1] by omp1043.mail.ne1.yahoo.com with NNFMP; 22 Apr 2011 05:49:12 -0000 Received: (qmail 73145 invoked by uid 60001); 22 Apr 2011 05:49:11 -0000 Message-ID: <592215.58786.qm@web112508.mail.gq1.yahoo.com> Received: from [123.238.92.44] by web112508.mail.gq1.yahoo.com via HTTP; Thu, 21 Apr 2011 22:49:11 PDT 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> Date: Fri, 22 Apr 2011 05:49:00 -0000 From: paawan oza Subject: Re: [PATCH] arm reversible : To: =?iso-8859-1?Q?Petr_Hluz=EDn?= Cc: gdb@sourceware.org, gdb-patches@sourceware.org In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 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/msg00412.txt.bz2 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 a= ny=20 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 docum= ents=20 in code. what we do is: first find out the registers and memories (where we require length also) in= the=20 beginning. and at the end in process_record just go for recording. in both arm_regs and arm_mems; first array field provides only information= =20 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= =20 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 > +{ > + uint32_t len; > + CORE_ADDR addr; > +}; > + > +typedef struct insn_decode_record_t > +{ > + struct gdbarch *gdbarch; > + struct regcache *regcache; > + CORE_ADDR this_addr; /* address of the insn being decoded. */ > + uint32_t arm_insn; /* should accomodate thumb. */ > + uint32_t cond; /* condition code. */ > + uint32_t id; /* type of insn. */ > + uint32_t opcode; /* insn opcode. */ > + uint32_t decode; /* insn decode bits. */ > + uint32_t *arm_regs; /* registers to be saved for this record= . */ > + struct arm_mem_r *arm_mems; /* memory to be saved for this record. = */ 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) > +{ > + > + /* (starting from numerical 0); bits 25, 26, 27 decodes type of arm > instruction. */ > + int (*const arm_handle_insn[NO_OF_TYPE_OF_ARM_INSNS]) (void*) =3D > + { > + arm_handle_data_proc_misc_load_str_insn, /* 000. */ > + arm_handle_data_proc_imm_insn, /* 001. */ > + arm_handle_ld_st_imm_offset_insn, /* 010. */ > + arm_handle_ld_st_reg_offset_insn, /* 011. */ > + arm_hamdle_ld_st_multiple_insn, /* 100. */ > + arm_handle_brn_insn, /* 101. */ > + arm_handle_coproc_insn, /* 110. */ > + arm_handle_coproc_data_proc_insn /* 111. */ > + }; > + > + /* (starting from numerical 0); bits 13,14,15 decodes type of thumb > instruction. */ > + int (*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. > + { > + thumb_handle_shift_add_sub_insn, /* 000. */ > + thumb_handle_add_sub_cmp_mov_insn, /* 001. */ > + thumb_handle_ld_st_reg_offset_insn, /* 010. */ > + thumb_handle_ld_st_imm_offset_insn, /* 011. */ > + thumb_hamdle_ld_st_stack_insn, /* 100. */ Typo, hamdle > + thumb_handle_misc_insn, /* 101. */ > + thumb_handle_swi_insn, /* 110. */ > + thumb_handle_branch_insn /* 111. */ > + }; 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