Fix PR61762, reads from string constants

On Fri, 11 Jul 2014, Richard Biener wrote:

> On Fri, 11 Jul 2014, Jakub Jelinek wrote:
> 
> > On Fri, Jul 11, 2014 at 10:00:01AM +0200, Richard Biener wrote:
> > > I started with adding an optional arg to native_encode_expr but then
> > > figured that all the current workers return failure if the entire
> > > expr doesn't fit within 'len'.  But what I want is a native_encode_expr
> > 
> > That is because the functions were written for VIEW_CONVERT_EXPR folding
> > where it is expected that both the types are the same size, and it doesn't
> > make sense to spend too much time on large stuff that doesn't fit into
> > the fold_view_convert_expr's fixed size buffer.  Now, for your case
> > the sizes aren't the same and thus partial data is just fine, because
> > what native_interpret_expr will do is small fixed size.
> > So I don't see a problem with not returning NULL and instead handling the
> > partial store into buffer if you ask for it through optional argument
> > (doing it always would be fine for fold_view_convert_expr assuming nobody
> > feeds us different sized argument vs. VCE type, but e.g.
> > tree-loop-distribution.c certainly appreciates if we return NULL for large
> > things, because if it had just partial buffer, it would not be able to
> > verify what it is looking for).
> 
> Yeah, if you pass 'offset' then 'len' suddenly changes semantics
> from specifying the buffer size to specifying the desired amount
> of encoded data ... (which the present interface computes for you,
> the caller doesn't even need to know how large it is).
> 
> The question is also if I ask native_encode_expr to produce
> data for [offset, offset + len] but the object only contains
> part of it - thus the tail is 'undefined' - do we want to fail
> or simply feed (undefined) "garbage" to the following
> native_interpret_expr?  (yes, native_encode_expr will of course
> return sth < len in this case)

Like the following (completely untested - lacks handling of
len < total_bytes - off as I just figured out).  If off is -1
then it should behave as the current interface, else we encode
the part [off, off + MIN (len, total_bytes)] and return
MIN (len, total_bytes).  Ah, a guard against off >= total_bytes
is also missing.

I wonder if it would be ok to start with gcc_assert (off == -1)
in all but native_encode_string.

Does the interface look sane?

Thanks,
Richard.

On Fri, Jul 11, 2014 at 11:13:12AM +0200, Richard Biener wrote:
> +      if (offset >= off)
> +       ptr[offset - off] = value;

For original off != 0, you aren't checking whether offset - off < len
though (various places), you don't want to write beyond end of buffer.

> -  return total_bytes;
> +  return total_bytes - off;

What will happen with originally off is bigger than total_bytes?
Returning negative value sounds wrong, IMHO you should in that case return
early 0.  So like:
if ((off == -1 && total_bytes > len) || off >= total_bytes)
  return 0;

> @@ -7290,7 +7293,8 @@ native_encode_fixed (const_tree expr, un
>    FIXED_VALUE_TYPE value;
>    tree i_value, i_type;
>  
> -  if (total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
> +  if (off == -1
> +      && total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
>      return 0;

This isn't comparing total_bytes to len, so IMHO shouldn't be changed.

> @@ -7324,8 +7328,11 @@ native_encode_real (const_tree expr, uns
>       up to 192 bits.  */
>    long tmp[6];
>  
> -  if (total_bytes > len)
> +  if (off == -1
> +      && total_bytes > len)

This can fit onto one line.

> -  rsize = native_encode_expr (part, ptr, len);
> +  rsize = native_encode_expr (part, ptr, len, off);
>    if (rsize == 0)
>      return 0;

If off is not -1 and len is too short, the above will do a partial
store.  But:
a) if it is a partial store, because some bytes didn't fit, then the
second native_encode_expr should probably not be invoked
b) what about the case when the first one returns 0 because you are asking
for few bytes from the imag part?

>    part = TREE_IMAGPART (expr);
> -  isize = native_encode_expr (part, ptr+rsize, len-rsize);
> -  if (isize != rsize)
> +  if (off != -1)
> +    off = MAX (0, off - rsize);
> +  isize = native_encode_expr (part, ptr+rsize, len-rsize, off);
> +  if (off == -1
> +      && isize != rsize)
>      return 0;
>    return rsize + isize;
>  }

> @@ -7396,9 +7408,13 @@ native_encode_vector (const_tree expr, u
>    for (i = 0; i < count; i++)
>      {
>        elem = VECTOR_CST_ELT (expr, i);
> -      if (native_encode_expr (elem, ptr+offset, len-offset) != size)
> +      int res = native_encode_expr (elem, ptr+offset, len-offset, off);
> +      if (off == -1
> +	  && res != size)
>  	return 0;

I don't think this will work correctly if off is not -1.

>    if (TREE_STRING_LENGTH (expr) < total_bytes)

No verification that you are not accessing beyond end of string here.
>      {
> -      memcpy (ptr, TREE_STRING_POINTER (expr), TREE_STRING_LENGTH (expr));
> -      memset (ptr + TREE_STRING_LENGTH (expr), 0,
> -	      total_bytes - TREE_STRING_LENGTH (expr));
> +      memcpy (ptr, TREE_STRING_POINTER (expr) + off,
> +	      TREE_STRING_LENGTH (expr) - off);
> +      memset (ptr + TREE_STRING_LENGTH (expr) - off, 0,
> +	      MIN (total_bytes, len) - TREE_STRING_LENGTH (expr) + off);
>      }
>    else
> -    memcpy (ptr, TREE_STRING_POINTER (expr), total_bytes);
> -  return total_bytes;
> +    memcpy (ptr, TREE_STRING_POINTER (expr) + off,
> +	    MIN (total_bytes, len));
> +  return MIN (total_bytes - off, len);
>  }

	Jakub

On Fri, 11 Jul 2014, Jakub Jelinek wrote:

> On Fri, Jul 11, 2014 at 11:13:12AM +0200, Richard Biener wrote:
> > +      if (offset >= off)
> > +       ptr[offset - off] = value;
> 
> For original off != 0, you aren't checking whether offset - off < len
> though (various places), you don't want to write beyond end of buffer.

Indeed.  Fixed.

> > -  return total_bytes;
> > +  return total_bytes - off;
> 
> What will happen with originally off is bigger than total_bytes?
> Returning negative value sounds wrong, IMHO you should in that case return
> early 0.  So like:
> if ((off == -1 && total_bytes > len) || off >= total_bytes)
>   return 0;

Fixed.

> > @@ -7290,7 +7293,8 @@ native_encode_fixed (const_tree expr, un
> >    FIXED_VALUE_TYPE value;
> >    tree i_value, i_type;
> >  
> > -  if (total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
> > +  if (off == -1
> > +      && total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
> >      return 0;
> 
> This isn't comparing total_bytes to len, so IMHO shouldn't be changed.

Oh, indeed.  (reading that and native_encode_int I think we want
a native_encode_wide_int and avoid creating an integer cst for
encoding fixed_csts)

> > @@ -7324,8 +7328,11 @@ native_encode_real (const_tree expr, uns
> >       up to 192 bits.  */
> >    long tmp[6];
> >  
> > -  if (total_bytes > len)
> > +  if (off == -1
> > +      && total_bytes > len)
> 
> This can fit onto one line.
> 
> > -  rsize = native_encode_expr (part, ptr, len);
> > +  rsize = native_encode_expr (part, ptr, len, off);
> >    if (rsize == 0)
> >      return 0;
> 
> If off is not -1 and len is too short, the above will do a partial
> store.  But:
> a) if it is a partial store, because some bytes didn't fit, then the
> second native_encode_expr should probably not be invoked

len will get negative so that might just be handled fine (fingers
crossing).  I suppose I need to come up with a testcase that ends
up native-encoding a vector partially (SCCVN also benefits from
this work).

> b) what about the case when the first one returns 0 because you are asking
> for few bytes from the imag part?

Yeah.  Sth like

> >    part = TREE_IMAGPART (expr);
> > -  isize = native_encode_expr (part, ptr+rsize, len-rsize);
> > -  if (isize != rsize)
> > +  if (off != -1)
> > +    off = MAX (0, off - rsize);

  if (off != -1)
    off = MAX (0, off - GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (part))));

instead.  Same mistake in vector encoding - we need to adjust
off by the size of the sub-object not by what was encoded.

> > +  isize = native_encode_expr (part, ptr+rsize, len-rsize, off);
> > +  if (off == -1
> > +      && isize != rsize)
> >      return 0;
> >    return rsize + isize;
> >  }
> 
> > @@ -7396,9 +7408,13 @@ native_encode_vector (const_tree expr, u
> >    for (i = 0; i < count; i++)
> >      {
> >        elem = VECTOR_CST_ELT (expr, i);
> > -      if (native_encode_expr (elem, ptr+offset, len-offset) != size)
> > +      int res = native_encode_expr (elem, ptr+offset, len-offset, off);
> > +      if (off == -1
> > +	  && res != size)
> >  	return 0;
> 
> I don't think this will work correctly if off is not -1.

I have

      elem = VECTOR_CST_ELT (expr, i);
      int res = native_encode_expr (elem, ptr+offset, len-offset, off);
      if (off == -1
          && res != size)
        return 0;
      offset += res;
      if (off != -1)
        off = MAX (0, off - size);

now, so 'offset' tracks the number of encoded bytes and off is
decremented properly.  Now it will of course break if we fail
to encode one vector element but not another.  But I hate
complicating things here ... (maybe guard all recursive calls
with off < size and check for a 0 return).  Like

  for (i = 0; i < count; i++)
    {
      if (off >= size)
        {
          off -= size;
          continue;
        }
      elem = VECTOR_CST_ELT (expr, i);
      int res = native_encode_expr (elem, ptr+offset, len-offset, off);
      if ((off == -1 && res != size)
          || res == 0)
        return 0;
      offset += res;
      if (off != -1)
        off = 0;

?

> >    if (TREE_STRING_LENGTH (expr) < total_bytes)
> 
> No verification that you are not accessing beyond end of string here.

Changed to

  if (TREE_STRING_LENGTH (expr) - off < MIN (total_bytes, len))
    {
      int written = 0;
      if (off < TREE_STRING_LENGTH (expr))
        {
          written = MIN (len, TREE_STRING_LENGTH (expr) - off);
          memcpy (ptr, TREE_STRING_POINTER (expr) + off, written);
        }
      memset (ptr + written, 0,
              MIN (total_bytes - written, len - written));
    }
  else
    memcpy (ptr, TREE_STRING_POINTER (expr) + off, MIN (total_bytes, 
len));

ugh.

Now I have to think about getting at least some testing coverage ...

Richard.

Index: gcc/fold-const.c
===================================================================
*** gcc/fold-const.c.orig	2014-07-11 11:13:22.380607780 +0200
--- gcc/fold-const.c	2014-07-11 12:05:02.352394351 +0200
*************** fold_plusminus_mult_expr (location_t loc
*** 7239,7253 ****
     upon failure.  */
  
  static int
! native_encode_int (const_tree expr, unsigned char *ptr, int len)
  {
    tree type = TREE_TYPE (expr);
    int total_bytes = GET_MODE_SIZE (TYPE_MODE (type));
    int byte, offset, word, words;
    unsigned char value;
  
!   if (total_bytes > len)
      return 0;
    words = total_bytes / UNITS_PER_WORD;
  
    for (byte = 0; byte < total_bytes; byte++)
--- 7239,7256 ----
     upon failure.  */
  
  static int
! native_encode_int (const_tree expr, unsigned char *ptr, int len, int off)
  {
    tree type = TREE_TYPE (expr);
    int total_bytes = GET_MODE_SIZE (TYPE_MODE (type));
    int byte, offset, word, words;
    unsigned char value;
  
!   if ((off == -1 && total_bytes > len)
!       || off >= total_bytes)
      return 0;
+   if (off == -1)
+     off = 0;
    words = total_bytes / UNITS_PER_WORD;
  
    for (byte = 0; byte < total_bytes; byte++)
*************** native_encode_int (const_tree expr, unsi
*** 7270,7278 ****
  	}
        else
  	offset = BYTES_BIG_ENDIAN ? (total_bytes - 1) - byte : byte;
!       ptr[offset] = value;
      }
!   return total_bytes;
  }
  
  
--- 7273,7283 ----
  	}
        else
  	offset = BYTES_BIG_ENDIAN ? (total_bytes - 1) - byte : byte;
!       if (offset >= off
! 	  && offset - off < len)
! 	ptr[offset - off] = value;
      }
!   return MIN (len, total_bytes - off);
  }
  
  
*************** native_encode_int (const_tree expr, unsi
*** 7282,7288 ****
     upon failure.  */
  
  static int
! native_encode_fixed (const_tree expr, unsigned char *ptr, int len)
  {
    tree type = TREE_TYPE (expr);
    enum machine_mode mode = TYPE_MODE (type);
--- 7287,7293 ----
     upon failure.  */
  
  static int
! native_encode_fixed (const_tree expr, unsigned char *ptr, int len, int off)
  {
    tree type = TREE_TYPE (expr);
    enum machine_mode mode = TYPE_MODE (type);
*************** native_encode_fixed (const_tree expr, un
*** 7302,7308 ****
    value = TREE_FIXED_CST (expr);
    i_value = double_int_to_tree (i_type, value.data);
  
!   return native_encode_int (i_value, ptr, len);
  }
  
  
--- 7307,7313 ----
    value = TREE_FIXED_CST (expr);
    i_value = double_int_to_tree (i_type, value.data);
  
!   return native_encode_int (i_value, ptr, len, off);
  }
  
  
*************** native_encode_fixed (const_tree expr, un
*** 7312,7318 ****
     upon failure.  */
  
  static int
! native_encode_real (const_tree expr, unsigned char *ptr, int len)
  {
    tree type = TREE_TYPE (expr);
    int total_bytes = GET_MODE_SIZE (TYPE_MODE (type));
--- 7317,7323 ----
     upon failure.  */
  
  static int
! native_encode_real (const_tree expr, unsigned char *ptr, int len, int off)
  {
    tree type = TREE_TYPE (expr);
    int total_bytes = GET_MODE_SIZE (TYPE_MODE (type));
*************** native_encode_real (const_tree expr, uns
*** 7324,7331 ****
       up to 192 bits.  */
    long tmp[6];
  
!   if (total_bytes > len)
      return 0;
    words = (32 / BITS_PER_UNIT) / UNITS_PER_WORD;
  
    real_to_target (tmp, TREE_REAL_CST_PTR (expr), TYPE_MODE (type));
--- 7329,7339 ----
       up to 192 bits.  */
    long tmp[6];
  
!   if ((off == -1 && total_bytes > len)
!       || off >= total_bytes)
      return 0;
+   if (off == -1)
+     off = 0;
    words = (32 / BITS_PER_UNIT) / UNITS_PER_WORD;
  
    real_to_target (tmp, TREE_REAL_CST_PTR (expr), TYPE_MODE (type));
*************** native_encode_real (const_tree expr, uns
*** 7349,7357 ****
  	}
        else
  	offset = BYTES_BIG_ENDIAN ? 3 - byte : byte;
!       ptr[offset + ((bitpos / BITS_PER_UNIT) & ~3)] = value;
      }
!   return total_bytes;
  }
  
  /* Subroutine of native_encode_expr.  Encode the COMPLEX_CST
--- 7357,7368 ----
  	}
        else
  	offset = BYTES_BIG_ENDIAN ? 3 - byte : byte;
!       offset = offset + ((bitpos / BITS_PER_UNIT) & ~3);
!       if (offset >= off
! 	  && offset - off < len)
! 	ptr[offset - off] = value;
      }
!   return MIN (len, total_bytes - off);
  }
  
  /* Subroutine of native_encode_expr.  Encode the COMPLEX_CST
*************** native_encode_real (const_tree expr, uns
*** 7360,7377 ****
     upon failure.  */
  
  static int
! native_encode_complex (const_tree expr, unsigned char *ptr, int len)
  {
    int rsize, isize;
    tree part;
  
    part = TREE_REALPART (expr);
!   rsize = native_encode_expr (part, ptr, len);
!   if (rsize == 0)
      return 0;
    part = TREE_IMAGPART (expr);
!   isize = native_encode_expr (part, ptr+rsize, len-rsize);
!   if (isize != rsize)
      return 0;
    return rsize + isize;
  }
--- 7371,7392 ----
     upon failure.  */
  
  static int
! native_encode_complex (const_tree expr, unsigned char *ptr, int len, int off)
  {
    int rsize, isize;
    tree part;
  
    part = TREE_REALPART (expr);
!   rsize = native_encode_expr (part, ptr, len, off);
!   if (off == -1
!       && rsize == 0)
      return 0;
    part = TREE_IMAGPART (expr);
!   if (off != -1)
!     off = MAX (0, off - GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (part))));
!   isize = native_encode_expr (part, ptr+rsize, len-rsize, off);
!   if (off == -1
!       && isize != rsize)
      return 0;
    return rsize + isize;
  }
*************** native_encode_complex (const_tree expr,
*** 7383,7389 ****
     upon failure.  */
  
  static int
! native_encode_vector (const_tree expr, unsigned char *ptr, int len)
  {
    unsigned i, count;
    int size, offset;
--- 7398,7404 ----
     upon failure.  */
  
  static int
! native_encode_vector (const_tree expr, unsigned char *ptr, int len, int off)
  {
    unsigned i, count;
    int size, offset;
*************** native_encode_vector (const_tree expr, u
*** 7395,7404 ****
    size = GET_MODE_SIZE (TYPE_MODE (itype));
    for (i = 0; i < count; i++)
      {
        elem = VECTOR_CST_ELT (expr, i);
!       if (native_encode_expr (elem, ptr+offset, len-offset) != size)
  	return 0;
!       offset += size;
      }
    return offset;
  }
--- 7410,7428 ----
    size = GET_MODE_SIZE (TYPE_MODE (itype));
    for (i = 0; i < count; i++)
      {
+       if (off >= size)
+ 	{
+ 	  off -= size;
+ 	  continue;
+ 	}
        elem = VECTOR_CST_ELT (expr, i);
!       int res = native_encode_expr (elem, ptr+offset, len-offset, off);
!       if ((off == -1 && res != size)
! 	  || res == 0)
  	return 0;
!       offset += res;
!       if (off != -1)
! 	off = 0;
      }
    return offset;
  }
*************** native_encode_vector (const_tree expr, u
*** 7410,7416 ****
     upon failure.  */
  
  static int
! native_encode_string (const_tree expr, unsigned char *ptr, int len)
  {
    tree type = TREE_TYPE (expr);
    HOST_WIDE_INT total_bytes;
--- 7434,7440 ----
     upon failure.  */
  
  static int
! native_encode_string (const_tree expr, unsigned char *ptr, int len, int off)
  {
    tree type = TREE_TYPE (expr);
    HOST_WIDE_INT total_bytes;
*************** native_encode_string (const_tree expr, u
*** 7421,7437 ****
        || !tree_fits_shwi_p (TYPE_SIZE_UNIT (type)))
      return 0;
    total_bytes = tree_to_shwi (TYPE_SIZE_UNIT (type));
!   if (total_bytes > len)
      return 0;
!   if (TREE_STRING_LENGTH (expr) < total_bytes)
      {
!       memcpy (ptr, TREE_STRING_POINTER (expr), TREE_STRING_LENGTH (expr));
!       memset (ptr + TREE_STRING_LENGTH (expr), 0,
! 	      total_bytes - TREE_STRING_LENGTH (expr));
      }
    else
!     memcpy (ptr, TREE_STRING_POINTER (expr), total_bytes);
!   return total_bytes;
  }
  
  
--- 7445,7469 ----
        || !tree_fits_shwi_p (TYPE_SIZE_UNIT (type)))
      return 0;
    total_bytes = tree_to_shwi (TYPE_SIZE_UNIT (type));
!   if ((off == -1 && total_bytes > len)
!       || off >= total_bytes)
      return 0;
!   if (off == -1)
!     off = 0;
!   if (TREE_STRING_LENGTH (expr) - off < MIN (total_bytes, len))
      {
!       int written = 0;
!       if (off < TREE_STRING_LENGTH (expr))
! 	{
! 	  written = MIN (len, TREE_STRING_LENGTH (expr) - off);
! 	  memcpy (ptr, TREE_STRING_POINTER (expr) + off, written);
! 	}
!       memset (ptr + written, 0,
! 	      MIN (total_bytes - written, len - written));
      }
    else
!     memcpy (ptr, TREE_STRING_POINTER (expr) + off, MIN (total_bytes, len));
!   return MIN (total_bytes - off, len);
  }
  
  
*************** native_encode_string (const_tree expr, u
*** 7441,7467 ****
     placed in the buffer, or zero upon failure.  */
  
  int
! native_encode_expr (const_tree expr, unsigned char *ptr, int len)
  {
    switch (TREE_CODE (expr))
      {
      case INTEGER_CST:
!       return native_encode_int (expr, ptr, len);
  
      case REAL_CST:
!       return native_encode_real (expr, ptr, len);
  
      case FIXED_CST:
!       return native_encode_fixed (expr, ptr, len);
  
      case COMPLEX_CST:
!       return native_encode_complex (expr, ptr, len);
  
      case VECTOR_CST:
!       return native_encode_vector (expr, ptr, len);
  
      case STRING_CST:
!       return native_encode_string (expr, ptr, len);
  
      default:
        return 0;
--- 7473,7499 ----
     placed in the buffer, or zero upon failure.  */
  
  int
! native_encode_expr (const_tree expr, unsigned char *ptr, int len, int off)
  {
    switch (TREE_CODE (expr))
      {
      case INTEGER_CST:
!       return native_encode_int (expr, ptr, len, off);
  
      case REAL_CST:
!       return native_encode_real (expr, ptr, len, off);
  
      case FIXED_CST:
!       return native_encode_fixed (expr, ptr, len, off);
  
      case COMPLEX_CST:
!       return native_encode_complex (expr, ptr, len, off);
  
      case VECTOR_CST:
!       return native_encode_vector (expr, ptr, len, off);
  
      case STRING_CST:
!       return native_encode_string (expr, ptr, len, off);
  
      default:
        return 0;
Index: gcc/fold-const.h
===================================================================
*** gcc/fold-const.h.orig	2014-07-11 11:13:22.397607779 +0200
--- gcc/fold-const.h	2014-07-11 11:14:59.472601095 +0200
*************** along with GCC; see the file COPYING3.
*** 25,31 ****
  extern int folding_initializer;
  
  /* Convert between trees and native memory representation.  */
! extern int native_encode_expr (const_tree, unsigned char *, int);
  extern tree native_interpret_expr (tree, const unsigned char *, int);
  
  /* Fold constants as much as possible in an expression.
--- 25,31 ----
  extern int folding_initializer;
  
  /* Convert between trees and native memory representation.  */
! extern int native_encode_expr (const_tree, unsigned char *, int, int off = -1);
  extern tree native_interpret_expr (tree, const unsigned char *, int);
  
  /* Fold constants as much as possible in an expression.