[REPOST] Invalid Code when reading from unaligned zero-sized array

[REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

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

Hi Jeff,

please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating
on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748

one test case is this one:

pr57748-3.c:
/* PR middle-end/57748 */
/* { dg-do run } */
/* wrong code in expand_expr_real_1.  */

#include <stdlib.h>

extern void abort (void);

typedef long long V
  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));

typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));

struct __attribute__((packed)) T { char c; P s; };

void __attribute__((noinline, noclone))
check (P *p)
{
  if (p->b[0][0] != 3 || p->b[0][1] != 4)
    abort ();
}

void __attribute__((noinline, noclone))
foo (struct T *t)
{
  V a = { 3, 4 };
  t->s.b[0] = a;
}

int
main ()
{
  struct T *t = (struct T *) calloc (128, 1);

  foo (t);
  check (&t->s);

  free (t);
  return 0;
}


and the other one is
pr57748-4.c:
/* PR middle-end/57748 */
/* { dg-do run } */
/* wrong code in expand_expr_real_1.  */

#include <stdlib.h>

extern void abort (void);

typedef long long V
  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));

typedef struct S { V b[1]; } P __attribute__((aligned (1)));

struct __attribute__((packed)) T { char c; P s; };

void __attribute__((noinline, noclone))
check (P *p)
{
  if (p->b[1][0] != 3 || p->b[1][1] != 4)
    abort ();
}

void __attribute__((noinline, noclone))
foo (struct T *t)
{
  V a = { 3, 4 };
  t->s.b[1] = a;
}

int
main ()
{
  struct T *t = (struct T *) calloc (128, 1);

  foo (t);
  check (&t->s);

  free (t);
  return 0;
}


The patch does add a boolean "expand_reference" parameter to expand_expr_real and
expand_expr_real_1. I pass true when I intend to use the returned memory context
as an array reference, instead of a value. At places where mis-aligned values are extracted,
I do not return a register with the extracted mis-aligned value if expand_reference is true.
When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference"
to the inner expand_expr_real call. Expand_reference, is pretty much similar to the
expand_modifier "EXPAND_MEMORY".

Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times).

Ok for trunk?


Thanks
Bernd. 		 	   		  

[-- Attachment #2: changelog-pr57748-2.txt --]
[-- Type: text/plain, Size: 579 bytes --]

2013-11-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/57748
	* expr.h (expand_expr_real, expand_expr_real_1): Add new parameter
	expand_reference.
	(expand_expr, expand_normal): Adjust.
	* expr.c (expand_expr_real, expand_expr_real_1): Add new parameter
	expand_reference. Use expand_reference to expand inner references.
	(store_expr): Adjust.
	* cfgexpand.c (expand_call_stmt): Adjust.

testsuite:
2013-11-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/57748
	* gcc.dg/torture/pr57748-3.c: New test.
	* gcc.dg/torture/pr57748-4.c: New test.


[-- Attachment #3: patch-pr57748-2.diff --]
[-- Type: application/octet-stream, Size: 10855 bytes --]

Index: gcc/expr.h
===================================================================
--- gcc/expr.h	(revision 204411)
+++ gcc/expr.h	(working copy)
@@ -41,7 +41,8 @@ along with GCC; see the file COPYING3.  If not see
     is a constant that is not a legitimate address.
    EXPAND_WRITE means we are only going to write to the resulting rtx.
    EXPAND_MEMORY means we are interested in a memory result, even if
-    the memory is constant and we could have propagated a constant value.  */
+    the memory is constant and we could have propagated a constant value,
+    or the memory is unaligned on a STRICT_ALIGNMENT target.  */
 enum expand_modifier {EXPAND_NORMAL = 0, EXPAND_STACK_PARM, EXPAND_SUM,
 		      EXPAND_CONST_ADDRESS, EXPAND_INITIALIZER, EXPAND_WRITE,
 		      EXPAND_MEMORY};
@@ -428,9 +429,9 @@ extern rtx force_operand (rtx, rtx);
 
 /* Work horses for expand_expr.  */
 extern rtx expand_expr_real (tree, rtx, enum machine_mode,
-			     enum expand_modifier, rtx *);
+			     enum expand_modifier, rtx *, bool);
 extern rtx expand_expr_real_1 (tree, rtx, enum machine_mode,
-			       enum expand_modifier, rtx *);
+			       enum expand_modifier, rtx *, bool);
 extern rtx expand_expr_real_2 (sepops, rtx, enum machine_mode,
 			       enum expand_modifier);
 
@@ -441,13 +442,13 @@ static inline rtx
 expand_expr (tree exp, rtx target, enum machine_mode mode,
 	     enum expand_modifier modifier)
 {
-  return expand_expr_real (exp, target, mode, modifier, NULL);
+  return expand_expr_real (exp, target, mode, modifier, NULL, false);
 }
 
 static inline rtx
 expand_normal (tree exp)
 {
-  return expand_expr_real (exp, NULL_RTX, VOIDmode, EXPAND_NORMAL, NULL);
+  return expand_expr_real (exp, NULL_RTX, VOIDmode, EXPAND_NORMAL, NULL, false);
 }
 
 /* At the start of a function, record that we have no previously-pushed
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 204411)
+++ gcc/expr.c	(working copy)
@@ -5211,7 +5211,7 @@ store_expr (tree exp, rtx target, int call_param_p
       temp = expand_expr_real (exp, tmp_target, GET_MODE (target),
 			       (call_param_p
 				? EXPAND_STACK_PARM : EXPAND_NORMAL),
-			       &alt_rtl);
+			       &alt_rtl, false);
     }
 
   /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not
@@ -7768,11 +7768,14 @@ expand_constructor (tree exp, rtx target, enum exp
    address, and ALT_RTL is non-NULL, then *ALT_RTL is set to the
    DECL_RTL of the VAR_DECL.  *ALT_RTL is also set if EXP is a
    COMPOUND_EXPR whose second argument is such a VAR_DECL, and so on
-   recursively.  */
+   recursively.
 
+   If EXPAND_REFERENCE is true, we are expanding an inner reference.  */
+
 rtx
 expand_expr_real (tree exp, rtx target, enum machine_mode tmode,
-		  enum expand_modifier modifier, rtx *alt_rtl)
+		  enum expand_modifier modifier, rtx *alt_rtl,
+		  bool expand_reference)
 {
   rtx ret;
 
@@ -7784,7 +7787,8 @@ expand_expr_real (tree exp, rtx target, enum machi
       return ret ? ret : const0_rtx;
     }
 
-  ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl);
+  ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl,
+			    expand_reference);
   return ret;
 }
 
@@ -9095,7 +9099,8 @@ stmt_is_replaceable_p (gimple stmt)
 
 rtx
 expand_expr_real_1 (tree exp, rtx target, enum machine_mode tmode,
-		    enum expand_modifier modifier, rtx *alt_rtl)
+		    enum expand_modifier modifier, rtx *alt_rtl,
+		    bool expand_reference)
 {
   rtx op0, op1, temp, decl_rtl;
   tree type;
@@ -9241,7 +9246,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 
 	  set_curr_insn_location (gimple_location (g));
 	  r = expand_expr_real (gimple_assign_rhs_to_tree (g), target,
-				tmode, modifier, NULL);
+				tmode, modifier, NULL, expand_reference);
 	  set_curr_insn_location (saved_loc);
 	  if (REG_P (r) && !REG_EXPR (r))
 	    set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (exp), r);
@@ -9462,7 +9467,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
     case SAVE_EXPR:
       {
 	tree val = treeop0;
-	rtx ret = expand_expr_real_1 (val, target, tmode, modifier, alt_rtl);
+	rtx ret = expand_expr_real_1 (val, target, tmode, modifier, alt_rtl,
+				      expand_reference);
 
 	if (!SAVE_EXPR_RESOLVED_P (exp))
 	  {
@@ -9520,6 +9526,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	align = get_object_alignment (exp);
 	if (modifier != EXPAND_WRITE
 	    && modifier != EXPAND_MEMORY
+	    && !expand_reference
 	    && mode != BLKmode
 	    && align < GET_MODE_ALIGNMENT (mode)
 	    /* If the target does not have special handling for unaligned
@@ -9600,6 +9607,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	  MEM_VOLATILE_P (temp) = 1;
 	if (modifier != EXPAND_WRITE
 	    && modifier != EXPAND_MEMORY
+	    && !expand_reference
 	    && mode != BLKmode
 	    && align < GET_MODE_ALIGNMENT (mode))
 	  {
@@ -9825,15 +9833,16 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	   computation, since it will need a temporary and TARGET is known
 	   to have to do.  This occurs in unchecked conversion in Ada.  */
 	orig_op0 = op0
-	  = expand_expr (tem,
-			 (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
-			  && COMPLETE_TYPE_P (TREE_TYPE (tem))
-			  && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
-			      != INTEGER_CST)
-			  && modifier != EXPAND_STACK_PARM
-			  ? target : NULL_RTX),
-			 VOIDmode,
-			 modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+	  = expand_expr_real (tem,
+			      (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
+			       && COMPLETE_TYPE_P (TREE_TYPE (tem))
+			       && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
+				   != INTEGER_CST)
+			       && modifier != EXPAND_STACK_PARM
+			       ? target : NULL_RTX),
+			      VOIDmode,
+			      modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
+			      NULL, true);
 
 	/* If the bitfield is volatile, we want to access it in the
 	   field's mode, not the computed mode.
@@ -10185,14 +10194,15 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	  {
 	    /* See the normal_inner_ref case for the rationale.  */
 	    orig_op0
-	      = expand_expr (tem,
-			     (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
-			      && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
-				  != INTEGER_CST)
-			      && modifier != EXPAND_STACK_PARM
-			      ? target : NULL_RTX),
-			     VOIDmode,
-			     modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+	      = expand_expr_real (tem,
+				  (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
+				   && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
+				       != INTEGER_CST)
+				   && modifier != EXPAND_STACK_PARM
+				   ? target : NULL_RTX),
+				  VOIDmode,
+				  modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
+				  NULL, true);
 
 	    if (MEM_P (orig_op0))
 	      {
@@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
       }
 
       if (!op0)
-	op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
+	op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
+				NULL, expand_reference);
 
       /* If the input and output modes are both the same, we are done.  */
       if (mode == GET_MODE (op0))
@@ -10286,7 +10297,10 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	      op0 = copy_rtx (op0);
 	      set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
 	    }
-	  else if (mode != BLKmode
+	  else if (modifier != EXPAND_WRITE
+		   && modifier != EXPAND_MEMORY
+		   && !expand_reference
+		   && mode != BLKmode
 		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)
 		   /* If the target does have special handling for unaligned
 		      loads of mode then use them.  */
@@ -10307,6 +10321,9 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	      return reg;
 	    }
 	  else if (STRICT_ALIGNMENT
+		   && modifier != EXPAND_WRITE
+		   && modifier != EXPAND_MEMORY
+		   && !expand_reference
 		   && mode != BLKmode
 		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode))
 	    {
@@ -10429,7 +10446,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
       /* WITH_SIZE_EXPR expands to its first argument.  The caller should
 	 have pulled out the size to use in whatever context it needed.  */
       return expand_expr_real (treeop0, original_target, tmode,
-			       modifier, alt_rtl);
+			       modifier, alt_rtl, expand_reference);
 
     default:
       return expand_expr_real_2 (&ops, target, tmode, modifier);
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 204411)
+++ gcc/cfgexpand.c	(working copy)
@@ -2189,7 +2189,7 @@ expand_call_stmt (gimple stmt)
   if (lhs)
     expand_assignment (lhs, exp, false);
   else
-    expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL);
+    expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL, false);
 
   mark_transaction_restart_calls (stmt);
 }
Index: gcc/testsuite/gcc.dg/torture/pr57748-3.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr57748-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57748-3.c	(revision 0)
@@ -0,0 +1,40 @@
+/* PR middle-end/57748 */
+/* { dg-do run } */
+/* wrong code in expand_expr_real_1.  */
+
+#include <stdlib.h>
+
+extern void abort (void);
+
+typedef long long V
+  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
+
+typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
+
+struct __attribute__((packed)) T { char c; P s; };
+
+void __attribute__((noinline, noclone))
+check (P *p)
+{
+  if (p->b[0][0] != 3 || p->b[0][1] != 4)
+    abort ();
+}
+
+void __attribute__((noinline, noclone))
+foo (struct T *t)
+{
+  V a = { 3, 4 };
+  t->s.b[0] = a;
+}
+
+int
+main ()
+{
+  struct T *t = (struct T *) calloc (128, 1);
+
+  foo (t);
+  check (&t->s);
+
+  free (t);
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr57748-4.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr57748-4.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57748-4.c	(revision 0)
@@ -0,0 +1,40 @@
+/* PR middle-end/57748 */
+/* { dg-do run } */
+/* wrong code in expand_expr_real_1.  */
+
+#include <stdlib.h>
+
+extern void abort (void);
+
+typedef long long V
+  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
+
+typedef struct S { V b[1]; } P __attribute__((aligned (1)));
+
+struct __attribute__((packed)) T { char c; P s; };
+
+void __attribute__((noinline, noclone))
+check (P *p)
+{
+  if (p->b[1][0] != 3 || p->b[1][1] != 4)
+    abort ();
+}
+
+void __attribute__((noinline, noclone))
+foo (struct T *t)
+{
+  V a = { 3, 4 };
+  t->s.b[1] = a;
+}
+
+int
+main ()
+{
+  struct T *t = (struct T *) calloc (128, 1);
+
+  foo (t);
+  check (&t->s);
+
+  free (t);
+  return 0;
+}

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi Jeff,
>
> please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating
> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748
>
> one test case is this one:
>
> pr57748-3.c:
> /* PR middle-end/57748 */
> /* { dg-do run } */
> /* wrong code in expand_expr_real_1.  */
>
> #include <stdlib.h>
>
> extern void abort (void);
>
> typedef long long V
>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>
> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
>
> struct __attribute__((packed)) T { char c; P s; };
>
> void __attribute__((noinline, noclone))
> check (P *p)
> {
>   if (p->b[0][0] != 3 || p->b[0][1] != 4)
>     abort ();
> }
>
> void __attribute__((noinline, noclone))
> foo (struct T *t)
> {
>   V a = { 3, 4 };
>   t->s.b[0] = a;
> }
>
> int
> main ()
> {
>   struct T *t = (struct T *) calloc (128, 1);
>
>   foo (t);
>   check (&t->s);
>
>   free (t);
>   return 0;
> }
>
>
> and the other one is
> pr57748-4.c:
> /* PR middle-end/57748 */
> /* { dg-do run } */
> /* wrong code in expand_expr_real_1.  */
>
> #include <stdlib.h>
>
> extern void abort (void);
>
> typedef long long V
>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>
> typedef struct S { V b[1]; } P __attribute__((aligned (1)));
>
> struct __attribute__((packed)) T { char c; P s; };
>
> void __attribute__((noinline, noclone))
> check (P *p)
> {
>   if (p->b[1][0] != 3 || p->b[1][1] != 4)
>     abort ();
> }
>
> void __attribute__((noinline, noclone))
> foo (struct T *t)
> {
>   V a = { 3, 4 };
>   t->s.b[1] = a;
> }
>
> int
> main ()
> {
>   struct T *t = (struct T *) calloc (128, 1);
>
>   foo (t);
>   check (&t->s);
>
>   free (t);
>   return 0;
> }
>
>
> The patch does add a boolean "expand_reference" parameter to expand_expr_real and
> expand_expr_real_1. I pass true when I intend to use the returned memory context
> as an array reference, instead of a value. At places where mis-aligned values are extracted,
> I do not return a register with the extracted mis-aligned value if expand_reference is true.
> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference"
> to the inner expand_expr_real call. Expand_reference, is pretty much similar to the
> expand_modifier "EXPAND_MEMORY".
>
> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times).
>
> Ok for trunk?

It still feels like papering over the underlying issue.  Let me have a
second (or third?) look.

Richard.

>
> Thanks
> Bernd.

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi Jeff,
>>
>> please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating
>> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748
>>
>> one test case is this one:
>>
>> pr57748-3.c:
>> /* PR middle-end/57748 */
>> /* { dg-do run } */
>> /* wrong code in expand_expr_real_1.  */
>>
>> #include <stdlib.h>
>>
>> extern void abort (void);
>>
>> typedef long long V
>>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>
>> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
>>
>> struct __attribute__((packed)) T { char c; P s; };
>>
>> void __attribute__((noinline, noclone))
>> check (P *p)
>> {
>>   if (p->b[0][0] != 3 || p->b[0][1] != 4)
>>     abort ();
>> }
>>
>> void __attribute__((noinline, noclone))
>> foo (struct T *t)
>> {
>>   V a = { 3, 4 };
>>   t->s.b[0] = a;
>> }
>>
>> int
>> main ()
>> {
>>   struct T *t = (struct T *) calloc (128, 1);
>>
>>   foo (t);
>>   check (&t->s);
>>
>>   free (t);
>>   return 0;
>> }
>>
>>
>> and the other one is
>> pr57748-4.c:
>> /* PR middle-end/57748 */
>> /* { dg-do run } */
>> /* wrong code in expand_expr_real_1.  */
>>
>> #include <stdlib.h>
>>
>> extern void abort (void);
>>
>> typedef long long V
>>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>
>> typedef struct S { V b[1]; } P __attribute__((aligned (1)));
>>
>> struct __attribute__((packed)) T { char c; P s; };
>>
>> void __attribute__((noinline, noclone))
>> check (P *p)
>> {
>>   if (p->b[1][0] != 3 || p->b[1][1] != 4)
>>     abort ();
>> }
>>
>> void __attribute__((noinline, noclone))
>> foo (struct T *t)
>> {
>>   V a = { 3, 4 };
>>   t->s.b[1] = a;
>> }
>>
>> int
>> main ()
>> {
>>   struct T *t = (struct T *) calloc (128, 1);
>>
>>   foo (t);
>>   check (&t->s);
>>
>>   free (t);
>>   return 0;
>> }
>>
>>
>> The patch does add a boolean "expand_reference" parameter to expand_expr_real and
>> expand_expr_real_1. I pass true when I intend to use the returned memory context
>> as an array reference, instead of a value. At places where mis-aligned values are extracted,
>> I do not return a register with the extracted mis-aligned value if expand_reference is true.
>> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference"
>> to the inner expand_expr_real call. Expand_reference, is pretty much similar to the
>> expand_modifier "EXPAND_MEMORY".
>>
>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times).
>>
>> Ok for trunk?
>
> It still feels like papering over the underlying issue.  Let me have a
> second (or third?) look.

