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

["Ulrich Weigand" <uweigand@de.ibm.com>]

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