Re: [PATCH] arm reversible : <phase_2_complete>

[oza Pawandeep <oza.pawandeep@gmail.com>]

Hi Yao,

I have implemented your comments, and will be sending the patch soon.

please find my comments below on some of your comments.

1) all the lines are truncated to < 80 chars now:

2) all the trailing spaces are deleted.

3) fixed indentation as much as I could find any.

4) Yao: arm_handle_brn_insn" is confusing, because there is no insn "brn".  I
would name this function as "arm_record_b_bl_blx

Oza: the name is changed, and rest of the names are also changed as
you suggested.

5)
Yao:
We don't have to record PS and LR for these three kinds of insns.  For
B, we don't have to do anything here.  We only record LR for BL and
record LS and PS for BLX.

Oza: yes, but in that case we just need to handle BL insn.
because BLX(1) reside in extension space which is/should be handled
separately in function
handle_instruction_space function.

so, BLX(1) will not fall here at all.

6)
Yao: It is not correct to me.  Both thumb 16-bit and 32-bit insn are handled
here, so it is wrong to pass THUMB_INSN_SIZE_BYTES which is 2.

Oza: yes currently we have support for only 16 bit thumb.
the moment we support 32 bit, we will have one more else case with
THUMB_FULL_INSN_SIZE_BYTES 4

it will look like this

else if (ARM_INSN_SIZE_BYTES == insn_size)
{
}
else if (THUMB_INSN_SIZE_BYTES == insn_size)
{
}
else if (THUMB_WORD_INSN_SIZE_BYTES == insn_size)
{
}

name is just an example:
if we want to handle thumb in same branch then could be changed
easily, depending on the writer of the code.
but as of now code doesnt support, so havent thought about it.

7) Yao: I don't see any function is installed on this function pointer.

Oza: yes, because phase 3 implementation is still pending which is
supposed to include system call function pointer and support of
syscall reverse.

Regards,
Oza.



On Sat, Oct 15, 2011 at 9:04 PM, oza Pawandeep <oza.pawandeep@gmail.com> wrote:
> Hi Yao,
>
> first of all thank you for your comments, will be sending the patch
> soon with your comments incorporated as much as possible.
> thank you again for sending test results;
>
> I suppose failed test case might be including
>
> 1) system call support
> 2) signal support
> 3) any other linux ABI support
> 4) there are some programs on x86 assembly which needs to be written
> for ARM and put separately.
>
> Thanks Yao,
> Oza.
>
>
> On Sat, Oct 15, 2011 at 6:02 PM, Yao Qi <yao@codesourcery.com> wrote:
>> On 10/15/2011 05:31 PM, Yao Qi wrote:
>>> I noticed that test cases in gdb.reverse are *not* enabled in default.
>>> You need to add the following two lines in your board file to turn them on,
>>>
>>>   set_board_info gdb,use_precord 1
>>>   set_board_info gdb,can_reverse 1
>>>
>>> It is good if you can post the test result in gdb.reverse so that we are
>>> more confidient to your patch.
>>       ^^^^^^^^^^ typo "confident".
>>
>> I run test cases in gdb.reverse with my own board file [1], and get
>> following result on x86:
>>
>>                === gdb Summary ===
>>
>> # of expected passes            2774
>> # of unexpected failures        22
>>
>> I also applied your patch, and run gdb.reverse test cases on arm natively:
>>
>>                === gdb Summary ===
>>
>> # of expected passes            1180
>> # of unexpected failures        1090
>> # of expected failures          142
>>
>> I don't think these fails are all related to your arm-reverse patch.
>> IMO, it means there are still some work to do, not only in arm bits, but
>> also in gdb general parts and test cases.  Sorry that I can't give any
>> useful suggestion here.
>>
>> --
>> Yao (齐尧)
>>
>> [1] My board file reverse.exp
>>
>> load_generic_config "unix"
>> process_multilib_options ""
>>
>> # The default compiler for this target.
>> set_board_info compiler "[find_gcc]"
>>
>> set_board_info gdb,can_reverse 1
>> set_board_info gdb,use_precord 1
>>
>