Various m68k fixes for gdb

Message ID 20200708135828.620884-1-tromey@adacore.com
State New
Headers show
Series
  • Various m68k fixes for gdb
Related show

Commit Message

Tom Tromey July 8, 2020, 1:58 p.m.
Recently I tried the m68k port of gdb.  It had some issues, which are
fixed in this patch.

* Various types of return values were not being handled properly.  In
  particular:

  * arrays are returned by following the same convention as
    structures.  This matters in languages like Ada, where an array
    can in fact be returned as a value.

  * "long double" was not being handled correctly in
    m68k_svr4_return_value.

  * GCC's m68k back end does not return vector types in registers, so
    change gdb to follow.

  * GCC's m68k back end doesn't faithfully implement the ABI, and so
    some objects with unusual size (not possible in C, but possible in
    Ada) are not returned correctly.

* gcc implements an m68k ABI variant that it simply describes as
  "embedded".  This ABI is similar to the SVR4 ABI, but rather than
  returning pointer-typed values in %a0, such values are returned in
  %d0.  To support this, an ELF osabi sniffer is added.

* Commit 85f7484a ("m68k: tag floating-point ABI used") adds an
  attribute that can be used to recognize when hard- or soft-float is
  in use.  gdb can now read this tag and choose the ABI accordingly.

I was unable to run the gdb test suite with this patch.  Instead, I
tested it using qemu and the internal AdaCore test suite.

gdb/ChangeLog
2020-07-02  Tom Tromey  <tromey@adacore.com>

	* m68k-tdep.c (m68k_extract_return_value): Use
	pointer_result_regnum.
	(m68k_store_return_value): Likewise.
	(m68k_reg_struct_return_p): Handle vectors and arrays.
	(m68k_return_value): Handle arrays.
	(m68k_svr4_return_value): Fix single-element aggregate handling.
	Handle long double.  Adjust for embedded ABI.
	(m68k_svr4_init_abi): Set pointer_result_regnum.
	(m68k_embedded_init_abi): New function.
	(m68k_gdbarch_init): Handle Tag_GNU_M68K_ABI_FP.
	(m68k_osabi_sniffer): New function.
	(_initialize_m68k_tdep): Register osabi sniffer.
	* m68k-tdep.h (struct gdbarch_tdep) <pointer_result_regnum>: New
	member.
---
 gdb/ChangeLog   |  17 +++++
 gdb/m68k-tdep.c | 172 +++++++++++++++++++++++++++++++++++-------------
 gdb/m68k-tdep.h |   4 ++
 3 files changed, 147 insertions(+), 46 deletions(-)

-- 
2.26.2

Comments

Hannes Domani via Gdb-patches July 10, 2020, 3:47 p.m. | #1
On Wed,  8 Jul 2020 07:58:28 -0600
Tom Tromey <tromey@adacore.com> wrote:

> Recently I tried the m68k port of gdb.  It had some issues, which are

> fixed in this patch.

> 

> * Various types of return values were not being handled properly.  In

>   particular:

> 

>   * arrays are returned by following the same convention as

>     structures.  This matters in languages like Ada, where an array

>     can in fact be returned as a value.

> 

>   * "long double" was not being handled correctly in

>     m68k_svr4_return_value.

> 

>   * GCC's m68k back end does not return vector types in registers, so

>     change gdb to follow.

> 

>   * GCC's m68k back end doesn't faithfully implement the ABI, and so

>     some objects with unusual size (not possible in C, but possible in

>     Ada) are not returned correctly.

> 

> * gcc implements an m68k ABI variant that it simply describes as

>   "embedded".  This ABI is similar to the SVR4 ABI, but rather than

>   returning pointer-typed values in %a0, such values are returned in

>   %d0.  To support this, an ELF osabi sniffer is added.

> 

> * Commit 85f7484a ("m68k: tag floating-point ABI used") adds an

>   attribute that can be used to recognize when hard- or soft-float is

>   in use.  gdb can now read this tag and choose the ABI accordingly.

> 

> I was unable to run the gdb test suite with this patch.  Instead, I

