Re: [PATCH] arm reversible : <phase_2_complete>

[paawan oza <paawan1982@yahoo.com>]

I appreciate your effort on this Chandra K; will be sending you updated patch 
with Tom's comments fixed.
thanks for you work and time.
Oza.



----- Original Message ----
From: chandra krishnappa <chandra_roadking@yahoo.com>
To: paawan oza <paawan1982@yahoo.com>; Tom Tromey <tromey@redhat.com>
Cc: gdb-patches@sourceware.org; Petr Hluzín <petr.hluzin@gmail.com>
Sent: Wed, July 13, 2011 7:33:50 AM
Subject: Re: [PATCH] arm reversible : <phase_2_complete>

Hi,

I am trying to the test if the existing gdb.reverse works with the ARM porting 
done by Oza..

I am a learner, and I am taking more time than a average developer to get 
documentation, read, understand and complete the testing of ARM porting of 
reversible....


Thanks & Regards,
-Chandra K


--- On Wed, 7/13/11, Tom Tromey <tromey@redhat.com> wrote:

> From: Tom Tromey <tromey@redhat.com>
> Subject: Re: [PATCH] arm reversible : <phase_2_complete>
> To: "paawan oza" <paawan1982@yahoo.com>
> Cc: gdb-patches@sourceware.org, "Petr Hluzín" <petr.hluzin@gmail.com>
> Date: Wednesday, July 13, 2011, 2:29 AM
> >>>>> "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
>