[patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Kazu Hirata]

Hi,

Attached is a revised patch to fix binary compatibility problem with
-mno-strict-align on m68k-elf.

The previous iteration of this patch is at:

  http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01437.html

In this iteration, Nathan has made a change to effectively comment out
TARGET_RETURN_IN_MEMORY and m68k_return_in_memory if
M68K_HONOR_TARGET_STRICT_ALIGNMENT is defined.  This way, this patch
has no effect on m68k-linux, preserving the ABI.

Tested on m68k-elf.  OK to apply?

Kazu Hirata

2007-08-25  Nathan Sidwell  <nathan@codesourcery.com>
	    Kazu Hirata  <kazu@codesourcery.com>

	* gcc/config/m68k/linux.h
	(M68K_HONOR_TARGET_STRICT_ALIGNMENT): Redefine as 0.
	* config/m68k/m68k.c (TARGET_RETURN_IN_MEMORY): New.
	(m68k_return_in_memory): New.
	* gcc/config/m68k/m68k.h (M68K_HONOR_TARGET_STRICT_ALIGNMENT):
	New.

Index: gcc/config/m68k/linux.h
===================================================================
--- gcc/config/m68k/linux.h	(revision 127787)
+++ gcc/config/m68k/linux.h	(working copy)
@@ -31,6 +31,8 @@ along with GCC; see the file COPYING3.  
 
 #undef STRICT_ALIGNMENT
 #define STRICT_ALIGNMENT 0
+#undef M68K_HONOR_TARGET_STRICT_ALIGNMENT
+#define M68K_HONOR_TARGET_STRICT_ALIGNMENT 0
 
 /* Here are four prefixes that are used by asm_fprintf to
    facilitate customization for alternate assembler syntaxes.
Index: gcc/config/m68k/m68k.c
===================================================================
--- gcc/config/m68k/m68k.c	(revision 127787)
+++ gcc/config/m68k/m68k.c	(working copy)
@@ -134,6 +134,9 @@ static void m68k_compute_frame_layout (v
 static bool m68k_save_reg (unsigned int regno, bool interrupt_handler);
 static bool m68k_ok_for_sibcall_p (tree, tree);
 static bool m68k_rtx_costs (rtx, int, int, int *);
+#if M68K_HONOR_TARGET_STRICT_ALIGNMENT
+static bool m68k_return_in_memory (tree, tree);
+#endif
 \f
 
 /* Specify the identification number of the library being built */
@@ -205,6 +208,11 @@ int m68k_last_compare_had_fp_operands;
 #undef TARGET_FUNCTION_OK_FOR_SIBCALL
 #define TARGET_FUNCTION_OK_FOR_SIBCALL m68k_ok_for_sibcall_p
 
+#if M68K_HONOR_TARGET_STRICT_ALIGNMENT
+#undef TARGET_RETURN_IN_MEMORY
+#define TARGET_RETURN_IN_MEMORY m68k_return_in_memory
+#endif
+
 static const struct attribute_spec m68k_attribute_table[] =
 {
   /* { name, min_len, max_len, decl_req, type_req, fn_type_req, handler } */
@@ -4386,3 +4394,25 @@ m68k_function_value (const_tree valtype,
   else
     return gen_rtx_REG (mode, D0_REG);
 }
+
+/* Worker function for TARGET_RETURN_IN_MEMORY.  */
+#if M68K_HONOR_TARGET_STRICT_ALIGNMENT
+static bool
+m68k_return_in_memory (tree type, tree fntype ATTRIBUTE_UNUSED)
+{
+  enum machine_mode mode = TYPE_MODE (type);
+
+  if (mode == BLKmode)
+    return true;
+
+  /* If TYPE's known alignment is less than the alignment of MODE that
+     would contain the structure, then return in memory.  We need to
+     do so to maintain the compatibility between code compiled with
+     -mstrict-align and that compiled with -mno-strict-align.  */
+  if (AGGREGATE_TYPE_P (type)
+      && TYPE_ALIGN (type) < GET_MODE_ALIGNMENT (mode))
+    return true;
+
+  return false;
+}
+#endif
Index: gcc/config/m68k/m68k.h
===================================================================
--- gcc/config/m68k/m68k.h	(revision 127787)
+++ gcc/config/m68k/m68k.h	(working copy)
@@ -310,6 +310,7 @@ along with GCC; see the file COPYING3.  
 #define BIGGEST_ALIGNMENT (TARGET_ALIGN_INT ? 32 : 16)
 
 #define STRICT_ALIGNMENT (TARGET_STRICT_ALIGNMENT)
+#define M68K_HONOR_TARGET_STRICT_ALIGNMENT 1
 
 #define INT_TYPE_SIZE (TARGET_SHORT ? 16 : 32)
 

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Andreas Schwab]

