Re: [PATCH] arm reversible : <phase_2_complete>

[oza Pawandeep <oza dot pawandeep at gmail dot com>]

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
>
>