m68k-elf return value registers

m68k-elf return value registers

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 1331 bytes --]


This patch, originally written by Nathan Sidwel, makes m68k-tdep.c use right 
registrers for returning values from functions on bare metal. There are 
actually 3 parts:

1. Right now, it's assumed that pointer values are returned in %a0. However,
for m68k-elf, they are returned in %d0. So, this patch makes %a0 the default,
and then makes OSABI sniffers set %a0 again.

2. Structures can be returned in either %a0 or %a1. This was discussed 
previously at:

	http://thread.gmane.org/gmane.comp.gdb.devel/20117

I have now put together a table listing what register gcc uses, and how
gdb does the same with this patch. See:

	http://tinyurl.com/5r2j5x6

As you see, in the end we have only uclinuxoldeabi using wrong register
(just as now). I am not sure whether (i) this can be fixed, and (ii) anybody
care, given that this target was proposed for deprecation in gcc 4.6. Anyway,
if somebody cares, it can be done with a separate patch.

3. This patch also updates the logic that decides whether structure is 
returned in memory or registers. This is a black magic written by Nathan
that I don't pretend to understand.

Is this OK? I have tests still running, but they will only cover elf and 
linux, so it would be great to have somebody check behaviour for other
targets.


-- 
Vladimir Prus
Mentor Graphics
+7 (812) 677-68-40

[-- Attachment #2: m68k-return.diff --]
[-- Type: text/x-patch, Size: 8507 bytes --]

commit 45c4637b3cf602268bb69b9e7fc045d5cbb70d71
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Fri Jan 14 00:10:12 2011 +0300

    Use the right structure and pointer return registers on m68k-elf.
    
    	* m68k-tdep.h (struct gdbarch_tdep): Add ptr_value_regnum
    	field.
    	* m68k-tdep.c (m68k_gdbarch_init): Use A0 for structure
    	returns on ELF targets, A1 otherwise.  Use %d0 for ptr_value_regnum.
    	(m68k_svr4_extract_return_value): Use
    	tdep->ptr_value_regnum for pointer returns.
    	(m68k_svr4_store_return_value): Likewise.
    	(m68k_reg_struct_return_r): New, broken out of ...
    	(m68k_reg_struct_return_p): ... here.  Implement gcc's structure
    	mode algorithm.
    	(m68k_svr4_init_abi): No need to specify %a0 for structure
    	returns here.  Set ptr_value_regnum, though.
    
    	* m68kbsd-tdep.c (m68kbsd_aout_init_abi): Set ptr_value_regnum to
    	A0.
    	(m68kbsd_elf_init_abi): Use A1 for struct return on OpenBSD.

diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index 082cb84..194ecf3 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -320,7 +320,7 @@ m68k_svr4_extract_return_value (struct type *type, struct regcache *regcache,
       convert_typed_floating (buf, fpreg_type, valbuf, type);
     }
   else if (TYPE_CODE (type) == TYPE_CODE_PTR && len == 4)
-    regcache_raw_read (regcache, M68K_A0_REGNUM, valbuf);
+    regcache_raw_read (regcache, tdep->ptr_value_regnum, valbuf);
   else
     m68k_extract_return_value (type, regcache, valbuf);
 }
@@ -362,16 +362,77 @@ m68k_svr4_store_return_value (struct type *type, struct regcache *regcache,
       regcache_raw_write (regcache, M68K_FP0_REGNUM, buf);
     }
   else if (TYPE_CODE (type) == TYPE_CODE_PTR && len == 4)
