Patchwork Fix double write of unchecked conversion to volatile variable

login
register
mail settings
Submitter Eric Botcazou
Date Oct. 15, 2012, 10:43 a.m.
Message ID <1363136.7lblyRCjsk@polaris>
Download mbox | patch
Permalink /patch/191517/
State New
Headers show

Comments

Eric Botcazou - Oct. 15, 2012, 10:43 a.m.
For the attached Ada testcase, the compiler generates a double write to the 
volatile variable at -O1.  The problem is a VIEW_CONVERT_EXPR on the rhs.

We have in store_expr:

     If TEMP and TARGET compare equal according to rtx_equal_p, but
     one or both of them are volatile memory refs, we have to distinguish
     two cases:
     - expand_expr has used TARGET.  In this case, we must not generate
       another copy.  This can be detected by TARGET being equal according
       to == .
     - expand_expr has not used TARGET - that means that the source just
       happens to have the same RTX form.  Since temp will have been created
       by expand_expr, it will compare unequal according to == .
       We must generate a copy in this case, to reach the correct number
       of volatile memory references.  */

So store_expr expects that, if expand_expr returns TEMP != TARGET, then TARGET 
hasn't been used and a copy is needed.

Now in the VIEW_CONVERT_EXPR case of expand_expr, we have:

      /* At this point, OP0 is in the correct mode.  If the output type is
	 such that the operand is known to be aligned, indicate that it is.
	 Otherwise, we need only be concerned about alignment for non-BLKmode
	 results.  */
      if (MEM_P (op0))
	{
	  enum insn_code icode;

	  op0 = copy_rtx (op0);

i.e. op0 is blindly copied, which breaks the assumption of store_expr.

The attached patch removes the copy_rtx from the main path and puts it back 
only on the sub-path where it is presumably still needed.  Of course it's 
probably still problematic wrt store_expr, but it's the TYPE_ALIGN_OK case and 
I'm not sure it matters in practice; the patch contains a ??? note though.

Tested on x86_64-suse-linux, applied on the mainline.


2012-10-15  Eric Botcazou  <ebotcazou@adacore.com>

	* expr.c (expand_expr_real_1) <VIEW_CONVERT_EXPR>: Do not unnecessarily
	copy the object in the MEM_P case.


2012-10-15  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/unchecked_convert9.ad[sb]: New test.

Patch

Index: expr.c
===================================================================
--- expr.c	(revision 192447)
+++ expr.c	(working copy)
@@ -10270,10 +10270,15 @@  expand_expr_real_1 (tree exp, rtx target
 	{
 	  enum insn_code icode;
 
-	  op0 = copy_rtx (op0);
-
 	  if (TYPE_ALIGN_OK (type))
-	    set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
+	    {
+	      /* ??? Copying the MEM without substantially changing it might
+		 run afoul of the code handling volatile memory references in
+		 store_expr, which assumes that TARGET is returned unmodified
+		 if it has been used.  */
+	      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