Re: [PATCH] arm reversible : <phase_2_complete>

paawan oza
2011-04-22T05:49:31Z

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