-    {
-      regcache_raw_write (regcache, M68K_A0_REGNUM, valbuf);
-      regcache_raw_write (regcache, M68K_D0_REGNUM, valbuf);
-    }
+    regcache_raw_write (regcache, tdep->ptr_value_regnum, valbuf);
   else
     m68k_store_return_value (type, regcache, valbuf);
 }
 
 /* Return non-zero if TYPE, which is assumed to be a structure or
    union type, should be returned in registers for architecture
+   GDBARCH.
+
+   Unfortunately GCC incorrectly implements this optimization.  Rather
+   than simply return all small structs, or even just those of size
+   2^N, it uses the mode of the structure to determine this.  BLKmode
+   structs will be returned by memory and QI,HI,SI,DI,SF&DF mode
+   structs will be returned by register.  For m68k a struct is BLKmode
+   unless it's size is 2^N and the mode for that size does not need a
+   greater alignment than the structure itself.  This is horrible.  */
+
+
+static int
+m68k_reg_struct_return_r (struct type *type, int *align_p)
+{
+  enum type_code code = TYPE_CODE (type);
+  int len = TYPE_LENGTH (type);
+  int field_align = 1;
+  int my_align = len > 2 ? 2 : len;
+  int ix;
+
+  if (code != TYPE_CODE_STRUCT && code != TYPE_CODE_UNION)
+    {
+      if (align_p && my_align > *align_p)
+	*align_p = my_align;
+      return 1;
+    }
+
+  if ((len & -len) != len)
+    /* Length is not 2^n. */
+    return 0;
+
+  for (ix = 0; ix != TYPE_NFIELDS (type); ix++)
+    {
+      struct type *field_type;
+      int field_len;
+
+      if (field_is_static (&TYPE_FIELD (type, ix)))
+	/* Skip static fields.  */
+	continue;
+
+      field_type = TYPE_FIELD_TYPE (type, ix);
+      field_type = check_typedef (field_type);
+      field_len = TYPE_LENGTH (field_type);
+
+      /* Look through arrays.  */
+      while (TYPE_CODE (field_type) == TYPE_CODE_ARRAY)
+	{
+	  field_type = TYPE_TARGET_TYPE (field_type);
+	  field_type = check_typedef (field_type);
+	  field_len = TYPE_LENGTH (field_type);
+	}
+
+      if (!m68k_reg_struct_return_r (field_type, &field_align))
+	return 0;
+    }
+
+  if (align_p && field_align > *align_p)
+    *align_p = field_align;
+
+  return align_p || my_align <= field_align;
+}
+
+/* Return non-zero if TYPE, which is assumed to be a structure or
+   union type, should be returned in registers for architecture
    GDBARCH.  */
 
 static int
@@ -386,7 +447,11 @@ m68k_reg_struct_return_p (struct gdbarch *gdbarch, struct type *type)
   if (tdep->struct_return == pcc_struct_return)
     return 0;
 
-  return (len == 1 || len == 2 || len == 4 || len == 8);
+  if (len > 8)
+    /* Length is too big. */
+    return 0;
+
+  return m68k_reg_struct_return_r (type, NULL);
 }
 
 /* Determine, for architecture GDBARCH, how a return value of TYPE
@@ -440,25 +505,11 @@ m68k_svr4_return_value (struct gdbarch *gdbarch, struct type *func_type,
   if ((code == TYPE_CODE_STRUCT || code == TYPE_CODE_UNION)
       && !m68k_reg_struct_return_p (gdbarch, type))
     {
-      /* The System V ABI says that:
-
-	 "A function returning a structure or union also sets %a0 to
-	 the value it finds in %a0.  Thus when the caller receives
-	 control again, the address of the returned object resides in
-	 register %a0."
-
-	 So the ABI guarantees that we can always find the return
-	 value just after the function has returned.  */
-
-      if (readbuf)
-	{
-	  ULONGEST addr;
-
-	  regcache_raw_read_unsigned (regcache, M68K_A0_REGNUM, &addr);
-	  read_memory (addr, readbuf, TYPE_LENGTH (type));
-	}
-
-      return RETURN_VALUE_ABI_RETURNS_ADDRESS;
+      /* Although they SYSV ABI specifies that a function returning a
+	 structure this way should preserve %a0, GCC doesn't do that.
+	 Furthermore there's no point changeing GCC to make it do it,
+	 as that would just be bloat. */
+      return RETURN_VALUE_STRUCT_CONVENTION;
     }
 
   /* This special case is for structures consisting of a single
@@ -1053,8 +1104,7 @@ m68k_svr4_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* SVR4 uses a different calling convention.  */
   set_gdbarch_return_value (gdbarch, m68k_svr4_return_value);
 
-  /* SVR4 uses %a0 instead of %a1.  */
-  tdep->struct_value_regnum = M68K_A0_REGNUM;
+  tdep->ptr_value_regnum = M68K_A0_REGNUM;
 }
 \f
 
@@ -1228,7 +1278,17 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Function call & return.  */
   set_gdbarch_push_dummy_call (gdbarch, m68k_push_dummy_call);
   set_gdbarch_return_value (gdbarch, m68k_return_value);
+  tdep->struct_return = reg_struct_return;
 
