diff mbox series

[v1] target/m68k: fix gdb for m68xxx

Message ID 1587391275-12748-1-git-send-email-frederic.konrad@adacore.com
State New
Headers show
Series [v1] target/m68k: fix gdb for m68xxx | expand

Commit Message

KONRAD Frederic April 20, 2020, 2:01 p.m. UTC
From: KONRAD Frederic <frederic.konrad@adacore.com>

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.

This is not OK because the m68881 floats registers are 96 bits wide so it
crashes GDB with the following error message:

(gdb) target remote localhost:7960
Remote debugging using localhost:7960
warning: Register "fp0" has an unsupported size (96 bits)
warning: Register "fp1" has an unsupported size (96 bits)
...
Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
  00000000000[...]0000

With this patch: qemu-system-m68k -M none -cpu m68020 -s -S

(gdb) tar rem :1234
Remote debugging using :1234
warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
0x00000000 in ?? ()
(gdb) p $fp0
$1 = nan(0xffffffffffffffff)

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 configure             |  2 +-
 gdb-xml/m68k-core.xml | 29 +++++++++++++++++++++++++++++
 target/m68k/cpu.c     | 30 +++++++++++++++++++++++++-----
 3 files changed, 55 insertions(+), 6 deletions(-)
 create mode 100644 gdb-xml/m68k-core.xml

Comments

Laurent Vivier April 20, 2020, 2:47 p.m. UTC | #1
Le 20/04/2020 à 16:01, frederic.konrad@adacore.com a écrit :
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> 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.
> 
> This is not OK because the m68881 floats registers are 96 bits wide so it
> crashes GDB with the following error message:
> 
> (gdb) target remote localhost:7960
> Remote debugging using localhost:7960
> warning: Register "fp0" has an unsupported size (96 bits)
> warning: Register "fp1" has an unsupported size (96 bits)
> ...
> Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
>   00000000000[...]0000
> 
> With this patch: qemu-system-m68k -M none -cpu m68020 -s -S
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> 0x00000000 in ?? ()
> (gdb) p $fp0
> $1 = nan(0xffffffffffffffff)
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  configure             |  2 +-
>  gdb-xml/m68k-core.xml | 29 +++++++++++++++++++++++++++++
>  target/m68k/cpu.c     | 30 +++++++++++++++++++++++++-----
>  3 files changed, 55 insertions(+), 6 deletions(-)
>  create mode 100644 gdb-xml/m68k-core.xml
> 
> diff --git a/configure b/configure
> index 23b5e93..2b07b85 100755
> --- a/configure
> +++ b/configure
> @@ -7825,7 +7825,7 @@ case "$target_name" in
>    ;;
>    m68k)
>      bflt="yes"
> -    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
> +    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
>      TARGET_SYSTBL_ABI=common
>    ;;
>    microblaze|microblazeel)
> diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
> new file mode 100644
> index 0000000..5b092d2
> --- /dev/null
> +++ b/gdb-xml/m68k-core.xml
> @@ -0,0 +1,29 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2008 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     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.m68k.core">

So I guess you should also update feature name in gdb-xml/m68k-fp.xml ?

Thanks,
Laurent
Laurent Vivier April 20, 2020, 3:46 p.m. UTC | #2
Le 20/04/2020 à 16:01, frederic.konrad@adacore.com a écrit :
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> 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"/>
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.

Thanks,
Laurent
Alex Bennée April 20, 2020, 4:13 p.m. UTC | #3
frederic.konrad@adacore.com writes:

> From: KONRAD Frederic <frederic.konrad@adacore.com>
>
> 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.
>
> This is not OK because the m68881 floats registers are 96 bits wide so it
> crashes GDB with the following error message:
>
> (gdb) target remote localhost:7960
> Remote debugging using localhost:7960
> warning: Register "fp0" has an unsupported size (96 bits)
> warning: Register "fp1" has an unsupported size (96 bits)
> ...
> Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
>   00000000000[...]0000
>
> With this patch: qemu-system-m68k -M none -cpu m68020 -s -S
>
> (gdb) tar rem :1234
> Remote debugging using :1234
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> 0x00000000 in ?? ()
> (gdb) p $fp0
> $1 = nan(0xffffffffffffffff)
>
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>

Well it solves the connection issue - but the FP values are still
hopelessly broken, from float_convs:

  from single: f32(-0x1.1874b200000000000000p+103:0xf30c3a59)

in gdb

  fp0            <invalid float value> (raw 0x8c3a5900000000004066)

