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

[Yury Gribov <y dot gribov at samsung dot com>]

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

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  */