+  /* These register numbers may be overridden by an OSABI
+     sniffer.  */
+  if (info.abfd == NULL)
+    tdep->struct_value_regnum = M68K_A0_REGNUM;
+  else if (bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
+    tdep->struct_value_regnum = M68K_A0_REGNUM;
+  else
+    tdep->struct_value_regnum = M68K_A1_REGNUM;
+  tdep->ptr_value_regnum = M68K_D0_REGNUM;
 
   /* Disassembler.  */
   set_gdbarch_print_insn (gdbarch, print_insn_m68k);
@@ -1239,8 +1299,6 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 #else
   tdep->jb_pc = -1;
 #endif
-  tdep->struct_value_regnum = M68K_A1_REGNUM;
-  tdep->struct_return = reg_struct_return;
 
   /* Frame unwinder.  */
   set_gdbarch_dummy_id (gdbarch, m68k_dummy_id);
diff --git a/gdb/m68k-tdep.h b/gdb/m68k-tdep.h
index 596a8cb..3ea7f24 100644
--- a/gdb/m68k-tdep.h
+++ b/gdb/m68k-tdep.h
@@ -83,6 +83,9 @@ struct gdbarch_tdep
   /* Convention for returning structures.  */
   enum struct_return struct_return;
 
+  /* Register in which pointers are returned.  */
+  int ptr_value_regnum;
+
   /* Convention for returning floats.  zero in int regs, non-zero in float.  */
   int float_return;
 
diff --git a/gdb/m68kbsd-tdep.c b/gdb/m68kbsd-tdep.c
index 81e34e9..f5bfa1a 100644
--- a/gdb/m68kbsd-tdep.c
+++ b/gdb/m68kbsd-tdep.c
@@ -213,11 +213,12 @@ m68kbsd_aout_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   m68kbsd_init_abi (info, gdbarch);
 
   tdep->struct_return = reg_struct_return;
+  tdep->ptr_value_regnum = M68K_A0_REGNUM;
 
   tramp_frame_prepend_unwinder (gdbarch, &m68kobsd_sigtramp);
 }
 
-/* NetBSD ELF.  */
+/* OpenBSD and NetBSD ELF.  */
 
 static void
 m68kbsd_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
@@ -233,6 +234,15 @@ m68kbsd_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* NetBSD ELF uses SVR4-style shared libraries.  */
   set_solib_svr4_fetch_link_map_offsets
     (gdbarch, svr4_ilp32_fetch_link_map_offsets);
+
+  /* OpenBSD uses %a1to return structures.  */
+  if (info.abfd)
+    {
+      enum gdb_osabi osabi = gdbarch_lookup_osabi (info.abfd);
+
+      if (osabi == GDB_OSABI_OPENBSD_ELF)
+	tdep->struct_value_regnum = M68K_A1_REGNUM;
+    }
 }
 \f
 

Re: m68k-elf return value registers

[Mark Kettenis]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Fri, 14 Jan 2011 01:57:38 +0300
> 
> This patch, originally written by Nathan Sidwel, makes m68k-tdep.c use right 
> registrers for returning values from functions on bare metal. There are 
> actually 3 parts:
> 
> 1. Right now, it's assumed that pointer values are returned in %a0. However,
> for m68k-elf, they are returned in %d0. So, this patch makes %a0 the default,
> and then makes OSABI sniffers set %a0 again.
> 
> 2. Structures can be returned in either %a0 or %a1. This was discussed 
> previously at:
> 
> 	http://thread.gmane.org/gmane.comp.gdb.devel/20117
> 
> I have now put together a table listing what register gcc uses, and how
> gdb does the same with this patch. See:
> 
> 	http://tinyurl.com/5r2j5x6
> 
> As you see, in the end we have only uclinuxoldeabi using wrong register
> (just as now). I am not sure whether (i) this can be fixed, and (ii) anybody
> care, given that this target was proposed for deprecation in gcc 4.6. Anyway,
> if somebody cares, it can be done with a separate patch.
> 
> 3. This patch also updates the logic that decides whether structure is 
> returned in memory or registers. This is a black magic written by Nathan
> that I don't pretend to understand.
> 
> Is this OK? I have tests still running, but they will only cover elf and 
> linux, so it would be great to have somebody check behaviour for other
> targets.

Sorry, but this doesn't make an awful lot of sense to me.  For one
thing, there is no such thing as an OpenBSD ELF platform.
OpenBSD/m68k still uses a.out as its executable format.

Second, there is a formal System V ABI for the Motorola 68000.  I
think GCC has never properly implemented that ABI.  But it seems that
GCC has diverged even more from it in the last couple of years.
Probably the point has been reached that you shouldn't call this the
System V ABI anymore.  So it'd make more sense to implement the GCC
ABI in its own set of functions, and leave the SVR4 variants (largely)
alone.

Then, m68k_reg_struct_return_r() is a nonsensical name.  That function
should probably be renamed in something like
m68k_gcc_reg_struct_return_p().  The comment isn't really helpful,
since it uses GCC-specific terminology.  Can the rules be formulated
in a more GDB-centric way?  I never quite understood what BLKmode is,
and I always have to look up what QI mode, SF mode, etc. etc. are.

Also, you should make sure that that code doesn't get used for
OpenBSD/m68k a.out, since it will be wrong.

