Re: [PATCH] FreeBSD powerpc support

Pedro Alves
2012-11-23T19:34:10Z

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