> ---
>  configure             |  2 +-
>  gdb-xml/m68k-core.xml | 29 +++++++++++++++++++++++++++++
>  target/m68k/cpu.c     | 30 +++++++++++++++++++++++++-----
>  3 files changed, 55 insertions(+), 6 deletions(-)
>  create mode 100644 gdb-xml/m68k-core.xml
>
> diff --git a/configure b/configure
> index 23b5e93..2b07b85 100755
> --- a/configure
> +++ b/configure
> @@ -7825,7 +7825,7 @@ case "$target_name" in
>    ;;
>    m68k)
>      bflt="yes"
> -    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
> +    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
>      TARGET_SYSTBL_ABI=common
>    ;;
>    microblaze|microblazeel)
> diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
> new file mode 100644
> index 0000000..5b092d2
> --- /dev/null
> +++ b/gdb-xml/m68k-core.xml
> @@ -0,0 +1,29 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2008 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     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.m68k.core">
> +  <reg name="d0" bitsize="32"/>
> +  <reg name="d1" bitsize="32"/>
> +  <reg name="d2" bitsize="32"/>
> +  <reg name="d3" bitsize="32"/>
> +  <reg name="d4" bitsize="32"/>
> +  <reg name="d5" bitsize="32"/>
> +  <reg name="d6" bitsize="32"/>
> +  <reg name="d7" bitsize="32"/>
> +  <reg name="a0" bitsize="32" type="data_ptr"/>
> +  <reg name="a1" bitsize="32" type="data_ptr"/>
> +  <reg name="a2" bitsize="32" type="data_ptr"/>
> +  <reg name="a3" bitsize="32" type="data_ptr"/>
> +  <reg name="a4" bitsize="32" type="data_ptr"/>
> +  <reg name="a5" bitsize="32" type="data_ptr"/>
> +  <reg name="fp" bitsize="32" type="data_ptr"/>
> +  <reg name="sp" bitsize="32" type="data_ptr"/>
> +
> +  <reg name="ps" bitsize="32"/>
> +  <reg name="pc" bitsize="32" type="code_ptr"/>
> +
> +</feature>
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 9445fcd..976e624 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -297,6 +297,21 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>      dc->vmsd = &vmstate_m68k_cpu;
>  }
>  
> +static void m68k_cpu_class_init_m68k_core(ObjectClass *c, void *data)
> +{
> +    CPUClass *cc = CPU_CLASS(c);
> +
> +    cc->gdb_core_xml_file = "m68k-core.xml";
> +}
> +
> +#define DEFINE_M68K_CPU_TYPE_WITH_CLASS(cpu_model, initfn, classinit)      \
> +    {                                                                      \
> +        .name = M68K_CPU_TYPE_NAME(cpu_model),                             \
> +        .instance_init = initfn,                                           \
> +        .parent = TYPE_M68K_CPU,                                           \
> +        .class_init = classinit,                                           \
> +    }
> +
>  #define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
>      {                                           \
>          .name = M68K_CPU_TYPE_NAME(cpu_model),  \
> @@ -314,11 +329,16 @@ static const TypeInfo m68k_cpus_type_infos[] = {
>          .class_size = sizeof(M68kCPUClass),
>          .class_init = m68k_cpu_class_init,
>      },
> -    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68000", m68000_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68020", m68020_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68030", m68030_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68040", m68040_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68060", m68060_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
>      DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
>      DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
>      DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),
KONRAD Frederic April 20, 2020, 6:54 p.m. UTC | #4
Le 4/20/20 à 6:13 PM, Alex Bennée a écrit :
> 
> frederic.konrad@adacore.com writes:
> 
>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>
>> 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.
>>
>> This is not OK because the m68881 floats registers are 96 bits wide so it
>> crashes GDB with the following error message:
>>
>> (gdb) target remote localhost:7960
>> Remote debugging using localhost:7960
>> warning: Register "fp0" has an unsupported size (96 bits)
>> warning: Register "fp1" has an unsupported size (96 bits)
>> ...
>> Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
>>    00000000000[...]0000
>>
>> With this patch: qemu-system-m68k -M none -cpu m68020 -s -S
>>
>> (gdb) tar rem :1234
>> Remote debugging using :1234
>> warning: No executable has been specified and target does not support
>> determining executable automatically.  Try using the "file" command.
>> 0x00000000 in ?? ()
>> (gdb) p $fp0
>> $1 = nan(0xffffffffffffffff)
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> Well it solves the connection issue - but the FP values are still
> hopelessly broken, from float_convs:
> 
>    from single: f32(-0x1.1874b200000000000000p+103:0xf30c3a59)
> 
> in gdb
> 
>    fp0            <invalid float value> (raw 0x8c3a5900000000004066)

Hi Alex, do you have a reproducer?

With this simple example:
=========================

.global _start
.text
_start:
         lea val, %a0
         fmoves (%a0),%fp0
         fmoves 4(%a0), %fp1
end:
         bra end

.align 0x4
val:
         .float 0.12345678
         .float 3.141592653589793

$ m68k-linux-gnu-gcc -g -Wl,-Ttext=0 main.S -mcpu=68020 -nostdlib -o main.elf
$ m68k-linux-gnu-objdump -d main.elf

main.elf:     file format elf32-m68k

Disassembly of section .text:

00000000 <_start>:
    0:	41fa 0012      	lea %pc@(14 <val>),%a0
    4:	f210 4400      	fmoves %a0@,%fp0
    8:	f228 4480 0004 	fmoves %a0@(4),%fp1

0000000e <end>:
    e:	6000 fffe      	braw e <end>
	...

00000014 <val>:
   14:	3dfc           	.short 0x3dfc
   16:	d6e9 4049      	addaw %a1@(16457),%a3
   1a:	0fdb           	bset %d7,%a3@+

I can run the executable in 5.0-rc3 + the patch:
================================================

$ xxx/qemu-for-upstream.git/m68k-softmmu/qemu-system-m68k --kernel main.elf \
     -cpu m68020 -s -S -nographic

And a GDB 8.3 I just built:
===========================

$ xxx/bin/m68k-elf-gdb main.elf
GNU gdb (GDB) 8.3
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "--host=x86_64-pc-linux-gnu --target=m68k-elf".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
     <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from main.elf...
(gdb) tar rem :1234
Remote debugging using :1234
_start () at main.S:5
5	        lea val, %a0
(gdb) b end
Breakpoint 1 at 0xe: file main.S, line 9.
(gdb) c
Continuing.

