Re: [PATCH] FreeBSD powerpc support

[Pedro Alves <palves@redhat.com>]

On 11/27/2012 05:45 PM, Andreas Tobler wrote:

>> - 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?

The tdep files are usually called ARCH-OS-tdep.c, so that would be
ppc64-common-tdep.c.  That raises the question of what's  different between
ppc64-common-tdep.c and ppc64-tdep.c.  Clearer alternatives could be
ppc64-unix-tdep.c or some other token that describes the commonality between
what's being shared (as opposed to just the fact that something is shared,
as in "common").  In any case, the name of the file is the minor detail.


>> 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.

That'd be nice.

>> 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.

You have, thanks.

> 
>> 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?

I guess that's fine then.

> Attached a revised version of the diff.

Thanks.  Please always paste the ChangeLog entry along with the patch
too, even if it hadn't needed changes.


> +#include "features/rs6000/powerpc-32l.c"
> +#include "features/rs6000/powerpc-64l.c"

This is a problem.  I notice you have tdesc related things in your patch,
but nothing is actually making use of the target descriptions -- neither
the core support, nor the native debugger support code is implementing
the "return target's target description" hooks.  Furthermore, you're
using the target descriptions and calling the same initialization
functions that ppc-linux-tdep.c calls.  Please try an --enable-targets=all
build -- I'm guessing you'll see multiple-definition link errors.

-- 
Pedro Alves