Re: [PATCH] FreeBSD powerpc support

[Andreas Tobler <andreast-list@fgznet.ch>]

On 11.12.12 21:07, Pedro Alves wrote:
> 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.


Ok. I'll try to prepare something. Hopefully I'll find some time over
the next weeks :)

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

Ok, will do so.

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

Will do so next time.

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

Ok, I didn't try the enable-targets=all yet.

I must have gotten something wrong here. Back when I started this patch
I thought this was needed to pass the 'xml' tests. But now I see that I
pass these test w/o this tdesc stuff.

I removed it now.

Pedro, thanks for the feedback.

If you're ok I try to update the patch with a new file which contains
the 'common' code for ppc64.

Andreas