Breakpoint 1, end () at main.S:9
9	        bra end
(gdb) info registers all
d0             0x0                 0
d1             0x0                 0
d2             0x0                 0
d3             0x0                 0
d4             0x0                 0
d5             0x0                 0
d6             0x0                 0
d7             0x0                 0
a0             0x14                0x14 <val>
a1             0x0                 0x0 <_start>
a2             0x0                 0x0 <_start>
a3             0x0                 0x0 <_start>
a4             0x0                 0x0 <_start>
a5             0x0                 0x0 <_start>
fp             0x0                 0x0 <_start>
sp             0x0                 0x0 <_start>
ps             0x2700              9984
pc             0xe                 0xe <end>
fp0            0.123456783592700958252 (raw 0x3ffb0000fcd6e90000000000)
fp1            3.14159274101257324219 (raw 0x40000000c90fdb0000000000)
fp2            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp3            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp4            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp5            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp6            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp7            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fpcontrol      0x0                 0
fpstatus       0x0                 0
fpiaddr        0x0                 0x0 <_start>

The value doesn't seems to much broken, the monitor gives me:

(qemu) info registers
D0 = 00000000   A0 = 00000014   F0 = 3ffb fcd6e90000000000  (    0.123457)
D1 = 00000000   A1 = 00000000   F1 = 4000 c90fdb0000000000  (     3.14159)
D2 = 00000000   A2 = 00000000   F2 = 7fff ffffffffffffffff  (         nan)
D3 = 00000000   A3 = 00000000   F3 = 7fff ffffffffffffffff  (         nan)
D4 = 00000000   A4 = 00000000   F4 = 7fff ffffffffffffffff  (         nan)
D5 = 00000000   A5 = 00000000   F5 = 7fff ffffffffffffffff  (         nan)
D6 = 00000000   A6 = 00000000   F6 = 7fff ffffffffffffffff  (         nan)
D7 = 00000000   A7 = 00000000   F7 = 7fff ffffffffffffffff  (         nan)

Did I miss anything?

Cheers,
Fred

> 
>> ---
>>   configure             |  2 +-
>>   gdb-xml/m68k-core.xml | 29 +++++++++++++++++++++++++++++
>>   target/m68k/cpu.c     | 30 +++++++++++++++++++++++++-----
>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>   create mode 100644 gdb-xml/m68k-core.xml
>>
>> diff --git a/configure b/configure
>> index 23b5e93..2b07b85 100755
>> --- a/configure
>> +++ b/configure
>> @@ -7825,7 +7825,7 @@ case "$target_name" in
>>     ;;
>>     m68k)
>>       bflt="yes"
>> -    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
>> +    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
>>       TARGET_SYSTBL_ABI=common
>>     ;;
>>     microblaze|microblazeel)
>> diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
>> new file mode 100644
>> index 0000000..5b092d2
>> --- /dev/null
>> +++ b/gdb-xml/m68k-core.xml
>> @@ -0,0 +1,29 @@
>> +<?xml version="1.0"?>
>> +<!-- Copyright (C) 2008 Free Software Foundation, Inc.
>> +
>> +     Copying and distribution of this file, with or without modification,
>> +     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.m68k.core">
>> +  <reg name="d0" bitsize="32"/>
>> +  <reg name="d1" bitsize="32"/>
>> +  <reg name="d2" bitsize="32"/>
>> +  <reg name="d3" bitsize="32"/>
>> +  <reg name="d4" bitsize="32"/>
>> +  <reg name="d5" bitsize="32"/>
>> +  <reg name="d6" bitsize="32"/>
>> +  <reg name="d7" bitsize="32"/>
>> +  <reg name="a0" bitsize="32" type="data_ptr"/>
>> +  <reg name="a1" bitsize="32" type="data_ptr"/>
>> +  <reg name="a2" bitsize="32" type="data_ptr"/>
>> +  <reg name="a3" bitsize="32" type="data_ptr"/>
>> +  <reg name="a4" bitsize="32" type="data_ptr"/>
>> +  <reg name="a5" bitsize="32" type="data_ptr"/>
>> +  <reg name="fp" bitsize="32" type="data_ptr"/>
>> +  <reg name="sp" bitsize="32" type="data_ptr"/>
>> +
>> +  <reg name="ps" bitsize="32"/>
>> +  <reg name="pc" bitsize="32" type="code_ptr"/>
>> +
>> +</feature>
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index 9445fcd..976e624 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -297,6 +297,21 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>>       dc->vmsd = &vmstate_m68k_cpu;
>>   }
>>   
>> +static void m68k_cpu_class_init_m68k_core(ObjectClass *c, void *data)
>> +{
>> +    CPUClass *cc = CPU_CLASS(c);
>> +
>> +    cc->gdb_core_xml_file = "m68k-core.xml";
>> +}
>> +
>> +#define DEFINE_M68K_CPU_TYPE_WITH_CLASS(cpu_model, initfn, classinit)      \
>> +    {                                                                      \
>> +        .name = M68K_CPU_TYPE_NAME(cpu_model),                             \
>> +        .instance_init = initfn,                                           \
>> +        .parent = TYPE_M68K_CPU,                                           \
>> +        .class_init = classinit,                                           \
>> +    }
>> +
>>   #define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
>>       {                                           \
>>           .name = M68K_CPU_TYPE_NAME(cpu_model),  \
>> @@ -314,11 +329,16 @@ static const TypeInfo m68k_cpus_type_infos[] = {
>>           .class_size = sizeof(M68kCPUClass),
>>           .class_init = m68k_cpu_class_init,
>>       },
>> -    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68000", m68000_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68020", m68020_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68030", m68030_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68040", m68040_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68060", m68060_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>>       DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
>>       DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
>>       DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),
> 
>
KONRAD Frederic April 20, 2020, 7:08 p.m. UTC | #5
Le 4/20/20 à 5:46 PM, Laurent Vivier a écrit :
> Le 20/04/2020 à 16:01, frederic.konrad@adacore.com a écrit :
>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>
>> 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.

