Re: [PATCH v1] target/m68k: fix gdb for m68xxx

[Laurent Vivier]

Le 21/04/2020 à 11:47, KONRAD Frederic a écrit :
> 
> 
> Le 4/20/20 à 10:43 PM, Laurent Vivier a écrit :
>> Le 20/04/2020 à 21:08, KONRAD Frederic a écrit :
>>>
>>>
>>> Le 4/20/20 à 5:46 PM, Laurent Vivier a écrit :
>>>> Le 20/04/2020 à 16:01, address@hidden a écrit :
>>>>> From: KONRAD Frederic <address@hidden>
>>>>>
>>>>> Currently "cf-core.xml" is sent to GDB when using any m68k flavor.
>>>>> Thing is
>>>>> it uses the "org.gnu.gdb.coldfire.core" feature name and gdb 8.3 then
>>>>> expects
>>>>> a coldfire FPU instead of the default m68881 FPU.
>>>>
>>>>
>>>> I checked in gdb sources and there is no cf definition.
>>>>
>>>> Moreover if I change only the cf to m68k in QEMU it seems to work in
>>>> both cases:
>>>>
>>>> diff --git a/gdb-xml/cf-core.xml b/gdb-xml/cf-core.xml
>>>> index b90af3042c..5b092d26de 100644
>>>> --- a/gdb-xml/cf-core.xml
>>>> +++ b/gdb-xml/cf-core.xml
>>>> @@ -5,7 +5,7 @@
>>>>         are permitted in any medium without royalty provided the
>>>> copyright
>>>>         notice and this notice are preserved.  -->
>>>>    <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>>>> -<feature name="org.gnu.gdb.coldfire.core">
>>>> +<feature name="org.gnu.gdb.m68k.core">
>>>>      <reg name="d0" bitsize="32"/>
>>>>      <reg name="d1" bitsize="32"/>
>>>>      <reg name="d2" bitsize="32"/>
>>>
>>> Doesn't that break gdb with coldfire?
>>>
>>>> diff --git a/gdb-xml/m68k-fp.xml b/gdb-xml/m68k-fp.xml
>>>> index 64290d1630..0ef74f7488 100644
>>>> --- a/gdb-xml/m68k-fp.xml
>>>> +++ b/gdb-xml/m68k-fp.xml
>>>> @@ -5,7 +5,7 @@
>>>>         are permitted in any medium without royalty provided the
>>>> copyright
>>>>         notice and this notice are preserved.  -->
>>>>    <!DOCTYPE feature SYSTEM "gdb-target.dtd">
>>>> -<feature name="org.gnu.gdb.coldfire.fp">
>>>> +<feature name="org.gnu.gdb.m68k.fp">
>>>>      <reg name="fp0" bitsize="96" type="float" group="float"/>
>>>>      <reg name="fp1" bitsize="96" type="float" group="float"/>
>>>>      <reg name="fp2" bitsize="96" type="float" group="float"/>
>>>>
>>>> As I have not checked the gdb sources for that, I'd like to have your
>>>> opinion.
>>>
>>> In the GDB 8.3 sources: m68k-tdep.c:1091:
>>>
>>>        feature = tdesc_find_feature (info.target_desc,
>>>                      "org.gnu.gdb.m68k.core");
>>>        if (feature == NULL)
>>>      {
>>>        feature = tdesc_find_feature (info.target_desc,
>>>                      "org.gnu.gdb.coldfire.core");
>>>        if (feature != NULL)
>>>          flavour = m68k_coldfire_flavour;
>>>      }
>>>
>>> Hence the change I suggested.  Little later it has also:
>>>
>>>        feature = tdesc_find_feature (info.target_desc,
>>>                      "org.gnu.gdb.coldfire.fp");
>>>        if (feature != NULL)
>>>      {
>>>        valid_p = 1;
>>>        for (i = M68K_FP0_REGNUM; i <= M68K_FPI_REGNUM; i++)
>>>          valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
>>>                          m68k_register_names[i]);
>>>        if (!valid_p)
>>>          {
>>>            tdesc_data_cleanup (tdesc_data);
>>>            return NULL;
>>>          }
>>>      }
>>>        else
>>>      has_fp = 0;
>>>
>>> Which is why I didn't made the change you suggested about the
>>> m68k-fp.xml but I
>>> just tried with this additional change and it doesn't seem to hurt.
>>
>> Thank you for your analysis, it seems a simpler patch works with
>> coldfire and m68k.
> 
> Hi Laurent,
> 
> Arg sorry I though I said that in an other email but apparently I forgot
> to hit
> the send button.  The issue with this simpler patch is that GDB will not
> set:
> 
>   flavour = m68k_coldfire_flavour
> 
> when we are running coldfire emulation, and that might break the ABI
> within GDB.
> According to the comments there, float are returned within D0 for
> ColdFire and
> not the other one.  That's why I cared to keep them separate ie: send
> the right
> "feature name" for the right cpu we are modelling.

Yes, you are right. I've added some traces in GDB to check the result.
I trace the features pointer, the flavour, etc.

Here what I have with your patch:

coldfire:

org.gnu.gdb.m68k.core (nil)
org.gnu.gdb.coldfire.core 0x10e6bf0
org.gnu.gdb.coldfire.fp 0x1197a70
tdep->fpregs_present 1
dep->flavour m68k_coldfire_flavour
floatformats_ieee_double
set_gdbarch_decr_pc_after_break 2
tdep->float_return 0


m68k:

org.gnu.gdb.m68k.core 0x1c21310
org.gnu.gdb.coldfire.fp 0x1b65d10
tdep->fpregs_present 1
tdep->flavour m68k_no_flavour
floatformats_m68881_ext
tdep->float_return 1

All the values are ok, but I try change "org.gnu.gdb.coldfire.fp" to
"org.gnu.gdb.m68k.fp" in gdb-xml/m68k-fp.xml and I think it breaks
something because tdep->fpregs_present turns to be 0:

m68k:

org.gnu.gdb.m68k.core 0x2796b60
org.gnu.gdb.coldfire.fp (nil)
tdep->fpregs_present 0
tdep->flavour m68k_no_flavour
floatformats_m68881_ext

I also tried my patch and as you said it doesn't set the good
floatformat for coldfire (and not the good value for fpregs_present for
m68k):

coldfire:

org.gnu.gdb.m68k.core 0xbdb320
org.gnu.gdb.coldfire.fp 0xbd38d0
tdep->fpregs_present 1
tdep->flavour m68k_no_flavour
floatformats_m68881_ext
tdep->float_return 1

m68k:

org.gnu.gdb.m68k.core 0x1e2cb60
org.gnu.gdb.coldfire.fp (nil)
tdep->fpregs_present 0
tdep->flavour m68k_no_flavour
floatformats_m68881_ext

Thanks,
Laurent