Don't make all MEM_REFs with TREE_READONLY arguments TREE_READONLY (PR sanitizer/64336)

On Wed, Dec 17, 2014 at 03:28:15PM +0100, Richard Biener wrote:
> On Wed, 17 Dec 2014, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > MEM_REF (the only tcc_reference code with 2 operands) has TREE_READONLY set
> > whenever all the arguments are TREE_READONLY, which is wrong, if the
> > pointer/reference is read-only, it doesn't say anything about whether
> > what it points to is also read-only.
> > 
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > trunk?
> 
> Thinking about this some more I think that we should instead do sth like
> 
>   read_only = 1;
>   side_effects = TREE_SIDE_EFFECTS (t);
> 
>   if (code == MEM_REF)
>    {
>      if (TREE_CODE (arg0) == ADDR_EXPR)
>        {
>          tree t = arg0;
>          PROCESS_ARG (0);
>        }
>      else
>        read_only = 0;

This doesn't work, PROCESS_ARG among other things assigns the arguments,
computing the flags is just one thing.  So the above would crash if arg0 is
NULL (also happens), and if it is ADDR_EXPR would attempt to assign
TREE_OPERANDS (arg0, 0) = arg0; and otherwise leave the argument NULL.

Here is a new version of the patch, the flags should be preinitialized to
0 by make_node_stat, so can be assigned for *MEM_REFs only if arg0 is
ADDR_EXPR, 0 is right otherwise for both flags.  In generic code MEM_REF
is the only 2 operand tcc_reference, but C++ FE has some further ones,
so I need to keep the old TREE_THIS_VOLATILE assignment.  There are no
5 operand tcc_references beyond TARGET_MEM_REF, but I'd keep it this way
as is anyway.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2015-01-06  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/64336
	* tree.c (build2_stat): Fix up initialization of TREE_READONLY
	and TREE_THIS_VOLATILE for MEM_REFs.
	(build5_stat): Fix up initialization of TREE_READONLY and
	TREE_THIS_VOLATILE for TARGET_MEM_REFs.



	Jakub

On Tue, 6 Jan 2015, Jakub Jelinek wrote:

> On Wed, Dec 17, 2014 at 03:28:15PM +0100, Richard Biener wrote:
> > On Wed, 17 Dec 2014, Jakub Jelinek wrote:
> > 
> > > Hi!
> > > 
> > > MEM_REF (the only tcc_reference code with 2 operands) has TREE_READONLY set
> > > whenever all the arguments are TREE_READONLY, which is wrong, if the
> > > pointer/reference is read-only, it doesn't say anything about whether
> > > what it points to is also read-only.
> > > 
> > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > > trunk?
> > 
> > Thinking about this some more I think that we should instead do sth like
> > 
> >   read_only = 1;
> >   side_effects = TREE_SIDE_EFFECTS (t);
> > 
> >   if (code == MEM_REF)
> >    {
> >      if (TREE_CODE (arg0) == ADDR_EXPR)
> >        {
> >          tree t = arg0;
> >          PROCESS_ARG (0);
> >        }
> >      else
> >        read_only = 0;
> 
> This doesn't work, PROCESS_ARG among other things assigns the arguments,
> computing the flags is just one thing.  So the above would crash if arg0 is
> NULL (also happens), and if it is ADDR_EXPR would attempt to assign
> TREE_OPERANDS (arg0, 0) = arg0; and otherwise leave the argument NULL.
> 
> Here is a new version of the patch, the flags should be preinitialized to
> 0 by make_node_stat, so can be assigned for *MEM_REFs only if arg0 is
> ADDR_EXPR, 0 is right otherwise for both flags.  In generic code MEM_REF
> is the only 2 operand tcc_reference, but C++ FE has some further ones,
> so I need to keep the old TREE_THIS_VOLATILE assignment.  There are no
> 5 operand tcc_references beyond TARGET_MEM_REF, but I'd keep it this way
> as is anyway.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2015-01-06  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/64336
> 	* tree.c (build2_stat): Fix up initialization of TREE_READONLY
> 	and TREE_THIS_VOLATILE for MEM_REFs.
> 	(build5_stat): Fix up initialization of TREE_READONLY and
> 	TREE_THIS_VOLATILE for TARGET_MEM_REFs.
> 
> --- gcc/tree.c.jj	2015-01-05 13:07:15.000000000 +0100
> +++ gcc/tree.c	2015-01-06 12:33:46.466016025 +0100
> @@ -4358,12 +4358,24 @@ build2_stat (enum tree_code code, tree t
>    PROCESS_ARG (0);
>    PROCESS_ARG (1);
>  
> -  TREE_READONLY (t) = read_only;
> -  TREE_CONSTANT (t) = constant;
>    TREE_SIDE_EFFECTS (t) = side_effects;
> -  TREE_THIS_VOLATILE (t)
> -    = (TREE_CODE_CLASS (code) == tcc_reference
> -       && arg0 && TREE_THIS_VOLATILE (arg0));
> +  if (code == MEM_REF)
> +    {
> +      if (arg0 && TREE_CODE (arg0) == ADDR_EXPR)
> +	{
> +	  tree o = TREE_OPERAND (arg0, 0);
> +	  TREE_READONLY (t) = TREE_READONLY (o);
> +	  TREE_THIS_VOLATILE (t) = TREE_THIS_VOLATILE (o);
> +	}
> +    }
> +  else
> +    {
> +      TREE_READONLY (t) = read_only;
> +      TREE_CONSTANT (t) = constant;
> +      TREE_THIS_VOLATILE (t)
> +	= (TREE_CODE_CLASS (code) == tcc_reference
> +	   && arg0 && TREE_THIS_VOLATILE (arg0));
> +    }
>  
>    return t;
>  }
> @@ -4458,9 +4470,19 @@ build5_stat (enum tree_code code, tree t
>    PROCESS_ARG (4);
>  
>    TREE_SIDE_EFFECTS (t) = side_effects;
> -  TREE_THIS_VOLATILE (t)
> -    = (TREE_CODE_CLASS (code) == tcc_reference
> -       && arg0 && TREE_THIS_VOLATILE (arg0));
> +  if (code == TARGET_MEM_REF)
> +    {
> +      if (arg0 && TREE_CODE (arg0) == ADDR_EXPR)
> +	{
> +	  tree o = TREE_OPERAND (arg0, 0);
> +	  TREE_READONLY (t) = TREE_READONLY (o);
> +	  TREE_THIS_VOLATILE (t) = TREE_THIS_VOLATILE (o);
> +	}
> +    }
> +  else
> +    TREE_THIS_VOLATILE (t)
> +      = (TREE_CODE_CLASS (code) == tcc_reference
> +	 && arg0 && TREE_THIS_VOLATILE (arg0));
>  
>    return t;
>  }
> 
> 
> 	Jakub
> 
>