Re: [PATCH] arm reversible : <phase_2_complete>

Yao Qi
2011-12-04T14:46:07Z

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 (é½å°§)