Also, I think you're trying to fix too many issues in a single diff,
which makes it hard to review.  Can you split this up in seperate
diffs each addressing a seperate issue?  That should allow me to give
a bit more constructive feedback.  I probably won't be able to do so
until the 2nd half of February though.

Re: m68k-elf return value registers

[Vladimir Prus]

On Friday, January 14, 2011 13:37:15 Mark Kettenis wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Fri, 14 Jan 2011 01:57:38 +0300
> > 
> > This patch, originally written by Nathan Sidwel, makes m68k-tdep.c use
> > right registrers for returning values from functions on bare metal.
> > There are actually 3 parts:
> > 
> > 1. Right now, it's assumed that pointer values are returned in %a0.
> > However, for m68k-elf, they are returned in %d0. So, this patch makes
> > %a0 the default, and then makes OSABI sniffers set %a0 again.
> > 
> > 2. Structures can be returned in either %a0 or %a1. This was discussed
> > 
> > previously at:
> > 	http://thread.gmane.org/gmane.comp.gdb.devel/20117
> > 
> > I have now put together a table listing what register gcc uses, and how
> > 
> > gdb does the same with this patch. See:
> > 	http://tinyurl.com/5r2j5x6
> > 
> > As you see, in the end we have only uclinuxoldeabi using wrong register
> > (just as now). I am not sure whether (i) this can be fixed, and (ii)
> > anybody care, given that this target was proposed for deprecation in gcc
> > 4.6. Anyway, if somebody cares, it can be done with a separate patch.
> > 
> > 3. This patch also updates the logic that decides whether structure is
> > returned in memory or registers. This is a black magic written by Nathan
> > that I don't pretend to understand.
> > 
> > Is this OK? I have tests still running, but they will only cover elf and
> > linux, so it would be great to have somebody check behaviour for other
> > targets.
> 
> Sorry, but this doesn't make an awful lot of sense to me.  For one
> thing, there is no such thing as an OpenBSD ELF platform.
> OpenBSD/m68k still uses a.out as its executable format.

Oh. I might have mistakenly assumed that OpenBSD uses the same format
on x86 and m68k. Well, if it's a.out on m68k this actually simplified things.
Please refer to updated table.

	http://tinyurl.com/5r2j5x6

For OpenBSD/a.out, m68k-tdep.c sets structure return register to a1, which matches
that GCC uses for that purpose. No need for override m68kbsd-tdep.c

> Second, there is a formal System V ABI for the Motorola 68000.  I
> think GCC has never properly implemented that ABI.  But it seems that
> GCC has diverged even more from it in the last couple of years.
> Probably the point has been reached that you shouldn't call this the
> System V ABI anymore.  So it'd make more sense to implement the GCC
> ABI in its own set of functions, and leave the SVR4 variants (largely)
> alone.

Are there any other compilers that matter for m68k and for which GDB should be
used. If not, then probably, those functions should be just renamed?

> Then, m68k_reg_struct_return_r() is a nonsensical name.  That function
> should probably be renamed in something like
> m68k_gcc_reg_struct_return_p().  

Well, there's m68k_reg_struct_return_p() and m68k_reg_struct_return_r(), with
the _r version being a recursive version of _p. I suppose I can add gcc to both
function, but that won't change the need to have non-recursive front-end
and recursive back-end functions, I don't think.

> The comment isn't really helpful,
> since it uses GCC-specific terminology.  Can the rules be formulated
> in a more GDB-centric way?  I never quite understood what BLKmode is,
> and I always have to look up what QI mode, SF mode, etc. etc. are.

I think the problem with describing things in GDB-centric way is that nobody
will be able to match that description with GCC logic -- which is pretty
much the point of this function. 

> 
> Also, you should make sure that that code doesn't get used for
> OpenBSD/m68k a.out, since it will be wrong.

Ok, I'll think about making this code only used for ELF targets.

> 
> Also, I think you're trying to fix too many issues in a single diff,
> which makes it hard to review.  Can you split this up in seperate
> diffs each addressing a seperate issue?  

How about this split:

- Register used to return structures
- Logic used to decide whether a structure is returned in register
- Adjusting register used for returning pointers

Thanks,

-- 
Vladimir Prus
Mentor Graphics
+7 (812) 677-68-40

Re: m68k-elf return value registers

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 502 bytes --]

On Tuesday, January 25, 2011 19:49:15 Vladimir Prus wrote:

> How about this split:
> 
> - Register used to return structures
> - Logic used to decide whether a structure is returned in register
> - Adjusting register used for returning pointers

Here's a patch to adjust the register for returning pointers.
Comments? Note that I still not able to test on classic m68k,
therefore eye inspection or testing there would be still required.

Thanks,


-- 
Vladimir Prus
Mentor Graphics
+7 (812) 677-68-40