So I believe the issue is that we are asking the target for an optab
for movmisaling with the wrong mode and then create a movmisalign
with a wrong mode.

Index: gcc/expr.c
===================================================================
--- gcc/expr.c  (revision 205623)
+++ gcc/expr.c  (working copy)
@@ -9737,7 +9737,7 @@ expand_expr_real_1 (tree exp, rtx target
            && mode != BLKmode
            && align < GET_MODE_ALIGNMENT (mode))
          {
-           if ((icode = optab_handler (movmisalign_optab, mode))
+           if ((icode = optab_handler (movmisalign_optab, tmode))
                != CODE_FOR_nothing)
              {
                struct expand_operand ops[2];
@@ -9745,7 +9745,7 @@ expand_expr_real_1 (tree exp, rtx target
                /* We've already validated the memory, and we're creating a
                   new pseudo destination.  The predicates really can't fail,
                   nor can the generator.  */
-               create_output_operand (&ops[0], NULL_RTX, mode);
+               create_output_operand (&ops[0], NULL_RTX, tmode);
                create_fixed_operand (&ops[1], temp);
                expand_insn (icode, 2, ops);
                temp = ops[0].value;
@@ -9966,7 +9966,7 @@ expand_expr_real_1 (tree exp, rtx target
                              != INTEGER_CST)
                          && modifier != EXPAND_STACK_PARM
                          ? target : NULL_RTX),
-                        VOIDmode,
+                        mode1,
                         modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);

        /* If the bitfield is volatile, we want to access it in the

fixes the testcases (the VIEW_CONVERT_EXPR path would need a
similar fix I think, both for the recursion and the movmisaling handling).
Note that the alignment checks guarding the movmisalign handling
use the wrong mode as well, they are also somewhat non-sensical
as they apply to the non-offsetted object.

That said, the "get me a 'base' MEM for this" via recursing is misdesigned.
I still think that splitting it out is the correct fix in the end (doing the
movmisaling handling only there, but with correct info from the parent).

I will try if I can come up with something that feels reasonably safe
for stage3.

Richard.

> Richard.
>
>>
>> Thanks
>> Bernd.

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

----------------------------------------
> Date: Tue, 3 Dec 2013 14:51:21 +0100
> Subject: Re: [REPOST] Invalid Code when reading from unaligned zero-sized array
> From: richard.guenther@gmail.com
> To: bernd.edlinger@hotmail.de
> CC: law@redhat.com; gcc-patches@gcc.gnu.org; jakub@redhat.com
>
> On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Hi Jeff,
>>>
>>> please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating
>>> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748
>>>
>>> one test case is this one:
>>>
>>> pr57748-3.c:
>>> /* PR middle-end/57748 */
>>> /* { dg-do run } */
>>> /* wrong code in expand_expr_real_1. */
>>>
>>> #include <stdlib.h>
>>>
>>> extern void abort (void);
>>>
>>> typedef long long V
>>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>>
>>> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
>>>
>>> struct __attribute__((packed)) T { char c; P s; };
>>>
>>> void __attribute__((noinline, noclone))
>>> check (P *p)
>>> {
>>> if (p->b[0][0] != 3 || p->b[0][1] != 4)
>>> abort ();
>>> }
>>>
>>> void __attribute__((noinline, noclone))
>>> foo (struct T *t)
>>> {
>>> V a = { 3, 4 };
>>> t->s.b[0] = a;
>>> }
>>>
>>> int
>>> main ()
>>> {
>>> struct T *t = (struct T *) calloc (128, 1);
>>>
>>> foo (t);
>>> check (&t->s);
>>>
>>> free (t);
>>> return 0;
>>> }
>>>
>>>
>>> and the other one is
>>> pr57748-4.c:
>>> /* PR middle-end/57748 */
>>> /* { dg-do run } */
>>> /* wrong code in expand_expr_real_1. */
>>>
>>> #include <stdlib.h>
>>>
>>> extern void abort (void);
>>>
>>> typedef long long V
>>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>>
>>> typedef struct S { V b[1]; } P __attribute__((aligned (1)));
>>>
>>> struct __attribute__((packed)) T { char c; P s; };
>>>
>>> void __attribute__((noinline, noclone))
>>> check (P *p)
>>> {
>>> if (p->b[1][0] != 3 || p->b[1][1] != 4)
>>> abort ();
>>> }
>>>
>>> void __attribute__((noinline, noclone))
>>> foo (struct T *t)
>>> {
>>> V a = { 3, 4 };
>>> t->s.b[1] = a;
>>> }
>>>
>>> int
>>> main ()
>>> {
>>> struct T *t = (struct T *) calloc (128, 1);
>>>
>>> foo (t);
>>> check (&t->s);
>>>
>>> free (t);
>>> return 0;
>>> }
>>>
>>>
>>> The patch does add a boolean "expand_reference" parameter to expand_expr_real and
>>> expand_expr_real_1. I pass true when I intend to use the returned memory context
>>> as an array reference, instead of a value. At places where mis-aligned values are extracted,
>>> I do not return a register with the extracted mis-aligned value if expand_reference is true.
>>> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference"
>>> to the inner expand_expr_real call. Expand_reference, is pretty much similar to the
>>> expand_modifier "EXPAND_MEMORY".
>>>
>>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times).
>>>
>>> Ok for trunk?
>>
>> It still feels like papering over the underlying issue. Let me have a
>> second (or third?) look.
>
> So I believe the issue is that we are asking the target for an optab
> for movmisaling with the wrong mode and then create a movmisalign
> with a wrong mode.
>
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c (revision 205623)
> +++ gcc/expr.c (working copy)
> @@ -9737,7 +9737,7 @@ expand_expr_real_1 (tree exp, rtx target
> && mode != BLKmode
> && align < GET_MODE_ALIGNMENT (mode))
> {
> - if ((icode = optab_handler (movmisalign_optab, mode))
> + if ((icode = optab_handler (movmisalign_optab, tmode))
> != CODE_FOR_nothing)
> {
> struct expand_operand ops[2];
> @@ -9745,7 +9745,7 @@ expand_expr_real_1 (tree exp, rtx target
> /* We've already validated the memory, and we're creating a
> new pseudo destination. The predicates really can't fail,
> nor can the generator. */
> - create_output_operand (&ops[0], NULL_RTX, mode);
> + create_output_operand (&ops[0], NULL_RTX, tmode);
> create_fixed_operand (&ops[1], temp);
> expand_insn (icode, 2, ops);
> temp = ops[0].value;
> @@ -9966,7 +9966,7 @@ expand_expr_real_1 (tree exp, rtx target
> != INTEGER_CST)
> && modifier != EXPAND_STACK_PARM
> ? target : NULL_RTX),
> - VOIDmode,
> + mode1,
> modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
>
> /* If the bitfield is volatile, we want to access it in the
>
> fixes the testcases (the VIEW_CONVERT_EXPR path would need a
> similar fix I think, both for the recursion and the movmisaling handling).
> Note that the alignment checks guarding the movmisalign handling
> use the wrong mode as well, they are also somewhat non-sensical
> as they apply to the non-offsetted object.
>
> That said, the "get me a 'base' MEM for this" via recursing is misdesigned.
> I still think that splitting it out is the correct fix in the end (doing the
> movmisaling handling only there, but with correct info from the parent).
>
> I will try if I can come up with something that feels reasonably safe
> for stage3.
>

Fine. What is tmode and mode in the test example?

Note: we can run in the same problem if this is executed:

            else if (SLOW_UNALIGNED_ACCESS (mode, align))
              temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
                                        0, TYPE_UNSIGNED (TREE_TYPE (exp)),
                                        (modifier == EXPAND_STACK_PARM
                                         ? NULL_RTX : target),
                                        mode, mode);

maybe with %s/mode/tmode/g

Maybe just #define SLOW_UNALIGNED_ACCESS 1 on X86-64 to see
what might happen on a different target....

Thanks
Bernd. 		 	   		  

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi Jeff,
>>
>> please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating
>> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748
>>
>> one test case is this one:
>>
>> pr57748-3.c:
>> /* PR middle-end/57748 */
>> /* { dg-do run } */
>> /* wrong code in expand_expr_real_1.  */
>>
>> #include <stdlib.h>
>>
>> extern void abort (void);
>>
>> typedef long long V
>>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>
>> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
>>
>> struct __attribute__((packed)) T { char c; P s; };
>>
>> void __attribute__((noinline, noclone))
>> check (P *p)
>> {
>>   if (p->b[0][0] != 3 || p->b[0][1] != 4)
>>     abort ();
>> }
>>
>> void __attribute__((noinline, noclone))
>> foo (struct T *t)
>> {
>>   V a = { 3, 4 };
>>   t->s.b[0] = a;
>> }
>>
>> int
>> main ()
>> {
>>   struct T *t = (struct T *) calloc (128, 1);
>>
>>   foo (t);
>>   check (&t->s);
>>
>>   free (t);
>>   return 0;
>> }
>>
>>
>> and the other one is
>> pr57748-4.c:
>> /* PR middle-end/57748 */
>> /* { dg-do run } */
>> /* wrong code in expand_expr_real_1.  */
>>
>> #include <stdlib.h>
>>
>> extern void abort (void);
>>
>> typedef long long V
>>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>
>> typedef struct S { V b[1]; } P __attribute__((aligned (1)));
>>
>> struct __attribute__((packed)) T { char c; P s; };
>>
>> void __attribute__((noinline, noclone))
>> check (P *p)
>> {
>>   if (p->b[1][0] != 3 || p->b[1][1] != 4)
>>     abort ();
>> }
>>
>> void __attribute__((noinline, noclone))
>> foo (struct T *t)
>> {
>>   V a = { 3, 4 };
>>   t->s.b[1] = a;
>> }
>>
>> int
>> main ()
>> {
>>   struct T *t = (struct T *) calloc (128, 1);
>>
>>   foo (t);
>>   check (&t->s);
>>
>>   free (t);
>>   return 0;
>> }
>>
>>
>> The patch does add a boolean "expand_reference" parameter to expand_expr_real and
>> expand_expr_real_1. I pass true when I intend to use the returned memory context
>> as an array reference, instead of a value. At places where mis-aligned values are extracted,
>> I do not return a register with the extracted mis-aligned value if expand_reference is true.
>> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference"
>> to the inner expand_expr_real call. Expand_reference, is pretty much similar to the
>> expand_modifier "EXPAND_MEMORY".
>>
>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times).
>>
>> Ok for trunk?
>
> It still feels like papering over the underlying issue.  Let me have a
> second (or third?) look.

Few comments on your patch.

@@ -9520,6 +9526,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
        align = get_object_alignment (exp);
        if (modifier != EXPAND_WRITE
            && modifier != EXPAND_MEMORY
+           && !expand_reference
            && mode != BLKmode
            && align < GET_MODE_ALIGNMENT (mode)
            /* If the target does not have special handling for unaligned

(TARGET_MEM_REF), expand_reference should never be true here,
there may be no component-refs around TARGET_MEM_REFs.

You miss adjusting the VIEW_CONVERT_EXPR path?  (line-numbers
are off a lot in your patch, context doesn't help very much :/  Does not
seem to be against 4.8 either ...)

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c     (revision 204411)
+++ gcc/cfgexpand.c     (working copy)
@@ -2189,7 +2189,7 @@ expand_call_stmt (gimple stmt)
   if (lhs)
     expand_assignment (lhs, exp, false);
   else
-    expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL);
+    expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL, false);

   mark_transaction_restart_calls (stmt);
 }

this should use

  expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);

anyway.

@@ -10286,7 +10297,10 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
              op0 = copy_rtx (op0);
              set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
            }
-         else if (mode != BLKmode
+         else if (modifier != EXPAND_WRITE
+                  && modifier != EXPAND_MEMORY
+                  && !expand_reference
+                  && mode != BLKmode
                   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)
                   /* If the target does have special handling for unaligned
                      loads of mode then use them.  */

@@ -10307,6 +10321,9 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
              return reg;
            }
          else if (STRICT_ALIGNMENT
+                  && modifier != EXPAND_WRITE
+                  && modifier != EXPAND_MEMORY
+                  && !expand_reference
                   && mode != BLKmode
                   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode))
            {

why the unrelated change to add the modifier checks?  Looks like both
if cases are close by and factoring out common code like

   else if (!expand_reference
             && mode != BLKmode
             && MEM_ALIGN (...
     {
        if (...)
        else if (STRICT_ALIGNMENT)

would be better, also matching the other similar occurances.

Looking for some more time your patch may be indeed the easiest
without big re-factoring.

Richard.

> Richard.
>
>>
>> Thanks
>> Bernd.

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> The patch does add a boolean "expand_reference" parameter to
> expand_expr_real and expand_expr_real_1. I pass true when I intend to use
> the returned memory context as an array reference, instead of a value. At
> places where mis-aligned values are extracted, I do not return a register
> with the extracted mis-aligned value if expand_reference is true. When I
> have a VIEW_CONVERT_EXPR I pay attention to pass down the outer
> "expand_reference" to the inner expand_expr_real call. Expand_reference, is
> pretty much similar to the expand_modifier "EXPAND_MEMORY".

IMO that's not acceptable as-is, you're papering over the real issue which are 
the out-of-bounds accesses in the code, i.e. you essentially have an object 
with variable size and non-BLKmode.  The patch is too big an hammer for such 
an obvious lie to the middle-end.  Moreover there is not a _single_ line of 
explanation in it.

If we really want to go for a hack instead of fixing the underlying issue, it 
should be restricted to the cases where stor-layout.c goes wrong.

-- 
Eric Botcazou

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

On Tue, 3 Dec 2013 16:14:49, Eric Botcazou wrote:
>
>> The patch does add a boolean "expand_reference" parameter to
>> expand_expr_real and expand_expr_real_1. I pass true when I intend to use
>> the returned memory context as an array reference, instead of a value. At
>> places where mis-aligned values are extracted, I do not return a register
>> with the extracted mis-aligned value if expand_reference is true. When I
>> have a VIEW_CONVERT_EXPR I pay attention to pass down the outer
>> "expand_reference" to the inner expand_expr_real call. Expand_reference, is
>> pretty much similar to the expand_modifier "EXPAND_MEMORY".
>
> IMO that's not acceptable as-is, you're papering over the real issue which are
> the out-of-bounds accesses in the code, i.e. you essentially have an object
> with variable size and non-BLKmode. The patch is too big an hammer for such
> an obvious lie to the middle-end. Moreover there is not a _single_ line of
> explanation in it.
>

OK, I should add some comments. Right.

> If we really want to go for a hack instead of fixing the underlying issue, it
> should be restricted to the cases where stor-layout.c goes wrong.
>

To me it does not feel like a hack at all, if the expansion needs to know in what
context the result will be used, if it is in an LHS or in a RHS or if the address of
the memory is needed.

To make that perfectly clear, I just want to help.

If you or Richard want to come forward with a better proposal, that's fine.

And even if I commit something, it can be reverted anytime, when we have
something better.

Thanks
Bernd

> --
> Eric Botcazou 		 	   		  

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jeff Law]

On 12/03/13 11:40, Bernd Edlinger wrote:
>>
>
> To me it does not feel like a hack at all, if the expansion needs to know in what
> context the result will be used, if it is in an LHS or in a RHS or if the address of
> the memory is needed.
Expansion needs some context and it's currently passed into expand_expr 
via a variety of methods.  Most are documented at the start of 
expand_expr_real.  It's unclean, but that doesn't mean we should make it 
worse.  Adding more context is something we should think long and hard 
about.

Eric's comment that we have a non-BLKmode object with a variable size I 
think is critical.  I'm pretty sure that's not a well-defined object. 
Given a variable sized object that is not BLKmode, how can we reliably 
get its size (you can't, that's part of the reason why BLKmode exists)

I'd pull on that thread.

>
> To make that perfectly clear, I just want to help.
Understood and its greatly appreciated.

>
> And even if I commit something, it can be reverted anytime, when we have
> something better.
We try to get it right the first time :-)  Reverting bits is undesirable 
and virtually impossible if it gets out into a release.

jeff 		

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

On Tue, 3 Dec 2013 12:16:39, Jeff Law wrote:
>
> On 12/03/13 11:40, Bernd Edlinger wrote:
>>>
>>
>> To me it does not feel like a hack at all, if the expansion needs to know in what
>> context the result will be used, if it is in an LHS or in a RHS or if the address of
>> the memory is needed.
> Expansion needs some context and it's currently passed into expand_expr
> via a variety of methods. Most are documented at the start of
> expand_expr_real. It's unclean, but that doesn't mean we should make it
> worse. Adding more context is something we should think long and hard
> about.
>
> Eric's comment that we have a non-BLKmode object with a variable size I
> think is critical. I'm pretty sure that's not a well-defined object.
> Given a variable sized object that is not BLKmode, how can we reliably
> get its size (you can't, that's part of the reason why BLKmode exists)
>
> I'd pull on that thread.
>

Well, there are examples of invalid C that produce invalid Code (without warning).
Garbage-In-Garbage-Out, That's OK.

Then there is this one:
struct S {
  X a;
  Y b[0];
};

it has the mode of X, which is questionable. I could detect that in expansion
and only set expand_reference if b is accessed. Maybe.

But then we have:
struct S {
  Y b[1];
};

it has the Mode of Y

and even...
struct S {
  Y b[2];
};

this one has an Integer Mode of size(Y)*2

I have no idea how to detect this in expansion, and how to handle
for instance a volatile b correctly. And I cannot declare that data structure
invalid.

Of course this depends on the back-end, what modes it supports.
And if there are movmisalign optabs, or SLOW_UNALIGNED_ACCESS.

>>
>> To make that perfectly clear, I just want to help.
> Understood and its greatly appreciated.
>
>>
>> And even if I commit something, it can be reverted anytime, when we have
>> something better.
> We try to get it right the first time :-) Reverting bits is undesirable
> and virtually impossible if it gets out into a release.
>
> jeff
> 		 	   		  

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

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

On Tue, 3 Dec 2013 15:12:05, Richard Biener wrote:
>
> On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Hi Jeff,
>>>
>>> please find attached the patch (incl. test cases) for the unaligned read BUG that I found while investigating
>>> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748
>>>
>>> one test case is this one:
>>>
>>> pr57748-3.c:
>>> /* PR middle-end/57748 */
>>> /* { dg-do run } */
>>> /* wrong code in expand_expr_real_1. */
>>>
>>> #include <stdlib.h>
>>>
>>> extern void abort (void);
>>>
>>> typedef long long V
>>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>>
>>> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
>>>
>>> struct __attribute__((packed)) T { char c; P s; };
>>>
>>> void __attribute__((noinline, noclone))
>>> check (P *p)
>>> {
>>> if (p->b[0][0] != 3 || p->b[0][1] != 4)
>>> abort ();
>>> }
>>>
>>> void __attribute__((noinline, noclone))
>>> foo (struct T *t)
>>> {
>>> V a = { 3, 4 };
>>> t->s.b[0] = a;
>>> }
>>>
>>> int
>>> main ()
>>> {
>>> struct T *t = (struct T *) calloc (128, 1);
>>>
>>> foo (t);
>>> check (&t->s);
>>>
>>> free (t);
>>> return 0;
>>> }
>>>
>>>
>>> and the other one is
>>> pr57748-4.c:
>>> /* PR middle-end/57748 */
>>> /* { dg-do run } */
>>> /* wrong code in expand_expr_real_1. */
>>>
>>> #include <stdlib.h>
>>>
>>> extern void abort (void);
>>>
>>> typedef long long V
>>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>>>
>>> typedef struct S { V b[1]; } P __attribute__((aligned (1)));
>>>
>>> struct __attribute__((packed)) T { char c; P s; };
>>>
>>> void __attribute__((noinline, noclone))
>>> check (P *p)
>>> {
>>> if (p->b[1][0] != 3 || p->b[1][1] != 4)
>>> abort ();
>>> }
>>>
>>> void __attribute__((noinline, noclone))
>>> foo (struct T *t)
>>> {
>>> V a = { 3, 4 };
>>> t->s.b[1] = a;
>>> }
>>>
>>> int
>>> main ()
>>> {
>>> struct T *t = (struct T *) calloc (128, 1);
>>>
>>> foo (t);
>>> check (&t->s);
>>>
>>> free (t);
>>> return 0;
>>> }
>>>
>>>
>>> The patch does add a boolean "expand_reference" parameter to expand_expr_real and
>>> expand_expr_real_1. I pass true when I intend to use the returned memory context
>>> as an array reference, instead of a value. At places where mis-aligned values are extracted,
>>> I do not return a register with the extracted mis-aligned value if expand_reference is true.
>>> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer "expand_reference"
>>> to the inner expand_expr_real call. Expand_reference, is pretty much similar to the
>>> expand_modifier "EXPAND_MEMORY".
>>>
>>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times).
>>>
>>> Ok for trunk?
>>
>> It still feels like papering over the underlying issue. Let me have a
>> second (or third?) look.
>
> Few comments on your patch.
>
> @@ -9520,6 +9526,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
> align = get_object_alignment (exp);
> if (modifier != EXPAND_WRITE
> && modifier != EXPAND_MEMORY
> + && !expand_reference
> && mode != BLKmode
> && align < GET_MODE_ALIGNMENT (mode)
> /* If the target does not have special handling for unaligned
>
> (TARGET_MEM_REF), expand_reference should never be true here,
> there may be no component-refs around TARGET_MEM_REFs.
>

Ok, I was not sure. Removed - Thanks.

> You miss adjusting the VIEW_CONVERT_EXPR path? (line-numbers
> are off a lot in your patch, context doesn't help very much :/ Does not
> seem to be against 4.8 either ...)
>

Sorry, The line-numbers moved a lot> 100 lines.
The patch is against 4.9-trunk. Re-generated.

> Index: gcc/cfgexpand.c
> ===================================================================
> --- gcc/cfgexpand.c (revision 204411)
> +++ gcc/cfgexpand.c (working copy)
> @@ -2189,7 +2189,7 @@ expand_call_stmt (gimple stmt)
> if (lhs)
> expand_assignment (lhs, exp, false);
> else
> - expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL);
> + expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL, false);
>
> mark_transaction_restart_calls (stmt);
> }
>
> this should use
>
> expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);
>
> anyway.
>

expand_expr_real_1 is expand_expr_real without the ERROR check?

Ok, changed to expand_expr.


> @@ -10286,7 +10297,10 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
> op0 = copy_rtx (op0);
> set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
> }
> - else if (mode != BLKmode
> + else if (modifier != EXPAND_WRITE
> + && modifier != EXPAND_MEMORY
> + && !expand_reference
> + && mode != BLKmode
> && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)
> /* If the target does have special handling for unaligned
> loads of mode then use them. */
>
> @@ -10307,6 +10321,9 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
> return reg;
> }
> else if (STRICT_ALIGNMENT
> + && modifier != EXPAND_WRITE
> + && modifier != EXPAND_MEMORY
> + && !expand_reference
> && mode != BLKmode
> && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode))
> {
>
> why the unrelated change to add the modifier checks? Looks like both
> if cases are close by and factoring out common code like
>
> else if (!expand_reference
> && mode != BLKmode
> && MEM_ALIGN (...
> {
> if (...)
> else if (STRICT_ALIGNMENT)
>
> would be better, also matching the other similar occurances.
>

Ok, re-factored that if cascade.



Thanks.
Bernd.

> Looking for some more time your patch may be indeed the easiest
> without big re-factoring.
>
> Richard.
>
>> Richard.
>>
>>>
>>> Thanks
>>> Bernd. 		 	   		  

[-- Attachment #2: changelog-pr57748-2.txt --]
[-- Type: text/plain, Size: 579 bytes --]

2013-11-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/57748
	* expr.h (expand_expr_real, expand_expr_real_1): Add new parameter
	expand_reference.
	(expand_expr, expand_normal): Adjust.
	* expr.c (expand_expr_real, expand_expr_real_1): Add new parameter
	expand_reference. Use expand_reference to expand inner references.
	(store_expr): Adjust.
	* cfgexpand.c (expand_call_stmt): Adjust.

testsuite:
2013-11-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/57748
	* gcc.dg/torture/pr57748-3.c: New test.
	* gcc.dg/torture/pr57748-4.c: New test.


[-- Attachment #3: patch-pr57748-2.diff --]
[-- Type: application/octet-stream, Size: 12746 bytes --]

Index: gcc/testsuite/gcc.dg/torture/pr57748-4.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr57748-4.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57748-4.c	(revision 0)
@@ -0,0 +1,40 @@
+/* PR middle-end/57748 */
+/* { dg-do run } */
+/* wrong code in expand_expr_real_1.  */
+
+#include <stdlib.h>
+
+extern void abort (void);
+
+typedef long long V
+  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
+
+typedef struct S { V b[1]; } P __attribute__((aligned (1)));
+
+struct __attribute__((packed)) T { char c; P s; };
+
+void __attribute__((noinline, noclone))
+check (P *p)
+{
+  if (p->b[1][0] != 3 || p->b[1][1] != 4)
+    abort ();
+}
+
+void __attribute__((noinline, noclone))
+foo (struct T *t)
+{
+  V a = { 3, 4 };
+  t->s.b[1] = a;
+}
+
+int
+main ()
+{
+  struct T *t = (struct T *) calloc (128, 1);
+
+  foo (t);
+  check (&t->s);
+
+  free (t);
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr57748-3.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr57748-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57748-3.c	(revision 0)
@@ -0,0 +1,40 @@
+/* PR middle-end/57748 */
+/* { dg-do run } */
+/* wrong code in expand_expr_real_1.  */
+
+#include <stdlib.h>
+
+extern void abort (void);
+
+typedef long long V
+  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
+
+typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
+
+struct __attribute__((packed)) T { char c; P s; };
+
+void __attribute__((noinline, noclone))
+check (P *p)
+{
+  if (p->b[0][0] != 3 || p->b[0][1] != 4)
+    abort ();
+}
+
+void __attribute__((noinline, noclone))
+foo (struct T *t)
+{
+  V a = { 3, 4 };
+  t->s.b[0] = a;
+}
+
+int
+main ()
+{
+  struct T *t = (struct T *) calloc (128, 1);
+
+  foo (t);
+  check (&t->s);
+
+  free (t);
+  return 0;
+}
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 205641)
+++ gcc/expr.c	(working copy)
@@ -5322,7 +5322,7 @@ store_expr (tree exp, rtx target, int call_param_p
       temp = expand_expr_real (exp, tmp_target, GET_MODE (target),
 			       (call_param_p
 				? EXPAND_STACK_PARM : EXPAND_NORMAL),
-			       &alt_rtl);
+			       &alt_rtl, false);
     }
 
   /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not
@@ -7908,11 +7908,14 @@ expand_constructor (tree exp, rtx target, enum exp
    address, and ALT_RTL is non-NULL, then *ALT_RTL is set to the
    DECL_RTL of the VAR_DECL.  *ALT_RTL is also set if EXP is a
    COMPOUND_EXPR whose second argument is such a VAR_DECL, and so on
-   recursively.  */
+   recursively.
 
+   If EXPAND_REFERENCE is true, we are expanding an inner reference.  */
+
 rtx
 expand_expr_real (tree exp, rtx target, enum machine_mode tmode,
-		  enum expand_modifier modifier, rtx *alt_rtl)
+		  enum expand_modifier modifier, rtx *alt_rtl,
+		  bool expand_reference)
 {
   rtx ret;
 
@@ -7924,7 +7927,8 @@ expand_expr_real (tree exp, rtx target, enum machi
       return ret ? ret : const0_rtx;
     }
 
-  ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl);
+  ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl,
+			    expand_reference);
   return ret;
 }
 
@@ -9229,7 +9233,8 @@ stmt_is_replaceable_p (gimple stmt)
 
 rtx
 expand_expr_real_1 (tree exp, rtx target, enum machine_mode tmode,
-		    enum expand_modifier modifier, rtx *alt_rtl)
+		    enum expand_modifier modifier, rtx *alt_rtl,
+		    bool expand_reference)
 {
   rtx op0, op1, temp, decl_rtl;
   tree type;
@@ -9375,7 +9380,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 
 	  set_curr_insn_location (gimple_location (g));
 	  r = expand_expr_real (gimple_assign_rhs_to_tree (g), target,
-				tmode, modifier, NULL);
+				tmode, modifier, NULL, expand_reference);
 	  set_curr_insn_location (saved_loc);
 	  if (REG_P (r) && !REG_EXPR (r))
 	    set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (exp), r);
@@ -9596,7 +9601,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
     case SAVE_EXPR:
       {
 	tree val = treeop0;
-	rtx ret = expand_expr_real_1 (val, target, tmode, modifier, alt_rtl);
+	rtx ret = expand_expr_real_1 (val, target, tmode, modifier, alt_rtl,
+				      expand_reference);
 
 	if (!SAVE_EXPR_RESOLVED_P (exp))
 	  {
@@ -9734,6 +9740,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	  MEM_VOLATILE_P (temp) = 1;
 	if (modifier != EXPAND_WRITE
 	    && modifier != EXPAND_MEMORY
+	    && !expand_reference
 	    && mode != BLKmode
 	    && align < GET_MODE_ALIGNMENT (mode))
 	  {
@@ -9959,15 +9966,16 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	   computation, since it will need a temporary and TARGET is known
 	   to have to do.  This occurs in unchecked conversion in Ada.  */
 	orig_op0 = op0
-	  = expand_expr (tem,
-			 (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
-			  && COMPLETE_TYPE_P (TREE_TYPE (tem))
-			  && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
-			      != INTEGER_CST)
-			  && modifier != EXPAND_STACK_PARM
-			  ? target : NULL_RTX),
-			 VOIDmode,
-			 modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+	  = expand_expr_real (tem,
+			      (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
+			       && COMPLETE_TYPE_P (TREE_TYPE (tem))
+			       && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
+				   != INTEGER_CST)
+			       && modifier != EXPAND_STACK_PARM
+			       ? target : NULL_RTX),
+			      VOIDmode,
+			      modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
+			      NULL, true);
 
 	/* If the bitfield is volatile, we want to access it in the
 	   field's mode, not the computed mode.
@@ -10319,14 +10327,15 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	  {
 	    /* See the normal_inner_ref case for the rationale.  */
 	    orig_op0
-	      = expand_expr (tem,
-			     (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
-			      && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
-				  != INTEGER_CST)
-			      && modifier != EXPAND_STACK_PARM
-			      ? target : NULL_RTX),
-			     VOIDmode,
-			     modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+	      = expand_expr_real (tem,
+				  (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
+				   && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
+				       != INTEGER_CST)
+				   && modifier != EXPAND_STACK_PARM
+				   ? target : NULL_RTX),
+				  VOIDmode,
+				  modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
+				  NULL, true);
 
 	    if (MEM_P (orig_op0))
 	      {
@@ -10353,7 +10362,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
       }
 
       if (!op0)
-	op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
+	op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
+				NULL, expand_reference);
 
       /* If the input and output modes are both the same, we are done.  */
       if (mode == GET_MODE (op0))
@@ -10420,50 +10430,53 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
 	      op0 = copy_rtx (op0);
 	      set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
 	    }
-	  else if (mode != BLKmode
-		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)
-		   /* If the target does have special handling for unaligned
-		      loads of mode then use them.  */
-		   && ((icode = optab_handler (movmisalign_optab, mode))
-		       != CODE_FOR_nothing))
-	    {
-	      rtx reg, insn;
-
-	      op0 = adjust_address (op0, mode, 0);
-	      /* We've already validated the memory, and we're creating a
-		 new pseudo destination.  The predicates really can't
-		 fail.  */
-	      reg = gen_reg_rtx (mode);
-
-	      /* Nor can the insn generator.  */
-	      insn = GEN_FCN (icode) (reg, op0);
-	      emit_insn (insn);
-	      return reg;
-	    }
-	  else if (STRICT_ALIGNMENT
+	  else if (modifier != EXPAND_WRITE
+		   && modifier != EXPAND_MEMORY
+		   && !expand_reference
 		   && mode != BLKmode
 		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode))
 	    {
-	      tree inner_type = TREE_TYPE (treeop0);
-	      HOST_WIDE_INT temp_size
-		= MAX (int_size_in_bytes (inner_type),
-		       (HOST_WIDE_INT) GET_MODE_SIZE (mode));
-	      rtx new_rtx
-		= assign_stack_temp_for_type (mode, temp_size, type);
-	      rtx new_with_op0_mode
-		= adjust_address (new_rtx, GET_MODE (op0), 0);
+	      /* If the target does have special handling for unaligned
+		 loads of mode then use them.  */
+	      if ((icode = optab_handler (movmisalign_optab, mode))
+		  != CODE_FOR_nothing)
+		{
+		  rtx reg, insn;
 
-	      gcc_assert (!TREE_ADDRESSABLE (exp));
+		  op0 = adjust_address (op0, mode, 0);
+		  /* We've already validated the memory, and we're creating a
+		     new pseudo destination.  The predicates really can't
+		     fail.  */
+		  reg = gen_reg_rtx (mode);
 
-	      if (GET_MODE (op0) == BLKmode)
-		emit_block_move (new_with_op0_mode, op0,
-				 GEN_INT (GET_MODE_SIZE (mode)),
-				 (modifier == EXPAND_STACK_PARM
-				  ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
-	      else
-		emit_move_insn (new_with_op0_mode, op0);
+		  /* Nor can the insn generator.  */
+		  insn = GEN_FCN (icode) (reg, op0);
+		  emit_insn (insn);
+		  return reg;
+		}
+	      else if (STRICT_ALIGNMENT)
+		{
+		  tree inner_type = TREE_TYPE (treeop0);
+		  HOST_WIDE_INT temp_size
+		    = MAX (int_size_in_bytes (inner_type),
+			   (HOST_WIDE_INT) GET_MODE_SIZE (mode));
+		  rtx new_rtx
+		    = assign_stack_temp_for_type (mode, temp_size, type);
+		  rtx new_with_op0_mode
+		    = adjust_address (new_rtx, GET_MODE (op0), 0);
 
-	      op0 = new_rtx;
+		  gcc_assert (!TREE_ADDRESSABLE (exp));
+
+		  if (GET_MODE (op0) == BLKmode)
+		    emit_block_move (new_with_op0_mode, op0,
+				     GEN_INT (GET_MODE_SIZE (mode)),
+				     (modifier == EXPAND_STACK_PARM
+				      ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
+		  else
+		    emit_move_insn (new_with_op0_mode, op0);
+
+		  op0 = new_rtx;
+		}
 	    }
 
 	  op0 = adjust_address (op0, mode, 0);
@@ -10563,7 +10576,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
       /* WITH_SIZE_EXPR expands to its first argument.  The caller should
 	 have pulled out the size to use in whatever context it needed.  */
       return expand_expr_real (treeop0, original_target, tmode,
-			       modifier, alt_rtl);
+			       modifier, alt_rtl, expand_reference);
 
     default:
       return expand_expr_real_2 (&ops, target, tmode, modifier);
Index: gcc/expr.h
===================================================================
--- gcc/expr.h	(revision 205641)
+++ gcc/expr.h	(working copy)
@@ -41,7 +41,8 @@ along with GCC; see the file COPYING3.  If not see
     is a constant that is not a legitimate address.
    EXPAND_WRITE means we are only going to write to the resulting rtx.
    EXPAND_MEMORY means we are interested in a memory result, even if
-    the memory is constant and we could have propagated a constant value.  */
+    the memory is constant and we could have propagated a constant value,
+    or the memory is unaligned on a STRICT_ALIGNMENT target.  */
 enum expand_modifier {EXPAND_NORMAL = 0, EXPAND_STACK_PARM, EXPAND_SUM,
 		      EXPAND_CONST_ADDRESS, EXPAND_INITIALIZER, EXPAND_WRITE,
 		      EXPAND_MEMORY};
@@ -437,9 +438,9 @@ extern rtx force_operand (rtx, rtx);
 
 /* Work horses for expand_expr.  */
 extern rtx expand_expr_real (tree, rtx, enum machine_mode,
-			     enum expand_modifier, rtx *);
+			     enum expand_modifier, rtx *, bool);
 extern rtx expand_expr_real_1 (tree, rtx, enum machine_mode,
-			       enum expand_modifier, rtx *);
+			       enum expand_modifier, rtx *, bool);
 extern rtx expand_expr_real_2 (sepops, rtx, enum machine_mode,
 			       enum expand_modifier);
 
@@ -450,13 +451,13 @@ static inline rtx
 expand_expr (tree exp, rtx target, enum machine_mode mode,
 	     enum expand_modifier modifier)
 {
-  return expand_expr_real (exp, target, mode, modifier, NULL);
+  return expand_expr_real (exp, target, mode, modifier, NULL, false);
 }
 
 static inline rtx
 expand_normal (tree exp)
 {
-  return expand_expr_real (exp, NULL_RTX, VOIDmode, EXPAND_NORMAL, NULL);
+  return expand_expr_real (exp, NULL_RTX, VOIDmode, EXPAND_NORMAL, NULL, false);
 }
 
 /* At the start of a function, record that we have no previously-pushed
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 205641)
+++ gcc/cfgexpand.c	(working copy)
@@ -2253,7 +2253,7 @@ expand_call_stmt (gimple stmt)
   if (lhs)
     expand_assignment (lhs, exp, false);
   else
-    expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL);
+    expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);
 
   mark_transaction_restart_calls (stmt);
 }

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jeff Law]

On 12/04/13 01:16, Bernd Edlinger wrote:

>
>> Looking for some more time your patch may be indeed the easiest
>> without big re-factoring.
Richard (or Bernd), can you comment on why?  Something seems "off" here.

Why do we need to handle inner references here specially?   If feels 
like we're catering to broken code elsewhere in GCC.

As for the patch itself.  In a few places within expand_expr_real_1 you 
changed calls to expand_expr to instead call expand_expr_real.   ISTM 
you could have gotten the same effect by just adding your extra argument 
to the existing code?


Jeff

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

Hi,


On Thu, 5 Dec 2013 22:16:15, Jeff Law wrote:
>
> On 12/04/13 01:16, Bernd Edlinger wrote:
>
>>
>>> Looking for some more time your patch may be indeed the easiest
>>> without big re-factoring.
> Richard (or Bernd), can you comment on why? Something seems "off" here.
>
> Why do we need to handle inner references here specially? If feels
> like we're catering to broken code elsewhere in GCC.
>

My first attempt at fixing this was just a one-line change.
like:

@@ -9905,7 +9861,7 @@ expand_expr_real_1 (tree exp, rtx target
                          && modifier != EXPAND_STACK_PARM
                          ? target : NULL_RTX),
                         VOIDmode,
-                        modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+                        EXPAND_MEMORY);


But the problem with that was, that it might loose too much information...
i.e. little we know what tem might be, and what a change to the original
expand_modifier would have done at other places where we had no
problems at all.

So this current version of the patch is a trying to get as much flexibility
as possible while changing as little as possible at the same time.

Technically we only have to switch off some places in MEM_REF,
and VIEW_CONVERT_EXPR, where an unaligned value is placed in a
register, which turns out to be unnecessary in this special context.



> As for the patch itself. In a few places within expand_expr_real_1 you
> changed calls to expand_expr to instead call expand_expr_real. ISTM
> you could have gotten the same effect by just adding your extra argument
> to the existing code?
>

Yes, but one goal is to keep the patch-file as small as possible,
and expand_expr is used everywhere.

Actually expand_expr is just a wrapper for

expand_expr_real (exp, target, mode, modifier, NULL, false)

Therefore I replaced expand_expr with expand_expr_real at these few
places, to keep the change smaller.

Another point is, that I do not want to pass true for expand_reference
at any other place than from exand_expr_real_1 where the inner
reference is expanded.


Thanks
Bernd.

>
> Jeff 		 	   		  

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

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

> one test case is this one:
> 
> pr57748-3.c:
> /* PR middle-end/57748 */
> /* { dg-do run } */
> /* wrong code in expand_expr_real_1.  */
> 
> #include <stdlib.h>
> 
> extern void abort (void);
> 
> typedef long long V
>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
> 
> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
> 
> struct __attribute__((packed)) T { char c; P s; };
> 
> void __attribute__((noinline, noclone))
> check (P *p)
> {
>   if (p->b[0][0] != 3 || p->b[0][1] != 4)
>     abort ();
> }
> 
> void __attribute__((noinline, noclone))
> foo (struct T *t)
> {
>   V a = { 3, 4 };
>   t->s.b[0] = a;
> }
> 
> int
> main ()
> {
>   struct T *t = (struct T *) calloc (128, 1);
> 
>   foo (t);
>   check (&t->s);
> 
>   free (t);
>   return 0;
> }
> 
> 
> and the other one is
> pr57748-4.c:
> /* PR middle-end/57748 */
> /* { dg-do run } */
> /* wrong code in expand_expr_real_1.  */
> 
> #include <stdlib.h>
> 
> extern void abort (void);
> 
> typedef long long V
>   __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
> 
> typedef struct S { V b[1]; } P __attribute__((aligned (1)));
> 
> struct __attribute__((packed)) T { char c; P s; };
> 
> void __attribute__((noinline, noclone))
> check (P *p)
> {
>   if (p->b[1][0] != 3 || p->b[1][1] != 4)
>     abort ();
> }
> 
> void __attribute__((noinline, noclone))
> foo (struct T *t)
> {
>   V a = { 3, 4 };
>   t->s.b[1] = a;
> }
> 
> int
> main ()
> {
>   struct T *t = (struct T *) calloc (128, 1);
> 
>   foo (t);
>   check (&t->s);
> 
>   free (t);
>   return 0;
> }

Here's the Correct Fix(tm).  We may or may not decide to go for it because of 
concerns about ABI changes; in the latter case, any kludge that we'll put in 
place instead must be restricted to the cases caught by this patch.


	* stor-layout.c (compute_record_mode): Return BLKmode for a trailing
 	array with size 0 or 1.


-- 
Eric Botcazou

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

Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 205727)
+++ stor-layout.c	(working copy)
@@ -1605,6 +1605,18 @@ compute_record_mode (tree type)
 	  || ! tree_fits_uhwi_p (DECL_SIZE (field)))
 	return;
 
