Re: [PATCH 2/3 v4] Process record support for PowerPC

Ulrich Weigand
2015-01-19T18:05:09Z

Wei-cheng Wang wrote:

> 2015-01-18 Wei-cheng Wang <cole945@gmail.com>
>
> * rs6000-tdep.c (ppc_process_record_op4, ppc_process_record_op19,
> ppc_process_record_op31, ppc_process_record_op59,
> ppc_process_record_op60, ppc_process_record_op63,
> ppc_process_record): Fix -Wformat warning.
> * rs6000-tdep.c (rs6000_epilogue_frame_cache, ppc_process_record_op60):
> Remove unused variables.

This is OK. Please check this in to fix the reported build issues.

> 2015-01-18 Wei-cheng Wang <cole945@gmail.com>
>
> * ppc-linux-tdep.c (ppc_skip_trampoline_code,
> ppc_canonicalize_syscall, ppc_linux_syscall_record,
> ppc_linux_record_signal, ppc_init_linux_record_tdep): Add comments.
> * ppc64-tdep.c (ppc64_skip_trampoline_code): Likewise.
> * rs6000-tdep.c (rs6000_epilogue_frame_cache,
> rs6000_epilogue_frame_this_id, rs6000_epilogue_frame_prev_register,
> rs6000_epilogue_frame_sniffer, ppc_record_vsr, ppc_process_record_op4,
> ppc_process_record_op19, ppc_process_record_op31,
> ppc_process_record_op59, ppc_process_record_op60,
> ppc_process_record_op63): Likewise.

A couple of minor issues:

> +/* Record registers which might be clobbered during system call.
> + Return 0 if succeed. */

I think proper English usage would be "Return 0 on success." or
"Return 0 if sucessful." (Here and elsewhere in the patch.)

> -/* Initialize linux_record_tdep if not initialized yet. */
> +/* Initialize linux_record_tdep if not initialized yet.
> + WORDSIZE is 4 or 8 for 32- or 64-bit PowerPC Linux respectively.
> + Sizes of data structures are initilzied accordingly. */

Typo "initilzied".

Otherwise, this is OK as well.

Two other things as an aside:

- When committing a patch to git, please add the ChangeLog to the
git commit message as well. Have a look with "git log" at the
existing practice how to format the commit messages for GDB.

- Given that you've now got commit access, please also add your name
to the MAINTAINERS file under "Write After Approval"; see here for
a recent example:

https://sourceware.org/ml/gdb-patches/2015-01/msg00517.html

Thanks,
Ulrich

--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
Ulrich.Weigand@de.ibm.com