[-- Attachment #2: ptr_value_regnum.diff --]
[-- Type: text/x-patch, Size: 3209 bytes --]

commit 607918e383bd3ba7f84ce5f3a1255da5ac5ee401
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Tue Apr 26 15:31:18 2011 +0400

    Make pointer return value be right on bare metal.
    
    	* m68k-tdep.h (struct gdbarch_tdep): New field
    	ptr_value_regnum.
    	* m68k-tdep.c (m68k_svr4_extract_return_value): Use
    	tdep->ptr_value_regnum.
    	(m68k_svr4_store_return_value): Likewise.
    	(m68k_svr4_init_abi): Set ptr_value_regnum to %a0.
    	(m68k_gdbarch_init): Set ptr_value_regnum to %d0.
    	* m68kbsd-tdep.c (m68kbsd_aout_init_abi): Set ptr_value_regnum
    	to %a0.

diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index 6d7a824..c6ae36d 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -329,7 +329,7 @@ m68k_svr4_extract_return_value (struct type *type, struct regcache *regcache,
       convert_typed_floating (buf, fpreg_type, valbuf, type);
     }
   else if (TYPE_CODE (type) == TYPE_CODE_PTR && len == 4)
-    regcache_raw_read (regcache, M68K_A0_REGNUM, valbuf);
+    regcache_raw_read (regcache, tdep->ptr_value_regnum, valbuf);
   else
     m68k_extract_return_value (type, regcache, valbuf);
 }
@@ -371,10 +371,7 @@ m68k_svr4_store_return_value (struct type *type, struct regcache *regcache,
       regcache_raw_write (regcache, M68K_FP0_REGNUM, buf);
     }
   else if (TYPE_CODE (type) == TYPE_CODE_PTR && len == 4)
-    {
-      regcache_raw_write (regcache, M68K_A0_REGNUM, valbuf);
-      regcache_raw_write (regcache, M68K_D0_REGNUM, valbuf);
-    }
+    regcache_raw_write (regcache, tdep->ptr_value_regnum, valbuf);
   else
     m68k_store_return_value (type, regcache, valbuf);
 }
@@ -1065,6 +1062,8 @@ m68k_svr4_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   /* SVR4 uses %a0 instead of %a1.  */
   tdep->struct_value_regnum = M68K_A0_REGNUM;
+
+  tdep->ptr_value_regnum = M68K_A0_REGNUM;
 }
 \f
 
@@ -1239,6 +1238,7 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_push_dummy_call (gdbarch, m68k_push_dummy_call);
   set_gdbarch_return_value (gdbarch, m68k_return_value);
 
+  tdep->ptr_value_regnum = M68K_D0_REGNUM;
 
   /* Disassembler.  */
   set_gdbarch_print_insn (gdbarch, print_insn_m68k);
diff --git a/gdb/m68k-tdep.h b/gdb/m68k-tdep.h
index 596a8cb..3ea7f24 100644
--- a/gdb/m68k-tdep.h
+++ b/gdb/m68k-tdep.h
@@ -83,6 +83,9 @@ struct gdbarch_tdep
   /* Convention for returning structures.  */
   enum struct_return struct_return;
 
+  /* Register in which pointers are returned.  */
+  int ptr_value_regnum;
+
   /* Convention for returning floats.  zero in int regs, non-zero in float.  */
   int float_return;
 
diff --git a/gdb/m68kbsd-tdep.c b/gdb/m68kbsd-tdep.c
index 81e34e9..3b5f5e0 100644
--- a/gdb/m68kbsd-tdep.c
+++ b/gdb/m68kbsd-tdep.c
@@ -213,11 +213,12 @@ m68kbsd_aout_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   m68kbsd_init_abi (info, gdbarch);
 
   tdep->struct_return = reg_struct_return;
+  tdep->ptr_value_regnum = M68K_A0_REGNUM;
 
   tramp_frame_prepend_unwinder (gdbarch, &m68kobsd_sigtramp);
 }
 
-/* NetBSD ELF.  */
+/* OpenBSD and NetBSD ELF.  */
 
 static void
 m68kbsd_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

Re: m68k-elf return value registers

[Vladimir Prus]

On Tuesday, April 26, 2011 15:37:17 Vladimir Prus wrote:
> On Tuesday, January 25, 2011 19:49:15 Vladimir Prus wrote:
> > How about this split:
> > 
> >
> > - Register used to return structures
> > - Logic used to decide whether a structure is returned in register
> > - Adjusting register used for returning pointers
> 
> Here's a patch to adjust the register for returning pointers.
> Comments? Note that I still not able to test on classic m68k,
> therefore eye inspection or testing there would be still required.

Mark,

were you able to take a look at this patch, as well as two others,
into which my original patch has been split?

