This is the mail archive of the
gdb@sourceware.org
mailing list for the GDB project.
Re: [PATCH] arm reversible : <phase_2_complete>
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