Kazu Hirata <kazu@codesourcery.com> writes:

> 2007-08-25  Nathan Sidwell  <nathan@codesourcery.com>
> 	    Kazu Hirata  <kazu@codesourcery.com>
>
> 	* gcc/config/m68k/linux.h
> 	(M68K_HONOR_TARGET_STRICT_ALIGNMENT): Redefine as 0.
> 	* config/m68k/m68k.c (TARGET_RETURN_IN_MEMORY): New.
> 	(m68k_return_in_memory): New.
> 	* gcc/config/m68k/m68k.h (M68K_HONOR_TARGET_STRICT_ALIGNMENT):
> 	New.

Ok.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Roman Zippel]

Hi,

On Sat, 25 Aug 2007, Kazu Hirata wrote:

> Hi,
> 
> Attached is a revised patch to fix binary compatibility problem with
> -mno-strict-align on m68k-elf.
> 
> The previous iteration of this patch is at:
> 
>   http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01437.html
> 
> In this iteration, Nathan has made a change to effectively comment out
> TARGET_RETURN_IN_MEMORY and m68k_return_in_memory if
> M68K_HONOR_TARGET_STRICT_ALIGNMENT is defined.  This way, this patch
> has no effect on m68k-linux, preserving the ABI.

Looking at this a little closer, I'm not sure why this is related to 
alignment in first place. Whether a structure is returned in a register 
should be controlled by -fpcc-struct-return/-freg-struct-return and the 
DEFAULT_PCC_STRUCT_RETURN value in the config. In 
function.c:aggregate_value_p there is shortly after the return_in_memory 
hook the following test:

  if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type))
    return 1;

IMO the alignment should have no influence here, so where is this coming 
from? 

Looking at the default value of DEFAULT_PCC_STRUCT_RETURN:

