From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14285 invoked by alias); 1 May 2011 01:20:29 -0000 Received: (qmail 14251 invoked by uid 22791); 1 May 2011 01:20:27 -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,TW_NR 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; Sun, 01 May 2011 01:20:05 +0000 Received: by fxm18 with SMTP id 18so3966265fxm.0 for ; Sat, 30 Apr 2011 18:20:03 -0700 (PDT) Received: by 10.223.106.70 with SMTP id w6mr4173393fao.68.1304212803200; Sat, 30 Apr 2011 18:20:03 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.74.3 with HTTP; Sat, 30 Apr 2011 18:19:43 -0700 (PDT) In-Reply-To: <172713.29831.qm@web112503.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> <172713.29831.qm@web112503.mail.gq1.yahoo.com> From: =?UTF-8?B?UGV0ciBIbHV6w61u?= Date: Sun, 01 May 2011 01:20: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 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-05/txt/msg00000.txt.bz2 On 25 April 2011 16:03, paawan oza wrote: > Hi Petr, > > I have implemented your review comments. Typo "hamdle" in thumb_hamdle_ld_st_stack_insn() still exists. I decoded the `arm_mem_r *arm_mems' strucutre: arm_mems[0].len - is number of valid records `arm_mems[i]' after [0]. arm_mems[i].len - is number of bytes modified by the instruction. arm_mems[0].addr - is undefined (never written, never read) - except on line: thumb_insn_r->arm_mems[0].addr = u_buf[0].s_word+u_buf[1].s_word; arm_mems[i].addr - is target address of the modified block (for i=1..) This is reusing field arm_mem_r::len for two different things. This is ugly. Move the counter into insn_decode_record_t. Or use struct arm_mem_r { int count; struct { uint32_t len; CORE_ADDR addr; } array[0]; } Each instance of insn_decode_record_t is allocated and freed before new instance is allocated, therefore its size does not matter. The same applies for arm_record.arm_regs[]. > +/* arm-reversible process reacord data structures. */ Typo, reacord. > +#define THUMB2_INSN_SIZE_BYTES 4 Not used. In decode_insn() I suggest adding else if (THUMB_INSN_SIZE_BYTES == insn_size) { arm_record->arm_insn = u_buf.s_word; arm_record->id = bits (arm_record->arm_insn, 13, 15); ret = thumb_handle_insn[arm_record->id] ((void*)arm_record); } + else if THUMB2_INSN_SIZE_BYTES == insn_size) + // not implemented, maybe complain loudly + else + assert(false); This would document the possible values. +#define ARM_RECORD_ARCH_LIST_ADD_REG(regnum) \ + record_arch_list_add_reg (arm_record.regcache, regnum) This is used only at two places: + ARM_RECORD_ARCH_LIST_ADD_REG(ARM_PC_REGNUM); + if (arm_record.arm_regs) + { + for (no_of_rec=1; no_of_rec<=arm_record.arm_regs[0]; no_of_rec++) + { + if (ARM_RECORD_ARCH_LIST_ADD_REG (arm_record.arm_regs[no_of_rec])) + ret = -1; + } + } This saves only few characters and no lines of code. But adds an indirection and hides use of local variable 'arm_record'. Also the caller already contains record_arch_list_add_* functions. Structure insn_decode_record: > + > + typedef struct insn_decode_record_t { > + uint32_t arm_insn; /* should accomodate thumb. */ Typo, accomodate -> accommodate. > + uint32_t opcode; /* insn opcode. */ This field is practically used as a local variable - all functions which read the field also initialize it. Furthermore `arm_insn' is a kind of "insn opcode", too. Local-like varibales make a structure look more complex than it is; this places fear into those who will want to modify the code. > + uint32_t id; /* type of insn. */ Practically local variable in decode_insn(). No other function ever uses it. > + uint32_t cond; /* condition code. */ Is not initialized if (THUMB_INSN_SIZE_BYTES == insn_size). It is not a bug since Thumb mode does not use condition prefixes. However I find useful to assign -1 or something to indicate that: * the field does not have an usual value * I did not acidentaly skipped initialization * I am not relying on initialization from other place * hopefully the special value will trigger assertion if Thumb code actually uses it. (Actually my runtime fills memory with 0xCCCCCCC or something for this reason, so I merelly add a comment about the non-use.) > + struct gdbarch *gdbarch; Most functions do not actually use the field. Consider removing the dummy variables in relevant functions. > + struct regcache *regcache; Many functions use unnecesary cast to init local var, e.g. `(struct regcache*) thumb_insn_r->regcache'. Most functions do not actually use the var. > + ...} insn_decode_record; I suggest to add comment to insn_decode_record_t : /* Contains opcode of current instruction and execution state (before entry to decode_insn()), contains list of to-be-modified registers and memory blocks (on return from decode_insn()). */ Moving on: In handle_extension_space() > + uint32_t reg_src1 = 0, reg_src2 = 0; Unused locals. In arm_handle_data_proc_imm_insn() > + uint32_t reg_src1 = 0, reg_src2 = 0, reg_dest = 0; > + uint32_t immed_high = 0, immed_low = 0, offset_8 = 0, tgt_mem_addr = 0; All these are unused. In other functions: GCC can detect when a local variable is used before it is initialized. Your code initializes by 0 at the start of the function so the advantage of GCC diagnostics is lost. I recommend to not initialize a local variable until its "true" value is know/computed. I also recommend to define a local variable in the smalles scope possible, however I understand that many programmers use weak tools and knowing its definition requires more effort than placing cursor on the variable - therefore they prefer definitions at the start of function. In arm_hamdle_ld_st_multiple_insn (I deleted irrelevant lines): > + switch(addr_mode) > + { > + /* Decrement after. */ > + case 0: > + start_address = (u_buf[0].s_word) - (register_count * 4) + 4; > + while (register_count) > + start_address = start_address + 4; > + break; > + /* Increment after. */ > + case 1: > + start_address = u_buf[0].s_word; > + while (register_count) > + start_address = start_address + 4; > + break; > + /* Decrement before. */ > + case 2: > + start_address = (u_buf[0].s_word) - (register_count * 4); > + while (register_count) > + start_address = start_address + 4; > + break; > + /* Increment before. */ > + case 3: > + start_address = u_buf[0].s_word + 4; > + while (register_count) > + start_address = start_address + 4; > + break; Initialization of `start_address' looks good but I guess the increment part should be: + start_address = start_address + 4 - start_address = start_address - 4 in two of the four cases. > + > + default: > + /* unreachable. */ Consider using gdb_assert_not_reached("Invalid addressing mode"). > + printf("handling store insn, immed offfset insn\n"); Typo offfset. Should be printed conditionally. > + /* LDR insn has a capability to do branching, if > + MOV LR, PC is precedded by LDR insn having Rn as R15 Typo precedded -> preccedded. Multiple times. > + /* handle incremenrt after/before and decrment after.before mode; > + Rn is chaging depending on W bit, but as of now we store Rn too wihtout going for > + optmization. */ Typos chaging, wihtout, optmization. My IDE has a spellchecker which underlines typos. No effort required. (Consider upgrading your tools.) > + ret = (0x0F != arm_record->cond)? > + arm_handle_insn[arm_record->id] ((void*)arm_record) : > + handle_extension_space(arm_record); The ? and : should be at the begging. GNU style guideline: "When you split an expression into multiple lines, split it before an operator, not after one." Also I find it more readable. In decode_insn(): > + arm_handle_insn[arm_record->id] ((void*)arm_record) : > + ret = thumb_handle_insn[arm_record->id] ((void*)arm_record); The (void*) casts are now unnecessary. In arm_handle_coproc_data_proc_insn(): > + tdep->arm_swi_record(reg_cache); When this line is executed this function still returns -1 (i.e. failure). I guess the two if's are intended to return success then. In thumb_handle_add_sub_cmp_mov_insn(): > + union { ... } u_buf; Is unused. So does in thumb_handle_shift_add_sub_insn(). Maybe more. In thumb_handle_ld_st_reg_offset_insn(): > + opcode1 = bits (thumb_insn_r->arm_insn, 11, 12); > + thumb_insn_r->opcode = bits (thumb_insn_r->arm_insn, 13, 15); The values are never read. If you remove the 'thumb_insn_r->opcode' assignment then you can simplify the function and remove the goto's. > @@ -200,6 +200,9 @@ > /* Return the expected next PC if FRAME is stopped at a syscall > instruction. */ > CORE_ADDR (*syscall_next_pc) (struct frame_info *frame); > + > + /* Parse swi args. */ > + int (*arm_swi_record) (struct regcache *regcache); This field is newer assigned (only initialized to NULL). I did not know that "swi" is an instruction. Is it a widely known fact for ARM developers? If not then use "swi insn" in the comment. The return value is never tested. What is its meaning? The parsed args seem to be stored as a side-effect. Where are they stored? (Answers should go to comments.) (Since this is an indirect call, the documentation should be more verbose than usual cases.) Disclaimers: I am not familiar with ARM, I did not check e.g. if all cases are handled, instructions have correct semantics etc. Also I have no vote in GDB development, so my patch approval would have little impact. GDB commiters: please reply whether you agree with my review. (So that paawan oza does not spend effort in vain.) -- Petr Hluzin