Cheers,
Fred

> 
> Thanks,
> Laurent
>
Laurent Vivier April 20, 2020, 7:59 p.m. UTC | #6
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, frederic.konrad@adacore.com a écrit :
>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>>
>>> 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?

No, it seems to work, but I didn't test this really carefully.

So perhaps we can only change the feature name in the two existing xml
files and it will work for all the cases? (and renaming cf-core.xml to
m68k-core.xml).

The core files are the same for cf and m68k, so according to the sample
code you show, if it finds "org.gnu.gdb.m68k.core" if will use for both.

I think the only bug we have is the feature name is not good in
m68k-fp.xml, and fixing that should be enough.

Thanks,
Laurent
Laurent Vivier April 20, 2020, 8:43 p.m. UTC | #7
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, frederic.konrad@adacore.com a écrit :
>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>>
>>> 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.

diff --git a/configure b/configure
index 23b5e93752..b3be6d9c4b 100755
--- a/configure
+++ b/configure
@@ -7825,7 +7825,7 @@ case "$target_name" in
   ;;
   m68k)
     bflt="yes"
-    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
+    gdb_xml_files="m68k-core.xml cf-fp.xml m68k-fp.xml"
     TARGET_SYSTBL_ABI=common
   ;;
   microblaze|microblazeel)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 9445fcd6df..4e942a0a8e 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -292,7 +292,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void
*data)
     cc->tcg_initialize = m68k_tcg_init;

     cc->gdb_num_core_regs = 18;
-    cc->gdb_core_xml_file = "cf-core.xml";
+    cc->gdb_core_xml_file = "m68k-core.xml";

     dc->vmsd = &vmstate_m68k_cpu;
 }
diff --git a/gdb-xml/cf-core.xml b/gdb-xml/m68k-core.xml
similarity index 96%
rename from gdb-xml/cf-core.xml
rename to gdb-xml/m68k-core.xml
index b90af3042c..5b092d26de 100644
--- a/gdb-xml/cf-core.xml
+++ b/gdb-xml/m68k-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"/>
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"/>

I have tested with both architectures:

* -M q800:

(gdb) info float
fp0            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp1            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp2            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp3            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp4            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp5            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp6            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fp7            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
fpcontrol      0x0                 0
fpstatus       0x0                 0
fpiaddr        0x0                 0x0
(gdb) info registers
d0             0x0                 0
d1             0x2                 2
d2             0x462a0             287392
d3             0x40                64
d4             0x0                 0
d5             0x0                 0
d6             0x0                 0
d7             0x0                 0
a0             0x3e0000            0x3e0000
a1             0x3e351c            0x3e351c
a2             0x3e351c            0x3e351c
a3             0x3e0000            0x3e0000
a4             0x46390             0x46390
a5             0x2eed7e            0x2eed7e
fp             0x2c65c             0x2c65c
sp             0x3e1fa4            0x3e1fa4
ps             0x2000              8192
pc             0x2f00              0x2f00
fpcontrol      0x0                 0
fpstatus       0x0                 0
fpiaddr        0x0                 0x0

* -cpu cfv4e

(gdb) info registers
d0             0x0                 0
d1             0x401c0b40          1075579712
d2             0x0                 0
d3             0x0                 0
d4             0x0                 0
d5             0x0                 0
d6             0x0                 0
d7             0x0                 0
a0             0x4015c008          0x4015c008
a1             0x40151092          0x40151092
a2             0x401146c8          0x401146c8
a3             0x4016b189          0x4016b189
a4             0x400ac60a          0x400ac60a
a5             0x40017078          0x40017078
fp             0x4015cff8          0x4015cff8
sp             0x4015cfcc          0x4015cfcc
ps             0x2000              8192
pc             0x40010a2a          0x40010a2a
fpcontrol      0x0                 0
fpstatus       0x0                 0
fpiaddr        0x0                 0x0
(gdb) info float
fp0            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fp1            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fp2            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fp3            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fp4            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fp5            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fp6            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fp7            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
fpcontrol      0x0                 0
fpstatus       0x0                 0
fpiaddr        0x0                 0x0


All with a native GDB from debian/sid (GNU gdb (Debian 9.1-3) 9.1).

Thanks,
Laurent
KONRAD Frederic April 21, 2020, 9:47 a.m. UTC | #8
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, frederic.konrad@adacore.com a écrit :
>>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>>>
>>>> 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.

