diff mbox

Fix argument pushes to unaligned stack slots

Message ID 397.1310444297@splode.eterna.com.au
State New
Headers show

Commit Message

matthew green July 12, 2011, 4:18 a.m. UTC
hi folks.

i'm having a problem with GCC 4.5.3 on netbsd-m68k target.  i've tracked
it down to this change from several years ago:

> 2007-02-06  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* expr.c (emit_push_insn): If STRICT_ALIGNMENT, copy to an
> 	unaligned stack slot via a suitably aligned slot.

the problem is that emit_library_call_value_1() calls emit_push_insn()
with TYPE_NULL which ends up triggering a NULL deref when emit_push_insn()
calls assign_temp() with type = TYPE_NULL, and assign_temp() crashes.

this simple change seems to be sufficient to avoid the crash and the
generated code appears to run OK.  if it is OK, could someone please
commit it?  thanks.  (feel free to update my log message if it could
be clearer or more correct.)


.mrg.


2011-07-10  matthew green  <mrg@eterna.com.au>

	* expr.c (emit_push_insn): Don't copy a TYPE_NULL expression
	to the stack for correct alignment.

Comments

Joseph Myers July 21, 2011, 7:27 p.m. UTC | #1
On Tue, 12 Jul 2011, matthew green wrote:

> i'm having a problem with GCC 4.5.3 on netbsd-m68k target.  i've tracked
> it down to this change from several years ago:
> 
> > 2007-02-06  Joseph Myers  <joseph@codesourcery.com>
> > 
> > 	* expr.c (emit_push_insn): If STRICT_ALIGNMENT, copy to an
> > 	unaligned stack slot via a suitably aligned slot.
> 
> the problem is that emit_library_call_value_1() calls emit_push_insn()
> with TYPE_NULL which ends up triggering a NULL deref when emit_push_insn()
> calls assign_temp() with type = TYPE_NULL, and assign_temp() crashes.
> 
> this simple change seems to be sufficient to avoid the crash and the
> generated code appears to run OK.  if it is OK, could someone please

I don't see how it can be safe; if the stack slot is insufficiently 
aligned, the special handling will be needed.  Maybe the alignment being 
passed to this code is wrong, but if it's correct then you need a way to 
handle the unaligned move properly; I don't know if it would be possible 
or safe to pass a non-NULL type here, for example.
matthew green July 22, 2011, 3:22 a.m. UTC | #2
> On Tue, 12 Jul 2011, matthew green wrote:
> 
> > i'm having a problem with GCC 4.5.3 on netbsd-m68k target.  i've tracked
> > it down to this change from several years ago:
> > 
> > > 2007-02-06  Joseph Myers  <joseph@codesourcery.com>
> > > 
> > > 	* expr.c (emit_push_insn): If STRICT_ALIGNMENT, copy to an
> > > 	unaligned stack slot via a suitably aligned slot.
> > 
> > the problem is that emit_library_call_value_1() calls emit_push_insn()
> > with TYPE_NULL which ends up triggering a NULL deref when emit_push_insn()
> > calls assign_temp() with type = TYPE_NULL, and assign_temp() crashes.
> > 
> > this simple change seems to be sufficient to avoid the crash and the
> > generated code appears to run OK.  if it is OK, could someone please
> 
> I don't see how it can be safe; if the stack slot is insufficiently 
> aligned, the special handling will be needed.  Maybe the alignment being 
> passed to this code is wrong, but if it's correct then you need a way to 
> handle the unaligned move properly; I don't know if it would be possible 
> or safe to pass a non-NULL type here, for example.

how can i confirm if it is wrong alignment being passed down?

i looked around for a little bit to see if some non-NULL type
could be passed in from emit_library_call_value_1() (or other
callers in the path) but i did not see any way.

i happy to play around with trying to fix this the right way,
but i haven't looked at this part of GCC before so any guidance
would be greatly appreciated.  thanks.


.mrg.
Joseph Myers July 22, 2011, 12:34 p.m. UTC | #3
On Fri, 22 Jul 2011, matthew green wrote:

> > I don't see how it can be safe; if the stack slot is insufficiently 
> > aligned, the special handling will be needed.  Maybe the alignment being 
> > passed to this code is wrong, but if it's correct then you need a way to 
> > handle the unaligned move properly; I don't know if it would be possible 
> > or safe to pass a non-NULL type here, for example.
> 
> how can i confirm if it is wrong alignment being passed down?

By comparing the alignment passed with what, according to the ABI in use 
(which may or may not be documented), is the actual guaranteed alignment 
of the stack slot.
diff mbox

Patch

Index: external/gpl3/gcc/dist/gcc/expr.c
===================================================================
RCS file: /cvsroot/src/external/gpl3/gcc/dist/gcc/expr.c,v
retrieving revision 1.1.1.1
diff -p -u -r1.1.1.1 expr.c
--- external/gpl3/gcc/dist/gcc/expr.c	21 Jun 2011 01:20:17 -0000	1.1.1.1
+++ external/gpl3/gcc/dist/gcc/expr.c	12 Jul 2011 04:17:00 -0000
@@ -3764,7 +3764,8 @@  emit_push_insn (rtx x, enum machine_mode
   xinner = x;
 
   if (mode == BLKmode
-      || (STRICT_ALIGNMENT && align < GET_MODE_ALIGNMENT (mode)))
+      || (STRICT_ALIGNMENT && align < GET_MODE_ALIGNMENT (mode)
+          && type != NULL_TREE))
     {
       /* Copy a block into the stack, entirely or partially.  */