Re: [PATCH] arm reversible : <phase_2_complete>

oza Pawandeep
2011-04-22T05:55:04Z

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 <paawan1982@yahoo.com> 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 documents
> 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;  first array field provides only information
> 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ín <petr.hluzin@gmail.com>
> To: paawan oza <paawan1982@yahoo.com>
> Cc: gdb@sourceware.org; gdb-patches@sourceware.org
> Sent: Fri, April 22, 2011 2:25:04 AM
> Subject: Re: [PATCH] arm reversible : <phase_2_complete>
>
> On 20 April 2011 21:16, paawan oza <paawan1982@yahoo.com> 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*) =
>> +  {
>> +      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*) =
>
> 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.
>
> --
> Petr Hluzin
>
>