> 
> diff --git a/configure b/configure
> index 23b5e93752..b3be6d9c4b 100755
> --- a/configure
> +++ b/configure
> @@ -7825,7 +7825,7 @@ case "$target_name" in
>     ;;
>     m68k)
>       bflt="yes"
> -    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
> +    gdb_xml_files="m68k-core.xml cf-fp.xml m68k-fp.xml"
>       TARGET_SYSTBL_ABI=common
>     ;;
>     microblaze|microblazeel)
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 9445fcd6df..4e942a0a8e 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -292,7 +292,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void
> *data)
>       cc->tcg_initialize = m68k_tcg_init;
> 
>       cc->gdb_num_core_regs = 18;
> -    cc->gdb_core_xml_file = "cf-core.xml";
> +    cc->gdb_core_xml_file = "m68k-core.xml";
> 
>       dc->vmsd = &vmstate_m68k_cpu;
>   }
> diff --git a/gdb-xml/cf-core.xml b/gdb-xml/m68k-core.xml
> similarity index 96%
> rename from gdb-xml/cf-core.xml
> rename to gdb-xml/m68k-core.xml
> index b90af3042c..5b092d26de 100644
> --- a/gdb-xml/cf-core.xml
> +++ b/gdb-xml/m68k-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"/>
> 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"/>
> 
> I have tested with both architectures:
> 
> * -M q800:
> 
> (gdb) info float
> fp0            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fp1            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fp2            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fp3            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fp4            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fp5            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fp6            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fp7            nan(0xffffffffffffffff) (raw 0x7fff0000ffffffffffffffff)
> fpcontrol      0x0                 0
> fpstatus       0x0                 0
> fpiaddr        0x0                 0x0
> (gdb) info registers
> d0             0x0                 0
> d1             0x2                 2
> d2             0x462a0             287392
> d3             0x40                64
> d4             0x0                 0
> d5             0x0                 0
> d6             0x0                 0
> d7             0x0                 0
> a0             0x3e0000            0x3e0000
> a1             0x3e351c            0x3e351c
> a2             0x3e351c            0x3e351c
> a3             0x3e0000            0x3e0000
> a4             0x46390             0x46390
> a5             0x2eed7e            0x2eed7e
> fp             0x2c65c             0x2c65c
> sp             0x3e1fa4            0x3e1fa4
> ps             0x2000              8192
> pc             0x2f00              0x2f00
> fpcontrol      0x0                 0
> fpstatus       0x0                 0
> fpiaddr        0x0                 0x0
> 
> * -cpu cfv4e
> 
> (gdb) info registers
> d0             0x0                 0
> d1             0x401c0b40          1075579712
> d2             0x0                 0
> d3             0x0                 0
> d4             0x0                 0
> d5             0x0                 0
> d6             0x0                 0
> d7             0x0                 0
> a0             0x4015c008          0x4015c008
> a1             0x40151092          0x40151092
> a2             0x401146c8          0x401146c8
> a3             0x4016b189          0x4016b189
> a4             0x400ac60a          0x400ac60a
> a5             0x40017078          0x40017078
> fp             0x4015cff8          0x4015cff8
> sp             0x4015cfcc          0x4015cfcc
> ps             0x2000              8192
> pc             0x40010a2a          0x40010a2a
> fpcontrol      0x0                 0
> fpstatus       0x0                 0
> fpiaddr        0x0                 0x0
> (gdb) info float
> fp0            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fp1            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fp2            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fp3            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fp4            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fp5            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fp6            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fp7            nan(0xfffffffffffff) (raw 0x7fffffffffffffff)
> fpcontrol      0x0                 0
> fpstatus       0x0                 0
> fpiaddr        0x0                 0x0
> 
> 
> All with a native GDB from debian/sid (GNU gdb (Debian 9.1-3) 9.1).
> 
> Thanks,
> Laurent
>
Laurent Vivier April 21, 2020, 2:08 p.m. UTC | #9
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, frederic.konrad@adacore.com a écrit :
>>>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>>>>
>>>>> 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
Laurent Vivier April 27, 2020, 7:53 a.m. UTC | #10
Le 20/04/2020 à 16:01, frederic.konrad@adacore.com a écrit :
> From: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> 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.
> 
> This is not OK because the m68881 floats registers are 96 bits wide so it
> crashes GDB with the following error message:
> 
> (gdb) target remote localhost:7960
> Remote debugging using localhost:7960
> warning: Register "fp0" has an unsupported size (96 bits)
> warning: Register "fp1" has an unsupported size (96 bits)
> ...
> Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
>   00000000000[...]0000
> 
> With this patch: qemu-system-m68k -M none -cpu m68020 -s -S
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> 0x00000000 in ?? ()
> (gdb) p $fp0
> $1 = nan(0xffffffffffffffff)
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  configure             |  2 +-
>  gdb-xml/m68k-core.xml | 29 +++++++++++++++++++++++++++++
>  target/m68k/cpu.c     | 30 +++++++++++++++++++++++++-----
>  3 files changed, 55 insertions(+), 6 deletions(-)
>  create mode 100644 gdb-xml/m68k-core.xml
> 
> diff --git a/configure b/configure
> index 23b5e93..2b07b85 100755
> --- a/configure
> +++ b/configure
> @@ -7825,7 +7825,7 @@ case "$target_name" in
>    ;;
>    m68k)
>      bflt="yes"
> -    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
> +    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
>      TARGET_SYSTBL_ABI=common
>    ;;
>    microblaze|microblazeel)
> diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
> new file mode 100644
> index 0000000..5b092d2
> --- /dev/null
> +++ b/gdb-xml/m68k-core.xml
> @@ -0,0 +1,29 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2008 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     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.m68k.core">
> +  <reg name="d0" bitsize="32"/>
> +  <reg name="d1" bitsize="32"/>
> +  <reg name="d2" bitsize="32"/>
> +  <reg name="d3" bitsize="32"/>
> +  <reg name="d4" bitsize="32"/>
> +  <reg name="d5" bitsize="32"/>
> +  <reg name="d6" bitsize="32"/>
> +  <reg name="d7" bitsize="32"/>
> +  <reg name="a0" bitsize="32" type="data_ptr"/>
> +  <reg name="a1" bitsize="32" type="data_ptr"/>
> +  <reg name="a2" bitsize="32" type="data_ptr"/>
> +  <reg name="a3" bitsize="32" type="data_ptr"/>
> +  <reg name="a4" bitsize="32" type="data_ptr"/>
> +  <reg name="a5" bitsize="32" type="data_ptr"/>
> +  <reg name="fp" bitsize="32" type="data_ptr"/>
> +  <reg name="sp" bitsize="32" type="data_ptr"/>
> +
> +  <reg name="ps" bitsize="32"/>
> +  <reg name="pc" bitsize="32" type="code_ptr"/>
> +
> +</feature>
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 9445fcd..976e624 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -297,6 +297,21 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>      dc->vmsd = &vmstate_m68k_cpu;
>  }
>  
> +static void m68k_cpu_class_init_m68k_core(ObjectClass *c, void *data)
> +{
> +    CPUClass *cc = CPU_CLASS(c);
> +
> +    cc->gdb_core_xml_file = "m68k-core.xml";
> +}