+      /* As a GNU extension, we support out-of-bounds accesses for a trailing
+	 array with size 0 or 1.  In this case, the record type effectively
+	 has variable size so it needs to have BLKmode.  */
+      if (!DECL_CHAIN (field) && TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE)
+	{
+	  tree domain_type = TYPE_DOMAIN (TREE_TYPE (field));
+	  if (!TYPE_MAX_VALUE (domain_type)
+	      || integer_zerop (TYPE_MAX_VALUE (domain_type))
+	      || integer_onep (TYPE_MAX_VALUE (domain_type)))
+	    return;
+	}
+
       /* If this field is the whole struct, remember its mode so
 	 that, say, we can put a double in a class into a DF
 	 register instead of forcing it to live in the stack.  */

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

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

> Here's the Correct Fix(tm).  We may or may not decide to go for it because
> of concerns about ABI changes; in the latter case, any kludge that we'll
> put in place instead must be restricted to the cases caught by this patch.
> 
> 
> 	* stor-layout.c (compute_record_mode): Return BLKmode for a trailing
>  	array with size 0 or 1.

Revised version without the one-by-one error...

-- 
Eric Botcazou

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

Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 205727)
+++ stor-layout.c	(working copy)
@@ -1605,6 +1605,17 @@ compute_record_mode (tree type)
 	  || ! tree_fits_uhwi_p (DECL_SIZE (field)))
 	return;
 
