Fix minimal alignment calculation for user-aligned types (PR63802)

Hi all,

This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63802 by 
only limiting minimal type alignment with BIGGEST_ALIGNMENT for types 
with no __attribute__((aligned)).

Bootstrapped and regtested on x64.  Ok for trunk?

-Y

On Fri, Nov 14, 2014 at 09:46:14AM +0300, Yury Gribov wrote:
> Hi all,
> 
> This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63802 by only
> limiting minimal type alignment with BIGGEST_ALIGNMENT for types with no
> __attribute__((aligned)).
> 
> Bootstrapped and regtested on x64.  Ok for trunk?

The function is primarily used by the C FE for _Alignas, and I have no idea
if such a change is desirable for that very much user visible case.  Joseph?

Alternatively, you can just change ubsan.c caller of min_align_of_type,
use TYPE_USER_ALIGN (type) ? TYPE_ALIGN_UNIT (type) : min_align_of_type (type)
there instead.

> >From 7e5d09453dcff22f591162e1b5c5a115b17b0014 Mon Sep 17 00:00:00 2001
> From: Yury Gribov <y.gribov@samsung.com>
> Date: Thu, 13 Nov 2014 21:29:51 +0300
> Subject: [PATCH] 2014-11-14  Yury Gribov  <y.gribov@samsung.com>
> 
> 	PR sanitizer/63802
> 
> gcc/
> 	* stor-layout.c (min_align_of_type): Respect user alignment
> 	more.
> 
> gcc/testsuite/
> 	* c-c++-common/ubsan/pr63802.c: New test.
> ---
>  gcc/stor-layout.c                          |    2 +-
>  gcc/testsuite/c-c++-common/ubsan/pr63802.c |   23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/c-c++-common/ubsan/pr63802.c
> 
> diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
> index 431b207..db09855 100644
> --- a/gcc/stor-layout.c
> +++ b/gcc/stor-layout.c
> @@ -2430,9 +2430,9 @@ unsigned int
>  min_align_of_type (tree type)
>  {
>    unsigned int align = TYPE_ALIGN (type);
> -  align = MIN (align, BIGGEST_ALIGNMENT);
>    if (!TYPE_USER_ALIGN (type))
>      {
> +      align = MIN (align, BIGGEST_ALIGNMENT);
>  #ifdef BIGGEST_FIELD_ALIGNMENT
>        align = MIN (align, BIGGEST_FIELD_ALIGNMENT);
>  #endif
> diff --git a/gcc/testsuite/c-c++-common/ubsan/pr63802.c b/gcc/testsuite/c-c++-common/ubsan/pr63802.c
> new file mode 100644
> index 0000000..454c098
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/ubsan/pr63802.c
> @@ -0,0 +1,23 @@
> +/* Limit this to known non-strict alignment targets.  */
> +/* { dg-do run { target { i?86-*-linux* x86_64-*-linux* } } } */
> +/* { dg-options "-fsanitize=alignment" } */
> +
> +#define __round_mask(x, y) ((__typeof__(x))((y)-1))
> +#define round_up(x, y) ((((x)-1) | __round_mask(x, y))+1)
> +
> +struct test_struct {
> +  unsigned long a;
> +  int b;
> +} __attribute__((__aligned__(64)));
> +
> +char a[200];
> +
> +int main ()
> +{
> +  volatile int x = ((struct test_struct*)(round_up((unsigned long)a, 64) + 16))->b;
> +  volatile int y = ((struct test_struct*)(round_up((unsigned long)a, 64) + 15))->b;
> +
> +  return 0;
> +}
> +
> +/* { dg-output "\.c:18:\[0-9]*: \[^\n\r]*member access within misaligned address 0x\[0-9a-fA-F]* for type 'struct test_struct', which requires 64 byte alignment.*" } */
> -- 
> 1.7.9.5
> 


	Jakub

On 11/14/2014 10:02 AM, Jakub Jelinek wrote:
> On Fri, Nov 14, 2014 at 09:46:14AM +0300, Yury Gribov wrote:
>> Hi all,
>>
>> This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63802 by only
>> limiting minimal type alignment with BIGGEST_ALIGNMENT for types with no
>> __attribute__((aligned)).
>>
>> Bootstrapped and regtested on x64.  Ok for trunk?
>
> The function is primarily used by the C FE for _Alignas, and I have no idea
> if such a change is desirable for that very much user visible case.  Joseph?
>
> Alternatively, you can just change ubsan.c caller of min_align_of_type,
> use TYPE_USER_ALIGN (type) ? TYPE_ALIGN_UNIT (type) : min_align_of_type (type)
> there instead.

That's what I planned to do initially but change seemed so natural that 
I gave it a try.  Let's wait for Joseph's comment.

-Y

On Fri, 14 Nov 2014, Jakub Jelinek wrote:

> On Fri, Nov 14, 2014 at 09:46:14AM +0300, Yury Gribov wrote:
> > Hi all,
> > 
> > This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63802 by only
> > limiting minimal type alignment with BIGGEST_ALIGNMENT for types with no
> > __attribute__((aligned)).
> > 
> > Bootstrapped and regtested on x64.  Ok for trunk?
> 
> The function is primarily used by the C FE for _Alignas, and I have no idea
> if such a change is desirable for that very much user visible case.  Joseph?

If it is true that a type satisfying TYPE_USER_ALIGN will never be 
allocated at an address less-aligned than its TYPE_ALIGN, even if that's 
greater than BIGGEST_ALIGNMENT, then the change seems correct for C11 
_Alignof.

On Fri, Nov 14, 2014 at 06:15:16PM +0000, Joseph Myers wrote:
> On Fri, 14 Nov 2014, Jakub Jelinek wrote:
> 
> > On Fri, Nov 14, 2014 at 09:46:14AM +0300, Yury Gribov wrote:
> > > Hi all,
> > > 
> > > This patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63802 by only
> > > limiting minimal type alignment with BIGGEST_ALIGNMENT for types with no
> > > __attribute__((aligned)).
> > > 
> > > Bootstrapped and regtested on x64.  Ok for trunk?
> > 
> > The function is primarily used by the C FE for _Alignas, and I have no idea
> > if such a change is desirable for that very much user visible case.  Joseph?
> 
> If it is true that a type satisfying TYPE_USER_ALIGN will never be 
> allocated at an address less-aligned than its TYPE_ALIGN, even if that's 
> greater than BIGGEST_ALIGNMENT, then the change seems correct for C11 
> _Alignof.

I think it depends on which target and where.
In structs (unless packed) the user aligned fields should be properly
aligned with respect to start of struct and the struct should have user
alignment in that case, automatic vars these days use alloca with
realignment if not handled better by the target, so should be fine too.
For data section vars and for common vars I think it really depends on the
target.  Perhaps for TYPE_USER_ALIGN use minimum of the TYPE_ALIGN and
MAX_OFILE_ALIGNMENT ?
For heap objects, it really depends on how it has been allocated, but if
allocated through malloc, the extra alignment is never guaranteed.
So, it really depends in malloc counts or not.

	Jakub

On Mon, 17 Nov 2014, Jakub Jelinek wrote:

> > If it is true that a type satisfying TYPE_USER_ALIGN will never be 
> > allocated at an address less-aligned than its TYPE_ALIGN, even if that's 
> > greater than BIGGEST_ALIGNMENT, then the change seems correct for C11 
> > _Alignof.
> 
> I think it depends on which target and where.
> In structs (unless packed) the user aligned fields should be properly
> aligned with respect to start of struct and the struct should have user
> alignment in that case, automatic vars these days use alloca with
> realignment if not handled better by the target, so should be fine too.
> For data section vars and for common vars I think it really depends on the
> target.  Perhaps for TYPE_USER_ALIGN use minimum of the TYPE_ALIGN and
> MAX_OFILE_ALIGNMENT ?
> For heap objects, it really depends on how it has been allocated, but if
> allocated through malloc, the extra alignment is never guaranteed.
> So, it really depends in malloc counts or not.

The question, for both _Alignas and ubsan, is the alignment guaranteed *in 
valid programs*.

malloc only provides sufficient alignment for types with fundamental 
alignment requirements (although there are various problems with the C11 
wording; see DR#445).  So if you use malloc to allocate a type with an 
extended alignment requirement (without doing extra realignment on the 
result of malloc), that's not a valid program.  And if an alignment is 
larger than MAX_OFILE_ALIGNMENT, you get an error for declaring non-stack 
variables requiring that alignment.  So I don't think there's any need to 
check MAX_OFILE_ALIGNMENT here.

On Mon, Nov 17, 2014 at 05:46:55PM +0000, Joseph Myers wrote:
> On Mon, 17 Nov 2014, Jakub Jelinek wrote:
> 
> > > If it is true that a type satisfying TYPE_USER_ALIGN will never be 
> > > allocated at an address less-aligned than its TYPE_ALIGN, even if that's 
> > > greater than BIGGEST_ALIGNMENT, then the change seems correct for C11 
> > > _Alignof.
> > 
> > I think it depends on which target and where.
> > In structs (unless packed) the user aligned fields should be properly
> > aligned with respect to start of struct and the struct should have user
> > alignment in that case, automatic vars these days use alloca with
> > realignment if not handled better by the target, so should be fine too.
> > For data section vars and for common vars I think it really depends on the
> > target.  Perhaps for TYPE_USER_ALIGN use minimum of the TYPE_ALIGN and
> > MAX_OFILE_ALIGNMENT ?
> > For heap objects, it really depends on how it has been allocated, but if
> > allocated through malloc, the extra alignment is never guaranteed.
> > So, it really depends in malloc counts or not.
> 
> The question, for both _Alignas and ubsan, is the alignment guaranteed *in 
> valid programs*.
> 
> malloc only provides sufficient alignment for types with fundamental 
> alignment requirements (although there are various problems with the C11 
> wording; see DR#445).  So if you use malloc to allocate a type with an 
> extended alignment requirement (without doing extra realignment on the 
> result of malloc), that's not a valid program.  And if an alignment is 
> larger than MAX_OFILE_ALIGNMENT, you get an error for declaring non-stack 
> variables requiring that alignment.  So I don't think there's any need to 
> check MAX_OFILE_ALIGNMENT here.

If so, then Yuri's original patch (the one changing min_align_of_type)
should be fine, right?

	Jakub

On Mon, 17 Nov 2014, Jakub Jelinek wrote:

> > The question, for both _Alignas and ubsan, is the alignment guaranteed *in 
> > valid programs*.
> > 
> > malloc only provides sufficient alignment for types with fundamental 
> > alignment requirements (although there are various problems with the C11 
> > wording; see DR#445).  So if you use malloc to allocate a type with an 
> > extended alignment requirement (without doing extra realignment on the 
> > result of malloc), that's not a valid program.  And if an alignment is 
> > larger than MAX_OFILE_ALIGNMENT, you get an error for declaring non-stack 
> > variables requiring that alignment.  So I don't think there's any need to 
> > check MAX_OFILE_ALIGNMENT here.
> 
> If so, then Yuri's original patch (the one changing min_align_of_type)
> should be fine, right?

Yes.