Re: [RFC] patch to refactor ppc64 specific code from ppc-linux-tdep

[Pedro Alves <palves@redhat.com>]

Hi Andreas,

On 12/27/2012 09:36 PM, Andreas Tobler wrote:

> in order to avoid code duplication for the FreeBSD powerpc port I
> started to cut off common code from ppc-linux-tdep.c into a new file to
> be used for FreeBSD and GNU/Linux PowerPC 64-bit. The file name is open
> so far. Better namings are welcome.

Thanks for doing this.

IMO, the "common" monitor is practically meaningless, and raises the question
of what's different between ppc64-common-tdep.c, and
an hypothetical ppc64-tdep.c, and/or the rs6000-tdep.c, as the
second would be about common ppc64 bits, and the latter, extant, _is also_
about common ppc bits.  IOW, if we need a new ppc function in the future that's
be used by several OSs, how do we decide where it goes?  "common" doesn't
give any clue.  So I'd suggest ppc64-linbsd-tdep.c, and/or
rs6000-tdep.c/ppc64-tdep.c, for shared code that is not OS specific (yeah,
rs6000-tdep.c is a misnomer nowadays).

> 
> Attached my first attempt, tested on GNU/Linux ppc64, Fedora 17 and on
> x86_64-*freebsd* with --eanble-targets=all. On the Linux side I do not
> see any regression.
> So far I have only covered functions which I can use on FreeBSD
> powerpc64. There might be others too but I do not see any for now.
> 
> I'd appreciate comments, corrections.
> 
> TIA,
> Andreas
> 
> 2012-12-19  Andreas Tobler  <andreast@neon.andreas.nets>
> 
> 	* Makefile.in (ALL_TARGET_OBS): Add new file ppc64-common-tdep.o

Missing period.

> 	(HFILES_NO_SRCDIR): Likewise.
> 	(ALLDEPFILES): Likewise.
> 	* configure.tgt: Add new file for powerpc-linux.
> 	* ppc64-common-tdep.h: New file.
> 	* ppc64-common-tdep.c New file.
> 	(insn_d, insn_ds, insn_xfx, read_insn)
> 	(insns_match_pattern, insn_d_field, insn_ds_field)
> 	(ppc64_desc_entry_point): Move from ppc-linux-tdep.c to here.
> 	(PPC64_STANDARD_LINKAGE1_LEN, PPC64_STANDARD_LINKAGE2_LEN)
> 	(PPC64_STANDARD_LINKAGE2_LEN): Likewise and use ARRAY_SIZE macro.
> 	(ppc64_standard_linkage1_target, ppc64_standard_linkage2_target)
> 	(ppc64_standard_linkage3_target, ppc64_skip_trampoline_code): Move
> 	from ppc-linux-tdep.c to here.
> 	(ppc64_convert_from_func_ptr_addr): Rename it from
> 	ppc64_linux_convert_from_func_ptr_addr to
> 	ppc64_convert_from_func_ptr_addr and move it from ppc-linux-tdep.c to
> 	here.
> 	* ppc-linux-tdep.c: Include ppc64-common-tdep.h.
> 	Removed above functions.
> 	(ppc_linux_init_abi): Rename
> 	ppc64_linux_convert_from_func_ptr_addr to
> 	ppc64_linux_convert_from_func_ptr_addr.



> +/* An instruction to match.  */
> +
> +struct insn_pattern
...
> +
> +int
> +insns_match_pattern (CORE_ADDR pc, struct insn_pattern *pattern,
> +		       unsigned int *insn);
> +CORE_ADDR
> +insn_d_field (unsigned int insn);

These look like basic architecture/instruction pattern matching.
I suggest moving to ppc-tdep.h/rs6000-tdep.c instead.

If we're making these extern, then it'd be good to add a "ppc_" or
"ppc64_" prefix (could be a separate step).

Also, GDB's convention is that the function name goes on the first
column in function definitions only, not declarations, and that
declarations in .h files get an explicit "extern".

Otherwise looks fine to me.

-- 
Pedro Alves