Re: [PATCH] arm reversible : <phase_2_complete>

[paawan oza <paawan1982@yahoo.com>]

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