Re: [PATCH] arm reversible : <phase_2_complete>

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

Hi Petr,

I have updated the patch with your comments; please find the explanation below.

> In arm_process_record():
> Expression `insn_id = bits (arm_record.arm_insn, 11, 15);' uses value
> of `arm_record.arm_insn'
> which is always zero. When you moved the test from decode_insn() I
> guess forgot to copy these lines:
> + ? ? ?arm_record->arm_insn = (uint32_t) extract_unsigned_integer
> (&u_buf.buf[0],
> + ? ? ? ? ? THUMB_INSN_SIZE_BYTES, gdbarch_byte_order (arm_record->gdbarch));
>
>
Oza : done, corrected, thanks for this comment.

> Macro INSN_RECORDED:
> I suggested INSN_IS_RECORDED to make it clear it is boolean, Tom
> Tromey did not comment on that, Oza left the name as is. Well, it is
> not a big deal.
> However arguments of macros should be surrounded by parentheses - like this:
> (ARM_RECORD)->reg_rec_count
> This is necessary if INSN_RECORDED() is passed more complex expression
> when the order of evaluation matters. This is a common C issue and
> customary solution, it is not GDB specific.
Oza: have taken care of parenthesis stuff and added; and comment is
also added to make it more clear.

>> Petr >>In arm_record_strx()
>> Petr >>Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syntax?
>>
>> 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.

oza: Ahhhh.....I got you now; I have changed, but personally my coding
style have never let me think that pointer style is unusual as
pointers make the things more clear in my mind. because it always
reminds me when i look at the code after a long time that I am playing
with address : )    [of course your array syntax is also nothing but
address, but probably its just my mind thinks that way)
I have changed it as you suggested.

>
> Have you considered adding the assertions I suggested in [2]? They are
> not necessary but they improve understanding of code.

Oza: yes I have added assert as you suggested.
arm_record_extension_space already returns positive/negative value
and decode_insn already catching the return value, and one more point
is some insns may not be supported so in that case the code must reach
back to process_record and its caller to throw correct record error.

PS: next mail will contain the latest patch

Regards,
Oza.

On Sat, Dec 3, 2011 at 10:01 PM, Petr Hluzín <petr.hluzin@gmail.com> wrote:
> I did not review the 2011-11-19 nor 2011-11-09 patch.
>
> Tom:
>> Yeah, it is a bit of a pain. ?I wish we had a tool that could reformat
>> the code according to our standards.
>
> I guess 'indent' could do that. Its default settings formats using GNU
> guidelines.
> Some editors can be configured to automatically format new lines and
> have a button to reformat existing lines. I suppose no editor can
> autodetect the style from file yet.
>
>
> Review of the 2011-11-03 patch:
>
> In arm_process_record():
> Expression `insn_id = bits (arm_record.arm_insn, 11, 15);' uses value
> of `arm_record.arm_insn'
> which is always zero. When you moved the test from decode_insn() I
> guess forgot to copy these lines:
> + ? ? ?arm_record->arm_insn = (uint32_t) extract_unsigned_integer
> (&u_buf.buf[0],
> + ? ? ? ? ? THUMB_INSN_SIZE_BYTES, gdbarch_byte_order (arm_record->gdbarch));
>
>
> Remaining from previous reviews:
>
> Macro INSN_RECORDED:
> I suggested INSN_IS_RECORDED to make it clear it is boolean, Tom
> Tromey did not comment on that, Oza left the name as is. Well, it is
> not a big deal.
> However arguments of macros should be surrounded by parentheses - like this:
> (ARM_RECORD)->reg_rec_count
> This is necessary if INSN_RECORDED() is passed more complex expression
> when the order of evaluation matters. This is a common C issue and
> customary solution, it is not GDB specific.
>
>> Petr >>In arm_record_strx()
>> Petr >>Why don't you use `record_buf_mem[0]' and `record_buf_mem[1]' syntax?
>>
>> 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.
>
> The lines can be converted from `*(record_buf_mem + 1)' to
> `record_buf_mem[1]' with no changes to REG_ALLOC() calls. The
> allocation calls would remain unscattered in main decoding functions.
> The code is correct, but I do not understand why you use the unusual
> syntax.
>
> Have you considered adding the assertions I suggested in [2]? They are
> not necessary but they improve understanding of code.
>
> [2] http://sourceware.org/ml/gdb-patches/2011-10/msg00617.html
>
> --
> Petr Hluzin