Re: [PATCH] arm reversible : <phase_2_complete>

[Petr HluzÃn <petr dot hluzin at gmail dot com>]

Hi Oza, hi folks

On 22 October 2011 17:41, oza Pawandeep <oza.pawandeep@gmail.com> wrote:
> @Chandra: please take this patch and run your tests on this. (it is
> integrated with gdb7.3)

You probably meant to say the patch should be applied to gdb-7.3. It
is not there yet.

> +
> +#define INSN_RECORDED(ARM_RECORD) \
> + Â((ARM_RECORD->arm_regs || ARM_RECORD->arm_mems))

Is it supposed to return a boolean-like value? If yes then it would be
more descriptive to use INSN_IS_RECORDED. However I do not know if GDB
uses this convention. Tom?
Also there are extra parentheses. There should be parentheses around
ARM_RECORD argument.
Consider adding `== NULL' to the two tests to make it clear the fields
are pointers, not integers or booleans, and that the || is supposed to
be logical, not bit-wise. (I have heard few people have tools good
enough to tell them the types.)

In arm_record_strx:
> + Â Â Âif (ARM_RECORD_STRH == str_type)
> + Â Â Â Â{
> + Â Â Â Â Â*(record_buf_mem) = 2;
> + Â Â Â Â Â*(record_buf_mem + 1) = tgt_mem_addr;

Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syntax?

> + Â Â Â Â Âarm_insn_r->mem_rec_count = 1;
> + Â Â Â Â}
> + Â Â Âelse if (ARM_RECORD_STRD == str_type)
> + Â Â Â Â{
> + Â Â Â Â Â*(record_buf_mem) = 4;
> + Â Â Â Â Â*(record_buf_mem + 1) = tgt_mem_addr;
> + Â Â Â Â Â*(record_buf_mem + 2) = 4;
> + Â Â Â Â Â*(record_buf_mem + 3) = tgt_mem_addr + 4;

The same here and several more places in this function. Search for "*("

In arm_record_extension_space:
> +
> + Âopcode1 = bits (arm_insn_r->arm_insn, 20, 27);
> + Âopcode1 = bits (arm_insn_r->arm_insn, 4, 7);
> + Âif (arm_insn_r->cond)
> + Â Â{
> + Â Â Â/* PLD has no affect on architectural state, it just affects
> the caches. Â*/
> + Â Â Âif (5 == ((opcode1 & 0xE0) >> 5))

This condition is never true because `opcode1 = bits
(arm_insn_r->arm_insn, 4, 7)' is value in range 0..15 therefore
`opcode1 & 0xE0' is always zero. I think.

> + Â Â Â Â{
> + Â Â Â Â Â/* BLX(1) */
> + Â Â Â Â Ârecord_buf[0] = ARM_PS_REGNUM;
> + Â Â Â Â Ârecord_buf[1] = ARM_LR_REGNUM;
> + Â Â Â Â Âarm_insn_r->reg_rec_count = 2;
> + Â Â Â Â}
> + Â Â Â/* STC2, LDC2, MCR2, MRC2, CDP2: <TBD>, cprocessor insn. Â*/

Typo cprocessor.

> +
> + Âopcode1 = bits (arm_insn_r->arm_insn, 23, 27);
> + Âif ((24 == opcode1) && bit (arm_insn_r->arm_insn, 21) &&
> + Â Â!(INSN_RECORDED(arm_insn_r)))

Operators should go to beginning of a new line, not at the end of previous line.
I.e.
if ((24 == opcode1) && bit (arm_insn_r->arm_insn, 21)
   && !(INSN_RECORDED(arm_insn_r)))

It is on multiple places involving the macro.

> + Â Â Âif (!INSN_RECORDED(arm_record))
> + Â Â Â Â{
> + Â Â Â Â Âarm_handle_insn[insn_id] (arm_record);
> + Â Â Â Â}
> + Â Â}
> + Âelse if (THUMB_INSN_SIZE_BYTES == insn_size)
> + Â Â{
> + Â Â Â/* As thumb does not have condition codes, we set negatiive. Â*/

Typo negatiive.

In function arm_record_extension_space:
The function stores some values in local variable `uint32_t
record_buf[8]' but these values are never read. I suspect you forgot
to add calls to:
+  REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count, record_buf);
+  MEM_ALLOC (arm_insn_r->arm_mems, arm_insn_r->mem_rec_count, record_buf_mem);

In function arm_record_extension_space:
The calls to INSN_RECORDED(arm_insn_r) will always return false,
because the fields referenced by the macro are always 0 at entry to
arm_record_extension_space() and the function does not modify them.
The missing calls to REG_ALLOC() & MEM_ALLOC() would modify them in
the end but it would be late anyway. I guess you will have to either
add a local flag variable or alter control flow - either way the
INSN_RECORDED seems like going to be unused.
(Function arm_record_strx() works differently and is not affected.)

In function arm_record_extension_space:
Consider adding `gdb_assert (!INSN_RECORDED(arm_insn_r))' at the
beginning of the function. That way it will be easy to understand
which part of code does change the state.

In function decode_insn:
Consider adding `gdb_assert (!INSN_RECORDED(arm_insn_r))' just before
call to arm_record_extension_space. That way it will be easy to
understand which part of code does change the state. Or use return
value from arm_record_extension_space() -  this would make the
understanding even faster.

In function decode_insn:
Consider adding these assertions at the exit:
+ gdb_assert (arm_record->mem_rec_count == 0 || arm_record->arm_mems != NULL);
+ gdb_assert (arm_record->reg_rec_count == 0 || arm_record->arm_regs != NULL);
This would catch the missing REG_ALLOC calls in arm_record_extension_space().


Issues remaining since the previous revision:

Tom Tromey or someone should decide whether to use gdb_assert or not
(in the situation described in [1]).

Probably turn field `decode' & friends in insn_decode_record_t to
local variables. (Phase 3?)

[1] http://sourceware.org/ml/gdb-patches/2011-10/msg00449.html

-- 
Petr Hluzin