Re: [PATCH] arm reversible : <phase_2_complete>

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

Hi Petr,

Thanks for your comments again.

I have tried to take care of all your comments except

1) there is contradiction between yours and Tom's comment about
gdb_assert_not_reached.
I have changed back and forth earlier.
currently it return -1, indirectly gdb throws __error which results
into record stop completely.

2) the local variable usage as you sueggsted definately improves cache
line usage and performance
I want to leave it to regression probably in phase - 3; because other
changes from Yao and you have taken good amount of time.

please excuse me If you have to repeat he comments:
as this patch has grown too much that I miss something some or the other thing

the next patch update include the implementation of arm_extension_space insn.

Regards,
Oza.

On Mon, Oct 17, 2011 at 2:32 AM, Petr Hluzín <petr.hluzin@gmail.com> wrote:
> On 15 October 2011 05:45, paawan oza <paawan1982@yahoo.com> wrote:
>> please find the patch below.
>
> I did not verify the style guidelines (spaces etc.), I am not familiar enough.
> However some places have a comma ',' at the beginning of new line - I
> think the "operator at newline" guideline does not apply to commas.
>
> I did not check ARM semantics. It looks plausible, though.
>
> The current patch (2011-10-15) is definitely an improvement to 2011-05-12.
> Only the assertion thing got worse:
>
> In arm_handle_ld_st_imm_offset_insn()
>>+ ? switch (arm_insn_r->opcode)
> In arm_handle_ld_st_reg_offset_insn():
>>+ ? switch (arm_insn_r->opcode)
>>+ ? switch (offset_12)
>>+ ? switch (arm_insn_r->opcode)
> In arm_handle_ld_st_multiple_insn()
>>+ ? switch (addr_mode)
>
> These switches seem to have `return -1;' in cases which are not
> reachable, therefore shoudl have gdb_assert_not_reached().
> The guideline - which I think Tom was reffering to - is that
> impossible states and coding bugs in gdb should trigger assertions
> however user's input (no matter how malformed) should trigger warning
> or error messages.
> (This would mean to turn the lines with `default:' to the previous
> revision. I understand this sucks.)
>
> Some situations are difficult to decide whether they are trigger-able
> by user input or not.
> If my code is not coded or intended to handle such situations I prefer
> to kill the process (or whatever are assertions configured to do) and
> get feedback from user.
> I am not familiar with GDB customs, though. Tom?
>
> Oza> + ? ? ?gdb_assert_not_reached ("no decoding pattern found");
>>
> Tom> It seems wrong to use an assert in this code. ?At least, it is not
> Tom> obvious to me that this represents a logic error in gdb as opposed to a
> Tom> merely unrecognized instruction. ?An unrecognized instruction can occur
> Tom> for many reasons, e.g., a bad jump.
>
> The switch variable `arm_insn_r->opcode' cannot be initialized to any
> value different from 0..15 because of the expression:
> arm_insn_r->opcode = bits (arm_insn_r->arm_insn, 21, 24).
> The switch variable `offset_12' cannot be initialized to any value
> different from 0..3 because of the expression: offset_12 = bits
> (arm_insn_r->arm_insn, 5, 6).
> The switch variable `addr_mode' cannot be initialized to any value
> different from 0..3 because of the expression: addr_mode = bits
> (arm_insn_r->arm_insn, 23, 24).
> It would be bit easier to see that if the variables were local (as I suggested).
> Other values are not possible to create, even with corrupted input or
> unrecognized instructions.
>
> Paawan: If Tom and I give you contradicting suggestions then you
> should complain.
>
>
> Issues remaining from my previous reviews:
>
> In arm_handle_coproc_data_proc_insn()
> + ? if (15 == arm_insn_r->opcode)
> ...
> + ? ? ? ?/* Handle arm syscall insn. ?*/
> + ? ? ? ?if (tdep->arm_swi_record != NULL)
> + ? ? ? ? ?{
> + ? ? ? ? ? ?tdep->arm_swi_record(reg_cache);
> + ? ? ? ? ?}
> ...
> + ? return -1;
>
> The function still returns -1 even when the instruction was recognized
> (15) and OS syscall support function is not NULL.
> Yes, there is no way to set it to non-NULL value yet but when it is
> implemented then you would have to do this change anyway:
> - ? tdep->arm_swi_record(reg_cache);
> + ? return tdep->arm_swi_record(reg_cache);
>
> I guess the function should use `arm_insn_r' argument to record the
> changes - which is missing.
> In thumb_handle_swi_insn() the situation is the opposite: it returns 0
> no matter what the arm_swi_record() returns.
>
>
> In arm_handle_data_proc_misc_ld_str_insn()
>>> + ?struct
>>> + ? ?{
>>> + ? ? ?ULONGEST unsigned_regval;
>>> + ? ?} u_buf[2];
>>>
>>> You can get the same result (and simpler code) with
>>> ULONGEST u_buf[2];
>>> or maybe also better name with
>>> ULONGEST u_regvals[2];
>>>
>>> The same applies to other functions.
>>
>> Oza: It is correct, it was mis-evolved as inistially it was union with 2 members
>> and I fixed Tom’s review comments for endianness. I will change this, but pelase
>> do not mind if it is not there in the immediate versions of patch, eventually
>> after testing it will be changed.
>
> This is still true.
>
>
>> Oza: please report as I don’t have any automated tool which reports them, if it
>> is not effort for you please report all if possible. Gcc also give some warning
>> about unused one, but not sure about gdb warning suppression.
>
> Unused local variables, as requested:
> arm_handle_ld_st_imm_offset_insn: reg_src2, immed_high, immed_low
> arm_handle_ld_st_reg_offset_insn: immed_high, immed_low
> arm_handle_ld_st_multiple_insn: shift_imm, reg_src2
> arm_handle_coproc_data_proc_insn - all of them: shift_imm, reg_src1,
> reg_src2, addr_mode, start_address
> thumb_handle_ld_st_imm_offset_insn: reg_val1
> thumb_handle_ld_st_stack_insn: reg_val1
> thumb_handle_misc_insn: reg_val1, reg_src1 - write-only in case `(2 ==
> opcode)', immed_8, immed_5
> thumb_handle_swi_insn: reg_val1
> thumb_handle_branch_insn: reg_val1, reg_src1, opcode, immed_5
>
> Typos:
> preccedded -> precceded (3x), Accorindly -> Accordingly (2x),
> addresing -> addressing, optmization -> optimization, Wihtout
> optmization ?-> Without optimization,
>
> Otherwise the patch looks good.
>
> --
> Petr Hluzin
>