Re: [PATCH] arm reversible : <phase_2_complete>

[Petr HluzÃn <petr dot hluzin at gmail dot com>]

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