+      /* As a GNU extension, we support out-of-bounds accesses for a trailing
+	 array with size 0 or 1.  In this case, the record type effectively
+	 has variable size so it needs to have BLKmode.  */
+      if (!DECL_CHAIN (field) && TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE)
+	{
+	  tree domain_type = TYPE_DOMAIN (TREE_TYPE (field));
+	  if (!TYPE_MAX_VALUE (domain_type)
+	      || integer_zerop (TYPE_MAX_VALUE (domain_type)))
+	    return;
+	}
+
       /* If this field is the whole struct, remember its mode so
 	 that, say, we can put a double in a class into a DF
 	 register instead of forcing it to live in the stack.  */

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Fri, Dec 6, 2013 at 10:11 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Here's the Correct Fix(tm).  We may or may not decide to go for it because
>> of concerns about ABI changes; in the latter case, any kludge that we'll
>> put in place instead must be restricted to the cases caught by this patch.
>>
>>
>>       * stor-layout.c (compute_record_mode): Return BLKmode for a trailing
>>       array with size 0 or 1.
>
> Revised version without the one-by-one error...

It's not fully fixing the issue as _all_ aggregates that may be
accessed beyond their
declarations size are broken.

I'd say we should simply stop giving aggregates a mode besides BLKmode.  What
can possibly break with that ...

struct { char c[4]; }

has SImode, we accept all trailing arrays as possibly extending beyond the
struct declaration.

Alternatively all structs with aggregate members should not have a
mode != BLKmode.

Previously you said it doesn't have ABI impacts and I doubt it has optimization
impacts.

Richard.

> --
> Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Fri, Dec 6, 2013 at 6:16 AM, Jeff Law <law@redhat.com> wrote:
> On 12/04/13 01:16, Bernd Edlinger wrote:
>
>>
>>> Looking for some more time your patch may be indeed the easiest
>>> without big re-factoring.
>
> Richard (or Bernd), can you comment on why?  Something seems "off" here.
>
> Why do we need to handle inner references here specially?   If feels like
> we're catering to broken code elsewhere in GCC.

The issue is that we handle expansion of misaligned moves in the code
we recurse to - but that misaligned move handling can only work at
the "outermost" level of the recursion as it is supposed to see the
"real" access (alignment and mode of the memory access, not of
some base of it).

So we need a recursion that skips this part (and others - which already
works), just processing as-if in "expand the base of some memory access"
mode.

That we recurse at all when expanding memory accesses makes this
expansion path quite a twisted maze - which is why I suggested to
re-factor the whole thing to not require recursion (but that will be
a very big change, not suitable for this stage).

The easiest is to add a flag to indicate the "we're-expanding-the-base",
doing another expand modifier doesn't work as they are not combinable
and the passed modifier is already modified for the recursion - and that
dependent on stuff.

"Fixing" the mode of the base object isn't really fixing the fact that
the recursion shouldn't even try to generate a movmisalign mem,
it just papers over this issue by making it (hopefully) never trigger
(at least for valid code) for bases.

Richard.

> As for the patch itself.  In a few places within expand_expr_real_1 you
> changed calls to expand_expr to instead call expand_expr_real.   ISTM you
> could have gotten the same effect by just adding your extra argument to the
> existing code?
>
>
> Jeff

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jeff Law]

On 12/06/13 03:06, Richard Biener wrote:
>
> The issue is that we handle expansion of misaligned moves in the code
> we recurse to - but that misaligned move handling can only work at
> the "outermost" level of the recursion as it is supposed to see the
> "real" access (alignment and mode of the memory access, not of
> some base of it).
>
> So we need a recursion that skips this part (and others - which already
> works), just processing as-if in "expand the base of some memory access"
> mode.
So it's really not a case of someone outside expand_expr needing 
different behaviour, but a case of stopping the recursion within 
expand_expr.  That's a bit less concerning.

>
> That we recurse at all when expanding memory accesses makes this
> expansion path quite a twisted maze - which is why I suggested to
> re-factor the whole thing to not require recursion (but that will be
> a very big change, not suitable for this stage).
No doubt.  In general expand_expr is a mess and has been, well, forever.


>
> The easiest is to add a flag to indicate the "we're-expanding-the-base",
> doing another expand modifier doesn't work as they are not combinable
> and the passed modifier is already modified for the recursion - and that
> dependent on stuff.
We could always make the modifiers a bitmask, but probably not something 
we really need to do during stage3.

>
> "Fixing" the mode of the base object isn't really fixing the fact that
> the recursion shouldn't even try to generate a movmisalign mem,
> it just papers over this issue by making it (hopefully) never trigger
> (at least for valid code) for bases.
Ok, that answers the other question I had after looking at other parts 
of this thread.  Though one could argue that the modes are in fact wrong.

Jeff

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jeff Law]

On 12/06/13 01:51, Bernd Edlinger wrote:
>>> As for the patch itself. In a few places within expand_expr_real_1 you
>> changed calls to expand_expr to instead call expand_expr_real. ISTM
>> you could have gotten the same effect by just adding your extra argument
>> to the existing code?
>>
>
> Yes, but one goal is to keep the patch-file as small as possible,
> and expand_expr is used everywhere.
>
> Actually expand_expr is just a wrapper for
>
> expand_expr_real (exp, target, mode, modifier, NULL, false)
Sorry, I glossed over that -- mentally I saw the change to expand_expr 
and assumed you passed the new flag through.  But that's not how the 
change is implemented.  My bad.

So clearly for the cases where you want the flag to be true, you can't 
go through the wrapper.  Again, my bad.


jeff

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jeff Law]

On 12/06/13 02:11, Eric Botcazou wrote:
>> Here's the Correct Fix(tm).  We may or may not decide to go for it because
>> of concerns about ABI changes; in the latter case, any kludge that we'll
>> put in place instead must be restricted to the cases caught by this patch.
>>
>>
>> 	* stor-layout.c (compute_record_mode): Return BLKmode for a trailing
>>   	array with size 0 or 1.
>
> Revised version without the one-by-one error...
I'd certainly be concerned.  Ports have (for better or worse) keyed on 
BLKmode rather than looking at the underlying types.  So if something 
which was previously SImode or DImode is now BLKmode, there's a nonzero 
chance we're going to change how it gets passed.

jeff

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> It's not fully fixing the issue as _all_ aggregates that may be
> accessed beyond their declarations size are broken.

Sure, but we don't need to support such nonsense in the general case.  And not 
every language allows it, for example in Ada you cannot do that of course.

> I'd say we should simply stop giving aggregates a mode besides BLKmode. 
> What can possibly break with that ...

Nothing, but this will unnecessarily pessimize well-behaved languages, e.g. in 
Ada we overalign structures in some cases to give them integral modes.

> struct { char c[4]; }
> 
> has SImode, we accept all trailing arrays as possibly extending beyond the
> struct declaration.
>
> Alternatively all structs with aggregate members should not have a
> mode != BLKmode.

This could work as well, although I'd restrict this to arrays, recursively.

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> I'd certainly be concerned.  Ports have (for better or worse) keyed on
> BLKmode rather than looking at the underlying types.  So if something
> which was previously SImode or DImode is now BLKmode, there's a nonzero
> chance we're going to change how it gets passed.

Well, we have been saying that calling conventions need to be keyed on types 
rather than modes for more than a decade...  I recall auditing and fixing the 
SPARC back-end circa 2003, so how long are we going to use this argument?

