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: [PATCH] FreeBSD powerpc support


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.

Andreas

Attachment: gdb_ppc_20121127-1.diff
Description: Text document


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