diff mbox

Fix PR middle-end/59138

Message ID 6624042.yQSHnR5AAP@polaris
State New
Headers show

Commit Message

Eric Botcazou Nov. 22, 2013, 7:44 p.m. UTC 
Hi,

this is a regression present on the mainline and 4.8 branch.  emit_group_store 
happily writes past the end of a packed structure, thus accessing a distinct 
variable stored there.  Then the scheduler swaps a couple of writes, leading 
to wrong code.  Fixed by preventing emit_group_store from writing past the end 
of the structure.

Tested on x86_64-suse-linux, OK for the mainline and 4.8 branch?

2013-11-22  Eric Botcazou  <ebotcazou@adacore.com>

	PR middle-end/59138
	* expr.c (emit_group_store): Do not write past the end of the structure.
	(store_bit_field): Fix formatting.


2013-11-22  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.c-torture/execute/20131122-1.c: New test.

Comments

Jeff Law Nov. 22, 2013, 9:24 p.m. UTC | #1 
On 11/22/13 12:44, Eric Botcazou wrote:
> Hi,
>
> this is a regression present on the mainline and 4.8 branch.  emit_group_store
> happily writes past the end of a packed structure, thus accessing a distinct
> variable stored there.  Then the scheduler swaps a couple of writes, leading
> to wrong code.  Fixed by preventing emit_group_store from writing past the end
> of the structure.
>
> Tested on x86_64-suse-linux, OK for the mainline and 4.8 branch?
>
> 2013-11-22  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	PR middle-end/59138
> 	* expr.c (emit_group_store): Do not write past the end of the structure.
> 	(store_bit_field): Fix formatting.
>
>
> 2013-11-22  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* gcc.c-torture/execute/20131122-1.c: New test.
It looks like this patch was for gcc-4.8; the code has changed a little 
since then.

I'm having a hard time seeing why this change was made:

        /* Optimize the access just a bit.  */