Thanks,

-- 
Vladimir Prus
CodeSourcery / Mentor Graphics
+7 (812) 677-68-40

> -- 
> Vladimir Prus
> Mentor Graphics
> +7 (812) 677-68-40
> ptr_value_regnum.diff
>   commit 607918e383bd3ba7f84ce5f3a1255da5ac5ee401
> Author: Vladimir Prus <vladimir@codesourcery.com>
> Date:   Tue Apr 26 15:31:18 2011 +0400
> 
>     Make pointer return value be right on bare metal.
>     
>         * m68k-tdep.h (struct gdbarch_tdep): New field
>         ptr_value_regnum.
>         * m68k-tdep.c (m68k_svr4_extract_return_value): Use
>         tdep->ptr_value_regnum.
>         (m68k_svr4_store_return_value): Likewise.
>         (m68k_svr4_init_abi): Set ptr_value_regnum to %a0.
>         (m68k_gdbarch_init): Set ptr_value_regnum to %d0.
>         * m68kbsd-tdep.c (m68kbsd_aout_init_abi): Set ptr_value_regnum
>         to %a0.
> 
> diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
> index 6d7a824..c6ae36d 100644
> --- a/gdb/m68k-tdep.c
> +++ b/gdb/m68k-tdep.c
> @@ -329,7 +329,7 @@ m68k_svr4_extract_return_value (struct type *type,
> struct regcache *regcache, convert_typed_floating (buf, fpreg_type,
> valbuf, type);
>      }
>    else if (TYPE_CODE (type) == TYPE_CODE_PTR && len == 4)
> -    regcache_raw_read (regcache, M68K_A0_REGNUM, valbuf);
> +    regcache_raw_read (regcache, tdep->ptr_value_regnum, valbuf);
>    else
>      m68k_extract_return_value (type, regcache, valbuf);
>  }
> @@ -371,10 +371,7 @@ m68k_svr4_store_return_value (struct type *type,
> struct regcache *regcache, regcache_raw_write (regcache, M68K_FP0_REGNUM,
> buf);
>      }
>    else if (TYPE_CODE (type) == TYPE_CODE_PTR && len == 4)
> -    {
> -      regcache_raw_write (regcache, M68K_A0_REGNUM, valbuf);
> -      regcache_raw_write (regcache, M68K_D0_REGNUM, valbuf);
> -    }
> +    regcache_raw_write (regcache, tdep->ptr_value_regnum, valbuf);
>    else
>      m68k_store_return_value (type, regcache, valbuf);
>  }
> @@ -1065,6 +1062,8 @@ m68k_svr4_init_abi (struct gdbarch_info info, struct
> gdbarch *gdbarch) 
>    /* SVR4 uses %a0 instead of %a1.  */
>    tdep->struct_value_regnum = M68K_A0_REGNUM;
> +
> +  tdep->ptr_value_regnum = M68K_A0_REGNUM;
>  }
>  \f
>  
> @@ -1239,6 +1238,7 @@ m68k_gdbarch_init (struct gdbarch_info info, struct
> gdbarch_list *arches) set_gdbarch_push_dummy_call (gdbarch,
> m68k_push_dummy_call);
>    set_gdbarch_return_value (gdbarch, m68k_return_value);
>  
> +  tdep->ptr_value_regnum = M68K_D0_REGNUM;
>  
>    /* Disassembler.  */
>    set_gdbarch_print_insn (gdbarch, print_insn_m68k);
> diff --git a/gdb/m68k-tdep.h b/gdb/m68k-tdep.h
> index 596a8cb..3ea7f24 100644
> --- a/gdb/m68k-tdep.h
> +++ b/gdb/m68k-tdep.h
> @@ -83,6 +83,9 @@ struct gdbarch_tdep
>    /* Convention for returning structures.  */
>    enum struct_return struct_return;
>  
> +  /* Register in which pointers are returned.  */
> +  int ptr_value_regnum;
> +
>    /* Convention for returning floats.  zero in int regs, non-zero in
> float.  */ int float_return;
>  
> diff --git a/gdb/m68kbsd-tdep.c b/gdb/m68kbsd-tdep.c
> index 81e34e9..3b5f5e0 100644
> --- a/gdb/m68kbsd-tdep.c
> +++ b/gdb/m68kbsd-tdep.c
> @@ -213,11 +213,12 @@ m68kbsd_aout_init_abi (struct gdbarch_info info,
> struct gdbarch *gdbarch) m68kbsd_init_abi (info, gdbarch);
>  
>    tdep->struct_return = reg_struct_return;
> +  tdep->ptr_value_regnum = M68K_A0_REGNUM;
>  
>    tramp_frame_prepend_unwinder (gdbarch, &m68kobsd_sigtramp);
>  }
>  
> -/* NetBSD ELF.  */
> +/* OpenBSD and NetBSD ELF.  */
>  
>  static void
>  m68kbsd_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