$ grep DEFAULT_PCC_STRUCT_RETURN gcc/*.c gcc/config/m68k/*.h
gcc/toplev.c:#ifndef DEFAULT_PCC_STRUCT_RETURN
gcc/toplev.c:#define DEFAULT_PCC_STRUCT_RETURN 1
gcc/toplev.c:int flag_pcc_struct_return = DEFAULT_PCC_STRUCT_RETURN;
gcc/config/m68k/linux.h:#define DEFAULT_PCC_STRUCT_RETURN 0
gcc/config/m68k/m68kemb.h:#define DEFAULT_PCC_STRUCT_RETURN 0
gcc/config/m68k/netbsd-elf.h:#undef DEFAULT_PCC_STRUCT_RETURN
gcc/config/m68k/netbsd-elf.h:#define DEFAULT_PCC_STRUCT_RETURN 1
gcc/config/m68k/openbsd.h:#define DEFAULT_PCC_STRUCT_RETURN 0

m68k-elf includes m68kemb.h, which sets this to 0, so why isn't this 
honored?

bye, Roman

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Kazu Hirata]

Hi Roman,

> Looking at this a little closer, I'm not sure why this is related to 
> alignment in first place. Whether a structure is returned in a register 
> should be controlled by -fpcc-struct-return/-freg-struct-return and the 
> DEFAULT_PCC_STRUCT_RETURN value in the config. In 
> function.c:aggregate_value_p there is shortly after the return_in_memory 
> hook the following test:
> 
>   if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type))
>     return 1;
> 
> IMO the alignment should have no influence here, so where is this coming 
> from? 

STRICT_ALIGNMENT affects us in compute_record_mode and 
default_return_in_memory.  (Note that m68k_return_in_memory emulates 
default_return_in_memory with STRICT_ALIGNMENT 1.)

Given a return type, compute_record_mode computes a return mode.  If 
STRICT_ALIGNMENT is 1, compute_record_mode chooses BLKmode for the type. 
(See the last "if" block in compute_record_mode.)  default_return_in_memory 
simply returns 1 if it sees BLKmode.

> Looking at the default value of DEFAULT_PCC_STRUCT_RETURN:
> 
> $ grep DEFAULT_PCC_STRUCT_RETURN gcc/*.c gcc/config/m68k/*.h
> gcc/toplev.c:#ifndef DEFAULT_PCC_STRUCT_RETURN
> gcc/toplev.c:#define DEFAULT_PCC_STRUCT_RETURN 1
> gcc/toplev.c:int flag_pcc_struct_return = DEFAULT_PCC_STRUCT_RETURN;
> gcc/config/m68k/linux.h:#define DEFAULT_PCC_STRUCT_RETURN 0
> gcc/config/m68k/m68kemb.h:#define DEFAULT_PCC_STRUCT_RETURN 0
> gcc/config/m68k/netbsd-elf.h:#undef DEFAULT_PCC_STRUCT_RETURN
> gcc/config/m68k/netbsd-elf.h:#define DEFAULT_PCC_STRUCT_RETURN 1
> gcc/config/m68k/openbsd.h:#define DEFAULT_PCC_STRUCT_RETURN 0
> 
> m68k-elf includes m68kemb.h, which sets this to 0, so why isn't this 
> honored?

When the return mode is BLKmode, we don't get to evaluate 
flag_pcc_struct_return in aggregate_value_p because 
targetm.calls.return_in_memory is called earlier.

Kazu Hirata

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Roman Zippel]

Hi,

On Wed, 29 Aug 2007, Kazu Hirata wrote:

> Hi Roman,
> 
> > Looking at this a little closer, I'm not sure why this is related to
> > alignment in first place. Whether a structure is returned in a register
> > should be controlled by -fpcc-struct-return/-freg-struct-return and the
> > DEFAULT_PCC_STRUCT_RETURN value in the config. In
> > function.c:aggregate_value_p there is shortly after the return_in_memory
> > hook the following test:
> > 
> >   if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type))
> >     return 1;
> > 
> > IMO the alignment should have no influence here, so where is this coming
> > from? 
> 
> STRICT_ALIGNMENT affects us in compute_record_mode and
> default_return_in_memory.  (Note that m68k_return_in_memory emulates
> default_return_in_memory with STRICT_ALIGNMENT 1.)
> 
> Given a return type, compute_record_mode computes a return mode.  If
> STRICT_ALIGNMENT is 1, compute_record_mode chooses BLKmode for the type. (See
> the last "if" block in compute_record_mode.)  default_return_in_memory simply
> returns 1 if it sees BLKmode.

I see.
What concerns me here is that this produces incompatibilities for ColdFire 
code between Linux and other targets. It also makes the support for this 
in libffi interesting (e.g. if you ever want Java support for ColdFire).

Since this is a special case I would seriously consider to ignore the 
alignment mode for this in m68k_return_in_memory. Since this is an ABI 
issue there probably should be an option to get the old behaviour.

bye, Roman

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Nathan Sidwell]

Roman Zippel wrote:

> Looking at this a little closer, I'm not sure why this is related to 
> alignment in first place. Whether a structure is returned in a register 
> should be controlled by -fpcc-struct-return/-freg-struct-return and the 
> DEFAULT_PCC_STRUCT_RETURN value in the config. In 
> function.c:aggregate_value_p there is shortly after the return_in_memory 
> hook the following test:
> 
>   if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type))
>     return 1;
> 
> IMO the alignment should have no influence here, so where is this coming 
> from? 

store layout sets the mode of a structure depending on STRICT_ALIGNMENT.  For 
instance a {short, short} struct will get SI mode if STRICT_ALIGNMENT is false. 
  When it's true such a struct will get BLKmode.  The default can-return-in-reg 
hook examines the type's mode.  If it's BLKmode, it is returned in memory, if 
it's an integral mode it is returned in a register.

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Nathan Sidwell]

Roman Zippel wrote:

> I see.
> What concerns me here is that this produces incompatibilities for ColdFire 
> code between Linux and other targets. It also makes the support for this 
> in libffi interesting (e.g. if you ever want Java support for ColdFire).

Agreed.  But unfortunately it is what it is.  There's more to ABIs than just 
whether structures are returned in memory.  For instance, %d0 and %a0 are used 
differently too wrt structure returning.  And of course there are other ABI 
considerations too.

> Since this is a special case I would seriously consider to ignore the 
> alignment mode for this in m68k_return_in_memory. Since this is an ABI 
> issue there probably should be an option to get the old behaviour.

I'm strongly in favour of not breaking existing ABIs.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Roman Zippel]

Hi,

On Wed, 29 Aug 2007, Nathan Sidwell wrote:

> > Since this is a special case I would seriously consider to ignore the
> > alignment mode for this in m68k_return_in_memory. Since this is an ABI issue
> > there probably should be an option to get the old behaviour.
> 
> I'm strongly in favour of not breaking existing ABIs.

The question is if this is more an ABI bug. Does it really make sense to 
return structures of the same size differently based on its contents?
In the end I don't care that much, since Linux already does the sane 
thing.

bye, Roman

Re: [patch] m68k: Fix binary compatibility problem with -mno-strict-align. (Take 2)

[Nathan Sidwell]

Roman Zippel wrote:

> The question is if this is more an ABI bug. Does it really make sense to 
> return structures of the same size differently based on its contents?

Of course it doesn't make sense for the ABI to depend on internal implementation 
features of the compiler :)  Not much we can do about it at this time, though :(

> In the end I don't care that much, since Linux already does the sane 
> thing.

ok then.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery