| Message ID | alpine.LNX.2.00.1303221103500.3543@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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
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.
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);