Re: m68k-elf return value registers

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 293 bytes --]

On Tuesday, January 25, 2011 19:49:15 Vladimir Prus wrote:

> How about this split:
> 
> - Register used to return structures
> - Logic used to decide whether a structure is returned in register

Here's a patch for this aspect.

- Volodya

-- 
Vladimir Prus
Mentor Graphics
+7 (812) 677-68-40

[-- Attachment #2: is_using_register_for_struct_return.diff --]
[-- Type: text/x-patch, Size: 4279 bytes --]

commit 46907f38a4f2d3fadf755aad647fb089d32ad6cb
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Tue Apr 26 15:47:50 2011 +0400

    Adjust "is register used for struct returns" logic to what GCC does.
    
        	* m68k-tdep.c (m68k_reg_struct_return_r): New, broken out of ...
        	(m68k_reg_struct_return_p): ... here.  Implement gcc's structure
        	mode algorithm.
    	(m68k_svr4_return_value): Adjust to gcc behaviour.

diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index c6ae36d..736bf5b 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -378,6 +378,70 @@ m68k_svr4_store_return_value (struct type *type, struct regcache *regcache,
 
 /* Return non-zero if TYPE, which is assumed to be a structure or
    union type, should be returned in registers for architecture
+   GDBARCH.
+
+   Unfortunately GCC incorrectly implements this optimization.  Rather
+   than simply return all small structs, or even just those of size
+   2^N, it uses the mode of the structure to determine this.  BLKmode
+   structs will be returned by memory and QI,HI,SI,DI,SF&DF mode
+   structs will be returned by register.  For m68k a struct is BLKmode
+   unless it's size is 2^N and the mode for that size does not need a
+   greater alignment than the structure itself.  This is horrible.  */
+
+
+static int
+m68k_reg_struct_return_r (struct type *type, int *align_p)
+{
+  enum type_code code = TYPE_CODE (type);
+  int len = TYPE_LENGTH (type);
+  int field_align = 1;
+  int my_align = len > 2 ? 2 : len;
+  int ix;
+
+  if (code != TYPE_CODE_STRUCT && code != TYPE_CODE_UNION)
+    {
+      if (align_p && my_align > *align_p)
+	*align_p = my_align;
+      return 1;
+    }
+
+  if ((len & -len) != len)
+    /* Length is not 2^n. */
+    return 0;
+
+  for (ix = 0; ix != TYPE_NFIELDS (type); ix++)
+    {
+      struct type *field_type;
+      int field_len;
+
+      if (field_is_static (&TYPE_FIELD (type, ix)))
+	/* Skip static fields.  */
+	continue;
+
+      field_type = TYPE_FIELD_TYPE (type, ix);
+      field_type = check_typedef (field_type);
+      field_len = TYPE_LENGTH (field_type);
+
+      /* Look through arrays.  */
+      while (TYPE_CODE (field_type) == TYPE_CODE_ARRAY)
+	{
+	  field_type = TYPE_TARGET_TYPE (field_type);
+	  field_type = check_typedef (field_type);
+	  field_len = TYPE_LENGTH (field_type);
+	}
+
+      if (!m68k_reg_struct_return_r (field_type, &field_align))
+	return 0;
+    }
+
+  if (align_p && field_align > *align_p)
+    *align_p = field_align;
+
+  return align_p || my_align <= field_align;
+}
+
+/* Return non-zero if TYPE, which is assumed to be a structure or
+   union type, should be returned in registers for architecture
    GDBARCH.  */
 
 static int
@@ -392,7 +456,11 @@ m68k_reg_struct_return_p (struct gdbarch *gdbarch, struct type *type)
   if (tdep->struct_return == pcc_struct_return)
     return 0;
 
-  return (len == 1 || len == 2 || len == 4 || len == 8);
+  if (len > 8)
+    /* Length is too big. */
+    return 0;
+
+  return m68k_reg_struct_return_r (type, NULL);
 }
 
 /* Determine, for architecture GDBARCH, how a return value of TYPE
@@ -446,25 +514,11 @@ m68k_svr4_return_value (struct gdbarch *gdbarch, struct type *func_type,
   if ((code == TYPE_CODE_STRUCT || code == TYPE_CODE_UNION)
       && !m68k_reg_struct_return_p (gdbarch, type))
     {
-      /* The System V ABI says that:
-
-	 "A function returning a structure or union also sets %a0 to
-	 the value it finds in %a0.  Thus when the caller receives
-	 control again, the address of the returned object resides in
-	 register %a0."
-
-	 So the ABI guarantees that we can always find the return
-	 value just after the function has returned.  */
-
-      if (readbuf)
-	{
-	  ULONGEST addr;
-
-	  regcache_raw_read_unsigned (regcache, M68K_A0_REGNUM, &addr);
-	  read_memory (addr, readbuf, TYPE_LENGTH (type));
-	}
-
-      return RETURN_VALUE_ABI_RETURNS_ADDRESS;
+      /* Although they SYSV ABI specifies that a function returning a
+	 structure this way should preserve %a0, GCC doesn't do that.
+	 Furthermore there's no point changeing GCC to make it do it,
+	 as that would just be bloat. */
+      return RETURN_VALUE_STRUCT_CONVENTION;
     }
 
   /* This special case is for structures consisting of a single

Re: m68k-elf return value registers

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 665 bytes --]

On Tuesday, January 25, 2011 19:49:15 Vladimir Prus wrote:

> How about this split:
> 
> - Register used to return structures
> - Logic used to decide whether a structure is returned in register
> - Adjusting register used for returning pointers

And here's the patch that adjusts the register used to return structures.

Earlier, you said:

> > Also, you should make sure that that code doesn't get used for
> > OpenBSD/m68k a.out, since it will be wrong.

but I've lost track of what bit of the patch you meant. Could you clarify
that, as part of reviewing whatever individual patch is applicable?

- Volodya

-- 
Vladimir Prus
Mentor Graphics
+7 (812) 677-68-40

[-- Attachment #2: struct_return_register.diff --]
[-- Type: text/x-patch, Size: 2610 bytes --]

commit 6dfd0b7446fce82dba3ae07e24fadb89b9908455
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Fri Jan 14 00:10:12 2011 +0300

    Use the right structure and pointer return registers on m68k-elf.
    
    	* m68k-tdep.c (m68k_gdbarch_init): Use A0 for structure
    	returns on ELF targets, A1 otherwise.
    	(m68k_svr4_init_abi): No need to specify %a0 for structure
    	returns here.
    	* m68kbsd-tdep.c (m68kbsd_elf_init_abi): Use A1 for struct return on OpenBSD.

diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index 736bf5b..accffc8 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -1114,8 +1114,7 @@ m68k_svr4_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* SVR4 uses a different calling convention.  */
   set_gdbarch_return_value (gdbarch, m68k_svr4_return_value);
 
