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

[Richard Biener <rguenther at suse dot de>]

On Tue, 28 May 2013, Martin Jambor wrote:

> 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?

With the following minor adjustment:

> 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;
> +	    }
> +	}
> +

      t = TREE_OPERAND (t, 0);

here instead of ...

>        /* 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;
> +	    }

... doing this after t = TREE_OPERAND (t, 0) (so, do it before).

Thanks,
Richard.