Re: [PATCH] FreeBSD powerpc support

Andreas Tobler
2012-12-12T20:07:21Z

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