Re: [PATCH] arm reversible : <phase_2_complete>

Petr Hluzín
2011-05-01T01:20:29Z

On 25 April 2011 16:03, paawan oza <paawan1982@yahoo.com> wrote:
> Hi Petr,
>
> I have implemented your review comments.

Typo "hamdle" in thumb_hamdle_ld_st_stack_insn() still exists.

I decoded the `arm_mem_r *arm_mems' strucutre:

arm_mems[0].len - is number of valid records `arm_mems[i]' after [0].
arm_mems[i].len - is number of bytes modified by the instruction.
arm_mems[0].addr - is undefined (never written, never read) - except
on line: thumb_insn_r->arm_mems[0].addr =
u_buf[0].s_word+u_buf[1].s_word;
arm_mems[i].addr - is target address of the modified block (for i=1..)

This is reusing field arm_mem_r::len for two different things.
This is ugly.

Move the counter into insn_decode_record_t. Or use struct arm_mem_r {
int count;
struct {
uint32_t len;
CORE_ADDR addr;
} array[0];
}

Each instance of insn_decode_record_t is allocated and freed before
new instance is allocated, therefore its size does not matter.

The same applies for arm_record.arm_regs[].

> +/* arm-reversible process reacord data structures. */

Typo, reacord.

> +#define THUMB2_INSN_SIZE_BYTES 4

Not used.
In decode_insn() I suggest adding
else if (THUMB_INSN_SIZE_BYTES == insn_size)
{
arm_record->arm_insn = u_buf.s_word;
arm_record->id = bits (arm_record->arm_insn, 13, 15);
ret = thumb_handle_insn[arm_record->id] ((void*)arm_record);
}
+ else if THUMB2_INSN_SIZE_BYTES == insn_size)
+ // not implemented, maybe complain loudly
+ else
+ assert(false);

This would document the possible values.

+#define ARM_RECORD_ARCH_LIST_ADD_REG(regnum) \
+ record_arch_list_add_reg (arm_record.regcache, regnum)

This is used only at two places:

+ ARM_RECORD_ARCH_LIST_ADD_REG(ARM_PC_REGNUM);
+ if (arm_record.arm_regs)
+ {
+ for (no_of_rec=1; no_of_rec<=arm_record.arm_regs[0]; no_of_rec++)
+ {
+ if (ARM_RECORD_ARCH_LIST_ADD_REG (arm_record.arm_regs[no_of_rec]))
+ ret = -1;
+ }
+ }

This saves only few characters and no lines of code.
But adds an indirection and hides use of local variable 'arm_record'.
Also the caller already contains record_arch_list_add_* functions.

Structure insn_decode_record:

> +
> + typedef struct insn_decode_record_t {
> + uint32_t arm_insn; /* should accomodate thumb. */

Typo, accomodate -> accommodate.

> + uint32_t opcode; /* insn opcode. */

This field is practically used as a local variable - all functions
which read the field also initialize it.
Furthermore `arm_insn' is a kind of "insn opcode", too.
Local-like varibales make a structure look more complex than it is;
this places fear into those who will want to modify the code.

> + uint32_t id; /* type of insn. */

Practically local variable in decode_insn(). No other function ever uses it.

> + uint32_t cond; /* condition code. */

Is not initialized if (THUMB_INSN_SIZE_BYTES == insn_size).
It is not a bug since Thumb mode does not use condition prefixes.
However I find useful to assign -1 or something to indicate that:
* the field does not have an usual value
* I did not acidentaly skipped initialization
* I am not relying on initialization from other place
* hopefully the special value will trigger assertion if Thumb code
actually uses it.
(Actually my runtime fills memory with 0xCCCCCCC or something for this
reason, so I merelly add a comment about the non-use.)

> + struct gdbarch *gdbarch;

Most functions do not actually use the field. Consider removing the
dummy variables in relevant functions.

> + struct regcache *regcache;

Many functions use unnecesary cast to init local var, e.g. `(struct
regcache*) thumb_insn_r->regcache'.
Most functions do not actually use the var.

> + ...} insn_decode_record;

I suggest to add comment to insn_decode_record_t :

/* Contains opcode of current instruction and execution state (before
entry to decode_insn()),
contains list of to-be-modified registers and memory blocks (on return
from decode_insn()). */

Moving on:

In handle_extension_space()
> + uint32_t reg_src1 = 0, reg_src2 = 0;

Unused locals.

In arm_handle_data_proc_imm_insn()
> + uint32_t reg_src1 = 0, reg_src2 = 0, reg_dest = 0;
> + uint32_t immed_high = 0, immed_low = 0, offset_8 = 0, tgt_mem_addr = 0;

