Re: [PATCH] arm reversible : <phase_2_complete>

[oza Pawandeep <oza dot pawandeep at gmail dot com>]

Hi All,

sorry for delay in reply. was on vacation.
I have already addressed the comments from both Petr and Yao in the next patch.
the comments which I have not fixed please find the explanation below.

Petr >>
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.)

Oza >> I have changed the condition itself now.
you were right; after changing now it makes sense.


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


Oza >> because calling function takes care of REG_ALLOC routine calls.
I did not want ALLOC calls to be scattered into any other function
other than main decoding functions.


Petr >>
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.

Oza >> you are right, I fixed it now.
Oza >> typo mistakes are corrected.

Petr >> Operators should go to beginning of a new line, not at the end
of previous line.

Oza >> corrected.

Petr >>
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);



Oza >> yes, thanks for this comment, I added them now.


Petr >> n 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().


Oza >> the whole logic is falling in place now, everything is taken
care with the change of the MACRO IS_INSN_RECORDED.


Yao >> macro THUMB2_INSN_SIZE_BYTES is never used.  Please remove it.

Oza >> it is intropduced again as required in the logic which you have proposed.



Yao >> This function is used to match opcode for instructions.  Why don't
you use bit operation (AND and OR) and logic operation to match
instruction?  Bit operation and logic operation are widely used in
gdb.  It is efficient and easy to read.  I suggest to replace
sbo_sbz by bit/logical operation when matching instruction.


Oza >> I am not sure what you meant by re-writting sbo_sbz function.
as there is already '&' and '!' and '>>' which all are bit operators.

Yao >> 15 is a magic number here and somewhere else.
Please use ARM_PC_REGNUM.

Oza >> corrected.

Yao >> Comment here is confusing.  STRH (register) and STRH (immediate) are
handled here, but comment doesn't reflect this.

Oza >> done

Yao >> The name of function is incorrect.  Better to name it
"thumb_record_ldm_stm_swi", or something similar.

Oza >> done

Yao >> Again, don't have to store register PS for BL.

Oza >> done

Yao >> This kind of design leaves thumb2-32 bit support difficult in the
future.  We can let the caller, arm_process_record, to differentiate
arm/thumb 16bit/thumb 32bit, and call different decode routines
respectively, like this,

Oza >> proposed logic incorporated in caller.

Regards,
Oza.


On Mon, Nov 7, 2011 at 9:08 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Petr" == Petr Hluzín <petr.hluzin@gmail.com> writes:
>
> Petr> How strong is the "can't happen" requirement? Is programmer required
> Petr> to prove the assertion cannot be triggered? The difficulty in general
> Petr> case would be equivalent to proving a code is bug-free - i.e. it is
> Petr> impossible. At my job we try to verify an assumption until we have a
> Petr> sort-of-proof or the search becomes difficult and no clue/indication
> Petr> to the contrary has been found yet.
> Petr> I am asking because GDB code contains surprisingly few assertions.
>
> I don't think there is a hard-and-fast rule in gdb. ?I am not completely
> sure though.
>
> I think in gdb it is best to error instead of assert if there is any
> doubt. ?That's because I think when people turn to gdb they are already
> a bit frustrated, and then if gdb itself fails, this is extremely
> irritating. ?That's certainly been the case for me in the past.
>
> I realize you can sort of recover from internal_error (and hence
> assertions). ?But my impression is that internal_error is treated like
> "not an exception-thrower" in gdb, leading to cleanup problems and the
> like.
>
> Petr> Anyway, the patch had used assertions correctly because expression
> Petr> `bits (X, 21, 24)' yields value in range 0..15 for any value of X - no
> Petr> matter how bogus X value. Yes, all 2^32 values map to 0..15. The
> Petr> assertions satisfy the guideline. (This is not even the hard-to-prove
> Petr> case I was speculating above.)
>
> Yeah, my review was wrong here.
>
> Tom
>