Could you also add a m68k_cpu_class_init_cf_core() and move the
cf-core.xml into it?

> +
> +#define DEFINE_M68K_CPU_TYPE_WITH_CLASS(cpu_model, initfn, classinit)      \
> +    {                                                                      \
> +        .name = M68K_CPU_TYPE_NAME(cpu_model),                             \
> +        .instance_init = initfn,                                           \
> +        .parent = TYPE_M68K_CPU,                                           \
> +        .class_init = classinit,                                           \
> +    }
> +

I would prefer to have two macros with no class parameter, something
like DEFINE_M68K_CPU_TYPE_M68K() and DEFINE_M68K_CPU_TYPE_CF().

>  #define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
>      {                                           \
>          .name = M68K_CPU_TYPE_NAME(cpu_model),  \
> @@ -314,11 +329,16 @@ static const TypeInfo m68k_cpus_type_infos[] = {
>          .class_size = sizeof(M68kCPUClass),
>          .class_init = m68k_cpu_class_init,
>      },
> -    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
> -    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68000", m68000_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68020", m68020_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68030", m68030_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68040", m68040_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68060", m68060_cpu_initfn,
> +                                    m68k_cpu_class_init_m68k_core),
>      DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
>      DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
>      DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),
> 

I think we can also have something like
DEFINE_M68K_CPU_TYPE_M68K(m68000) or DEFINE_M68K_CPU_TYPE_CF(m5206) and
deduce initfn function name from the macro parameter.

Thanks,
Laurent
KONRAD Frederic April 28, 2020, 1:19 p.m. UTC | #11
Le 4/27/20 à 9:53 AM, Laurent Vivier a écrit :
> Le 20/04/2020 à 16:01, frederic.konrad@adacore.com a écrit :
>> From: KONRAD Frederic <frederic.konrad@adacore.com>
>>
>> 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.
>>
>> This is not OK because the m68881 floats registers are 96 bits wide so it
>> crashes GDB with the following error message:
>>
>> (gdb) target remote localhost:7960
>> Remote debugging using localhost:7960
>> warning: Register "fp0" has an unsupported size (96 bits)
>> warning: Register "fp1" has an unsupported size (96 bits)
>> ...
>> Remote 'g' packet reply is too long (expected 148 bytes, got 180 bytes):    \
>>    00000000000[...]0000
>>
>> With this patch: qemu-system-m68k -M none -cpu m68020 -s -S
>>
>> (gdb) tar rem :1234
>> Remote debugging using :1234
>> warning: No executable has been specified and target does not support
>> determining executable automatically.  Try using the "file" command.
>> 0x00000000 in ?? ()
>> (gdb) p $fp0
>> $1 = nan(0xffffffffffffffff)
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>> ---
>>   configure             |  2 +-
>>   gdb-xml/m68k-core.xml | 29 +++++++++++++++++++++++++++++
>>   target/m68k/cpu.c     | 30 +++++++++++++++++++++++++-----
>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>   create mode 100644 gdb-xml/m68k-core.xml
>>
>> diff --git a/configure b/configure
>> index 23b5e93..2b07b85 100755
>> --- a/configure
>> +++ b/configure
>> @@ -7825,7 +7825,7 @@ case "$target_name" in
>>     ;;
>>     m68k)
>>       bflt="yes"
>> -    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
>> +    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
>>       TARGET_SYSTBL_ABI=common
>>     ;;
>>     microblaze|microblazeel)
>> diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
>> new file mode 100644
>> index 0000000..5b092d2
>> --- /dev/null
>> +++ b/gdb-xml/m68k-core.xml
>> @@ -0,0 +1,29 @@
>> +<?xml version="1.0"?>
>> +<!-- Copyright (C) 2008 Free Software Foundation, Inc.
>> +
>> +     Copying and distribution of this file, with or without modification,
>> +     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.m68k.core">
>> +  <reg name="d0" bitsize="32"/>
>> +  <reg name="d1" bitsize="32"/>
>> +  <reg name="d2" bitsize="32"/>
>> +  <reg name="d3" bitsize="32"/>
>> +  <reg name="d4" bitsize="32"/>
>> +  <reg name="d5" bitsize="32"/>
>> +  <reg name="d6" bitsize="32"/>
>> +  <reg name="d7" bitsize="32"/>
>> +  <reg name="a0" bitsize="32" type="data_ptr"/>
>> +  <reg name="a1" bitsize="32" type="data_ptr"/>
>> +  <reg name="a2" bitsize="32" type="data_ptr"/>
>> +  <reg name="a3" bitsize="32" type="data_ptr"/>
>> +  <reg name="a4" bitsize="32" type="data_ptr"/>
>> +  <reg name="a5" bitsize="32" type="data_ptr"/>
>> +  <reg name="fp" bitsize="32" type="data_ptr"/>
>> +  <reg name="sp" bitsize="32" type="data_ptr"/>
>> +
>> +  <reg name="ps" bitsize="32"/>
>> +  <reg name="pc" bitsize="32" type="code_ptr"/>
>> +
>> +</feature>
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index 9445fcd..976e624 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -297,6 +297,21 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>>       dc->vmsd = &vmstate_m68k_cpu;
>>   }
>>   
>> +static void m68k_cpu_class_init_m68k_core(ObjectClass *c, void *data)
>> +{
>> +    CPUClass *cc = CPU_CLASS(c);
>> +
>> +    cc->gdb_core_xml_file = "m68k-core.xml";
>> +}
> 
> Could you also add a m68k_cpu_class_init_cf_core() and move the
> cf-core.xml into it?