That being said, the concern is certainly valid so we may want to go for a 
kludge instead of the fix.  The point is that the kludge should do exactly 
what the fix would have done in the RTL expander and nothing more; it's out of 
question to pessimize all the other languages and all the other cases in the C 
family of languages for highly artificial testcases using non-portable code.

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> That being said, the concern is certainly valid so we may want to go for a
> kludge instead of the fix.  The point is that the kludge should do exactly
> what the fix would have done in the RTL expander and nothing more; it's out
> of question to pessimize all the other languages and all the other cases in
> the C family of languages for highly artificial testcases using
> non-portable code.

What about:
 1. a new boolean language hook support_trailing_arrays,
 2. a new flag on RECORD_OR_UNION_TYPE_P types,
 3. modifying stor-layout.c so that it sets the new flag on the appropriate 
types when support_trailing_arrays is true,
 4. modifying expr.c along the lines of Bernd's latest idea to avoid returning 
anything but a MEM for objects with a type on which the flag is set?

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

Eric Botcazou <ebotcazou@adacore.com> wrote:
>> It's not fully fixing the issue as _all_ aggregates that may be
>> accessed beyond their declarations size are broken.
>
>Sure, but we don't need to support such nonsense in the general case. 
>And not 
>every language allows it, for example in Ada you cannot do that of
>course.

Well, we certainly need to support it as far as not ICEing

>> I'd say we should simply stop giving aggregates a mode besides
>BLKmode. 
>> What can possibly break with that ...
>
>Nothing, but this will unnecessarily pessimize well-behaved languages,
>e.g. in 
>Ada we overalign structures in some cases to give them integral modes.

What are the transformations that are enabled by making something not BLKmode?

On the gimple level I cannot think of one..

>> struct { char c[4]; }
>> 
>> has SImode, we accept all trailing arrays as possibly extending
>beyond the
>> struct declaration.
>>
>> Alternatively all structs with aggregate members should not have a
>> mode != BLKmode.
>
>This could work as well, although I'd restrict this to arrays,
>recursively.

Works for me.

Thanks,
Richard.

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

Eric Botcazou <ebotcazou@adacore.com> wrote:
>> That being said, the concern is certainly valid so we may want to go
>for a
>> kludge instead of the fix.  The point is that the kludge should do
>exactly
>> what the fix would have done in the RTL expander and nothing more;
>it's out
>> of question to pessimize all the other languages and all the other
>cases in
>> the C family of languages for highly artificial testcases using
>> non-portable code.
>
>What about:
> 1. a new boolean language hook support_trailing_arrays,
> 2. a new flag on RECORD_OR_UNION_TYPE_P types,
>3. modifying stor-layout.c so that it sets the new flag on the
>appropriate 
>types when support_trailing_arrays is true,
>4. modifying expr.c along the lines of Bernd's latest idea to avoid
>returning 
>anything but a MEM for objects with a type on which the flag is set?

Sounds more complicated than the other two options. Fix the types mode or add the expand flag.

I'm ok with either variant and I'm not worried about the ABI thing too much for the important archs.

Richard.

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jeff Law]

On 12/07/13 03:44, Eric Botcazou wrote:
>> I'd certainly be concerned.  Ports have (for better or worse) keyed on
>> BLKmode rather than looking at the underlying types.  So if something
>> which was previously SImode or DImode is now BLKmode, there's a nonzero
>> chance we're going to change how it gets passed.
>
> Well, we have been saying that calling conventions need to be keyed on types
> rather than modes for more than a decade...  I recall auditing and fixing the
> SPARC back-end circa 2003, so how long are we going to use this argument?
I don't recall such a change in policy -- but that doesn't mean it 
didn't happen :-)

If we already declared that ports should be looking at the underlying 
types rather than the mode and it's been in place that long, then I 
think any port that hasn't been audited/updated deserves its fate and we 
shouldn't let them stand in the way of making progress on this issue.

jeff

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

> On 12/07/13 03:44, Eric Botcazou wrote:
>>> I'd certainly be concerned. Ports have (for better or worse) keyed on
>>> BLKmode rather than looking at the underlying types. So if something
>>> which was previously SImode or DImode is now BLKmode, there's a nonzero
>>> chance we're going to change how it gets passed.
>>
>> Well, we have been saying that calling conventions need to be keyed on types
>> rather than modes for more than a decade... I recall auditing and fixing the
>> SPARC back-end circa 2003, so how long are we going to use this argument?
> I don't recall such a change in policy -- but that doesn't mean it
> didn't happen :-)
>
> If we already declared that ports should be looking at the underlying
> types rather than the mode and it's been in place that long, then I
> think any port that hasn't been audited/updated deserves its fate and we
> shouldn't let them stand in the way of making progress on this issue.
>
> jeff

The ports are one thing, but this non-BLKmode thing can also be visible to
the user code:

If we have

struct s{ char x[8]; };

We can place that in a register like this:

register struct s x __asm__("eax");

Previous experimentation showed that this code can break with that ABI change.

So if we change that structure mode, we should make sure that this has no other
unexpected implications, especially in phase 3.

Bernd. 		 	   		  

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Mon, Dec 9, 2013 at 10:07 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>> On 12/07/13 03:44, Eric Botcazou wrote:
>>>> I'd certainly be concerned. Ports have (for better or worse) keyed on
>>>> BLKmode rather than looking at the underlying types. So if something
>>>> which was previously SImode or DImode is now BLKmode, there's a nonzero
>>>> chance we're going to change how it gets passed.
>>>
>>> Well, we have been saying that calling conventions need to be keyed on types
>>> rather than modes for more than a decade... I recall auditing and fixing the
>>> SPARC back-end circa 2003, so how long are we going to use this argument?
>> I don't recall such a change in policy -- but that doesn't mean it
>> didn't happen :-)
>>
>> If we already declared that ports should be looking at the underlying
>> types rather than the mode and it's been in place that long, then I
>> think any port that hasn't been audited/updated deserves its fate and we
>> shouldn't let them stand in the way of making progress on this issue.
>>
>> jeff
>
> The ports are one thing, but this non-BLKmode thing can also be visible to
> the user code:
>
> If we have
>
> struct s{ char x[8]; };
>
> We can place that in a register like this:
>
> register struct s x __asm__("eax");
>
> Previous experimentation showed that this code can break with that ABI change.
>
> So if we change that structure mode, we should make sure that this has no other
> unexpected implications, especially in phase 3.

We supposedly can force the mode of the DECL to be that of EAX
at the declaration, even if the mode of the type is BLKmode.

Richard.

> Bernd.

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> What are the transformations that are enabled by making something not
> BLKmode?
> 
> On the gimple level I cannot think of one..

On the RTL level, it's simple: anything BLKmode is forced to memory instead of 
being loaded into registers.

> >This could work as well, although I'd restrict this to arrays,
> >recursively.
>
> Works for me.

Are we sure that we really support out-of-bounds accesses for arbitrary arrays 
though?  It seems to me that we easily take advantage of them e.g. in loops to 
invoke undefined behavior.  IMO it's not clear whether we want to risk more 
accidental ABI changes if they are not supported throughout the compiler.

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jakub Jelinek]

On Tue, Dec 10, 2013 at 11:04:53AM +0100, Eric Botcazou wrote:
> > What are the transformations that are enabled by making something not
> > BLKmode?
> > 
> > On the gimple level I cannot think of one..
> 
> On the RTL level, it's simple: anything BLKmode is forced to memory instead of 
> being loaded into registers.
> 
> > >This could work as well, although I'd restrict this to arrays,
> > >recursively.
> >
> > Works for me.
> 
> Are we sure that we really support out-of-bounds accesses for arbitrary arrays 
> though?  It seems to me that we easily take advantage of them e.g. in loops to 
> invoke undefined behavior.  IMO it's not clear whether we want to risk more 
> accidental ABI changes if they are not supported throughout the compiler.

I think we don't support out-of-bounds accesses for global vars (ok, there
are vars with real flexible array members as GNU extension initialized by
initializers that initialize the flexible array members, but then the decl
size etc. should be adjusted for that), similarly for automatic vars (and in
both cases that includes the hard register vars).  What we support is
out of bounds accesses for heap vars if the var's type has flexible array member
or something we treat similarly and there is the possibility that there
could be payload after the heap var that could be accessed from the flexible
array members or similar arrays.  And of course if you have just pointer to
some var and can't see what it points to, we need to treat it as if it could
be heap var.

So, I don't see what is the big deal with BLKmode, because all the cases
which actually could have flexible array member extra payloads (or similar)
must necessarily live in memory, if it is the compiler that decides whether
to put it into memory or keep in registers etc., then it can't be heap
allocated.

	Jakub

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> What we support is out of bounds accesses for heap vars if the var's type
> has flexible array member or something we treat similarly and there is the
> possibility that there could be payload after the heap var that could be 
> accessed from the flexible array members or similar arrays.

My question was about the above similar arrays, i.e. whether we consider all 
trailing arrays in structures as flexible-like or not.  No strong opinion.

> So, I don't see what is the big deal with BLKmode, because all the cases
> which actually could have flexible array member extra payloads (or similar)
> must necessarily live in memory, if it is the compiler that decides whether
> to put it into memory or keep in registers etc., then it can't be heap
> allocated.

The invariant is that types for which objects can effectively have variable 
size must have BLKmode, otherwise you need to add very ugly code in the RTL 
expander to mask the lie.

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Tue, Dec 10, 2013 at 11:53 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> What we support is out of bounds accesses for heap vars if the var's type
>> has flexible array member or something we treat similarly and there is the
>> possibility that there could be payload after the heap var that could be
>> accessed from the flexible array members or similar arrays.
>
> My question was about the above similar arrays, i.e. whether we consider all
> trailing arrays in structures as flexible-like or not.  No strong opinion.

Yes we do, even for struct { struct { int a; char a[1] } }; (note the not really
"trailing" as there is padding after the trailing array).  We do take
size limitations from a DECL (if we see one) into account to limit the
effect of this trailing-array-supporting, so it effectively only applies to
indirect accesses (and the padding example above, you can use the whole
padding if DECL_SIZE allows that).

>> So, I don't see what is the big deal with BLKmode, because all the cases
>> which actually could have flexible array member extra payloads (or similar)
>> must necessarily live in memory, if it is the compiler that decides whether
>> to put it into memory or keep in registers etc., then it can't be heap
>> allocated.
>
> The invariant is that types for which objects can effectively have variable
> size must have BLKmode, otherwise you need to add very ugly code in the RTL
> expander to mask the lie.

I wonder if we can make the expander more rely on the DECLs mode
and optimize only the DECLs mode (if we can constrain its size via
DECL_SIZE) to non-BLKmode instead of doing that for the TYPEs mode.
Yes, you'd have DECL_MODE != TYPE_MODE that way.

Or rather I wonder if the expander doesn't already work that way
(looks at DECL_MODE).

Richard.

> --
> Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Tue, Dec 10, 2013 at 4:02 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Dec 10, 2013 at 11:53 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> What we support is out of bounds accesses for heap vars if the var's type
>>> has flexible array member or something we treat similarly and there is the
>>> possibility that there could be payload after the heap var that could be
>>> accessed from the flexible array members or similar arrays.
>>
>> My question was about the above similar arrays, i.e. whether we consider all
>> trailing arrays in structures as flexible-like or not.  No strong opinion.
>
> Yes we do, even for struct { struct { int a; char a[1] } }; (note the not really
> "trailing" as there is padding after the trailing array).  We do take
> size limitations from a DECL (if we see one) into account to limit the
> effect of this trailing-array-supporting, so it effectively only applies to
> indirect accesses (and the padding example above, you can use the whole
> padding if DECL_SIZE allows that).
>
>>> So, I don't see what is the big deal with BLKmode, because all the cases
>>> which actually could have flexible array member extra payloads (or similar)
>>> must necessarily live in memory, if it is the compiler that decides whether
>>> to put it into memory or keep in registers etc., then it can't be heap
>>> allocated.
>>
>> The invariant is that types for which objects can effectively have variable
>> size must have BLKmode, otherwise you need to add very ugly code in the RTL
>> expander to mask the lie.
>
> I wonder if we can make the expander more rely on the DECLs mode
> and optimize only the DECLs mode (if we can constrain its size via
> DECL_SIZE) to non-BLKmode instead of doing that for the TYPEs mode.
> Yes, you'd have DECL_MODE != TYPE_MODE that way.
>
> Or rather I wonder if the expander doesn't already work that way
> (looks at DECL_MODE).

To speak in patches (completely untested) - sth like the following ontop
of making more types have BLKmode:

Index: gcc/stor-layout.c
===================================================================
--- gcc/stor-layout.c   (revision 205857)
+++ gcc/stor-layout.c   (working copy)
@@ -569,8 +569,17 @@ layout_decl (tree decl, unsigned int kno
                                          bitsize_unit_node));

   if (code != FIELD_DECL)
-    /* For non-fields, update the alignment from the type.  */
-    do_type_align (type, decl);
+    {
+      /* For non-fields, update the alignment from the type.  */
+      do_type_align (type, decl);
+      /* Optimize the mode of the decl.
+        ???  Ensure choosen mode alignment fits decl alignment.  */
+      if (DECL_MODE (decl) == BLKmode
+         && DECL_SIZE (decl)
+         && compare_tree_int (DECL_SIZE (t), MAX_FIXED_MODE_SIZE) <= 0)
+       DECL_MODE (decl)
+         = mode_for_size (TREE_INT_CST_LOW (DECL_SIZE (decl)), MODE_INT, 1);
+    }
   else
     /* For fields, it's a bit more complicated...  */
     {


> Richard.
>
>> --
>> Eric Botcazou

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

Hi,

On Tue, 10 Dec 2013 16:14:43, Richard Biener wrote:
>
> On Tue, Dec 10, 2013 at 4:02 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Dec 10, 2013 at 11:53 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>>> What we support is out of bounds accesses for heap vars if the var's type
>>>> has flexible array member or something we treat similarly and there is the
>>>> possibility that there could be payload after the heap var that could be
>>>> accessed from the flexible array members or similar arrays.
>>>
>>> My question was about the above similar arrays, i.e. whether we consider all
>>> trailing arrays in structures as flexible-like or not. No strong opinion.
>>
>> Yes we do, even for struct { struct { int a; char a[1] } }; (note the not really
>> "trailing" as there is padding after the trailing array). We do take
>> size limitations from a DECL (if we see one) into account to limit the
>> effect of this trailing-array-supporting, so it effectively only applies to
>> indirect accesses (and the padding example above, you can use the whole
>> padding if DECL_SIZE allows that).
>>
>>>> So, I don't see what is the big deal with BLKmode, because all the cases
>>>> which actually could have flexible array member extra payloads (or similar)
>>>> must necessarily live in memory, if it is the compiler that decides whether
>>>> to put it into memory or keep in registers etc., then it can't be heap
>>>> allocated.
>>>
>>> The invariant is that types for which objects can effectively have variable
>>> size must have BLKmode, otherwise you need to add very ugly code in the RTL
>>> expander to mask the lie.
>>
>> I wonder if we can make the expander more rely on the DECLs mode
>> and optimize only the DECLs mode (if we can constrain its size via
>> DECL_SIZE) to non-BLKmode instead of doing that for the TYPEs mode.
>> Yes, you'd have DECL_MODE != TYPE_MODE that way.
>>
>> Or rather I wonder if the expander doesn't already work that way
>> (looks at DECL_MODE).
>
> To speak in patches (completely untested) - sth like the following ontop
> of making more types have BLKmode:
>
> Index: gcc/stor-layout.c
> ===================================================================
> --- gcc/stor-layout.c (revision 205857)
> +++ gcc/stor-layout.c (working copy)
> @@ -569,8 +569,17 @@ layout_decl (tree decl, unsigned int kno
> bitsize_unit_node));
>
> if (code != FIELD_DECL)
> - /* For non-fields, update the alignment from the type. */
> - do_type_align (type, decl);
> + {
> + /* For non-fields, update the alignment from the type. */
> + do_type_align (type, decl);
> + /* Optimize the mode of the decl.
> + ??? Ensure choosen mode alignment fits decl alignment. */
> + if (DECL_MODE (decl) == BLKmode
> + && DECL_SIZE (decl)
> + && compare_tree_int (DECL_SIZE (t), MAX_FIXED_MODE_SIZE) <= 0)
> + DECL_MODE (decl)
> + = mode_for_size (TREE_INT_CST_LOW (DECL_SIZE (decl)), MODE_INT, 1);
> + }
> else
> /* For fields, it's a bit more complicated... */
> {
>
>
>> Richard.
>>
>>> --
>>> Eric Botcazou

Whatever the fix will be, it should contain at least the two test cases from
my patch, and, maybe - if that is possible - it would be nice to test it with
with SLOW_UNALIGNED_ACCESS defined like this:

config/i386/i386.h:
#define SLOW_UNALIGNED_ACCESS(MODE, ALIGN) 1


Bernd. 		 	   		  

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

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

> Yes we do, even for struct { struct { int a; char a[1] } }; (note the not
> really "trailing" as there is padding after the trailing array).  We do
> take size limitations from a DECL (if we see one) into account to limit the
> effect of this trailing-array-supporting, so it effectively only applies to
> indirect accesses (and the padding example above, you can use the whole
> padding if DECL_SIZE allows that).

OK, so we want the attached patch?  FWIW it passed

  make -k check-c check-c++ RUNTESTFLAGS="compat.exp struct-layout-1.exp"

on x86/Linux, x86-64/Linux, PowerPC/Linux [*], IA-64/Linux, SPARC/Solaris and 
SPARC64/Solaris with ALT_CC_UNDER_TEST set to the unpatched compiler.

[*] the failures (DFP related) are the same as with the unpatched compiler.

-- 
Eric Botcazou

[-- Attachment #2: pr57748-3.diff --]
[-- Type: text/x-patch, Size: 865 bytes --]

Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 205881)
+++ stor-layout.c	(working copy)
@@ -1605,6 +1605,15 @@ compute_record_mode (tree type)
 	  || ! tree_fits_uhwi_p (DECL_SIZE (field)))
 	return;
 
