PING Re: [PATCH] FreeBSD powerpc support

[Andreas Tobler <andreast-list at fgznet dot 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