Re: [PATCH] Do not allow non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Mon, May 27, 2013 at 10:02:19AM +0200, Richard Biener wrote:
> On Fri, 24 May 2013, Martin Jambor wrote:
> 
> > Hi,
> > 
> > On Thu, May 23, 2013 at 11:38:10AM +0200, Richard Biener wrote:
> > > On Thu, 23 May 2013, Eric Botcazou wrote:
> > > 
> > > > > earlier this week I asked on IRC whether we could have non-top-level
> > > > > BIT_FIELD_REFs and Richi said that we could.  However, when I later
> > > > > looked at SRA code, quite apparently it is not designed to handle
> > > > > non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs.  So in
> > > > > order to test whether that assumption is OK, I added the following
> > > > > into the gimple verifier and ran bootstrap and testsuite of all
> > > > > languages including Ada and ObjC++ on x86_64.  It survived, which
> > > > > makes me wondering whether we do not want it in trunk.
> > > > 
> > > > This looks plausible to me, but I think that you ought to verify the real 
> > > > assumption instead, which is that the type of the 3 nodes is always scalar.
> > > > The non-toplevelness of the nodes is merely a consequence of this property.
> > > 
> > > Yeah.  But please put the verification into tree-cfg.c:verify_expr
> > > instead.
> > > 
> > 
> > Like this?  Also bootstrapped and tested on x86_64-linux.
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2013-05-23  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* tree-cfg.c (verify_expr): Verify that BIT_FIELD_REFs, IMAGPART_EXPRs
> > 	and REALPART_EXPRs have scalar type.
> > 
> > Index: src/gcc/tree-cfg.c
> > ===================================================================
> > --- src.orig/gcc/tree-cfg.c
> > +++ src/gcc/tree-cfg.c
> > @@ -2669,10 +2669,17 @@ verify_expr (tree *tp, int *walk_subtree
> >  
> >      case REALPART_EXPR:
> >      case IMAGPART_EXPR:
> > +    case BIT_FIELD_REF:
> > +      if (!is_gimple_reg_type (TREE_TYPE (t)))
> > +	{
> > +	  error ("non-scalar BIT_FIELD_REF, IMAGPART_EXPR or REALPART_EXPR");
> > +	  return t;
> > +	}
> > +      /* Fall-through.  */
> >      case COMPONENT_REF:
> >      case ARRAY_REF:
> >      case ARRAY_RANGE_REF:
> > -    case BIT_FIELD_REF:
> >      case VIEW_CONVERT_EXPR:
> >        /* We have a nest of references.  Verify that each of the operands
> >  	 that determine where to reference is either a constant or a variable,
> 
> Yes, that looks good to me.  Note that this still does not verify
> that REALPART_EXPR, IMAGPART_EXPR and BIT_FIELD_REF are only
> outermost handled-component refs.  It merely verifies that if they
> are outermost then they are not aggregate.
> 
> Thus a followup would be to move the BIT_FIELD_REF handling in the
> loop below to the above case sub-set and disallow BIT_FIELD_REF,
> REALPART_EXPR and IMAGPART_EXPR inside that loop.
> 
> Though I'm pretty sure that evetually this will fail ...
> 
> The patch is ok, it's an improvement over the current state.

I've committed it s revision 199379, thanks.  As far as the
non-top-levelness is concerned, the following (on top of the previous
patch) also survives bootstrap and testsuite on x86_64 (all languages
including Ada and Obj-C++).  Do you think it would be acceptable as
well?

Thanks,

Martin

 
2013-05-27  Martin Jambor  <mjambor@suse.cz>

	* tree-cfg.c (verify_expr): Verify that BIT_FIELD_REF, REALPART_EXPR
	and IMAGPART_EXPR do not occur within other handled_components.

Index: src/gcc/tree-cfg.c
===================================================================
--- src.orig/gcc/tree-cfg.c
+++ src/gcc/tree-cfg.c
@@ -2675,6 +2675,33 @@ verify_expr (tree *tp, int *walk_subtree
 	  return t;
 	}
 
+      if (TREE_CODE (t) == BIT_FIELD_REF)
+	{
+	  if (!host_integerp (TREE_OPERAND (t, 1), 1)
+	      || !host_integerp (TREE_OPERAND (t, 2), 1))
+	    {
+	      error ("invalid position or size operand to BIT_FIELD_REF");
+	      return t;
+	    }
+	  if (INTEGRAL_TYPE_P (TREE_TYPE (t))
+	      && (TYPE_PRECISION (TREE_TYPE (t))
+		  != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
+	    {
+	      error ("integral result type precision does not match "
+		     "field size of BIT_FIELD_REF");
+	      return t;
+	    }
+	  else if (!INTEGRAL_TYPE_P (TREE_TYPE (t))
+		   && TYPE_MODE (TREE_TYPE (t)) != BLKmode
+		   && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t)))
+		       != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
+	    {
+	      error ("mode precision of non-integral result does not "
+		     "match field size of BIT_FIELD_REF");
+	      return t;
+	    }
+	}
+
       /* Fall-through.  */
     case COMPONENT_REF:
     case ARRAY_REF:
@@ -2697,35 +2724,16 @@ verify_expr (tree *tp, int *walk_subtree
 	      if (TREE_OPERAND (t, 3))
 		CHECK_OP (3, "invalid array stride");
 	    }
-	  else if (TREE_CODE (t) == BIT_FIELD_REF)
-	    {
-	      if (!host_integerp (TREE_OPERAND (t, 1), 1)
-		  || !host_integerp (TREE_OPERAND (t, 2), 1))
-		{
-		  error ("invalid position or size operand to BIT_FIELD_REF");
-		  return t;
-		}
-	      if (INTEGRAL_TYPE_P (TREE_TYPE (t))
-		  && (TYPE_PRECISION (TREE_TYPE (t))
-		      != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
-		{
-		  error ("integral result type precision does not match "
-			 "field size of BIT_FIELD_REF");
-		  return t;
-		}
-	      else if (!INTEGRAL_TYPE_P (TREE_TYPE (t))
-		       && !AGGREGATE_TYPE_P (TREE_TYPE (t))
-		       && TYPE_MODE (TREE_TYPE (t)) != BLKmode
-		       && (GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (t)))
-			   != TREE_INT_CST_LOW (TREE_OPERAND (t, 1))))
-		{
-		  error ("mode precision of non-integral result does not "
-			 "match field size of BIT_FIELD_REF");
-		  return t;
-		}
-	    }
 
 	  t = TREE_OPERAND (t, 0);
+	  if (TREE_CODE (t) == BIT_FIELD_REF
+	      || TREE_CODE (t) == REALPART_EXPR
+	      || TREE_CODE (t) == IMAGPART_EXPR)
+	    {
+	      error ("non-top-level BIT_FIELD_REF, IMAGPART_EXPR or "
+		     "REALPART_EXPR");
+	      return t;
+	    }
 	}
 
       if (!is_gimple_min_invariant (t) && !is_gimple_lvalue (t))