+      /* As a GNU extension, we support out-of-bounds accesses for a trailing
+	 array in a record type.  In this case, if the element type has a non
+	 zero size, then the record type effectively has variable size so it
+	 needs to have BLKmode.  */
+      if (!DECL_CHAIN (field)
+	  && TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
+	  && !integer_zerop (TYPE_SIZE (TREE_TYPE (TREE_TYPE (field)))))
+	return;
+
       /* If this field is the whole struct, remember its mode so
 	 that, say, we can put a double in a class into a DF
 	 register instead of forcing it to live in the stack.  */

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> OK, so we want the attached patch?  FWIW it passed
> 
>   make -k check-c check-c++ RUNTESTFLAGS="compat.exp struct-layout-1.exp"
> 
> on x86/Linux, x86-64/Linux, PowerPC/Linux [*], IA-64/Linux, SPARC/Solaris
> and SPARC64/Solaris with ALT_CC_UNDER_TEST set to the unpatched compiler.

As well as on ARM/EABI w/ and w/o -mfloat-abi=hard.

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Jeff Law]

On 12/11/13 12:19, Eric Botcazou wrote:
>> Yes we do, even for struct { struct { int a; char a[1] } }; (note the not
>> really "trailing" as there is padding after the trailing array).  We do
>> take size limitations from a DECL (if we see one) into account to limit the
>> effect of this trailing-array-supporting, so it effectively only applies to
>> indirect accesses (and the padding example above, you can use the whole
>> padding if DECL_SIZE allows that).
>
> OK, so we want the attached patch?  FWIW it passed
>
>    make -k check-c check-c++ RUNTESTFLAGS="compat.exp struct-layout-1.exp"
>
> on x86/Linux, x86-64/Linux, PowerPC/Linux [*], IA-64/Linux, SPARC/Solaris and
> SPARC64/Solaris with ALT_CC_UNDER_TEST set to the unpatched compiler.
>
> [*] the failures (DFP related) are the same as with the unpatched compiler.
Does this catch C99 VLAs?  I vaguely recall we have a different internal 
representation of those?!?   And don't those have the same problem? 
[I've been meaning to research the different ways we represent trailing 
arrays/VLAs, but haven't had the time yet. ]

I think this patch is a good one, but I'm not sure it's 100% complete yet.

jeff

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> Does this catch C99 VLAs?  I vaguely recall we have a different internal
> representation of those?!?   And don't those have the same problem?

No, VLAs have explicit variable size so they are not problematic here.

-- 
Eric Botcazou

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

Hi,


with Eric's patch this test case does no longer compile, while it did compile with my patch:

$ cat test.c
struct s{ char x[8];  };
int main()
{
  volatile register struct s x __asm__("eax");
  x.x[0] = 1;
  return x.x[0];
}

$ gcc -O3 -S test.c
test.c: In function 'main':
test.c:4:30: error: data type of 'x' isn't suitable for a register
   volatile register struct s x __asm__("eax");
                              ^


Bernd. 		 	   		  

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> with Eric's patch this test case does no longer compile, while it did
> compile with my patch:
> 
> $ cat test.c
> struct s{ char x[8];  };
> int main()
> {
>   volatile register struct s x __asm__("eax");
>   x.x[0] = 1;
>   return x.x[0];
> }
> 
> $ gcc -O3 -S test.c
> test.c: In function 'main':
> test.c:4:30: error: data type of 'x' isn't suitable for a register
>    volatile register struct s x __asm__("eax");

Well, sure, but try with:

  struct s { char x[7]; };

or

  struct s { char y; char x[7]; };

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Wed, Dec 11, 2013 at 8:19 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yes we do, even for struct { struct { int a; char a[1] } }; (note the not
>> really "trailing" as there is padding after the trailing array).  We do
>> take size limitations from a DECL (if we see one) into account to limit the
>> effect of this trailing-array-supporting, so it effectively only applies to
>> indirect accesses (and the padding example above, you can use the whole
>> padding if DECL_SIZE allows that).
>
> OK, so we want the attached patch?  FWIW it passed
>
>   make -k check-c check-c++ RUNTESTFLAGS="compat.exp struct-layout-1.exp"
>
> on x86/Linux, x86-64/Linux, PowerPC/Linux [*], IA-64/Linux, SPARC/Solaris and
> SPARC64/Solaris with ALT_CC_UNDER_TEST set to the unpatched compiler.
>
> [*] the failures (DFP related) are the same as with the unpatched compiler.

+      /* As a GNU extension, we support out-of-bounds accesses for a trailing
+     array in a record type.  In this case, if the element type has a non
+     zero size, then the record type effectively has variable size so it
+     needs to have BLKmode.  */
+      if (!DECL_CHAIN (field)

The next field may be a TYPE_DECL with
struct { char c[8]; typedef int x; };

Instead I'd suggest to keep a 'last_field_array_p' flag that you can
check at the end of the loop.

+      && TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
+      && !integer_zerop (TYPE_SIZE (TREE_TYPE (TREE_TYPE (field)))))
+    return;
+

Why does this exclude zero-sized element types?  That looks odd to me ;)

Btw, the loop already has

      if (TREE_CODE (TREE_TYPE (field)) == ERROR_MARK
          || (TYPE_MODE (TREE_TYPE (field)) == BLKmode
              && ! TYPE_NO_FORCE_BLK (TREE_TYPE (field))
              && !(TYPE_SIZE (TREE_TYPE (field)) != 0
                   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
          || ! tree_fits_uhwi_p (bit_position (field))
          || DECL_SIZE (field) == 0
          || ! tree_fits_uhwi_p (DECL_SIZE (field)))
        return;

which then is supposed to
handle struct { struct { char c[8]; } a; } - but it seems to
special-case zero-sized
members again, thus struct { struct { char c[0]; } a; } would still be
broken after
your patch?

The issue Bernd raises is real as well, though we probably should fix this
in a different way by using a different DECL_MODE based on the types
size for asm register vars?

Btw, do you think we can recover from some of the now BLKmodes by
having DECL_MODE != TYPE_MODE?

Thanks,
Richard.

> --
> Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> Instead I'd suggest to keep a 'last_field_array_p' flag that you can
> check at the end of the loop.

OK, I can do that.

> +      && TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
> +      && !integer_zerop (TYPE_SIZE (TREE_TYPE (TREE_TYPE (field)))))
> +    return;
> +
> 
> Why does this exclude zero-sized element types?  That looks odd to me ;)

Because the size cannot change if you add zero to it, even multiple times?

> Btw, the loop already has
> 
>       if (TREE_CODE (TREE_TYPE (field)) == ERROR_MARK
> 
>           || (TYPE_MODE (TREE_TYPE (field)) == BLKmode
> 
>               && ! TYPE_NO_FORCE_BLK (TREE_TYPE (field))
>               && !(TYPE_SIZE (TREE_TYPE (field)) != 0
>                    && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
> 
>           || ! tree_fits_uhwi_p (bit_position (field))
>           || DECL_SIZE (field) == 0
>           || ! tree_fits_uhwi_p (DECL_SIZE (field)))
> 
>         return;
> 
> which then is supposed to
> handle struct { struct { char c[8]; } a; } - but it seems to
> special-case zero-sized
> members again, thus struct { struct { char c[0]; } a; } would still be
> broken after your patch?

Probably indeed, this test was added by the same patch that added the 0-sized 
bitfield test in the RTL expander
   http://gcc.gnu.org/ml/gcc-patches/2003-10/msg00823.html
and that I think is obsolete now.  I can remove it.

> The issue Bernd raises is real as well, though we probably should fix this
> in a different way by using a different DECL_MODE based on the types
> size for asm register vars?

Really?  The number of open PRs for register variables not behaving properly 
at run time would make me think that this wouldn't be necessary bad...

> Btw, do you think we can recover from some of the now BLKmodes by
> having DECL_MODE != TYPE_MODE?

Well, if you want to do that, the straightforward solution is to keep the non-
BLKmode for the TYPE, put a flag on it and treat it as BLKmode when needed.

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Fri, Dec 13, 2013 at 12:08 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Instead I'd suggest to keep a 'last_field_array_p' flag that you can
>> check at the end of the loop.
>
> OK, I can do that.
>
>> +      && TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
>> +      && !integer_zerop (TYPE_SIZE (TREE_TYPE (TREE_TYPE (field)))))
>> +    return;
>> +
>>
>> Why does this exclude zero-sized element types?  That looks odd to me ;)
>
> Because the size cannot change if you add zero to it, even multiple times?

Of course - I wonder if you hit a testcase in the testsuite.

>> Btw, the loop already has
>>
>>       if (TREE_CODE (TREE_TYPE (field)) == ERROR_MARK
>>
>>           || (TYPE_MODE (TREE_TYPE (field)) == BLKmode
>>
>>               && ! TYPE_NO_FORCE_BLK (TREE_TYPE (field))
>>               && !(TYPE_SIZE (TREE_TYPE (field)) != 0
>>                    && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
>>
>>           || ! tree_fits_uhwi_p (bit_position (field))
>>           || DECL_SIZE (field) == 0
>>           || ! tree_fits_uhwi_p (DECL_SIZE (field)))
>>
>>         return;
>>
>> which then is supposed to
>> handle struct { struct { char c[8]; } a; } - but it seems to
>> special-case zero-sized
>> members again, thus struct { struct { char c[0]; } a; } would still be
>> broken after your patch?
>
> Probably indeed, this test was added by the same patch that added the 0-sized
> bitfield test in the RTL expander
>    http://gcc.gnu.org/ml/gcc-patches/2003-10/msg00823.html
> and that I think is obsolete now.  I can remove it.

Ok with me.

>> The issue Bernd raises is real as well, though we probably should fix this
>> in a different way by using a different DECL_MODE based on the types
>> size for asm register vars?
>
> Really?  The number of open PRs for register variables not behaving properly
> at run time would make me think that this wouldn't be necessary bad...

Yeah ... we can wait and see if anybody notices.  (especially register
vars that you access out-of-bounds behave erratically ...)

>> Btw, do you think we can recover from some of the now BLKmodes by
>> having DECL_MODE != TYPE_MODE?
>
> Well, if you want to do that, the straightforward solution is to keep the non-
> BLKmode for the TYPE, put a flag on it and treat it as BLKmode when needed.

That sounds less conservative though.  Anyway, that was just some thought
to fix the eventual fallout of making more types have BLKmode.

Thanks,
Richard.

> --
> Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

> That sounds less conservative though.  Anyway, that was just some thought
> to fix the eventual fallout of making more types have BLKmode.

In the end I was too optimistic...  Testing a revised patch on x86 revealed a 
obscure case where the new BLKmode triggered a layout change (I presume that 
there is some randomness in struct-layout-1.exp because I didn't see it for 
the earlier runs).  The culprit is:

int
x86_field_alignment (tree field, int computed)
{
  enum machine_mode mode;
  tree type = TREE_TYPE (field);

  if (TARGET_64BIT || TARGET_ALIGN_DOUBLE)
    return computed;
  mode = TYPE_MODE (strip_array_types (type));
  if (mode == DFmode || mode == DCmode
      || GET_MODE_CLASS (mode) == MODE_INT
      || GET_MODE_CLASS (mode) == MODE_COMPLEX_INT)
    return MIN (32, computed);
  return computed;
}

which of course blatantly violates the do-not-rely-on-mode rule.  Although the 
layout change apparently occurs very rarely, I think that this rules out the 
direct mode change in stor-layout.c... <snip>

At this point, I'd agree with either of the following 2 solutions:

 1. Instead of changing the mode, we set a flag on the type (conveniently we 
can reuse the TYPE_NO_FORCE_BLK bit since it's only set for types with BLKmode 
already) and test it during RTL expansion to force BLKmode,

 2. Bernd's latest patch, but with the new boolean parameter explicitly called 
KEEP_UNALIGNED_MEMORY and a fat comment along the lines of:

  If KEEP_UNALIGNED_MEMORY is true, we don't adjust a returned MEM rtx that
  wouldn't be sufficient aligned for its mode; instead, it's up to the caller
  to deal with it afterwards.  This is used to make sure that unaligned base
  objects for which out-of-bounds accesses are supported, for example record
  types with trailing arrays, aren't realigned behind the back of the caller.
  The normal operating mode is to pass FALSE for this parameter.

-- 
Eric Botcazou

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Joseph S. Myers]

On Mon, 16 Dec 2013, Eric Botcazou wrote:

> which of course blatantly violates the do-not-rely-on-mode rule.  Although the 
> layout change apparently occurs very rarely, I think that this rules out the 
> direct mode change in stor-layout.c... <snip>

Well - makes such a change unsuitable for 4.9.  Longer-term we still want 
to avoid ABIs depending on modes of structs / unions / arrays (and 
depending on modes of vectors can also have undesired effects in some 
cases - I took care when working on the ARM hard-float ABI variant to 
avoid the ABI for argument passing of GCC generic vectors depending on 
whether NEON vector support was enables).  That suggests to me:

* Produce a list of all ABI-affecting target macros and hooks that might, 
wrongly, use modes to determine the ABI (struct layout, alignment, 
argument passing, function return, etc.).

* Put that on a wiki page along with a description of the problem (when 
it's OK to use modes in determining the ABI and when it isn't).  Also make 
sure the documentation of the macros / hooks in the internals manual is 
clear about not using modes inappropriately.

* Put a table of target architectures on the wiki page with a column for 
architecture maintainers to record whether they have checked for and fixed 
any problematic uses of modes.

* Ask architecture maintainers to do the checks and fixes.  Fixes may be 
nontrivial in some cases, as compatibility means that if the ABI did 
depend on the mode you may need to replicate the relevant bits of 
stor-layout.c logic that determined the mode inside the back end.

* After a while (pinging maintainers as needed), obsolete then remove any 
architectures that have not been checked.

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

Hi,


On Mon, 16 Dec 2013 17:22:59, Eric Botcazou wrote:
>
>> That sounds less conservative though. Anyway, that was just some thought
>> to fix the eventual fallout of making more types have BLKmode.
>
> In the end I was too optimistic... Testing a revised patch on x86 revealed a
> obscure case where the new BLKmode triggered a layout change (I presume that
> there is some randomness in struct-layout-1.exp because I didn't see it for
> the earlier runs). The culprit is:
>
> int
> x86_field_alignment (tree field, int computed)
> {
> enum machine_mode mode;
> tree type = TREE_TYPE (field);
>
> if (TARGET_64BIT || TARGET_ALIGN_DOUBLE)
> return computed;
> mode = TYPE_MODE (strip_array_types (type));
> if (mode == DFmode || mode == DCmode
> || GET_MODE_CLASS (mode) == MODE_INT
> || GET_MODE_CLASS (mode) == MODE_COMPLEX_INT)
> return MIN (32, computed);
> return computed;
> }
>
> which of course blatantly violates the do-not-rely-on-mode rule. Although the
> layout change apparently occurs very rarely, I think that this rules out the
> direct mode change in stor-layout.c... <snip>
>
> At this point, I'd agree with either of the following 2 solutions:
>
> 1. Instead of changing the mode, we set a flag on the type (conveniently we
> can reuse the TYPE_NO_FORCE_BLK bit since it's only set for types with BLKmode
> already) and test it during RTL expansion to force BLKmode,
>
> 2. Bernd's latest patch, but with the new boolean parameter explicitly called
> KEEP_UNALIGNED_MEMORY and a fat comment along the lines of:
>
> If KEEP_UNALIGNED_MEMORY is true, we don't adjust a returned MEM rtx that
> wouldn't be sufficient aligned for its mode; instead, it's up to the caller
> to deal with it afterwards. This is used to make sure that unaligned base
> objects for which out-of-bounds accesses are supported, for example record
> types with trailing arrays, aren't realigned behind the back of the caller.
> The normal operating mode is to pass FALSE for this parameter.
>
> --
> Eric Botcazou

Thanks for you analysis, Eric.

In the moment I am under the impression, that it is not safe to change
the type-mode at all. The other example with register variables, of struct-type
which were sensitive to the BLKmode, does certainly also mean that the target
is peeking at the mode somewhere, and who knows what's next...

Renaming the parameter is of course an option, and often choosing the right name
helps more than lots of comments, but if I call it KEEP_UNALIGNED_MEMROY
it is not clear, that this parameter is only to be used to expand an inner reference.

In general I like the comment, and I am open for other suggestions how
to call the parameter.

How about this comment?

"If EXPAND_REFERENCE is true, we are expanding an inner reference. 
 In this case, we don't adjust a returned MEM rtx that wouldn't be sufficient
 aligned for its mode; instead, it's up to the caller to deal with it afterwards.
 This is used to make sure that unaligned base objects for which out-of-bounds
 accesses are supported, for example record types with trailing arrays, aren't
 realigned behind the back of the caller.
 The normal operating mode is to pass FALSE for this parameter."


Bernd. 		 	   		  

Re: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Eric Botcazou]

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

