Re: [PATCH] arm reversible : <phase_2_complete>

[chandra krishnappa <chandra_roadking at yahoo dot com>]

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
>