> tested it using qemu and the internal AdaCore test suite.

> 

> gdb/ChangeLog

> 2020-07-02  Tom Tromey  <tromey@adacore.com>

> 

> 	* m68k-tdep.c (m68k_extract_return_value): Use

> 	pointer_result_regnum.

> 	(m68k_store_return_value): Likewise.

> 	(m68k_reg_struct_return_p): Handle vectors and arrays.

> 	(m68k_return_value): Handle arrays.

> 	(m68k_svr4_return_value): Fix single-element aggregate handling.

> 	Handle long double.  Adjust for embedded ABI.

> 	(m68k_svr4_init_abi): Set pointer_result_regnum.

> 	(m68k_embedded_init_abi): New function.

> 	(m68k_gdbarch_init): Handle Tag_GNU_M68K_ABI_FP.

> 	(m68k_osabi_sniffer): New function.

> 	(_initialize_m68k_tdep): Register osabi sniffer.

> 	* m68k-tdep.h (struct gdbarch_tdep) <pointer_result_regnum>: New

> 	member.


I skimmed the patch.  LGTM.

Kevin
Tom Tromey Sept. 14, 2020, 2:29 p.m. | #2
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:


Tom> Recently I tried the m68k port of gdb.  It had some issues, which are
Tom> fixed in this patch.

...
Tom> gdb/ChangeLog
Tom> 2020-07-02  Tom Tromey  <tromey@adacore.com>

Tom> 	* m68k-tdep.c (m68k_extract_return_value): Use
Tom> 	pointer_result_regnum.
Tom> 	(m68k_store_return_value): Likewise.
Tom> 	(m68k_reg_struct_return_p): Handle vectors and arrays.
Tom> 	(m68k_return_value): Handle arrays.
Tom> 	(m68k_svr4_return_value): Fix single-element aggregate handling.
Tom> 	Handle long double.  Adjust for embedded ABI.
Tom> 	(m68k_svr4_init_abi): Set pointer_result_regnum.
Tom> 	(m68k_embedded_init_abi): New function.
Tom> 	(m68k_gdbarch_init): Handle Tag_GNU_M68K_ABI_FP.
Tom> 	(m68k_osabi_sniffer): New function.
Tom> 	(_initialize_m68k_tdep): Register osabi sniffer.
Tom> 	* m68k-tdep.h (struct gdbarch_tdep) <pointer_result_regnum>: New
Tom> 	member.

I'm checking this in now.

Tom

Patch

diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index 27870252a36..267aea17db7 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -34,6 +34,8 @@ 
 #include "target-descriptions.h"
 #include "floatformat.h"
 #include "target-float.h"
+#include "elf-bfd.h"
+#include "elf/m68k.h"
 
 #include "m68k-tdep.h"
 
