Re: [PATCH] arm reversible : <phase_2_complete>

[Tom Tromey <tromey@redhat.com>]

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