Re: [PATCH] arm reversible : <phase_2_complete>

paawan oza
2011-07-13T03:03:47Z

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
>