-      if (MEM_P (dest)
-	  && (! SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
-	      || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
-	  && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
-	  && bytelen == GET_MODE_SIZE (mode))
+      else if (MEM_P (dest)
+	       && (!SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
+		   || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
+	       && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
+	       && bytelen == GET_MODE_SIZE (mode))
  	emit_move_insn (adjust_address (dest, mode, bytepos), tmps[i]);

But that may be an artifact of looking at the trunk where the code is a 
bit different already.

Presumably you've verified this is still a problem on the trunk?

jeff
Eric Botcazou Nov. 22, 2013, 10:24 p.m. UTC | #2 
> It looks like this patch was for gcc-4.8; the code has changed a little
> since then.

Well, the baseline is:

eric@polaris:~/build/gcc/native> gcc/xgcc -v
Using built-in specs.
COLLECT_GCC=gcc/xgcc
Target: x86_64-suse-linux
Configured with: /home/eric/svn/gcc/configure --build=x86_64-suse-linux --
prefix=/home/eric/install/gcc --enable-languages=c,c++,objc,obj-
c++,java,fortran,ada --enable-checking=yes,rtl --enable-__cxa_atexit --
disable-nls
Thread model: posix
gcc version 4.9.0 20131122 (experimental) [trunk revision 205244] (GCC)

> I'm having a hard time seeing why this change was made:
> 
>         /* Optimize the access just a bit.  */
> -      if (MEM_P (dest)
> -	  && (! SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
> -	      || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
> -	  && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
> -	  && bytelen == GET_MODE_SIZE (mode))
> +      else if (MEM_P (dest)
> +	       && (!SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
> +		   || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
> +	       && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
> +	       && bytelen == GET_MODE_SIZE (mode))
>   	emit_move_insn (adjust_address (dest, mode, bytepos), tmps[i]);

Because we're adding a call to store_bit_field and don't want to store twice.
Jeff Law Nov. 27, 2013, 6:13 a.m. UTC | #3 
On 11/22/13 15:24, Eric Botcazou wrote:
> Well, the baseline is:
>
> eric@polaris:~/build/gcc/native> gcc/xgcc -v
> Using built-in specs.
> COLLECT_GCC=gcc/xgcc
> Target: x86_64-suse-linux
> Configured with: /home/eric/svn/gcc/configure --build=x86_64-suse-linux --
> prefix=/home/eric/install/gcc --enable-languages=c,c++,objc,obj-
> c++,java,fortran,ada --enable-checking=yes,rtl --enable-__cxa_atexit --
> disable-nls
> Thread model: posix
> gcc version 4.9.0 20131122 (experimental) [trunk revision 205244] (GCC)
Ha.  Now I see it.  At some point I must have moved from looking at 
emit_group_store to emit_group_load_1 which has the same comment 
"Optimize the access just a bit."  Confused the hell out of me.



>
>> I'm having a hard time seeing why this change was made:
>>
>>          /* Optimize the access just a bit.  */
>> -      if (MEM_P (dest)
>> -	  && (! SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
>> -	      || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
>> -	  && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
>> -	  && bytelen == GET_MODE_SIZE (mode))
>> +      else if (MEM_P (dest)
>> +	       && (!SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
>> +		   || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
>> +	       && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
>> +	       && bytelen == GET_MODE_SIZE (mode))
>>    	emit_move_insn (adjust_address (dest, mode, bytepos), tmps[i]);
>
> Because we're adding a call to store_bit_field and don't want to store twice.
Yea, that's pretty obvious when looking at the right code :-)

Good to go.  Sorry for the confusion and delay.

jeff
diff mbox

Patch

Index: expr.c
===================================================================
--- expr.c	(revision 205244)
+++ expr.c	(working copy)
@@ -2056,12 +2056,14 @@  emit_group_store (rtx orig_dst, rtx src,
       HOST_WIDE_INT bytepos = INTVAL (XEXP (XVECEXP (src, 0, i), 1));
       enum machine_mode mode = GET_MODE (tmps[i]);
       unsigned int bytelen = GET_MODE_SIZE (mode);
-      unsigned int adj_bytelen = bytelen;
+      unsigned int adj_bytelen;
       rtx dest = dst;
 
       /* Handle trailing fragments that run over the size of the struct.  */
       if (ssize >= 0 && bytepos + (HOST_WIDE_INT) bytelen > ssize)
 	adj_bytelen = ssize - bytepos;
+      else
+	adj_bytelen = bytelen;
 
       if (GET_CODE (dst) == CONCAT)
 	{
@@ -2102,6 +2104,7 @@  emit_group_store (rtx orig_dst, rtx src,
 	    }
 	}
 
+      /* Handle trailing fragments that run over the size of the struct.  */
       if (ssize >= 0 && bytepos + (HOST_WIDE_INT) bytelen > ssize)
 	{
 	  /* store_bit_field always takes its value from the lsb.
@@ -2119,16 +2122,22 @@  emit_group_store (rtx orig_dst, rtx src,
 	      tmps[i] = expand_shift (RSHIFT_EXPR, mode, tmps[i],
 				      shift, tmps[i], 0);
 	    }
-	  bytelen = adj_bytelen;
+
+	  /* Make sure not to write past the end of the struct.  */
+	  store_bit_field (dest,
+			   adj_bytelen * BITS_PER_UNIT, bytepos * BITS_PER_UNIT,
+			   bytepos * BITS_PER_UNIT, ssize * BITS_PER_UNIT,
+			   VOIDmode, tmps[i]);
 	}
 
       /* Optimize the access just a bit.  */
-      if (MEM_P (dest)
-	  && (! SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
-	      || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
-	  && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
-	  && bytelen == GET_MODE_SIZE (mode))
+      else if (MEM_P (dest)
+	       && (!SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (dest))
+		   || MEM_ALIGN (dest) >= GET_MODE_ALIGNMENT (mode))
+	       && bytepos * BITS_PER_UNIT % GET_MODE_ALIGNMENT (mode) == 0
+	       && bytelen == GET_MODE_SIZE (mode))
 	emit_move_insn (adjust_address (dest, mode, bytepos), tmps[i]);
+
       else
 	store_bit_field (dest, bytelen * BITS_PER_UNIT, bytepos * BITS_PER_UNIT,
 			 0, 0, mode, tmps[i]);
@@ -4771,8 +4780,7 @@  expand_assignment (tree to, tree from, b
 	  expand_insn (icode, 2, ops);
 	}
       else
-	store_bit_field (mem, GET_MODE_BITSIZE (mode),
-			 0, 0, 0, mode, reg);
+	store_bit_field (mem, GET_MODE_BITSIZE (mode), 0, 0, 0, mode, reg);
       return;
     }