Yes I can do that:
   - DEFINE_M68K_CPU_TYPE_M68K will use m68k_cpu_class_init_m68k_core.
   - DEFINE_M68K_CPU_TYPE_CF will use m68k_cpu_class_init_cf_core.
   - drop xxx_WITH_CLASS behind.

> 
>> +
>> +#define DEFINE_M68K_CPU_TYPE_WITH_CLASS(cpu_model, initfn, classinit)      \
>> +    {                                                                      \
>> +        .name = M68K_CPU_TYPE_NAME(cpu_model),                             \
>> +        .instance_init = initfn,                                           \
>> +        .parent = TYPE_M68K_CPU,                                           \
>> +        .class_init = classinit,                                           \
>> +    }
>> +
> 
> I would prefer to have two macros with no class parameter, something
> like DEFINE_M68K_CPU_TYPE_M68K() and DEFINE_M68K_CPU_TYPE_CF().
> 
>>   #define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
>>       {                                           \
>>           .name = M68K_CPU_TYPE_NAME(cpu_model),  \
>> @@ -314,11 +329,16 @@ static const TypeInfo m68k_cpus_type_infos[] = {
>>           .class_size = sizeof(M68kCPUClass),
>>           .class_init = m68k_cpu_class_init,
>>       },
>> -    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
>> -    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68000", m68000_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68020", m68020_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68030", m68030_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68040", m68040_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68060", m68060_cpu_initfn,
>> +                                    m68k_cpu_class_init_m68k_core),
>>       DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
>>       DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
>>       DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),

But what about the "any" which is out of context here?

     DEFINE_M68K_CPU_TYPE("any", any_cpu_initfn),

Should it be TYPE_M68K or TYPE_CF in which case which xml will it take?  I guess
we can take TYPE_CF so it doesn't change from what is done today.

>>
> 
> I think we can also have something like
> DEFINE_M68K_CPU_TYPE_M68K(m68000) or DEFINE_M68K_CPU_TYPE_CF(m5206) and
> deduce initfn function name from the macro parameter.
> 

Why not.

Fred

> Thanks,
> Laurent
>
Laurent Vivier April 28, 2020, 2:13 p.m. UTC | #12
Le 28/04/2020 à 15:19, KONRAD Frederic a écrit :
> 
> 
> Le 4/27/20 à 9:53 AM, Laurent Vivier a écrit :
>> Le 20/04/2020 à 16:01, frederic.konrad@adacore.com a écrit :
>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
...
>> I would prefer to have two macros with no class parameter, something
>> like DEFINE_M68K_CPU_TYPE_M68K() and DEFINE_M68K_CPU_TYPE_CF().
>>
>>>   #define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
>>>       {                                           \
>>>           .name = M68K_CPU_TYPE_NAME(cpu_model),  \
>>> @@ -314,11 +329,16 @@ static const TypeInfo m68k_cpus_type_infos[] = {
>>>           .class_size = sizeof(M68kCPUClass),
>>>           .class_init = m68k_cpu_class_init,
>>>       },
>>> -    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
>>> -    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
>>> -    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
>>> -    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
>>> -    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68000", m68000_cpu_initfn,
>>> +                                    m68k_cpu_class_init_m68k_core),
>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68020", m68020_cpu_initfn,
>>> +                                    m68k_cpu_class_init_m68k_core),
>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68030", m68030_cpu_initfn,
>>> +                                    m68k_cpu_class_init_m68k_core),
>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68040", m68040_cpu_initfn,
>>> +                                    m68k_cpu_class_init_m68k_core),
>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68060", m68060_cpu_initfn,
>>> +                                    m68k_cpu_class_init_m68k_core),
>>>       DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
>>>       DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
>>>       DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),
> 
> But what about the "any" which is out of context here?
> 
>     DEFINE_M68K_CPU_TYPE("any", any_cpu_initfn),
> 
> Should it be TYPE_M68K or TYPE_CF in which case which xml will it take? 
> I guess
> we can take TYPE_CF so it doesn't change from what is done today.
> 

Yes, "any" has been introduced with coldfire CPUs and defines CF_FPU, so
it uses 64bit float registers.

