Bug 56434 - document that __attribute__((__malloc__)) assumes returned pointer has BIGGEST_ALIGNMENT
Summary: document that __attribute__((__malloc__)) assumes returned pointer has BIGGES...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.7.2
: P3 normal
Target Milestone: ---
Assignee: Richard Biener
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2013-02-23 07:59 UTC by Chip Salzenberg
Modified: 2013-03-25 19:15 UTC (History)
3 users (show)

See Also:
Host:
Target: x86_64-*-*, i?86-*-*
Build:
Known to work: 4.9.0
Known to fail:
Last reconfirmed: 2013-02-25 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Chip Salzenberg 2013-02-23 07:59:16 UTC
The docs say that __attribute__((__malloc__)) only has one effect: informing the compiler that returned pointers do not alias other pointers.  But reading the compiler output, and then reading gcc source code, proves that it also has a second effect: informing the compiler that returned pointers are aligned to BIGGEST_ALIGNMENT.  To quote expand_call:

          /* The return value from a malloc-like function is a pointer.  */
          if (TREE_CODE (rettype) == POINTER_TYPE)
            mark_reg_pointer (temp, BIGGEST_ALIGNMENT);

This should be added to the documentation.

As a side issue, BIGGEST_ALIGNMENT changes on the i386 target depending on whether -mavx is specified (128 vs. 256).  Is it really a good idea for gcc to assume different things about the behavior of malloc() depending on -mavx?  It seems that perhaps an alignment of 128 should always be conferred on malloc on the i386 platform, regardless of -mavx?

What would the new target macro be?  SMALLEST_BIGGEST_ALIGNMENT?  :)
Comment 1 Richard Biener 2013-02-25 15:19:17 UTC
We have introduced MALLOC_ABI_ALIGNMENT for this, the calls.c:expand_call use
should be fixed to use that I think.  Not sure what makes use of this reg-note,
but it's a possible wrong-code issue (at least since AVX on x86).

Confirmed.
Comment 2 Chip Salzenberg 2013-02-25 17:51:36 UTC
I detected this by observing inlined strlen() on a malloc'd pointer did not first do an unaligned prologue.  I expected it to first advance by bytes until it detected alignment, but it didn't do any of that; it leapt right into the word-sized optimized loop.

This suggests that the compiler knows than an 8-byte-aligned (say) pointer has its low seven bits off and will evaporate away any code that depends on them being nonzero.  Or is the strlen inlining special-cased?
Comment 3 Chip Salzenberg 2013-02-25 17:54:23 UTC
I meant the low three bits off, for a maximum value of seven.  Of course.
Comment 4 Jakub Jelinek 2013-02-25 18:36:49 UTC
MALLOC_ABI_ALIGNMENT isn't properly set on most targets though.

E.g. with glibc, the alignment of malloced memory is 2 * sizeof (void *) (right now, might be bigger in the future), while MALLOC_ABI_ALIGNMENT is still 8 bits.

It is just fine to assume malloced memory must be at aligned enough for any of the standard C types, so if this is say on x86_64 where long double is 16-byte aligned, then there is really no reason to bother with aligning to 16-byte boundaries.  If you have malloc implementation that doesn't align at least that much, just fix it, or don't use malloc attribute for that.
Comment 5 Chip Salzenberg 2013-02-25 20:02:24 UTC
Indeed.  So MALLOC_ABI_ALIGNMENT should perhaps default to the largest alignment of all the C89 types, with platform overrides as needed?
Comment 6 Richard Biener 2013-02-26 10:03:46 UTC
(In reply to comment #4)
> MALLOC_ABI_ALIGNMENT isn't properly set on most targets though.
> 
> E.g. with glibc, the alignment of malloced memory is 2 * sizeof (void *) (right
> now, might be bigger in the future), while MALLOC_ABI_ALIGNMENT is still 8
> bits.
> 
> It is just fine to assume malloced memory must be at aligned enough for any of
> the standard C types, so if this is say on x86_64 where long double is 16-byte
> aligned, then there is really no reason to bother with aligning to 16-byte
> boundaries.  If you have malloc implementation that doesn't align at least that
> much, just fix it, or don't use malloc attribute for that.

True, but:

config/i386/i386.h:#define BIGGEST_ALIGNMENT (TARGET_AVX ? 256 : 128)

I don't think glibc aligns to 32 bytes currently, so the mark_reg_pointer
is definitely wrong (with -mavx).  Or BIGGEST_ALIGNMENT is wrong ;)

MALLOC_ABI_ALIGNMENT default was set very conservatively (because we use it
to derive information about pointer values).  I didn't realize we already
do that via the calls.c code (and even wrong ...).

Feel free to implement a better default setting for MALLOC_ABI_ALIGNMENT.

> Indeed.  So MALLOC_ABI_ALIGNMENT should perhaps default to the largest
> alignment of all the C89 types, with platform overrides as needed?

But this information is not readily accessible (it was supposed to be
BIGGEST_ALIGNMENT, but -mavx changed this - not sure what the psABI
says and whether this makes current glibc non-conforming to the psABI).
Comment 7 Chip Salzenberg 2013-03-21 00:31:40 UTC
So ... is there still a question of the Right Thing here?  It seems that fixing MALLOC_ABI_ALIGNMENT for the world, and ensuring that BIGGEST_ALIGNMENT never affects the ABI, are the actions to take.  If this were done soon we could even see it fixed for 4.8.0.  Help?
Comment 8 rguenther@suse.de 2013-03-22 09:56:37 UTC
On Thu, 21 Mar 2013, chip at pobox dot com wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56434
> 
> --- Comment #7 from Chip Salzenberg <chip at pobox dot com> 2013-03-21 00:31:40 UTC ---
> So ... is there still a question of the Right Thing here?  It seems that fixing
> MALLOC_ABI_ALIGNMENT for the world, and ensuring that BIGGEST_ALIGNMENT never
> affects the ABI, are the actions to take.  If this were done soon we could even
> see it fixed for 4.8.0.  Help?

The RTL code using BIGGEST_ALIGNMENT for the malloc result is simply
wrong and was wrong since forever.  What the replacement should be
is the question here, but as we use MALLOC_ABI_ALIGNMENT on the
tree level for _exactly the same purpose_ I think the answer is
obvious.

I will propose a patch.

Richard.
Comment 9 Richard Biener 2013-03-22 10:08:54 UTC
http://gcc.gnu.org/ml/gcc-patches/2013-03/msg00864.html

waiting for comments.
Comment 10 Chip Salzenberg 2013-03-22 20:20:11 UTC
Thanks muchly.  Then MALLOC_ABI_ALIGNMENT will need fixing, as Jakub observes, but that needed to happen anyway.
Comment 11 Richard Biener 2013-03-25 09:41:29 UTC
Author: rguenth
Date: Mon Mar 25 09:39:52 2013
New Revision: 197030

URL: http://gcc.gnu.org/viewcvs?rev=197030&root=gcc&view=rev
Log:
2013-03-25  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.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/calls.c
Comment 12 Chip Salzenberg 2013-03-25 19:15:10 UTC
Thank you.  I've filed #56726 with a patch to update MALLOC_ABI_ALIGNMENT on i386.