Re: [PATCH] arm reversible : <phase_2_complete>

Tom Tromey
2011-05-31T18:05:07Z

>>>>> "Oza" == paawan oza <paawan1982@yahoo.com> writes:

Oza> Fixed some minor issues with Patch.

Thanks.

Overall I think the basic structure is probably ok.
I'd appreciate it if an actual ARM person took a look.

Oza> +#define GET_REG_VAL(REGCACHE,NO,VAL) \
Oza> + regcache_raw_read_unsigned (REGCACHE, NO, VAL);
Oza> +
Oza> +#define GET_REG_VAL_SIGNED(REGCACHE,NO,VAL) \
Oza> + regcache_raw_read_unsigned (REGCACHE, NO, VAL);

I think it is better not to have these macros. They don't add anything,
but just obscure the underlying implementation.

I don't understand why the "SIGNED" macro is defined as calling a
function named ..._unsigned.

Oza> +#define REG_ALLOC(REGS,LENGTH,RECORD_BUF) \
Oza> +do \
Oza> + { \
Oza> + unsigned int reg_len = 0; \
Oza> + reg_len = LENGTH; \

You might as well coalesce these two lines.

Oza> + if (reg_len) \
Oza> + { \
Oza> + REGS = (uint32_t*) xmalloc (sizeof(uint32_t) * (reg_len)); \
Oza> + while (reg_len) \
Oza> + { \
Oza> + REGS[reg_len - 1] = RECORD_BUF[reg_len - 1]; \
Oza> + reg_len--; \

I think this could be replaced with memcpy.

Oza> +struct arm_mem_r
Oza> +{
Oza> + uint32_t len; /* record length. */

Wrong indentation.

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

Wrong formatting.

Oza> +/* checks ARM SBZ and SBO mendatory fields. */

Capitalize.
This problem appears more than once.

Oza> + memset(&u_buf, 0, sizeof(u_buf));

Wrong formatting.
This appears a few times too.

Oza> + printf_unfiltered (_("Process record does not support instruction "
Oza> + "0x%0x at address %s.\n"),
Oza> + arm_insn_r->arm_insn,
Oza> + paddress (arm_insn_r->gdbarch,

It seems odd that a failure is reported just with a printf.
What is the reason for not throwing an exception?

Oza> + gdb_assert_not_reached ("no decoding pattern found");

This text makes it sound like this assertion could possibly be reached
somehow. I didn't track through all the insn decoding logic; but if
this can be triggered by some value (perhaps an invalid instruction)
then it is extremely unfriendly to assert.

Oza> + struct
Oza> + {
Oza> + ULONGEST signed_word;

It is weird that a "signed" word has an unsigned type.
Why is this?

Oza> + /* Oza: FIX ME ? what if user hit breakpoint and type reverse, in
Oza> + that case, we need to go back with previous CPSR and
Oza> + Program Counter.. */

No names in comments.

Tom