All these are unused.

In other functions:
GCC can detect when a local variable is used before it is initialized.
Your code initializes by 0 at the start of the function so the
advantage of GCC diagnostics is lost.
I recommend to not initialize a local variable until its "true" value
is know/computed.
I also recommend to define a local variable in the smalles scope
possible, however I understand that many
programmers use weak tools and knowing its definition requires more
effort than placing cursor on the variable - therefore they prefer
definitions at the start of function.

In arm_hamdle_ld_st_multiple_insn (I deleted irrelevant lines):
> + switch(addr_mode)
> + {
> + /* Decrement after. */
> + case 0:
> + start_address = (u_buf[0].s_word) - (register_count * 4) + 4;
> + while (register_count)
> + start_address = start_address + 4;
> + break;
> + /* Increment after. */
> + case 1:
> + start_address = u_buf[0].s_word;
> + while (register_count)
> + start_address = start_address + 4;
> + break;
> + /* Decrement before. */
> + case 2:
> + start_address = (u_buf[0].s_word) - (register_count * 4);
> + while (register_count)
> + start_address = start_address + 4;
> + break;
> + /* Increment before. */
> + case 3:
> + start_address = u_buf[0].s_word + 4;
> + while (register_count)
> + start_address = start_address + 4;
> + break;

Initialization of `start_address' looks good but I guess the increment
part should be:
+ start_address = start_address + 4
- start_address = start_address - 4
in two of the four cases.

> +
> + default:
> + /* unreachable. */

Consider using gdb_assert_not_reached("Invalid addressing mode").

> + printf("handling store insn, immed offfset insn\n");

Typo offfset. Should be printed conditionally.

> + /* LDR insn has a capability to do branching, if
> + MOV LR, PC is precedded by LDR insn having Rn as R15

Typo precedded -> preccedded. Multiple times.

> + /* handle incremenrt after/before and decrment after.before mode;
> + Rn is chaging depending on W bit, but as of now we store Rn too wihtout going for
> + optmization. */

Typos chaging, wihtout, optmization.

My IDE has a spellchecker which underlines typos. No effort required.
(Consider upgrading your tools.)

> + ret = (0x0F != arm_record->cond)?
> + arm_handle_insn[arm_record->id] ((void*)arm_record) :
> + handle_extension_space(arm_record);

The ? and : should be at the begging.
GNU style guideline: "When you split an expression into multiple
lines, split it before an operator, not after one."
Also I find it more readable.

In decode_insn():
> + arm_handle_insn[arm_record->id] ((void*)arm_record) :
> + ret = thumb_handle_insn[arm_record->id] ((void*)arm_record);

The (void*) casts are now unnecessary.

In arm_handle_coproc_data_proc_insn():
> + tdep->arm_swi_record(reg_cache);

When this line is executed this function still returns -1 (i.e. failure).
I guess the two if's are intended to return success then.

In thumb_handle_add_sub_cmp_mov_insn():
> + union { ... } u_buf;

Is unused. So does in thumb_handle_shift_add_sub_insn(). Maybe more.

In thumb_handle_ld_st_reg_offset_insn():
> + opcode1 = bits (thumb_insn_r->arm_insn, 11, 12);
> + thumb_insn_r->opcode = bits (thumb_insn_r->arm_insn, 13, 15);

The values are never read.
If you remove the 'thumb_insn_r->opcode' assignment then you can
simplify the function and remove the goto's.

> @@ -200,6 +200,9 @@
> /* Return the expected next PC if FRAME is stopped at a syscall
> instruction. */
> CORE_ADDR (*syscall_next_pc) (struct frame_info *frame);
> +
> + /* Parse swi args. */
> + int (*arm_swi_record) (struct regcache *regcache);

This field is newer assigned (only initialized to NULL).

I did not know that "swi" is an instruction. Is it a widely known fact
for ARM developers?
If not then use "swi insn" in the comment.

The return value is never tested. What is its meaning?

The parsed args seem to be stored as a side-effect. Where are they stored?
(Answers should go to comments.)
(Since this is an indirect call, the documentation should be more
verbose than usual cases.)

Disclaimers:

I am not familiar with ARM, I did not check e.g. if all cases are
handled, instructions have correct semantics etc.
Also I have no vote in GDB development, so my patch approval would
have little impact.

GDB commiters: please reply whether you agree with my review. (So that
paawan oza does not spend effort in vain.)

--
Petr Hluzin