[PATCH] Fix PR56434

[PATCH] Fix PR56434

[Richard Biener]

This fixes PR56434 - the use of BIGGEST_ALIGNMENT to annotate
the pointer returned by malloc is wrong - BIGGEST_ALIGNMENT
has nothing to do with the alignment guaranteed by the ABI
for allocated memory.  For example on x86_64 it depends on
-mavx and thus can result in wrong code being generated.

The following patch fixes it to use what we use on the
GIMPLE level - MALLOC_ABI_ALIGNMENT.

Ok for trunk?

Thanks,
Richard.

2013-03-22  Richard Biener  <rguenther@suse.de>

	PR middle-end/56434
	* calls.c (expand_call): Use MALLOC_ABI_ALIGNMENT to annotate
	the pointer returned by calls with ECF_MALLOC set.

Index: gcc/calls.c
===================================================================
--- gcc/calls.c	(revision 196899)
+++ gcc/calls.c	(working copy)
@@ -3186,7 +3186,7 @@ expand_call (tree exp, rtx target, int i
 
 	  /* The return value from a malloc-like function is a pointer.  */
 	  if (TREE_CODE (rettype) == POINTER_TYPE)
-	    mark_reg_pointer (temp, BIGGEST_ALIGNMENT);
+	    mark_reg_pointer (temp, MALLOC_ABI_ALIGNMENT);
 
 	  emit_move_insn (temp, valreg);
 

Re: [PATCH] Fix PR56434

[Jakub Jelinek]

On Fri, Mar 22, 2013 at 11:06:53AM +0100, Richard Biener wrote:
> This fixes PR56434 - the use of BIGGEST_ALIGNMENT to annotate
> the pointer returned by malloc is wrong - BIGGEST_ALIGNMENT
> has nothing to do with the alignment guaranteed by the ABI
> for allocated memory.  For example on x86_64 it depends on
> -mavx and thus can result in wrong code being generated.
> 
> The following patch fixes it to use what we use on the
> GIMPLE level - MALLOC_ABI_ALIGNMENT.
> 
> Ok for trunk?

IMHO the change should be accompanied by defining MALLOC_ABI_ALIGNMENT
on at least a couple of popular targets.  E.g. glibc
will guarantee at least 2 * sizeof (void *) alignment on all architectures,
and even if one uses some other malloc implementation, it should be better
ISO C99 conforming on Linux (perhaps ignoring long double type (known
to be non-conforming e.g. on ppc32) and _Decimal* types).
So, at least for Linux I'd say MALLOC_ABI_ALIGNMENT should be defined
as maximum alignment of long, long long, double and void *.

Because, right now, MALLOC_ABI_ALIGNMENT is only defined to
non-__alignof__(char) on VMS.

> 2013-03-22  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/56434
> 	* calls.c (expand_call): Use MALLOC_ABI_ALIGNMENT to annotate
> 	the pointer returned by calls with ECF_MALLOC set.
> 
> Index: gcc/calls.c
> ===================================================================
> --- gcc/calls.c	(revision 196899)
> +++ gcc/calls.c	(working copy)
> @@ -3186,7 +3186,7 @@ expand_call (tree exp, rtx target, int i
>  
>  	  /* The return value from a malloc-like function is a pointer.  */
>  	  if (TREE_CODE (rettype) == POINTER_TYPE)
> -	    mark_reg_pointer (temp, BIGGEST_ALIGNMENT);
> +	    mark_reg_pointer (temp, MALLOC_ABI_ALIGNMENT);
>  
>  	  emit_move_insn (temp, valreg);
>  

	Jakub

Re: [PATCH] Fix PR56434

[Richard Biener]

On Fri, 22 Mar 2013, Jakub Jelinek wrote:

> On Fri, Mar 22, 2013 at 11:06:53AM +0100, Richard Biener wrote:
> > This fixes PR56434 - the use of BIGGEST_ALIGNMENT to annotate
> > the pointer returned by malloc is wrong - BIGGEST_ALIGNMENT
> > has nothing to do with the alignment guaranteed by the ABI
> > for allocated memory.  For example on x86_64 it depends on
> > -mavx and thus can result in wrong code being generated.
> > 
> > The following patch fixes it to use what we use on the
> > GIMPLE level - MALLOC_ABI_ALIGNMENT.
> > 
> > Ok for trunk?
> 
> IMHO the change should be accompanied by defining MALLOC_ABI_ALIGNMENT
> on at least a couple of popular targets.  E.g. glibc
> will guarantee at least 2 * sizeof (void *) alignment on all architectures,
> and even if one uses some other malloc implementation, it should be better
> ISO C99 conforming on Linux (perhaps ignoring long double type (known
> to be non-conforming e.g. on ppc32) and _Decimal* types).
> So, at least for Linux I'd say MALLOC_ABI_ALIGNMENT should be defined
> as maximum alignment of long, long long, double and void *.
> 
> Because, right now, MALLOC_ABI_ALIGNMENT is only defined to
> non-__alignof__(char) on VMS.

I think the wrong-code fix is orthogonal to code improvements
which will also trigger on the GIMPLE level (and where they
will have a bigger impact).

We can for example, in config/linux.h do

#if OPTION_GLIBC
#undef MALLOC_ABI_ALIGNMENT
#define MALLOC_ABI_ALIGNMENT (2 * sizeof (void *))
#endif

if that's what glibc really guarantees (does it maybe have a
feature macro for this?)

Richard.


> > 2013-03-22  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR middle-end/56434
> > 	* calls.c (expand_call): Use MALLOC_ABI_ALIGNMENT to annotate
> > 	the pointer returned by calls with ECF_MALLOC set.
> > 
> > Index: gcc/calls.c
> > ===================================================================
> > --- gcc/calls.c	(revision 196899)
> > +++ gcc/calls.c	(working copy)
> > @@ -3186,7 +3186,7 @@ expand_call (tree exp, rtx target, int i
> >  
> >  	  /* The return value from a malloc-like function is a pointer.  */
> >  	  if (TREE_CODE (rettype) == POINTER_TYPE)
> > -	    mark_reg_pointer (temp, BIGGEST_ALIGNMENT);
> > +	    mark_reg_pointer (temp, MALLOC_ABI_ALIGNMENT);
> >  
> >  	  emit_move_insn (temp, valreg);
> >  
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend

Re: [PATCH] Fix PR56434

[Ian Lance Taylor]

On Fri, Mar 22, 2013 at 3:27 AM, Richard Biener <rguenther@suse.de> wrote:
>
> I think the wrong-code fix is orthogonal to code improvements
> which will also trigger on the GIMPLE level (and where they
> will have a bigger impact).

I agree.  I think the patch to calls is fine unless Jakub objects.


> We can for example, in config/linux.h do
>
> #if OPTION_GLIBC
> #undef MALLOC_ABI_ALIGNMENT
> #define MALLOC_ABI_ALIGNMENT (2 * sizeof (void *))
> #endif
>
> if that's what glibc really guarantees (does it maybe have a
> feature macro for this?)

The code in glibc seems to be in malloc.c only.  The most conservative
version seems to be

#define INTERNAL_SIZE_T size_t
#define SIZE_SZ                (sizeof(INTERNAL_SIZE_T))
#  define MALLOC_ALIGNMENT       (2 * SIZE_SZ)

In GCC terms this would be 2 * int_size_in_bytes (size_type_node).

Ian

Re: [PATCH] Fix PR56434

[Richard Biener]

On Fri, 22 Mar 2013, Ian Lance Taylor wrote:

> On Fri, Mar 22, 2013 at 3:27 AM, Richard Biener <rguenther@suse.de> wrote:
> >
> > I think the wrong-code fix is orthogonal to code improvements
> > which will also trigger on the GIMPLE level (and where they
> > will have a bigger impact).
> 
> I agree.  I think the patch to calls is fine unless Jakub objects.
> 
> 
> > We can for example, in config/linux.h do
> >
> > #if OPTION_GLIBC
> > #undef MALLOC_ABI_ALIGNMENT
> > #define MALLOC_ABI_ALIGNMENT (2 * sizeof (void *))
> > #endif
> >
> > if that's what glibc really guarantees (does it maybe have a
> > feature macro for this?)
> 
> The code in glibc seems to be in malloc.c only.  The most conservative
> version seems to be
> 
> #define INTERNAL_SIZE_T size_t
> #define SIZE_SZ                (sizeof(INTERNAL_SIZE_T))
> #  define MALLOC_ALIGNMENT       (2 * SIZE_SZ)
> 
> In GCC terms this would be 2 * int_size_in_bytes (size_type_node).

Or, given

  /* Define what type to use for size_t.  */
  if (strcmp (SIZE_TYPE, "unsigned int") == 0)
    size_type_node = unsigned_type_node;
...

#define MALLOC_ABI_ALIGNMENT (2 * sizeof (### magic un-stringify SIZE_TYPE))

eh ... is there a more convenient way to access the targets size_t?

It's a target macro still ... not often used, so even

#define MALLOC_ABI_ALIGNMENT (2 * TREE_INT_CST_LOW (TYPE_SIZE_UNIT 
(size_type_node)))

would probably work, but ...

Richard.