Thanks,
Laurent
KONRAD Frederic April 28, 2020, 4:46 p.m. UTC | #13
Le 4/28/20 à 4:13 PM, Laurent Vivier a écrit :
> Le 28/04/2020 à 15:19, KONRAD Frederic a écrit :
>>
>>
>> Le 4/27/20 à 9:53 AM, Laurent Vivier a écrit :
>>> Le 20/04/2020 à 16:01, frederic.konrad@adacore.com a écrit :
>>>> From: KONRAD Frederic <frederic.konrad@adacore.com>
> ...
>>> I would prefer to have two macros with no class parameter, something
>>> like DEFINE_M68K_CPU_TYPE_M68K() and DEFINE_M68K_CPU_TYPE_CF().
>>>
>>>>    #define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
>>>>        {                                           \
>>>>            .name = M68K_CPU_TYPE_NAME(cpu_model),  \
>>>> @@ -314,11 +329,16 @@ static const TypeInfo m68k_cpus_type_infos[] = {
>>>>            .class_size = sizeof(M68kCPUClass),
>>>>            .class_init = m68k_cpu_class_init,
>>>>        },
>>>> -    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
>>>> -    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
>>>> -    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
>>>> -    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
>>>> -    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
>>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68000", m68000_cpu_initfn,
>>>> +                                    m68k_cpu_class_init_m68k_core),
>>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68020", m68020_cpu_initfn,
>>>> +                                    m68k_cpu_class_init_m68k_core),
>>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68030", m68030_cpu_initfn,
>>>> +                                    m68k_cpu_class_init_m68k_core),
>>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68040", m68040_cpu_initfn,
>>>> +                                    m68k_cpu_class_init_m68k_core),
>>>> +    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68060", m68060_cpu_initfn,
>>>> +                                    m68k_cpu_class_init_m68k_core),
>>>>        DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
>>>>        DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
>>>>        DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),
>>
>> But what about the "any" which is out of context here?
>>
>>      DEFINE_M68K_CPU_TYPE("any", any_cpu_initfn),
>>
>> Should it be TYPE_M68K or TYPE_CF in which case which xml will it take?
>> I guess
>> we can take TYPE_CF so it doesn't change from what is done today.
>>
> 
> Yes, "any" has been introduced with coldfire CPUs and defines CF_FPU, so
> it uses 64bit float registers.
> 
> Thanks,
> Laurent
> 

Ok done, will send a v2 with an other fix I've for mc68881.
diff mbox series

Patch

diff --git a/configure b/configure
index 23b5e93..2b07b85 100755
--- a/configure
+++ b/configure
@@ -7825,7 +7825,7 @@  case "$target_name" in
   ;;
   m68k)
     bflt="yes"
-    gdb_xml_files="cf-core.xml cf-fp.xml m68k-fp.xml"
+    gdb_xml_files="cf-core.xml cf-fp.xml m68k-core.xml m68k-fp.xml"
     TARGET_SYSTBL_ABI=common
   ;;
   microblaze|microblazeel)
diff --git a/gdb-xml/m68k-core.xml b/gdb-xml/m68k-core.xml
new file mode 100644
index 0000000..5b092d2
--- /dev/null
+++ b/gdb-xml/m68k-core.xml
@@ -0,0 +1,29 @@ 
+<?xml version="1.0"?>
+<!-- Copyright (C) 2008 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     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.m68k.core">
+  <reg name="d0" bitsize="32"/>
+  <reg name="d1" bitsize="32"/>
+  <reg name="d2" bitsize="32"/>
+  <reg name="d3" bitsize="32"/>
+  <reg name="d4" bitsize="32"/>
+  <reg name="d5" bitsize="32"/>
+  <reg name="d6" bitsize="32"/>
+  <reg name="d7" bitsize="32"/>
+  <reg name="a0" bitsize="32" type="data_ptr"/>
+  <reg name="a1" bitsize="32" type="data_ptr"/>
+  <reg name="a2" bitsize="32" type="data_ptr"/>
+  <reg name="a3" bitsize="32" type="data_ptr"/>
+  <reg name="a4" bitsize="32" type="data_ptr"/>
+  <reg name="a5" bitsize="32" type="data_ptr"/>
+  <reg name="fp" bitsize="32" type="data_ptr"/>
+  <reg name="sp" bitsize="32" type="data_ptr"/>
+
+  <reg name="ps" bitsize="32"/>
+  <reg name="pc" bitsize="32" type="code_ptr"/>
+
+</feature>
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 9445fcd..976e624 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -297,6 +297,21 @@  static void m68k_cpu_class_init(ObjectClass *c, void *data)
     dc->vmsd = &vmstate_m68k_cpu;
 }
 
+static void m68k_cpu_class_init_m68k_core(ObjectClass *c, void *data)
+{
+    CPUClass *cc = CPU_CLASS(c);
+
+    cc->gdb_core_xml_file = "m68k-core.xml";
+}
+
+#define DEFINE_M68K_CPU_TYPE_WITH_CLASS(cpu_model, initfn, classinit)      \
+    {                                                                      \
+        .name = M68K_CPU_TYPE_NAME(cpu_model),                             \
+        .instance_init = initfn,                                           \
+        .parent = TYPE_M68K_CPU,                                           \
+        .class_init = classinit,                                           \
+    }
+
 #define DEFINE_M68K_CPU_TYPE(cpu_model, initfn) \
     {                                           \
         .name = M68K_CPU_TYPE_NAME(cpu_model),  \
@@ -314,11 +329,16 @@  static const TypeInfo m68k_cpus_type_infos[] = {
         .class_size = sizeof(M68kCPUClass),
         .class_init = m68k_cpu_class_init,
     },
-    DEFINE_M68K_CPU_TYPE("m68000", m68000_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68020", m68020_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68030", m68030_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68040", m68040_cpu_initfn),
-    DEFINE_M68K_CPU_TYPE("m68060", m68060_cpu_initfn),
+    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68000", m68000_cpu_initfn,
+                                    m68k_cpu_class_init_m68k_core),
+    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68020", m68020_cpu_initfn,
+                                    m68k_cpu_class_init_m68k_core),
+    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68030", m68030_cpu_initfn,
+                                    m68k_cpu_class_init_m68k_core),
+    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68040", m68040_cpu_initfn,
+                                    m68k_cpu_class_init_m68k_core),
+    DEFINE_M68K_CPU_TYPE_WITH_CLASS("m68060", m68060_cpu_initfn,
+                                    m68k_cpu_class_init_m68k_core),
     DEFINE_M68K_CPU_TYPE("m5206", m5206_cpu_initfn),
     DEFINE_M68K_CPU_TYPE("m5208", m5208_cpu_initfn),
     DEFINE_M68K_CPU_TYPE("cfv4e", cfv4e_cpu_initfn),