Re: [PATCH] arm reversible : <phase_2_complete>

paawan oza
2011-07-13T03:17:09Z

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