Re: [PATCH] FreeBSD powerpc support

[Pedro Alves <palves@redhat.com>]

Hello,

On 11/19/2012 09:43 PM, Andreas Tobler wrote:
> Hi all,
>
> After a longer timeout here again :)
>
> The attached patch adds support for FreeBSD PowerPC, 32-bit and 64-bit.
> It is derived from ppcobsd* and ppc-linux with FreeBSD additions.
>
> There is room for improvement :) And I will continue, but I'd like to
> see this patch in the gdb cvs because it will be much easier for me fix
> outstanding issues when I can work with cvs iso. of local revisions.

Eh.  If it's easier, then maybe you're not using the proper tools; there's
always quilt.  :-)  Or better nowadays, you could also put it
in a public git repo somewhere.  We have a git mirror of cvs.
That said, I'm really not against putting it in early, if it's not riddled
with hacks.

> Also, other people might have a use of this work, even if not complete.
>
> Currently missing/incomplete:
> - Altivec support, kernel bits are missing.
> - HW watchpoints, also kernel bits are missing.
> - thread support.
> - tls support.
> - some sig tests.

I've skimmed the patch, and didn't notice anything horrible.
Then again, I'm on the low end of the scale that measures
PowerPC or FreeBSD expertness...

- Please make sure there's a blank line between introductory comment
  and function.

- I noticed that a few functions don't have introductory comment.

- If the function implements of a target/gdbarch/etc. method, then
  comment it as such.  E.g.,

  /* This is the implementation of gdbarch method FOOBAR.  */

- I noticed some functions with long comments are copies of existing
  code of other ports.  I wonder if we could perhaps share more code.


> +/* Read a PPC instruction from memory.  PPC instructions are always
> + *  big-endian, no matter what endianness the program is running in, so
> + *  we can't use read_memory_integer or one of its friends here.

read_memory_unsigned_integer nowadays has a byte_order parameter,
so just pass it BFD_ENDIAN_BIG, and you're set.

> + */
> +static unsigned int
> +read_insn (CORE_ADDR pc)
> +{
> +  unsigned char buf[4];
> +
> +  read_memory (pc, buf, 4);
> +  return (buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];
> +}
> +

> +
> +#define PPC64_STANDARD_LINKAGE2_LEN \
> +  (sizeof (ppc64_standard_linkage2) / sizeof (ppc64_standard_linkage2[0]))
> +

Use the existing ARRAY_SIZE macro.

+/* Signal trampolines.  */
> +
> +static int
> +ppcfbsd_sigtramp_frame_sniffer (const struct frame_unwind *self,
> +				struct frame_info *this_frame,
> +				void **this_cache)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  CORE_ADDR pc = get_frame_pc (this_frame);
> +  CORE_ADDR start_pc = (pc & ~(ppcfbsd_page_size - 1));
> +  const int *offset;
> +  const char *name;
> +
> +  find_pc_partial_function (pc, &name, NULL, NULL);
> +  if (name)
> +    return 0;

For some reason this bailing out if name is not null jumped at me.
It's not obvious to me what that means, so it felt like it deserves
a comment.

On 11/19/2012 09:43 PM, Andreas Tobler wrote:
> --- configure.host	30 May 2012 19:41:34 -0000	1.107
> +++ configure.host	19 Nov 2012 21:24:15 -0000
> @@ -125,6 +125,7 @@
>
>  powerpc-*-aix* | rs6000-*-*)
>  			gdb_host=aix ;;
> +powerpc*-*-freebsd*)	gdb_host=fbsd ;;

This seems to be 'powerpc-*-freebsd*' elsewhere I looked (top level, bfd).
Why the extra wildcard?

-- 
Pedro Alves