Re: [PATCH] arm reversible : <phase_2_complete>

[paawan oza <paawan1982 at yahoo dot com>]

Hi Tom,

please find my comments inlined below.

Regards,
Oza.



----- Original Message ----
From: Tom Tromey <tromey@redhat.com>
To: paawan oza <paawan1982@yahoo.com>
Cc: gdb-patches@sourceware.org; Petr Hluzín <petr.hluzin@gmail.com>
Sent: Tue, May 31, 2011 11:34:49 PM
Subject: Re: [PATCH] arm reversible : <phase_2_complete>

>>>>> "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:  macors removed and it was a mistake to have unsigned, corrected too.

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

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: yes, but just to make the logic look symmetrical with MEM_ALLOC I have kept 
that way.

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

Wrong indentation.

Oza: corrected.

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: corrected.

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

Capitalize.
This problem appears more than once.

Oza: Capitalization is taken care in all the comments now.

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

Wrong formatting.
This appears a few times too.

Oza: sizeof (...)   formatting correctedin memset, I am not sure if any other 
formatting you referred to.

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 : If you see next line
ret = -1
eventullly gdb record code throws an exception as below.
if (ret < 0)
    error (_("Process record: failed to record execution log."));

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: this code would never be reached, except in the case where ARM changes/adds 
decoding logic to this set of insns.
or compiler is producing faulty code.
both have rare chances.
basically I was looking for throwing assertion with message, and could not find 
macro other than this.
please suggest if any better way to handle.

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

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

oza: turned into LONGEST, corrected now.

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.

Oza: sure, removed. 

execuse me if you have to tell things twice as it is quite a long code base 
where i might have failed to take care of same error at all places.
though I tried best to eliminate duplicate errors.

Tom