Re: [PATCH] arm reversible : <phase_2_complete>

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

Hi Yao,

I have modified it; please have a look below.
I have observed that some people have given little description about
functions: but some places its missing.
I think it is better to give small description:

2011-12-03  Oza Pawandeep   <oza.pawandeep@gmail.com>

	* arm-linux-tdep.c (arm_linux_init_abi): Call
       set_gdbarch_process_record.
       Initialize `arm_swi_record' field.

	* arm-tdep.c (arm_process_record): New function.
	(deallocate_reg_mem): New function.
	(decode_insn): New function.
	(thumb_record_branch): New function.
	(thumb_record_ldm_stm_swi(): New function.
	(thumb_record_misc): New function.
	(thumb_record_ld_st_stack): New function.
	(thumb_record_ld_st_imm_offset): New function.
	(thumb_record_ld_st_reg_offset(): New function.
	(thumb_record_add_sub_cmp_mov): New function.
	(thumb_record_shift_add_sub): New function.
	(arm_record_coproc_data_proc): New function.
	(arm_record_coproc): New function.
	(arm_record_b_bl): New function.
	(arm_record_ld_st_multiple): New function.
	(arm_record_ld_st_reg_offset): New function.
	(arm_record_ld_st_imm_offset): New function.
	(arm_record_data_proc_imm): New function.
	(arm_record_data_proc_misc_ld_str): New function.
	(arm_record_extension_space): New function.
	(arm_record_strx): New function.
	(sbo_sbz): New function.
	(struct insn_decode_record): New Structure. arm insn record
	(REG_ALLOC): New Macros. use it for reg allocations
	(MEM_ALLOC): New Macros. use it for memory allocations

	* arm-tdep.h (struct gdbarch_tdep): New field 'arm_swi_record'


Regards,
Oza.

On Sun, Dec 4, 2011 at 8:15 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 12/04/2011 07:32 PM, oza Pawandeep wrote:
>> Updated patch contains all the 3 review comments fixed (Tom, Petr and Yao).
>> Change log is elaborated; I am looking forwarding to adding more
>> details if need. seeking for your feedback on the same.
>>
>
> The format of ChangeLog looks odd to me. ÂSome comments below,
>
>> diff -urN arm_orig/ChangeLog arm_new/ChangeLog
>> --- arm_orig/ChangeLog    Â2011-12-03 18:05:04.000000000 +0530
>> +++ arm_new/ChangeLog 2011-12-04 16:45:00.000000000 +0530
>> @@ -1,3 +1,65 @@
>> +2011-12-03 ÂOza Pawandeep  <oza.pawandeep@gmail.com>
>> +
>> + Â Â * arm-linux-tdep.c: arm_linux_init_abi modified to include
>> + Â Â Â Âarm reversible debugging feature.
>> + Â Â Â Â Âregistered arm_process_record to gdb_arch
>> + Â Â Â Âsyscall pointer initialization.
>
> It can be
>
> Â Â Â Â* arm-linux-tdep.c (arm_linux_init_abi): Call
> Â Â Â Âset_gdbarch_process_record.
> Â Â Â ÂInitialize `arm_swi_record' field.
>
> Please care about the format of changelog entry, usually it is like this,
>
> [tab] Â * file-you-modified.c (function-you-modified): What is your
> [tab] Â Change.
>
> Note that we write "what is changed" in ChangeLog, instead of "why is
> changed".
>
>> +
>> + Â Â * arm-tdep.c: arm-reversible-debugging implementation.
>> + Â Â Â newly added functions are as follows.
>> +
>> + Â Â Â Â Â> arm_process_record: handles basic initialization and record
>> + Â Â Â Â Â Â summarisation, decodes basic insn ids, on which it hands over
>> + Â Â Â Â Â Â controls to decode_insn.
>
> So, we don't need so much details in changelog like this, we can write
> it in this way,
>
> Â Â Â Â* arm-tdep.c (arm_process_record): New.
>
> There are still many new added functions, and I think they can
> documented in ChangeLog in the same way.
>
>> + Â Â Â Â Â> deallocate_reg_mem : clean up function
>> + Â Â Â Â Â> decode_insn: Decodes arm/thumb insn and calls appropriate
>> + Â Â Â Â Â Â decoding routine to record the change.
>> + Â Â Â Â Â> thumb_record_branch: branch insn reording (thumb)
>> + Â Â Â Â Â> thumb_record_ldm_stm_swi: load, store and sycall insn
>> + Â Â Â Â Â Â recoding Â(thumb)
>> + Â Â Â Â Â> thumb_record_misc: misc insn recording Â(thumb)
>> + Â Â Â Â Â> thumb_record_ld_st_stack: store and stack insn recording Â(thumb)
>> + Â Â Â Â Â> thumb_record_ld_st_imm_offset: load, store with immediate offset
>> + Â Â Â Â Â Â insn recording Â(thumb)
>> + Â Â Â Â Â> thumb_record_ld_st_reg_offset: load, store with register offset
>> + Â Â Â Â Â Â recording Â(thumb)
>> + Â Â Â Â Â> thumb_record_add_sub_cmp_mov: addition, subtractation, compare
>> + Â Â Â Â Â Â and move insn recording Â(thumb)
>> + Â Â Â Â Â> thumb_record_shift_add_sub: shift, add and sub insn recording
>> + Â Â Â Â Â Â (thumb)
>> + Â Â Â Â Â> arm_record_coproc_data_proc: coprocessor and data processing
>> + Â Â Â Â Â Â recording (partially implemented) (arm)
>> + Â Â Â Â Â> arm_record_coproc: coprocessor insn recording
>> + Â Â Â Â Â Â (partially implemented) (arm)
>> + Â Â Â Â Â> arm_record_b_bl: branch insn recording (arm)
>> + Â Â Â Â Â> arm_record_ld_st_multiple: load and store multiple insn recording
>> + Â Â Â Â Â Â (arm)
>> + Â Â Â Â Â> arm_record_ld_st_reg_offset: load and store reg offset recording
>> + Â Â Â Â Â Â (arm)
>> + Â Â Â Â Â> arm_record_ld_st_imm_offset: load and store immediate offset
>> + Â Â Â Â Â Â recording (arm)
>> + Â Â Â Â Â> arm_record_data_proc_imm: data processing insn recording Â(arm)
>> + Â Â Â Â Â> arm_record_data_proc_misc_ld_str: data processing, misc, load and
>> + Â Â Â Â Â Â store insn recording Â(arm)
>> + Â Â Â Â Â> arm_record_extension_space:arm extension space insn recording
>> + Â Â Â Â Â Â (arm)
>> + Â Â Â Â Â> arm_record_strx: str(X) type insn recording Â(arm)
>> + Â Â Â Â Â> sbo_sbz: checks for mendatory sbo and sbz fields in insn,
>> +
>> + Â Â Â Â Âadded new data structures:
>> + Â Â Â Â Â> insn_decode_record_t: local record structure which contains insn's
>> + Â Â Â Â Ârecord, which includes both reg and memory.
>> +
>> + Â Â Â Â ÂREG_ALLOC and MEM_ALLOC macros takes care of actual memory allocation
>> + Â Â Â Â Âin local record which is finally processed by arm_rocess_record.
>> +
>> + Â Â * arm-tdep.h: arm-reversible data structures
>> +
>> + Â Â Â > modified gdbarch_tdep: added member (function pointer) arm_swi_record
>> + Â Â Â Â Âwhich is supposed to be recording system calls
>
> This can be written in this way,
>
> Â Â Â Â* arm-tdep.h (struct gdbarch_tdep): New field `arm_swi_record'.
>
>> + Â Â Â > arm_process_record externed.
>> +
>> +
>
> Again, reading other existing changelog entries must be helpful for you
> to write your own in a correct way/format. :)
>
> --
> Yao (éå)