This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Pedro,

On 21.01.13 16:49, Pedro Alves wrote:

> 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).

First, thank you for the review.

I attached a revised version of the patch including the CL below. I hope
I matched your points.
I decided to leave ppc64-linbsd-tdep.c away and use ppc64-tdep.c|h with
the changes mentioned below to ppc-tdep.h/rs6000-tdep.c

Tested on ppc64-linux and on FreeBSD amd64 with an --enable-targets=all.

>> +/* 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.

Done.

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

Done with ppc_ since the instructions could be used on ppc and ppc64

> 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".

Done, not sure if I need the extern on ppc64-tdep.h too?

> Otherwise looks fine to me.

Again, thank you!

Andreas

2013-01-26  Andreas Tobler  <andreast@fgznet.ch>

	* Makefile.in (ALL_TARGET_OBS): Add new file ppc64-tdep.o.
	(HFILES_NO_SRCDIR): Likewise.
	(ALLDEPFILES): Likewise.
	* configure.tgt: Add new file for powerpc-linux.
	* ppc64-tdep.h: New file.
	* ppc64-tdep.c: New file.
	(insn_d, insn_ds, insn_xfx, 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.
	* rs6000-tdep.c:
	(read_insn): Move from ppc-linux-tdep.c to here.
	(insns_match_pattern, insn_d_field, insn_ds_field): Move
	from ppc-linux-tdep.c to here and rename them with the ppc_ prefix.
	* ppc-linux-tdep.c: Include ppc64-tdep.h.
	Removed above functions.
	(ppc_linux_init_abi): Rename
	ppc64_linux_convert_from_func_ptr_addr to
	ppc64_convert_from_func_ptr_addr.




Attachment: ppc64_tdep_1diff
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]