Re: [PATCH] arm reversible : <phase_2_complete>

[Petr HluzÃn <petr dot hluzin at gmail dot com>]

2011/12/4 oza Pawandeep <oza.pawandeep@gmail.com>:
> Hi,
>
> 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.
>
> 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.
> +
> + Â Â Â * 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.
> + Â Â Â Â Â> 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
> + Â Â Â Â > arm_process_record externed.
> +
> +

WOW, thats a lot of text. Other people's changelog entries are more
terse. For example the GNU guidelines say "For example, âNew functionâ
is enough for the change log when you add a function, because there
should be a comment before the function definition to explain what it
does."

In function decode_insn():
> +
> + Âif (extract_arm_insn (arm_record, insn_size))
> + Â Â{
> + Â Â Âif (record_debug)
> + Â Â Â Â{
> + Â Â Â Â Âprintf_unfiltered (_("Process record: error reading memory at "
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"addr %s len = %d.\n"),
> + Â Â Â Â Âpaddress (arm_record->gdbarch, arm_record->this_addr), insn_size);
> + Â Â Â Â Âreturn -1;
> + Â Â Â Â}
> + Â Â}

If the extract_arm_insn() call fails then the function returns only if
`record_debug' is true. This is strange. Enabling debugging traces
usually expected to only affect printing, here it affects behavior of
the decode_insn() function. Plus the rest of the function seems to
misbehave if it is passed failed result in `arm_record'. I guess the
`return -1;' statement should be moved outside of body of condition
`if (record_debug)'. Unortunatelly I did not notice that earlier.

In arm_process_record():
> +
> + Âif (extract_arm_insn (&arm_record, 2))
> + Â Â{
> + Â Â Âif (record_debug)
> + Â Â Â Â{
> + Â Â Â Â Âprintf_unfiltered (_("Process record: error reading memory at "
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â "addr %s len = %d.\n"),
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â paddress (arm_record.gdbarch,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â arm_record.this_addr), 2);
> + Â Â Â Â Âreturn -1;
> + Â Â Â Â}
> + Â Â}

The same applies here.

Except the one last thing the patch looks good to me now. My previous
suggestions have been resolved.
As always, I did not check whitespace, ARM semantics and Changelog entries.

-- 
Petr Hluzin