Re: [PATCH] arm reversible : <phase_2_complete>

[paawan oza <paawan1982 at yahoo dot com>]

Thanks for your comments; I think I will need to scrutinize the overall patch 
and make it correct with your comments.
I understand this patch has been lying around,but as Chandra K is working on the 
port of gdb.reverse, we will see the results soon.

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: Wed, July 13, 2011 2:29:36 AM
Subject: Re: [PATCH] arm reversible : <phase_2_complete>

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

Oza> any more comments are welcome make this patch ok, if ARM person can
Oza> have a look at it it would be great.

You have submitted this patch many times now and nobody has commented
on the details of the ARM decoding.  I think we should proceed on the
theory that this is simply not going to happen.

Also, I am not as concerned about the correctness of every detail as I
am about the general maintainability and style of the code.  I expect
there will be bugs; those can be fixed.

You need a ChangeLog entry.  A patch of this magnitude should also have
a NEWS entry.

Some kind of testing would be good.  Do the existing tests in
gdb.reverse work with your port?  If so then I think that is sufficient

Oza> +    unsigned int reg_len = 0; reg_len = LENGTH; \

Just write   unsigned int reg_len = LENGTH;

Oza> +        REGS = (uint32_t*) xmalloc (sizeof(uint32_t) * (reg_len)); \

Mind the spaces and parens.  Better, use XNEWVEC:

    REGS = XNEWVEC (uint32_t, reg_len);

Oza> +        while (reg_len) \
Oza> +          { \
Oza> +            REGS[reg_len - 1] = RECORD_BUF[reg_len - 1];  \
Oza> +            reg_len--;  \
Oza> +          } \

Just use memcpy.

Oza> +#define MEM_ALLOC(MEMS,LENGTH,RECORD_BUF) \

The same comments apply for this macro.

Oza> +/* ARM instruction record contains opcode of current insn and execution 
state 

Oza> (before entry to 

Oza> +decode_insn() ), contains list of to-be-modified registers and memory 
blocks 

Oza> (on return from 

Your email got corrupted.  Usually this is some bad MUA setting.

Oza> +  uint32_t mem_rec_count;       /* No of mem recors */

Typo, "recors"

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

Typo, should be "mandatory".

Oza> +  if(!sbo)

Spacing.

Oza> +  if ((3 == opcode1) && (bit (arm_insn_r->arm_insn, 4)))

Over-parenthesizing makes the code harder to read.  Please fix this.  I
noticed it in many places.  This specific case should read:

  if (3 == opcode1 && bit (arm_insn_r->arm_insn, 4))

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

Spacing.  Just go through the entire patch and fix all the spacing
issues.

I feel like I have mentioned this before.

Oza> +      regcache_raw_read_unsigned (reg_cache, reg_src1
Oza> +                                  , &u_buf[0].unsigned_regval);

What if this does not return REG_VALID?
There are multiple instances of this.

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

It seems wrong to use an assert in this code.  At least, it is not
obvious to me that this represents a logic error in gdb as opposed to a
merely unrecognized instruction.  An unrecognized instruction can occur
for many reasons, e.g., a bad jump.

Oza> +      if ((8 == arm_insn_r->opcode) || (10 == arm_insn_r->opcode)    
Oza> +      || (12 == arm_insn_r->opcode) || (14 == arm_insn_r->opcode)
Oza> +      || (9 == arm_insn_r->opcode) || (11 == arm_insn_r->opcode)    
Oza> +      || (13 == arm_insn_r->opcode) || (15 == arm_insn_r->opcode)        
Oza> +      || (0 == arm_insn_r->opcode) || (2 == arm_insn_r->opcode)    
Oza> +      || (4 == arm_insn_r->opcode) || (6 == arm_insn_r->opcode)
Oza> +      || (1 == arm_insn_r->opcode) || (3 == arm_insn_r->opcode)
Oza> +      || (5 == arm_insn_r->opcode) || (7 == arm_insn_r->opcode))

This reads very oddly.  Is there a particular reason behind the ordering
(if so -- document).  If not, isn't this:

  if (arm_insn_r->opcode >= 0 && arm_insn_r->opcode <= 15)

There are other odd-looking conditions like this.

Oza> +              default:
Oza> +                gdb_assert_not_reached ("Invalid addressing mode for 
insn");

Again, assert seems wrong.

I'm afraid I ran out of steam here.  Please fix all the issues already
noted and look at the rest of the patch with a critical eye to see what
else should be cleaned up.  I want this patch to go in, but first it
must comply to the usual gdb standards.

Tom