@@ -272,7 +274,12 @@  m68k_value_to_register (struct frame_info *frame, int regnum,
    %d0/%d1 instead of in memory by using -freg-struct-return.  This is
    the default on NetBSD a.out, OpenBSD and GNU/Linux and several
    embedded systems.  This convention is implemented by setting the
-   struct_return member of `struct gdbarch_tdep' to reg_struct_return.  */
+   struct_return member of `struct gdbarch_tdep' to reg_struct_return.
+
+   GCC also has an "embedded" ABI.  This works like the SVR4 ABI,
+   except that pointers are returned in %D0.  This is implemented by
+   setting the pointer_result_regnum member of `struct gdbarch_tdep'
+   as appropriate.  */
 
 /* Read a function return value of TYPE from REGCACHE, and copy that
    into VALBUF.  */
@@ -284,7 +291,13 @@  m68k_extract_return_value (struct type *type, struct regcache *regcache,
   int len = TYPE_LENGTH (type);
   gdb_byte buf[M68K_MAX_REGISTER_SIZE];
 
-  if (len <= 4)
+  if (type->code () == TYPE_CODE_PTR && len == 4)
+    {
+      struct gdbarch *gdbarch = regcache->arch ();
+      struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+      regcache->raw_read (tdep->pointer_result_regnum, valbuf);
+    }
+  else if (len <= 4)
     {
       regcache->raw_read (M68K_D0_REGNUM, buf);
       memcpy (valbuf, buf + (4 - len), len);
@@ -314,8 +327,6 @@  m68k_svr4_extract_return_value (struct type *type, struct regcache *regcache,
       regcache->raw_read (M68K_FP0_REGNUM, buf);
       target_float_convert (buf, fpreg_type, valbuf, type);
     }
-  else if (type->code () == TYPE_CODE_PTR && TYPE_LENGTH (type) == 4)
-    regcache->raw_read (M68K_A0_REGNUM, valbuf);
   else
     m68k_extract_return_value (type, regcache, valbuf);
 }
@@ -328,7 +339,16 @@  m68k_store_return_value (struct type *type, struct regcache *regcache,
 {
   int len = TYPE_LENGTH (type);
 
-  if (len <= 4)
+  if (type->code () == TYPE_CODE_PTR && len == 4)
+    {
+      struct gdbarch *gdbarch = regcache->arch ();
+      struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+      regcache->raw_write (tdep->pointer_result_regnum, valbuf);
+      /* gdb historically also set D0 in the SVR4 case.  */
+      if (tdep->pointer_result_regnum != M68K_D0_REGNUM)
+	regcache->raw_write (M68K_D0_REGNUM, valbuf);
+    }
+  else if (len <= 4)
     regcache->raw_write_part (M68K_D0_REGNUM, 4 - len, len, valbuf);
   else if (len <= 8)
     {
@@ -354,11 +374,6 @@  m68k_svr4_store_return_value (struct type *type, struct regcache *regcache,
       target_float_convert (valbuf, type, buf, fpreg_type);
       regcache->raw_write (M68K_FP0_REGNUM, buf);
     }
-  else if (type->code () == TYPE_CODE_PTR && TYPE_LENGTH (type) == 4)
-    {
-      regcache->raw_write (M68K_A0_REGNUM, valbuf);
-      regcache->raw_write (M68K_D0_REGNUM, valbuf);
-    }
   else
     m68k_store_return_value (type, regcache, valbuf);
 }
@@ -375,11 +390,28 @@  m68k_reg_struct_return_p (struct gdbarch *gdbarch, struct type *type)
   int len = TYPE_LENGTH (type);
 
   gdb_assert (code == TYPE_CODE_STRUCT || code == TYPE_CODE_UNION
-	      || code == TYPE_CODE_COMPLEX);
+	      || code == TYPE_CODE_COMPLEX || code == TYPE_CODE_ARRAY);
 
   if (tdep->struct_return == pcc_struct_return)
     return 0;
 
+  const bool is_vector = code == TYPE_CODE_ARRAY && TYPE_VECTOR (type);
+
+  if (is_vector
+      && check_typedef (TYPE_TARGET_TYPE (type))->code () == TYPE_CODE_FLT)
+    return 0;
+
+  /* According to m68k_return_in_memory in the m68k GCC back-end,
+     strange things happen for small aggregate types.  Aggregate types
+     with only one component are always returned like the type of the
+     component.  Aggregate types whose size is 2, 4, or 8 are returned
+     in registers if their natural alignment is at least 16 bits.
+
+     We reject vectors here, as experimentally this gives the correct
+     answer.  */
+  if (!is_vector && (len == 2 || len == 4 || len == 8))
+    return type_align (type) >= 2;
+
   return (len == 1 || len == 2 || len == 4 || len == 8);
 }
 
@@ -398,7 +430,7 @@  m68k_return_value (struct gdbarch *gdbarch, struct value *function,
 
   /* GCC returns a `long double' in memory too.  */
   if (((code == TYPE_CODE_STRUCT || code == TYPE_CODE_UNION
-	|| code == TYPE_CODE_COMPLEX)
+	|| code == TYPE_CODE_COMPLEX || code == TYPE_CODE_ARRAY)
        && !m68k_reg_struct_return_p (gdbarch, type))
       || (code == TYPE_CODE_FLT && TYPE_LENGTH (type) == 12))
     {
@@ -432,9 +464,23 @@  m68k_svr4_return_value (struct gdbarch *gdbarch, struct value *function,
 {
   enum type_code code = type->code ();
 
-  if ((code == TYPE_CODE_STRUCT || code == TYPE_CODE_UNION
-       || code == TYPE_CODE_COMPLEX)
-      && !m68k_reg_struct_return_p (gdbarch, type))
+  /* Aggregates with a single member are always returned like their
+     sole element.  */
+  if ((code == TYPE_CODE_STRUCT || code == TYPE_CODE_UNION)
+      && type->num_fields () == 1)
+    {
+      type = check_typedef (type->field (0).type ());
+      return m68k_svr4_return_value (gdbarch, function, type, regcache,
+				     readbuf, writebuf);
+    }
+
+  if (((code == TYPE_CODE_STRUCT || code == TYPE_CODE_UNION
+	|| code == TYPE_CODE_COMPLEX || code == TYPE_CODE_ARRAY)
+       && !m68k_reg_struct_return_p (gdbarch, type))
+      /* GCC may return a `long double' in memory too.  */
+      || (!gdbarch_tdep (gdbarch)->float_return
+	  && code == TYPE_CODE_FLT
+	  && TYPE_LENGTH (type) == 12))
     {
       /* The System V ABI says that:
 
@@ -444,32 +490,25 @@  m68k_svr4_return_value (struct gdbarch *gdbarch, struct value *function,
 	 register %a0."
 
 	 So the ABI guarantees that we can always find the return
-	 value just after the function has returned.  */
+	 value just after the function has returned.
+
+	 However, GCC also implements the "embedded" ABI.  That ABI
+	 does not preserve %a0 across calls, but does write the value
+	 back to %d0.  */
 
       if (readbuf)
 	{
+	  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 	  ULONGEST addr;
 
-	  regcache_raw_read_unsigned (regcache, M68K_A0_REGNUM, &addr);
+	  regcache_raw_read_unsigned (regcache, tdep->pointer_result_regnum,
+				      &addr);
 	  read_memory (addr, readbuf, TYPE_LENGTH (type));
 	}
 
       return RETURN_VALUE_ABI_RETURNS_ADDRESS;
     }
 
-  /* This special case is for structures consisting of a single
-     `float' or `double' member.  These structures are returned in
-     %fp0.  For these structures, we call ourselves recursively,
-     changing TYPE into the type of the first member of the structure.
-     Since that should work for all structures that have only one
-     member, we don't bother to check the member's type here.  */
-  if (code == TYPE_CODE_STRUCT && type->num_fields () == 1)
-    {
-      type = check_typedef (type->field (0).type ());
-      return m68k_svr4_return_value (gdbarch, function, type, regcache,
-				     readbuf, writebuf);
-    }
-
   if (readbuf)
     m68k_svr4_extract_return_value (type, regcache, readbuf);
   if (writebuf)
@@ -1062,7 +1101,23 @@  m68k_svr4_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   /* SVR4 uses %a0 instead of %a1.  */
   tdep->struct_value_regnum = M68K_A0_REGNUM;
+
+  /* SVR4 returns pointers in %a0.  */
+  tdep->pointer_result_regnum = M68K_A0_REGNUM;
+}
+
+/* GCC's m68k "embedded" ABI.  This is like the SVR4 ABI, but pointer
+   values are returned in %d0, not %a0.  */
+
+static void
+m68k_embedded_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  m68k_svr4_init_abi (info, gdbarch);
+  tdep->pointer_result_regnum = M68K_D0_REGNUM;
 }
+
 
 
 /* Function: m68k_gdbarch_init
@@ -1155,6 +1210,24 @@  m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	flavour = m68k_coldfire_flavour;
     }
   
+  /* Try to figure out if the arch uses floating registers to return
+     floating point values from functions.  On ColdFire, floating
+     point values are returned in D0.  */
+  int float_return = 0;
+  if (has_fp && flavour != m68k_coldfire_flavour)
+    float_return = 1;
+#ifdef HAVE_ELF
+  if (info.abfd && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
+    {
+      int fp_abi = bfd_elf_get_obj_attr_int (info.abfd, OBJ_ATTR_GNU,
+					     Tag_GNU_M68K_ABI_FP);
+      if (fp_abi == 1)
+	float_return = 1;
+      else if (fp_abi == 2)
+	float_return = 0;
+    }
+#endif /* HAVE_ELF */
+
   /* If there is already a candidate, use it.  */
   for (best_arch = gdbarch_list_lookup_by_info (arches, &info);
        best_arch != NULL;
@@ -1166,6 +1239,9 @@  m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       if (has_fp != gdbarch_tdep (best_arch->gdbarch)->fpregs_present)
 	continue;
 
+      if (float_return != gdbarch_tdep (best_arch->gdbarch)->float_return)
+	continue;
+
       break;
     }
 
@@ -1179,6 +1255,7 @@  m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   tdep = XCNEW (struct gdbarch_tdep);
   gdbarch = gdbarch_alloc (&info, tdep);
   tdep->fpregs_present = has_fp;
+  tdep->float_return = float_return;
   tdep->flavour = flavour;
 
   if (flavour == m68k_coldfire_flavour || flavour == m68k_fido_flavour)
@@ -1214,22 +1291,6 @@  m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   if (has_fp)
     set_gdbarch_fp0_regnum (gdbarch, M68K_FP0_REGNUM);
 
-  /* Try to figure out if the arch uses floating registers to return
-     floating point values from functions.  */
-  if (has_fp)
-    {
-      /* On ColdFire, floating point values are returned in D0.  */
-      if (flavour == m68k_coldfire_flavour)
-	tdep->float_return = 0;
-      else
-	tdep->float_return = 1;
-    }
-  else
-    {
-      /* No floating registers, so can't use them for returning values.  */
-      tdep->float_return = 0;
-    }
-
   /* Function call & return.  */
   set_gdbarch_push_dummy_call (gdbarch, m68k_push_dummy_call);
   set_gdbarch_return_value (gdbarch, m68k_return_value);
@@ -1242,6 +1303,7 @@  m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 #else
   tdep->jb_pc = -1;
 #endif
+  tdep->pointer_result_regnum = M68K_D0_REGNUM;
   tdep->struct_value_regnum = M68K_A1_REGNUM;
   tdep->struct_return = reg_struct_return;
 
@@ -1281,9 +1343,27 @@  m68k_dump_tdep (struct gdbarch *gdbarch, struct ui_file *file)
     return;
 }
 
+/* OSABI sniffer for m68k.  */
+
+static enum gdb_osabi
+m68k_osabi_sniffer (bfd *abfd)
+{
+  unsigned int elfosabi = elf_elfheader (abfd)->e_ident[EI_OSABI];
+
+  if (elfosabi == ELFOSABI_NONE)
+    return GDB_OSABI_SVR4;
+
+  return GDB_OSABI_UNKNOWN;
+}
+
 void _initialize_m68k_tdep ();
 void
 _initialize_m68k_tdep ()
 {
   gdbarch_register (bfd_arch_m68k, m68k_gdbarch_init, m68k_dump_tdep);
+
+  gdbarch_register_osabi_sniffer (bfd_arch_m68k, bfd_target_elf_flavour,
+				  m68k_osabi_sniffer);
+  gdbarch_register_osabi (bfd_arch_m68k, 0, GDB_OSABI_SVR4,
+			  m68k_embedded_init_abi);
 }
diff --git a/gdb/m68k-tdep.h b/gdb/m68k-tdep.h
index 1567505abf8..513190fe8cc 100644
--- a/gdb/m68k-tdep.h
+++ b/gdb/m68k-tdep.h
@@ -79,6 +79,10 @@  struct gdbarch_tdep
      passed to a function.  */
   int struct_value_regnum;
 
+  /* Register in which a pointer value is returned.  In the SVR4 ABI,
+     this is %a0, but in GCC's "embedded" ABI, this is %d0.  */
+  int pointer_result_regnum;
+
   /* Convention for returning structures.  */
   enum struct_return struct_return;