-  /* SVR4 uses %a0 instead of %a1.  */
-  tdep->struct_value_regnum = M68K_A0_REGNUM;
+  tdep->ptr_value_regnum = M68K_A0_REGNUM;
 
   tdep->ptr_value_regnum = M68K_A0_REGNUM;
 }
@@ -1291,7 +1290,16 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Function call & return.  */
   set_gdbarch_push_dummy_call (gdbarch, m68k_push_dummy_call);
   set_gdbarch_return_value (gdbarch, m68k_return_value);
+  tdep->struct_return = reg_struct_return;
 
+  /* These register numbers may be overridden by an OSABI
+     sniffer.  */
+  if (info.abfd == NULL)
+    tdep->struct_value_regnum = M68K_A0_REGNUM;
+  else if (bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
+    tdep->struct_value_regnum = M68K_A0_REGNUM;
+  else
+    tdep->struct_value_regnum = M68K_A1_REGNUM;
   tdep->ptr_value_regnum = M68K_D0_REGNUM;
 
   /* Disassembler.  */
@@ -1303,8 +1311,6 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 #else
   tdep->jb_pc = -1;
 #endif
-  tdep->struct_value_regnum = M68K_A1_REGNUM;
-  tdep->struct_return = reg_struct_return;
 
   /* Frame unwinder.  */
   set_gdbarch_dummy_id (gdbarch, m68k_dummy_id);
diff --git a/gdb/m68kbsd-tdep.c b/gdb/m68kbsd-tdep.c
index 3b5f5e0..f5bfa1a 100644
--- a/gdb/m68kbsd-tdep.c
+++ b/gdb/m68kbsd-tdep.c
@@ -234,6 +234,15 @@ m68kbsd_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* NetBSD ELF uses SVR4-style shared libraries.  */
   set_solib_svr4_fetch_link_map_offsets
     (gdbarch, svr4_ilp32_fetch_link_map_offsets);
+
+  /* OpenBSD uses %a1to return structures.  */
+  if (info.abfd)
+    {
+      enum gdb_osabi osabi = gdbarch_lookup_osabi (info.abfd);
+
+      if (osabi == GDB_OSABI_OPENBSD_ELF)
+	tdep->struct_value_regnum = M68K_A1_REGNUM;
+    }
 }
 \f