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

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

[Yury Gribov]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]

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

[-- Attachment #2: pr63802-1.diff --]
[-- Type: text/x-patch, Size: 2085 bytes --]

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

Re: [PATCH] Fix minimal alignment calculation for user-aligned types (PR63802)

[Jakub Jelinek]

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

Re: [PATCH] Fix minimal alignment calculation for user-aligned types (PR63802)

[Yury Gribov]

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

Re: [PATCH] Fix minimal alignment calculation for user-aligned types (PR63802)

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix minimal alignment calculation for user-aligned types (PR63802)

[Jakub Jelinek]

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

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

[Yury Gribov]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

On 11/17/2014 10:20 AM, Jakub Jelinek wrote:
> 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.

It looks like min_align_of_type is just too C11-specific to be usable in 
other contexts.  Here is a patch which does what Jakub originally 
proposed (use TYPE_ALIGN_UNIT for user-aligned types, fallback to 
min_align_of_type otherwise).

Again, bootstrapped and regtested on x64.

-Y

[-- Attachment #2: pr63802-2.diff --]
[-- Type: text/x-patch, Size: 3642 bytes --]

commit 1dc89eb74cceeb2c7f6021a40bf65fdf5f706909
Author: Yury Gribov <y.gribov@samsung.com>
Date:   Mon Nov 17 12:40:02 2014 +0300

    2014-11-17  Yury Gribov  <y.gribov@samsung.com>
    
    	PR sanitizer/63802
    
    gcc/
    	* ubsan.h (ubsan_align_of_type): Declare new function.
    	* ubsan.c (ubsan_align_of_type): New function.
    	(instrument_mem_ref): Call new function.
    
    gcc/c-family/
    	* c-ubsan.c (ubsan_maybe_instrument_reference_or_call):
    	Call new function.
    
    gcc/testsuite/
    	* c-c++-common/ubsan/pr63802.c: New test.

diff --git a/gcc/c-family/c-ubsan.c b/gcc/c-family/c-ubsan.c
index ab16799..00b68e5 100644
--- a/gcc/c-family/c-ubsan.c
+++ b/gcc/c-family/c-ubsan.c
@@ -397,7 +397,7 @@ ubsan_maybe_instrument_reference_or_call (location_t loc, tree op, tree type,
 
   if (flag_sanitize & SANITIZE_ALIGNMENT)
     {
-      mina = min_align_of_type (type);
+      mina = ubsan_align_of_type (type);
       if (mina <= 1)
 	mina = 0;
     }
@@ -408,7 +408,7 @@ ubsan_maybe_instrument_reference_or_call (location_t loc, tree op, tree type,
   if (TREE_CODE (op) == NOP_EXPR
       && TREE_CODE (TREE_TYPE (op)) == REFERENCE_TYPE)
     {
-      if (mina && mina > min_align_of_type (TREE_TYPE (TREE_TYPE (op))))
+      if (mina && mina > ubsan_align_of_type (TREE_TYPE (TREE_TYPE (op))))
 	instrument = true;
     }
   else
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.*" } */
diff --git a/gcc/ubsan.c b/gcc/ubsan.c
index b5b1b92..0f1ba9a 100644
--- a/gcc/ubsan.c
+++ b/gcc/ubsan.c
@@ -78,6 +78,15 @@ struct GTY(()) tree_type_map {
 #define tree_type_map_eq tree_map_base_eq
 #define tree_type_map_marked_p tree_map_base_marked_p
 
+/* Type alignment in bytes to be checked by UBSan.  */
+
+unsigned int
+ubsan_align_of_type (tree type)
+{
+  return TYPE_USER_ALIGN (type) ? TYPE_ALIGN_UNIT (type)
+    : min_align_of_type (type);
+}
+
 /* Hash from a tree in a tree_type_map.  */
 
 unsigned int
@@ -938,7 +947,7 @@ instrument_mem_ref (tree mem, tree base, gimple_stmt_iterator *iter,
   unsigned int align = 0;
   if (flag_sanitize & SANITIZE_ALIGNMENT)
     {
-      align = min_align_of_type (TREE_TYPE (base));
+      align = ubsan_align_of_type (TREE_TYPE (base));
       if (align <= 1)
 	align = 0;
     }
diff --git a/gcc/ubsan.h b/gcc/ubsan.h
index dcdbb4f..7537528 100644
--- a/gcc/ubsan.h
+++ b/gcc/ubsan.h
@@ -49,5 +49,6 @@ extern bool is_ubsan_builtin_p (tree);
 extern tree ubsan_build_overflow_builtin (tree_code, location_t, tree, tree, tree);
 extern tree ubsan_instrument_float_cast (location_t, tree, tree);
 extern tree ubsan_get_source_location_type (void);
+extern unsigned int ubsan_align_of_type (tree);
 
 #endif  /* GCC_UBSAN_H  */

Re: [PATCHv2] Fix minimal alignment calculation for user-aligned types (PR63802)

[Joseph Myers]

On Mon, 17 Nov 2014, Yury Gribov wrote:

> It looks like min_align_of_type is just too C11-specific to be usable in other
> contexts.  Here is a patch which does what Jakub originally proposed (use
> TYPE_ALIGN_UNIT for user-aligned types, fallback to min_align_of_type
> otherwise).

I think the requirements for min_align_of_type and ubsan are exactly the 
same, and we should not create a duplicate function for this.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix minimal alignment calculation for user-aligned types (PR63802)

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix minimal alignment calculation for user-aligned types (PR63802)

[Jakub Jelinek]

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

Re: [PATCH] Fix minimal alignment calculation for user-aligned types (PR63802)

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com