This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
PING Re: [PATCH] FreeBSD powerpc support
- From: Andreas Tobler <andreast-list at fgznet dot ch>
- To: Pedro Alves <palves at redhat dot com>, gdb-patches at sourceware dot org
- Date: Tue, 11 Dec 2012 16:48:08 +0100
- Subject: PING Re: [PATCH] FreeBSD powerpc support
- References: <50AAA80B.1000509@fgznet.ch> <50AFCF95.1080809@redhat.com> <50B4FC29.9050006@fgznet.ch>
On 27.11.12 18:45, Andreas Tobler wrote:
> Hello again,
>
> took a bit longer.
>
> On 23.11.12 20:33, Pedro Alves wrote:
>
>>> 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.
>
> Might be that I do not use the latest and greatest tools.
> The room for improvement, above, is in the direction of general FreeBSD
> stuff, not only limited to this particular port.
>
>>> 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 hope I didn't miss them.
>
>> - I noticed that a few functions don't have introductory comment.
>
> Also, I put one in where I could.
>
>> - 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.
>
> True, here I do not know how to share, maybe a common ppc64-tdep-common.c?
>
>>> +/* 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.
>
> Done. Even tested on ppc64-linux. I might send a patch for
> ppc-linux-tdep.c as well. Likewise for the below.
>
>>> +#define PPC64_STANDARD_LINKAGE2_LEN \
>>> + (sizeof (ppc64_standard_linkage2) / sizeof (ppc64_standard_linkage2[0]))
>>> +
>>
>> Use the existing ARRAY_SIZE macro.
>
> Done.
>
>> +/* 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.
>
> Also done, I hope I match the expectations.
>
>> 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?
>
>
> Hm, here I'm not sure. My targets report as powerpc-unknown-freebsd*
> (32-bit) and powerpc64-unknown-freebsd* (64-bit). So I thought I have to
> match both. Is this not correct?
>
> Attached a revised version of the diff.
>
> Pedro, again thank you very much for the feedback.
ping, anything I miss? Or is it the usual pre x-mas business ;)
TIA,
Andreas