> In general I like the comment, and I am open for other suggestions how
> to call the parameter.

I think that using EXPAND in the parameter's name is confusing because it 
needs to be distinguished from MODIFIER and its enumeration type.  And since 
it would be true only after calling get_inner_reference, it would be better to 
have the "inner" as well.  Maybe inner_reference_p or something equivalent.

For the record, I have also attached a patch that implements proposal #1, but 
it's lightly tested for the time being.


	* tree-core.h (tree_type_common): Rename no_force_blk_flag into
	blkmode_flag.
	* tree.h (TYPE_NO_FORCE_BLK): Delete.
	(TYPE_BLKMODE_FLAG): New macro.
	(TYPE_NO_FORCE_BLKMODE, SET_TYPE_NO_FORCE_BLKMODE): Likewise.
	(TYPE_BLKMODE_FOR_ACCESS, SET_TYPE_BLKMODE_FOR_ACCESS): Likewise.
	* lto-streamer-out.c (hash_tree): Adjust.
	* tree-streamer-out.c (pack_ts_type_common_value_fields): Likewise.
	* tree-streamer-in.c (unpack_ts_type_common_value_fields): Likewise.
	* print-tree.c (print_node): Likewise.
	* cfgexpand.c (expand_call_stmt): Call expand_expr.
	* expr.h (enum expand_modifier): Enhance comment.
	* expr.c (expand_expr_real_1) <case TARGET_MEM_REF>: Do not realign
	if the type has TYPE_BLKMODE_FOR_ACCESS.
	<case MEM_REF>: Likewise.
	<case VIEW_CONVERT_EXPR>: Factor out realignment code and disable it
	for EXPAND_{WRITE,MEMORY} or if the type has TYPE_BLKMODE_FOR_ACCESS.
	* stor-layout.c (compute_record_mode): Do not special-case BLKmode
	fields with zero size.  Compute TYPE_BLKMODE_FOR_ACCESS and adjust.
	(layout_type): Adjust.
lto/
	* lto.c (compare_tree_sccs_1): Adjust.


-- 
Eric Botcazou

