diff mbox

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

Message ID 3651709.ldS2nsTtfa@polaris
State New
Headers show

Commit Message

Eric Botcazou Dec. 19, 2013, 10:55 a.m. UTC 
> 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.

Comments

Bernd Edlinger Dec. 19, 2013, 12:52 p.m. UTC | #1 
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.
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.
Bernd Edlinger Jan. 7, 2014, 4:31 p.m. UTC | #2 
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.
Richard Biener Jan. 8, 2014, 11:23 a.m. UTC | #3 
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.
diff mbox

Patch

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;