[-- Attachment #2: pr57748-5.diff --]
[-- Type: text/x-patch, Size: 12942 bytes --]

Index: tree.h
===================================================================
--- tree.h	(revision 206105)
+++ tree.h	(working copy)
@@ -1589,11 +1589,18 @@ extern void protected_set_expr_location
    get one debug info record for them.  */
 #define TYPE_STUB_DECL(NODE) (TREE_CHAIN (TYPE_CHECK (NODE)))
 
-/* In a RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE or ARRAY_TYPE, it means
-   the type has BLKmode only because it lacks the alignment required for
-   its size.  */
-#define TYPE_NO_FORCE_BLK(NODE) \
-  (TYPE_CHECK (NODE)->type_common.no_force_blk_flag)
+/* In a RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE or ARRAY_TYPE with BLKmode,
+   it means the type has BLKmode only because it lacks the alignment required
+   for its size.  In a RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE or ARRAY_TYPE
+   with non-BLKmode, it means the type should have had BLKmode because out of
+   bounds access (but didn't because of backwards compatibility reasons).  */
+#define TYPE_BLKMODE_FLAG(NODE) (TYPE_CHECK (NODE)->type_common.blkmode_flag)
+#define TYPE_NO_FORCE_BLKMODE(NODE) \
+  (TYPE_MODE (TYPE_CHECK (NODE)) == BLKmode && TYPE_BLKMODE_FLAG (NODE))
+#define SET_TYPE_NO_FORCE_BLKMODE(NODE, X) (TYPE_BLKMODE_FLAG (NODE) = (X))
+#define TYPE_BLKMODE_FOR_ACCESS(NODE) \
+  (TYPE_MODE (TYPE_CHECK (NODE)) != BLKmode && TYPE_BLKMODE_FLAG (NODE))
+#define SET_TYPE_BLKMODE_FOR_ACCESS(NODE, X) (TYPE_BLKMODE_FLAG (NODE) = (X))
 
 /* Nonzero in a type considered volatile as a whole.  */
 #define TYPE_VOLATILE(NODE) (TYPE_CHECK (NODE)->base.volatile_flag)
Index: lto-streamer-out.c
===================================================================
--- lto-streamer-out.c	(revision 206105)
+++ lto-streamer-out.c	(working copy)
@@ -868,7 +868,7 @@ hash_tree (struct streamer_tree_cache_d
     {
       v = iterative_hash_host_wide_int (TYPE_MODE (t), v);
       v = iterative_hash_host_wide_int (TYPE_STRING_FLAG (t)
-					| (TYPE_NO_FORCE_BLK (t) << 1)
+					| (TYPE_BLKMODE_FLAG (t) << 1)
 					| (TYPE_NEEDS_CONSTRUCTING (t) << 2)
 					| (TYPE_PACKED (t) << 3)
 					| (TYPE_RESTRICT (t) << 4)
Index: expr.c
===================================================================
--- expr.c	(revision 206105)
+++ expr.c	(working copy)
@@ -9656,6 +9656,7 @@ expand_expr_real_1 (tree exp, rtx target
 	if (modifier != EXPAND_WRITE
 	    && modifier != EXPAND_MEMORY
 	    && mode != BLKmode
+	    && !TYPE_BLKMODE_FOR_ACCESS (type)
 	    && align < GET_MODE_ALIGNMENT (mode)
 	    /* If the target does not have special handling for unaligned
 	       loads of mode then it can use regular moves for them.  */
@@ -9736,6 +9737,7 @@ expand_expr_real_1 (tree exp, rtx target
 	if (modifier != EXPAND_WRITE
 	    && modifier != EXPAND_MEMORY
 	    && mode != BLKmode
+	    && !TYPE_BLKMODE_FOR_ACCESS (type)
 	    && align < GET_MODE_ALIGNMENT (mode))
 	  {
 	    if ((icode = optab_handler (movmisalign_optab, mode))
@@ -10426,50 +10428,53 @@ expand_expr_real_1 (tree exp, rtx target
 	      op0 = copy_rtx (op0);
 	      set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
 	    }
-	  else if (mode != BLKmode
-		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)
-		   /* If the target does have special handling for unaligned
-		      loads of mode then use them.  */
-		   && ((icode = optab_handler (movmisalign_optab, mode))
-		       != CODE_FOR_nothing))
-	    {
-	      rtx reg, insn;
-
-	      op0 = adjust_address (op0, mode, 0);
-	      /* We've already validated the memory, and we're creating a
-		 new pseudo destination.  The predicates really can't
-		 fail.  */
-	      reg = gen_reg_rtx (mode);
-
-	      /* Nor can the insn generator.  */
-	      insn = GEN_FCN (icode) (reg, op0);
-	      emit_insn (insn);
-	      return reg;
-	    }
-	  else if (STRICT_ALIGNMENT
+	  else if (modifier != EXPAND_WRITE
+		   && modifier != EXPAND_MEMORY
 		   && mode != BLKmode
+		   && !TYPE_BLKMODE_FOR_ACCESS (type)
 		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode))
 	    {
-	      tree inner_type = TREE_TYPE (treeop0);
-	      HOST_WIDE_INT temp_size
-		= MAX (int_size_in_bytes (inner_type),
-		       (HOST_WIDE_INT) GET_MODE_SIZE (mode));
-	      rtx new_rtx
-		= assign_stack_temp_for_type (mode, temp_size, type);
-	      rtx new_with_op0_mode
-		= adjust_address (new_rtx, GET_MODE (op0), 0);
-
-	      gcc_assert (!TREE_ADDRESSABLE (exp));
-
-	      if (GET_MODE (op0) == BLKmode)
-		emit_block_move (new_with_op0_mode, op0,
-				 GEN_INT (GET_MODE_SIZE (mode)),
-				 (modifier == EXPAND_STACK_PARM
-				  ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
-	      else
-		emit_move_insn (new_with_op0_mode, op0);
+	      /* If the target does have special handling for unaligned
+		 loads of mode then use them.  */
+	      if ((icode = optab_handler (movmisalign_optab, mode))
+		  != CODE_FOR_nothing)
+		{
+		  rtx reg, insn;
 
-	      op0 = new_rtx;
+		  op0 = adjust_address (op0, mode, 0);
+		  /* We've already validated the memory, and we're creating a
+		     new pseudo destination.  The predicates really can't
+		     fail.  */
+		  reg = gen_reg_rtx (mode);
+
+		  /* Nor can the insn generator.  */
+		  insn = GEN_FCN (icode) (reg, op0);
+		  emit_insn (insn);
+		  return reg;
+		}
+	      else if (STRICT_ALIGNMENT)
+		{
+		  tree inner_type = TREE_TYPE (treeop0);
+		  HOST_WIDE_INT temp_size
+		    = MAX (int_size_in_bytes (inner_type),
+			   (HOST_WIDE_INT) GET_MODE_SIZE (mode));
+		  rtx new_rtx
+		    = assign_stack_temp_for_type (mode, temp_size, type);
+		  rtx new_with_op0_mode
+		    = adjust_address (new_rtx, GET_MODE (op0), 0);
+
+		  gcc_assert (!TREE_ADDRESSABLE (exp));
+
+		  if (GET_MODE (op0) == BLKmode)
+		    emit_block_move (new_with_op0_mode, op0,
+				     GEN_INT (GET_MODE_SIZE (mode)),
+				     (modifier == EXPAND_STACK_PARM
+				      ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
+		  else
+		    emit_move_insn (new_with_op0_mode, op0);
+
+		  op0 = new_rtx;
+		}
 	    }
 
 	  op0 = adjust_address (op0, mode, 0);
Index: expr.h
===================================================================
--- expr.h	(revision 206105)
+++ expr.h	(working copy)
@@ -41,7 +41,8 @@ along with GCC; see the file COPYING3.
     is a constant that is not a legitimate address.
    EXPAND_WRITE means we are only going to write to the resulting rtx.
    EXPAND_MEMORY means we are interested in a memory result, even if
-    the memory is constant and we could have propagated a constant value.  */
+    the memory is constant and we could have propagated a constant value,
+    or the memory is unaligned on a STRICT_ALIGNMENT target.  */
 enum expand_modifier {EXPAND_NORMAL = 0, EXPAND_STACK_PARM, EXPAND_SUM,
 		      EXPAND_CONST_ADDRESS, EXPAND_INITIALIZER, EXPAND_WRITE,
 		      EXPAND_MEMORY};
Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 206105)
+++ stor-layout.c	(working copy)
@@ -1575,8 +1575,9 @@ finalize_record_size (record_layout_info
 void
 compute_record_mode (tree type)
 {
-  tree field;
   enum machine_mode mode = VOIDmode;
+  bool blkmode_for_access = false;
+  tree field;
 
   /* Most RECORD_TYPEs have BLKmode, so we start off assuming that.
      However, if possible, we use a mode that fits in a register
@@ -1597,9 +1598,7 @@ compute_record_mode (tree type)
 
       if (TREE_CODE (TREE_TYPE (field)) == ERROR_MARK
 	  || (TYPE_MODE (TREE_TYPE (field)) == BLKmode
-	      && ! TYPE_NO_FORCE_BLK (TREE_TYPE (field))
-	      && !(TYPE_SIZE (TREE_TYPE (field)) != 0
-		   && integer_zerop (TYPE_SIZE (TREE_TYPE (field)))))
+	      && ! TYPE_NO_FORCE_BLKMODE (TREE_TYPE (field)))
 	  || ! tree_fits_uhwi_p (bit_position (field))
 	  || DECL_SIZE (field) == 0
 	  || ! tree_fits_uhwi_p (DECL_SIZE (field)))
@@ -1615,6 +1614,15 @@ compute_record_mode (tree type)
 	 BLKmode structure as a scalar.  */
       if (targetm.member_type_forces_blk (field, mode))
 	return;
+
+      /* As an extension, we support out-of-bounds access for trailing arrays.
+	 In this case, if the element type has non-zero size, then the record
+	 type effectively has variable size so it needs to have BLKmode.  */
+      blkmode_for_access
+	= (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE
+	   && !integer_zerop (TYPE_SIZE (TREE_TYPE (TREE_TYPE (field)))))
+	  || (RECORD_OR_UNION_TYPE_P (TREE_TYPE (field))
+	      && TYPE_BLKMODE_FOR_ACCESS (TREE_TYPE (field)));
     }
 
   /* If we only have one real field; use its mode if that mode's size
@@ -1636,9 +1644,14 @@ compute_record_mode (tree type)
     {
       /* If this is the only reason this type is BLKmode, then
 	 don't force containing types to be BLKmode.  */
-      TYPE_NO_FORCE_BLK (type) = 1;
+      SET_TYPE_NO_FORCE_BLKMODE (type, 1);
       SET_TYPE_MODE (type, BLKmode);
     }
+
+  /* If the record type needs BLKmode for access but didn't get it through
+     the standard layout algorithm, record it for later.  */
+  if (TYPE_MODE (type) != BLKmode && blkmode_for_access)
+    SET_TYPE_BLKMODE_FOR_ACCESS (type, 1);
 }
 
 /* Compute TYPE_SIZE and TYPE_ALIGN for TYPE, once it has been laid
@@ -2245,7 +2258,7 @@ layout_type (tree type)
 	    /* BLKmode elements force BLKmode aggregate;
 	       else extract/store fields may lose.  */
 	    && (TYPE_MODE (TREE_TYPE (type)) != BLKmode
-		|| TYPE_NO_FORCE_BLK (TREE_TYPE (type))))
+		|| TYPE_NO_FORCE_BLKMODE (TREE_TYPE (type))))
 	  {
 	    SET_TYPE_MODE (type, mode_for_array (TREE_TYPE (type),
 						 TYPE_SIZE (type)));
@@ -2253,7 +2266,7 @@ layout_type (tree type)
 		&& STRICT_ALIGNMENT && TYPE_ALIGN (type) < BIGGEST_ALIGNMENT
 		&& TYPE_ALIGN (type) < GET_MODE_ALIGNMENT (TYPE_MODE (type)))
 	      {
-		TYPE_NO_FORCE_BLK (type) = 1;
+		SET_TYPE_NO_FORCE_BLKMODE (type, 1);
 		SET_TYPE_MODE (type, BLKmode);
 	      }
 	  }
Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 206105)
+++ cfgexpand.c	(working copy)
@@ -2253,7 +2253,7 @@ expand_call_stmt (gimple stmt)
   if (lhs)
     expand_assignment (lhs, exp, false);
   else
-    expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL);
+    expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);
 
   mark_transaction_restart_calls (stmt);
 }
Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 206105)
+++ lto/lto.c	(working copy)
@@ -1349,7 +1349,7 @@ compare_tree_sccs_1 (tree t1, tree t2, t
     {
       compare_values (TYPE_MODE);
       compare_values (TYPE_STRING_FLAG);
-      compare_values (TYPE_NO_FORCE_BLK);
+      compare_values (TYPE_BLKMODE_FLAG);
       compare_values (TYPE_NEEDS_CONSTRUCTING);
       if (RECORD_OR_UNION_TYPE_P (t1))
 	{
Index: tree-streamer-out.c
===================================================================
--- tree-streamer-out.c	(revision 206105)
+++ tree-streamer-out.c	(working copy)
@@ -302,7 +302,7 @@ pack_ts_type_common_value_fields (struct
 {
   bp_pack_enum (bp, machine_mode, MAX_MACHINE_MODE, TYPE_MODE (expr));
   bp_pack_value (bp, TYPE_STRING_FLAG (expr), 1);
-  bp_pack_value (bp, TYPE_NO_FORCE_BLK (expr), 1);
+  bp_pack_value (bp, TYPE_BLKMODE_FLAG (expr), 1);
   bp_pack_value (bp, TYPE_NEEDS_CONSTRUCTING (expr), 1);
   if (RECORD_OR_UNION_TYPE_P (expr))
     {
Index: print-tree.c
===================================================================
--- print-tree.c	(revision 206107)
+++ print-tree.c	(working copy)
@@ -583,8 +583,10 @@ print_node (FILE *file, const char *pref
       if (TYPE_UNSIGNED (node))
 	fputs (" unsigned", file);
 
-      if (TYPE_NO_FORCE_BLK (node))
-	fputs (" no-force-blk", file);
+      if (TYPE_NO_FORCE_BLKMODE (node))
+	fputs (" no-force-blkmode", file);
+      else if (TYPE_BLKMODE_FOR_ACCESS (node))
+	fputs (" blkmode-for-access", file);
 
       if (TYPE_STRING_FLAG (node))
 	fputs (" string-flag", file);
Index: tree-streamer-in.c
===================================================================
--- tree-streamer-in.c	(revision 206105)
+++ tree-streamer-in.c	(working copy)
@@ -352,7 +352,7 @@ unpack_ts_type_common_value_fields (stru
   mode = bp_unpack_enum (bp, machine_mode, MAX_MACHINE_MODE);
   SET_TYPE_MODE (expr, mode);
   TYPE_STRING_FLAG (expr) = (unsigned) bp_unpack_value (bp, 1);
-  TYPE_NO_FORCE_BLK (expr) = (unsigned) bp_unpack_value (bp, 1);
+  TYPE_BLKMODE_FLAG (expr) = (unsigned) bp_unpack_value (bp, 1);
   TYPE_NEEDS_CONSTRUCTING (expr) = (unsigned) bp_unpack_value (bp, 1);
   if (RECORD_OR_UNION_TYPE_P (expr))
     {
Index: tree-core.h
===================================================================
--- tree-core.h	(revision 206105)
+++ tree-core.h	(working copy)
@@ -1239,7 +1239,7 @@ struct GTY(()) tree_type_common {
   unsigned int uid;
 
   unsigned int precision : 10;
-  unsigned no_force_blk_flag : 1;
+  unsigned blkmode_flag : 1;
   unsigned needs_constructing_flag : 1;
   unsigned transparent_aggr_flag : 1;
   unsigned restrict_flag : 1;

RE: [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

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

Hi,


On Thu, 19 Dec 2013 11:55:03, Eric Botcazou wrote:
>
>> In general I like the comment, and I am open for other suggestions how
>> to call the parameter.
>
> I think that using EXPAND in the parameter's name is confusing because it
> needs to be distinguished from MODIFIER and its enumeration type. And since
> it would be true only after calling get_inner_reference, it would be better to
> have the "inner" as well. Maybe inner_reference_p or something equivalent.
>

Ok, thanks, you are right.
I like this name too, and I think it will be generally acceptable.

Attached is a new version of my patch with
s/expand_reference/inner_reference_p/ and your comment added.

Richard, I personally, would prefer this patch over proposal #1,
because it is a smaller change in the end.


OK for trunk?



Bernd. 		 	   		  

[-- Attachment #2: changelog-pr57748-2.txt --]
[-- Type: text/plain, Size: 582 bytes --]

2013-12-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/57748
	* expr.h (expand_expr_real, expand_expr_real_1): Add new parameter
	inner_reference_p.
	(expand_expr, expand_normal): Adjust.
	* expr.c (expand_expr_real, expand_expr_real_1): Add new parameter
	inner_reference_p. Use inner_reference_p to expand inner references.
	(store_expr): Adjust.
	* cfgexpand.c (expand_call_stmt): Adjust.

testsuite:
2013-12-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/57748
	* gcc.dg/torture/pr57748-3.c: New test.
	* gcc.dg/torture/pr57748-4.c: New test.


[-- Attachment #3: patch-pr57748-2.diff --]
[-- Type: application/octet-stream, Size: 13094 bytes --]

Index: gcc/testsuite/gcc.dg/torture/pr57748-4.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr57748-4.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57748-4.c	(revision 0)
@@ -0,0 +1,40 @@
+/* PR middle-end/57748 */
+/* { dg-do run } */
+/* wrong code in expand_expr_real_1.  */
+
+#include <stdlib.h>
+
+extern void abort (void);
+
+typedef long long V
+  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
+
+typedef struct S { V b[1]; } P __attribute__((aligned (1)));
+
+struct __attribute__((packed)) T { char c; P s; };
+
+void __attribute__((noinline, noclone))
+check (P *p)
+{
+  if (p->b[1][0] != 3 || p->b[1][1] != 4)
+    abort ();
+}
+
+void __attribute__((noinline, noclone))
+foo (struct T *t)
+{
+  V a = { 3, 4 };
+  t->s.b[1] = a;
+}
+
+int
+main ()
+{
+  struct T *t = (struct T *) calloc (128, 1);
+
+  foo (t);
+  check (&t->s);
+
+  free (t);
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/torture/pr57748-3.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr57748-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr57748-3.c	(revision 0)
@@ -0,0 +1,40 @@
+/* PR middle-end/57748 */
+/* { dg-do run } */
+/* wrong code in expand_expr_real_1.  */
+
+#include <stdlib.h>
+
+extern void abort (void);
+
+typedef long long V
+  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
+
+typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
+
+struct __attribute__((packed)) T { char c; P s; };
+
+void __attribute__((noinline, noclone))
+check (P *p)
+{
+  if (p->b[0][0] != 3 || p->b[0][1] != 4)
+    abort ();
+}
+
+void __attribute__((noinline, noclone))
+foo (struct T *t)
+{
+  V a = { 3, 4 };
+  t->s.b[0] = a;
+}
+
+int
+main ()
+{
+  struct T *t = (struct T *) calloc (128, 1);
+
+  foo (t);
+  check (&t->s);
+
+  free (t);
+  return 0;
+}
Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 205641)
+++ gcc/expr.c	(working copy)
@@ -5325,7 +5325,7 @@ store_expr (tree exp, rtx target, int ca
       temp = expand_expr_real (exp, tmp_target, GET_MODE (target),
 			       (call_param_p
 				? EXPAND_STACK_PARM : EXPAND_NORMAL),
-			       &alt_rtl);
+			       &alt_rtl, false);
     }
 
   /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not
@@ -7911,11 +7911,21 @@ expand_constructor (tree exp, rtx target
    address, and ALT_RTL is non-NULL, then *ALT_RTL is set to the
    DECL_RTL of the VAR_DECL.  *ALT_RTL is also set if EXP is a
    COMPOUND_EXPR whose second argument is such a VAR_DECL, and so on
-   recursively.  */
+   recursively.
+
+   If INNER_REFERENCE_P is true, we are expanding an inner reference.
+   In this case, we don't adjust a returned MEM rtx that wouldn't be
+   sufficiently aligned for its mode; instead, it's up to the caller
+   to deal with it afterwards.  This is used to make sure that unaligned
+   base objects for which out-of-bounds accesses are supported, for
+   example record types with trailing arrays, aren't realigned behind
+   the back of the caller.
+   The normal operating mode is to pass FALSE for this parameter.  */
 
 rtx
 expand_expr_real (tree exp, rtx target, enum machine_mode tmode,
-		  enum expand_modifier modifier, rtx *alt_rtl)
+		  enum expand_modifier modifier, rtx *alt_rtl,
+		  bool inner_reference_p)
 {
   rtx ret;
 
@@ -7927,7 +7937,8 @@ expand_expr_real (tree exp, rtx target,
       return ret ? ret : const0_rtx;
     }
 
-  ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl);
+  ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl,
+			    inner_reference_p);
   return ret;
 }
 
@@ -9232,7 +9243,8 @@ stmt_is_replaceable_p (gimple stmt)
 
 rtx
 expand_expr_real_1 (tree exp, rtx target, enum machine_mode tmode,
-		    enum expand_modifier modifier, rtx *alt_rtl)
+		    enum expand_modifier modifier, rtx *alt_rtl,
+		    bool inner_reference_p)
 {
   rtx op0, op1, temp, decl_rtl;
   tree type;
@@ -9378,7 +9390,7 @@ expand_expr_real_1 (tree exp, rtx target
 
 	  set_curr_insn_location (gimple_location (g));
 	  r = expand_expr_real (gimple_assign_rhs_to_tree (g), target,
-				tmode, modifier, NULL);
+				tmode, modifier, NULL, inner_reference_p);
 	  set_curr_insn_location (saved_loc);
 	  if (REG_P (r) && !REG_EXPR (r))
 	    set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (exp), r);
@@ -9597,7 +9609,8 @@ expand_expr_real_1 (tree exp, rtx target
     case SAVE_EXPR:
       {
 	tree val = treeop0;
-	rtx ret = expand_expr_real_1 (val, target, tmode, modifier, alt_rtl);
+	rtx ret = expand_expr_real_1 (val, target, tmode, modifier, alt_rtl,
+				      inner_reference_p);
 
 	if (!SAVE_EXPR_RESOLVED_P (exp))
 	  {
@@ -9735,6 +9748,7 @@ expand_expr_real_1 (tree exp, rtx target
 	  MEM_VOLATILE_P (temp) = 1;
 	if (modifier != EXPAND_WRITE
 	    && modifier != EXPAND_MEMORY
+	    && !inner_reference_p
 	    && mode != BLKmode
 	    && align < GET_MODE_ALIGNMENT (mode))
 	  {
@@ -9960,15 +9974,16 @@ expand_expr_real_1 (tree exp, rtx target
 	   computation, since it will need a temporary and TARGET is known
 	   to have to do.  This occurs in unchecked conversion in Ada.  */
 	orig_op0 = op0
-	  = expand_expr (tem,
-			 (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
-			  && COMPLETE_TYPE_P (TREE_TYPE (tem))
-			  && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
-			      != INTEGER_CST)
-			  && modifier != EXPAND_STACK_PARM
-			  ? target : NULL_RTX),
-			 VOIDmode,
-			 modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+	  = expand_expr_real (tem,
+			      (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
+			       && COMPLETE_TYPE_P (TREE_TYPE (tem))
+			       && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
+				   != INTEGER_CST)
+			       && modifier != EXPAND_STACK_PARM
+			       ? target : NULL_RTX),
+			      VOIDmode,
+			      modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
+			      NULL, true);
 
 	/* If the field has a mode, we want to access it in the
 	   field's mode, not the computed mode.
@@ -10325,14 +10340,15 @@ expand_expr_real_1 (tree exp, rtx target
 	  {
 	    /* See the normal_inner_ref case for the rationale.  */
 	    orig_op0
-	      = expand_expr (tem,
-			     (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
-			      && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
-				  != INTEGER_CST)
-			      && modifier != EXPAND_STACK_PARM
-			      ? target : NULL_RTX),
-			     VOIDmode,
-			     modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+	      = expand_expr_real (tem,
+				  (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
+				   && (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
+				       != INTEGER_CST)
+				   && modifier != EXPAND_STACK_PARM
+				   ? target : NULL_RTX),
+				  VOIDmode,
+				  modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
+				  NULL, true);
 
 	    if (MEM_P (orig_op0))
 	      {
@@ -10359,7 +10375,8 @@ expand_expr_real_1 (tree exp, rtx target
       }
 
       if (!op0)
-	op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
+	op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
+				NULL, inner_reference_p);
 
       /* If the input and output modes are both the same, we are done.  */
       if (mode == GET_MODE (op0))
@@ -10426,50 +10443,53 @@ expand_expr_real_1 (tree exp, rtx target
 	      op0 = copy_rtx (op0);
 	      set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
 	    }
-	  else if (mode != BLKmode
-		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)
-		   /* If the target does have special handling for unaligned
-		      loads of mode then use them.  */
-		   && ((icode = optab_handler (movmisalign_optab, mode))
-		       != CODE_FOR_nothing))
-	    {
-	      rtx reg, insn;
-
-	      op0 = adjust_address (op0, mode, 0);
-	      /* We've already validated the memory, and we're creating a
-		 new pseudo destination.  The predicates really can't
-		 fail.  */
-	      reg = gen_reg_rtx (mode);
-
-	      /* Nor can the insn generator.  */
-	      insn = GEN_FCN (icode) (reg, op0);
-	      emit_insn (insn);
-	      return reg;
-	    }
-	  else if (STRICT_ALIGNMENT
+	  else if (modifier != EXPAND_WRITE
+		   && modifier != EXPAND_MEMORY
+		   && !inner_reference_p
 		   && mode != BLKmode
 		   && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode))
 	    {
-	      tree inner_type = TREE_TYPE (treeop0);
-	      HOST_WIDE_INT temp_size
-		= MAX (int_size_in_bytes (inner_type),
-		       (HOST_WIDE_INT) GET_MODE_SIZE (mode));
-	      rtx new_rtx
-		= assign_stack_temp_for_type (mode, temp_size, type);
-	      rtx new_with_op0_mode
-		= adjust_address (new_rtx, GET_MODE (op0), 0);
-
-	      gcc_assert (!TREE_ADDRESSABLE (exp));
-
-	      if (GET_MODE (op0) == BLKmode)
-		emit_block_move (new_with_op0_mode, op0,
-				 GEN_INT (GET_MODE_SIZE (mode)),
-				 (modifier == EXPAND_STACK_PARM
-				  ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
-	      else
-		emit_move_insn (new_with_op0_mode, op0);
+	      /* If the target does have special handling for unaligned
+		 loads of mode then use them.  */
+	      if ((icode = optab_handler (movmisalign_optab, mode))
+		  != CODE_FOR_nothing)
+		{
+		  rtx reg, insn;
 
-	      op0 = new_rtx;
+		  op0 = adjust_address (op0, mode, 0);
+		  /* We've already validated the memory, and we're creating a
+		     new pseudo destination.  The predicates really can't
+		     fail.  */
+		  reg = gen_reg_rtx (mode);
+
+		  /* Nor can the insn generator.  */
+		  insn = GEN_FCN (icode) (reg, op0);
+		  emit_insn (insn);
+		  return reg;
+		}
+	      else if (STRICT_ALIGNMENT)
+		{
+		  tree inner_type = TREE_TYPE (treeop0);
+		  HOST_WIDE_INT temp_size
+		    = MAX (int_size_in_bytes (inner_type),
+			   (HOST_WIDE_INT) GET_MODE_SIZE (mode));
+		  rtx new_rtx
+		    = assign_stack_temp_for_type (mode, temp_size, type);
+		  rtx new_with_op0_mode
+		    = adjust_address (new_rtx, GET_MODE (op0), 0);
+
+		  gcc_assert (!TREE_ADDRESSABLE (exp));
+
+		  if (GET_MODE (op0) == BLKmode)
+		    emit_block_move (new_with_op0_mode, op0,
+				     GEN_INT (GET_MODE_SIZE (mode)),
+				     (modifier == EXPAND_STACK_PARM
+				      ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL));
+		  else
+		    emit_move_insn (new_with_op0_mode, op0);
+
+		  op0 = new_rtx;
+		}
 	    }
 
 	  op0 = adjust_address (op0, mode, 0);
@@ -10569,7 +10589,7 @@ expand_expr_real_1 (tree exp, rtx target
       /* WITH_SIZE_EXPR expands to its first argument.  The caller should
 	 have pulled out the size to use in whatever context it needed.  */
       return expand_expr_real (treeop0, original_target, tmode,
-			       modifier, alt_rtl);
+			       modifier, alt_rtl, inner_reference_p);
 
     default:
       return expand_expr_real_2 (&ops, target, tmode, modifier);
Index: gcc/expr.h
===================================================================
--- gcc/expr.h	(revision 205641)
+++ gcc/expr.h	(working copy)
@@ -41,7 +41,8 @@ along with GCC; see the file COPYING3.  If not see
     is a constant that is not a legitimate address.
    EXPAND_WRITE means we are only going to write to the resulting rtx.
    EXPAND_MEMORY means we are interested in a memory result, even if
-    the memory is constant and we could have propagated a constant value.  */
+    the memory is constant and we could have propagated a constant value,
+    or the memory is unaligned on a STRICT_ALIGNMENT target.  */
 enum expand_modifier {EXPAND_NORMAL = 0, EXPAND_STACK_PARM, EXPAND_SUM,
 		      EXPAND_CONST_ADDRESS, EXPAND_INITIALIZER, EXPAND_WRITE,
 		      EXPAND_MEMORY};
@@ -437,9 +438,9 @@ extern rtx force_operand (rtx, rtx);
 
 /* Work horses for expand_expr.  */
 extern rtx expand_expr_real (tree, rtx, enum machine_mode,
-			     enum expand_modifier, rtx *);
+			     enum expand_modifier, rtx *, bool);
 extern rtx expand_expr_real_1 (tree, rtx, enum machine_mode,
-			       enum expand_modifier, rtx *);
+			       enum expand_modifier, rtx *, bool);
 extern rtx expand_expr_real_2 (sepops, rtx, enum machine_mode,
 			       enum expand_modifier);
 
@@ -450,13 +451,13 @@ static inline rtx
 expand_expr (tree exp, rtx target, enum machine_mode mode,
 	     enum expand_modifier modifier)
 {
-  return expand_expr_real (exp, target, mode, modifier, NULL);
+  return expand_expr_real (exp, target, mode, modifier, NULL, false);
 }
 
 static inline rtx
 expand_normal (tree exp)
 {
-  return expand_expr_real (exp, NULL_RTX, VOIDmode, EXPAND_NORMAL, NULL);
+  return expand_expr_real (exp, NULL_RTX, VOIDmode, EXPAND_NORMAL, NULL, false);
 }
 
 /* At the start of a function, record that we have no previously-pushed
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 205641)
+++ gcc/cfgexpand.c	(working copy)
@@ -2253,7 +2253,7 @@ expand_call_stmt (gimple stmt)
   if (lhs)
     expand_assignment (lhs, exp, false);
   else
-    expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL);
+    expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);
 
   mark_transaction_restart_calls (stmt);
 }

[PING] [REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

Hello,

Ping...

We still need a decision how to fix this.

There are two alternative patches:

1. My latest proposal: http://gcc.gnu.org/ml/gcc-patches/2013-12/msg01675.html

2. Eric's latest proposal: http://gcc.gnu.org/ml/gcc-patches/2013-12/msg01667.html


Thanks
Bernd. 		 	   		  

Re: [PING] [REPOST] Invalid Code when reading from unaligned zero-sized array

[Richard Biener]

On Tue, Jan 7, 2014 at 5:31 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hello,
>
> Ping...
>
> We still need a decision how to fix this.
>
> There are two alternative patches:
>
> 1. My latest proposal: http://gcc.gnu.org/ml/gcc-patches/2013-12/msg01675.html
>
> 2. Eric's latest proposal: http://gcc.gnu.org/ml/gcc-patches/2013-12/msg01667.html

Let's go with 1., your patch adjusting how we recurse in expand.  That
seems safer to eventually backport.

For 4.10 we should re-visit this and fix all the backends for those ABI
issues with modes ...

Richard.

>
> Thanks
> Bernd.