[PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

Bug 81908 is about a -Wstringop-overflow warning for a Fortran
test triggered by a recent VRP improvement.  A simple test case
that approximates the warning is:

   void f (char *d, const char *s, size_t n)
   {
     if (n > 0 && n <= SIZE_MAX / 2)
       n = 0;

     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
   }

Since the only valid value of n is zero the call to memcpy can
be considered a no-op (a value of n > SIZE_MAX is in excess of
the maximum size of the largest object and would surely make
the call crash).

The important difference between the test case and Bug 81908
is that in the latter, the code is emitted by GCC itself from
what appears to be correct source (looks like it's the result
of the loop distribution pass).  I believe the warning for
the test case above and for other human-written code like it
is helpful, but warning for code emitted by GCC, even if it's
dead or unreachable, is obviously not (at least not to users).

The attached patch enhances the gimple_fold_builtin_memory_op
function to eliminate this patholohgical case by making use
of range information to fold into no-ops calls to memcpy whose
size argument is in a range where the only valid value is zero.
This gets rid of the warning and improves the emitted code.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-81908.diff --]
[-- Type: text/x-patch, Size: 5398 bytes --]

PR middle-end/81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32

gcc/ChangeLog:

	PR middle-end/81908
	* gimple-fold.c (only_valid_value): New function.
	(gimple_fold_builtin_memory_op): Call it.

gcc/testsuite/ChangeLog:

	PR middle-end/81908
	* gcc.dg/tree-ssa/builtins-folding-gimple-2.c: New test.
	* gcc.dg/tree-ssa/builtins-folding-gimple-3.c: New test.

diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 251446c..8ba8242 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -628,6 +628,37 @@ var_decl_component_p (tree var)
   return SSA_VAR_P (inner);
 }
 
+/* If the SIZE argument representing the size of an object is in a range
+   of values of which exactly one is valid, return that value.  The only
+   such valid value is zero.  */
+
+static tree
+only_valid_value (tree size)
+{
+  if (TREE_CODE (size) != SSA_NAME)
+    return NULL_TREE;
+
+  wide_int min, max;
+  enum value_range_type rtype = get_range_info (size, &min, &max);
+  if (rtype != VR_ANTI_RANGE)
+    return NULL_TREE;
+
+  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
+    return NULL_TREE;
+
+  tree type = TREE_TYPE (size);
+  int prec = TYPE_PRECISION (type);
+
+  wide_int wone = wi::one (prec);
+  wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1);
+
+  if (wi::eq_p (min, wone)
+      && wi::geu_p (max + 1, ssize_max))
+    return build_int_cst (type, 0);
+
+  return NULL_TREE;
+}
+
 /* Fold function call to builtin mem{{,p}cpy,move}.  Return
    false if no simplification can be made.
    If ENDP is 0, return DEST (like memcpy).
@@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
   tree destvar, srcvar;
   location_t loc = gimple_location (stmt);
 
+  if (tree valid_len = only_valid_value (len))
+    {
+      /* LEN is in range whose only valid value is zero.  */
+      len = valid_len;
+    }
+
   /* If the LEN parameter is zero, return DEST.  */
   if (integer_zerop (len))
     {
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c
new file mode 100644
index 0000000..9ebfdb6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c
@@ -0,0 +1,42 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#define INT_MAX    __INT_MAX__
+#define SHRT_MAX   __SHRT_MAX__
+#define SIZE_MAX   __SIZE_MAX__
+#define SSIZE_MAX  (SIZE_MAX / 2)
+
+typedef __PTRDIFF_TYPE__ ssize_t;
+typedef __SIZE_TYPE__    size_t;
+
+#define UNIQUE_FUNCNAME(func, line) test_ ## func ## _ ## line
+#define FUNCNAME(func, line)        UNIQUE_FUNCNAME (func, line)
+
+#define AR(func, type, min, max, val)					\
+  void __attribute__ ((noclone, noinline))				\
+  FUNCNAME (func, __LINE__) (char *d, const char *s, type n)		\
+  {									\
+    if ((type)min <= n && n <= (type)max)				\
+      n = val;								\
+    __builtin_ ## func (d, s, n);					\
+  } typedef void DummyType
+
+AR (memcpy, short, 1, SHRT_MAX, 0);
+AR (memcpy, int,   1, INT_MAX, 0);
+AR (memcpy, size_t,  1, SSIZE_MAX, 0);
+AR (memcpy, ssize_t, 1, SSIZE_MAX, 0);
+
+AR (memmove, short, 1, SHRT_MAX, 0);
+AR (memmove, int,   1, INT_MAX, 0);
+AR (memmove, ssize_t, 1, SSIZE_MAX, 0);
+AR (memmove, ssize_t, 1, SSIZE_MAX, 0);
+
+AR (mempcpy, short, 1, SHRT_MAX, 0);
+AR (mempcpy, int,   1, INT_MAX, 0);
+AR (mempcpy, size_t,  1, SSIZE_MAX, 0);
+AR (mempcpy, ssize_t, 1, SSIZE_MAX, 0);
+
+/* { dg-final { scan-tree-dump-not "builtin_memcpy" "optimized" } }
+   { dg-final { scan-tree-dump-not "builtin_memmove" "optimized" } }
+   { dg-final { scan-tree-dump-not "builtin_mempcpy" "optimized" } }  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c
new file mode 100644
index 0000000..b1b32b4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c
@@ -0,0 +1,40 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#define SHRT_MAX   __SHRT_MAX__
+#define SHRT_MIN   (-SHRT_MAX - 1)
+#define INT_MAX    __INT_MAX__
+#define INT_MIN    (-INT_MAX - 1)
+
+#define UNIQUE_FUNCNAME(func, line) test_ ## func ## _ ## line
+#define FUNCNAME(func, line)        UNIQUE_FUNCNAME (func, line)
+
+#define AR(func, type, min, max, val)					\
+  void __attribute__ ((noclone, noinline))				\
+  FUNCNAME (func, __LINE__) (char *d, const char *s, type n)		\
+  {									\
+    if ((type)min <= n && n <= (type)max)				\
+      n = val;								\
+    __builtin_ ## func (d, s, n);					\
+  } typedef void DummyType
+
+AR (memcpy, short, SHRT_MIN, 0, 1);
+AR (memcpy, short, SHRT_MIN, 1, 2);
+AR (memcpy, short, 2, SHRT_MAX, 1);
+
+AR (memcpy, int, INT_MIN, 0, 1);
+AR (memcpy, int, INT_MIN, 1, 2);
+AR (memcpy, int, INT_MIN, 2, 3);
+AR (memcpy, int, 2, INT_MAX, 1);
+AR (memcpy, int, 2, INT_MAX, 1);
+
+AR (memmove, short, 2, SHRT_MAX, 1);
+AR (memmove, int,   2, INT_MAX, 1);
+
+AR (mempcpy, short, 2, SHRT_MAX, 1);
+AR (mempcpy, int,   2, INT_MAX, 1);
+
+/* { dg-final { scan-tree-dump-times "builtin_memcpy" 8 "optimized" } }
+   { dg-final { scan-tree-dump-times "builtin_memmove" 2 "optimized" } }
+   { dg-final { scan-tree-dump-times "builtin_mempcpy" 2 "optimized" } }  */

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Richard Biener]

On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
> test triggered by a recent VRP improvement.  A simple test case
> that approximates the warning is:
>
>   void f (char *d, const char *s, size_t n)
>   {
>     if (n > 0 && n <= SIZE_MAX / 2)
>       n = 0;
>
>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>   }
>
> Since the only valid value of n is zero the call to memcpy can
> be considered a no-op (a value of n > SIZE_MAX is in excess of
> the maximum size of the largest object and would surely make
> the call crash).
>
> The important difference between the test case and Bug 81908
> is that in the latter, the code is emitted by GCC itself from
> what appears to be correct source (looks like it's the result
> of the loop distribution pass).  I believe the warning for
> the test case above and for other human-written code like it
> is helpful, but warning for code emitted by GCC, even if it's
> dead or unreachable, is obviously not (at least not to users).
>
> The attached patch enhances the gimple_fold_builtin_memory_op
> function to eliminate this patholohgical case by making use
> of range information to fold into no-ops calls to memcpy whose
> size argument is in a range where the only valid value is zero.
> This gets rid of the warning and improves the emitted code.
>
> Tested on x86_64-linux.

@@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
   tree destvar, srcvar;
   location_t loc = gimple_location (stmt);

+  if (tree valid_len = only_valid_value (len))
+    {
+      /* LEN is in range whose only valid value is zero.  */
+      len = valid_len;
+    }
+
   /* If the LEN parameter is zero, return DEST.  */
   if (integer_zerop (len))

just enhance this check to

  if (integer_zerop (len)
      || size_must_be_zero_p (len))

?  'only_valid_value' looks too generic for this.

+  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
+    return NULL_TREE;
+

why?

+  if (wi::eq_p (min, wone)
+      && wi::geu_p (max + 1, ssize_max))

   if (wi::eq_p (min, 1)
      && wi::gtu_p (max, wi::max_value (prec, SIGNED)))

your ssize_max isn't signed size max, and max + 1 might overflow to zero.

Richard.



> Martin

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

On 08/24/2017 08:03 AM, Richard Biener wrote:
> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>> test triggered by a recent VRP improvement.  A simple test case
>> that approximates the warning is:
>>
>>   void f (char *d, const char *s, size_t n)
>>   {
>>     if (n > 0 && n <= SIZE_MAX / 2)
>>       n = 0;
>>
>>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>   }
>>
>> Since the only valid value of n is zero the call to memcpy can
>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>> the maximum size of the largest object and would surely make
>> the call crash).
>>
>> The important difference between the test case and Bug 81908
>> is that in the latter, the code is emitted by GCC itself from
>> what appears to be correct source (looks like it's the result
>> of the loop distribution pass).  I believe the warning for
>> the test case above and for other human-written code like it
>> is helpful, but warning for code emitted by GCC, even if it's
>> dead or unreachable, is obviously not (at least not to users).
>>
>> The attached patch enhances the gimple_fold_builtin_memory_op
>> function to eliminate this patholohgical case by making use
>> of range information to fold into no-ops calls to memcpy whose
>> size argument is in a range where the only valid value is zero.
>> This gets rid of the warning and improves the emitted code.
>>
>> Tested on x86_64-linux.
>
> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
>    tree destvar, srcvar;
>    location_t loc = gimple_location (stmt);
>
> +  if (tree valid_len = only_valid_value (len))
> +    {
> +      /* LEN is in range whose only valid value is zero.  */
> +      len = valid_len;
> +    }
> +
>    /* If the LEN parameter is zero, return DEST.  */
>    if (integer_zerop (len))
>
> just enhance this check to
>
>   if (integer_zerop (len)
>       || size_must_be_zero_p (len))
>
> ?  'only_valid_value' looks too generic for this.

Sure.

FWIW, the reason I did it this was because my original patch
returned error_mark_node for entirely invalid ranges and had
the caller replace the call with a trap.  I decided not to
include that part in this fix to keep it contained.

>
> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
> +    return NULL_TREE;
> +
>
> why?

Only because I never remember what APIs are safe to use with
what input.

> +  if (wi::eq_p (min, wone)
> +      && wi::geu_p (max + 1, ssize_max))
>
>    if (wi::eq_p (min, 1)
>       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>
> your ssize_max isn't signed size max, and max + 1 might overflow to zero.

You're right that the addition to max would be better done
as subtraction from the result of (1 << N).  Thank you.

If (max + 1) overflowed then (max == TYPE_MAX) would have
to hold which I thought could never be true for an anti
range. (The patch includes tests for this case.)  Was I
wrong?

Attached is an updated version with the suggested changes
plus an additional test to verify the absence of warnings.

Martin

[-- Attachment #2: gcc-81908.diff --]
[-- Type: text/x-patch, Size: 7024 bytes --]

PR middle-end/81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32

gcc/ChangeLog:

	PR middle-end/81908
	* gimple-fold.c (size_must_be_zero_p): New function.
	(gimple_fold_builtin_memory_op): Call it.

gcc/testsuite/ChangeLog:

	PR middle-end/81908
	* gcc.dg/tree-ssa/builtins-folding-gimple-2.c: New test.
	* gcc.dg/tree-ssa/builtins-folding-gimple-3.c: New test.
	* gcc.dg/tree-ssa/pr81908.c: New test.

diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 251446c..c4184fa 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -628,6 +628,36 @@ var_decl_component_p (tree var)
   return SSA_VAR_P (inner);
 }
 
+/* If the SIZE argument representing the size of an object is in a range
+   of values of which exactly one is valid (and that is zero), return
+   true, otherwise false.  */
+
+static bool
+size_must_be_zero_p (tree size)
+{
+  if (integer_zerop (size))
+    return true;
+
+  if (TREE_CODE (size) != SSA_NAME)
+    return false;
+
+  wide_int min, max;
+  enum value_range_type rtype = get_range_info (size, &min, &max);
+  if (rtype != VR_ANTI_RANGE)
+    return false;
+
+  tree type = TREE_TYPE (size);
+  int prec = TYPE_PRECISION (type);
+
+  wide_int wone = wi::one (prec);
+
+  /* Compute the value of SSIZE_MAX, the largest positive value that
+     can be stored in ssize_t, the signed counterpart of size_t .  */
+  wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
+
+  return wi::eq_p (min, wone) && wi::geu_p (max, ssize_max);
+}
+
 /* Fold function call to builtin mem{{,p}cpy,move}.  Return
    false if no simplification can be made.
    If ENDP is 0, return DEST (like memcpy).
@@ -646,8 +676,9 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
   tree destvar, srcvar;
   location_t loc = gimple_location (stmt);
 
-  /* If the LEN parameter is zero, return DEST.  */
-  if (integer_zerop (len))
+  /* If the LEN parameter is a constant zero or in range where
+     the only valid value is zero, return DEST.  */
+  if (size_must_be_zero_p (len))
     {
       gimple *repl;
       if (gimple_call_lhs (stmt))
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c
new file mode 100644
index 0000000..5bdb342
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-2.c
@@ -0,0 +1,44 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   Test to verify that calls to memcpy et al. where the size is in a range
+   with just one valid value -- zero -- are eliminated.
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#define INT_MAX    __INT_MAX__
+#define SHRT_MAX   __SHRT_MAX__
+#define SIZE_MAX   __SIZE_MAX__
+#define SSIZE_MAX  (SIZE_MAX / 2)
+
+typedef __PTRDIFF_TYPE__ ssize_t;
+typedef __SIZE_TYPE__    size_t;
+
+#define UNIQUE_FUNCNAME(func, line) test_ ## func ## _ ## line
+#define FUNCNAME(func, line)        UNIQUE_FUNCNAME (func, line)
+
+#define AR(func, type, min, max, val)					\
+  void __attribute__ ((noclone, noinline))				\
+  FUNCNAME (func, __LINE__) (char *d, const char *s, type n)		\
+  {									\
+    if ((type)min <= n && n <= (type)max)				\
+      n = val;								\
+    __builtin_ ## func (d, s, n);					\
+  } typedef void DummyType
+
+AR (memcpy, short, 1, SHRT_MAX, 0);
+AR (memcpy, int,   1, INT_MAX, 0);
+AR (memcpy, size_t,  1, SSIZE_MAX, 0);
+AR (memcpy, ssize_t, 1, SSIZE_MAX, 0);
+
+AR (memmove, short, 1, SHRT_MAX, 0);
+AR (memmove, int,   1, INT_MAX, 0);
+AR (memmove, ssize_t, 1, SSIZE_MAX, 0);
+AR (memmove, ssize_t, 1, SSIZE_MAX, 0);
+
+AR (mempcpy, short, 1, SHRT_MAX, 0);
+AR (mempcpy, int,   1, INT_MAX, 0);
+AR (mempcpy, size_t,  1, SSIZE_MAX, 0);
+AR (mempcpy, ssize_t, 1, SSIZE_MAX, 0);
+
+/* { dg-final { scan-tree-dump-not "builtin_memcpy" "optimized" } }
+   { dg-final { scan-tree-dump-not "builtin_memmove" "optimized" } }
+   { dg-final { scan-tree-dump-not "builtin_mempcpy" "optimized" } }  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c
new file mode 100644
index 0000000..716be5b8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple-3.c
@@ -0,0 +1,43 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   Test to verify that calls to memcpy et al. where the size is in a range
+   with more than one valid value are not eliminated (this test complements
+   builtins-folding-gimple-2.c).
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#define SHRT_MAX   __SHRT_MAX__
+#define SHRT_MIN   (-SHRT_MAX - 1)
+#define INT_MAX    __INT_MAX__
+#define INT_MIN    (-INT_MAX - 1)
+
+#define UNIQUE_FUNCNAME(func, line) test_ ## func ## _ ## line
+#define FUNCNAME(func, line)        UNIQUE_FUNCNAME (func, line)
+
+#define AR(func, type, min, max, val)					\
+  void __attribute__ ((noclone, noinline))				\
+  FUNCNAME (func, __LINE__) (char *d, const char *s, type n)		\
+  {									\
+    if ((type)min <= n && n <= (type)max)				\
+      n = val;								\
+    __builtin_ ## func (d, s, n);					\
+  } typedef void DummyType
+
+AR (memcpy, short, SHRT_MIN, 0, 1);
+AR (memcpy, short, SHRT_MIN, 1, 2);
+AR (memcpy, short, 2, SHRT_MAX, 1);
+
+AR (memcpy, int, INT_MIN, 0, 1);
+AR (memcpy, int, INT_MIN, 1, 2);
+AR (memcpy, int, INT_MIN, 2, 3);
+AR (memcpy, int, 2, INT_MAX, 1);
+AR (memcpy, int, 2, INT_MAX, 1);
+
+AR (memmove, short, 2, SHRT_MAX, 1);
+AR (memmove, int,   2, INT_MAX, 1);
+
+AR (mempcpy, short, 2, SHRT_MAX, 1);
+AR (mempcpy, int,   2, INT_MAX, 1);
+
+/* { dg-final { scan-tree-dump-times "builtin_memcpy" 8 "optimized" } }
+   { dg-final { scan-tree-dump-times "builtin_memmove" 2 "optimized" } }
+   { dg-final { scan-tree-dump-times "builtin_mempcpy" 2 "optimized" } }  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr81908.c b/gcc/testsuite/gcc.dg/tree-ssa/pr81908.c
new file mode 100644
index 0000000..b1e316a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr81908.c
@@ -0,0 +1,46 @@
+/* PR 81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+#define SIZE_MAX __SIZE_MAX__
+typedef __SIZE_TYPE__ size_t;
+
+void f0 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX / 2 - 1)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);
+}
+
+void f1 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX / 2)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);   /* { dg-bogus "\\\[-Wstringop-overflow=]" } */
+}
+
+void f2 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX / 2 + 1)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);
+}
+
+void f3 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX - 1)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);   /* { dg-bogus "\\\[-Wstringop-overflow=]" } */
+}
+
+void f4 (char *d, const char *s, size_t n)
+{
+  if (n > 0 && n <= SIZE_MAX)
+    n = 0;
+
+  __builtin_memcpy (d, s, n);
+}

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Jeff Law]

On 08/24/2017 09:03 AM, Martin Sebor wrote:
> On 08/24/2017 08:03 AM, Richard Biener wrote:
>> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>>> test triggered by a recent VRP improvement.  A simple test case
>>> that approximates the warning is:
>>>
>>>   void f (char *d, const char *s, size_t n)
>>>   {
>>>     if (n > 0 && n <= SIZE_MAX / 2)
>>>       n = 0;
>>>
>>>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>>   }
>>>
>>> Since the only valid value of n is zero the call to memcpy can
>>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>>> the maximum size of the largest object and would surely make
>>> the call crash).
>>>
>>> The important difference between the test case and Bug 81908
>>> is that in the latter, the code is emitted by GCC itself from
>>> what appears to be correct source (looks like it's the result
>>> of the loop distribution pass).  I believe the warning for
>>> the test case above and for other human-written code like it
>>> is helpful, but warning for code emitted by GCC, even if it's
>>> dead or unreachable, is obviously not (at least not to users).
>>>
>>> The attached patch enhances the gimple_fold_builtin_memory_op
>>> function to eliminate this patholohgical case by making use
>>> of range information to fold into no-ops calls to memcpy whose
>>> size argument is in a range where the only valid value is zero.
>>> This gets rid of the warning and improves the emitted code.
>>>
>>> Tested on x86_64-linux.
>>
>> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op
>> (gimple_stmt_iterator *gsi,
>>    tree destvar, srcvar;
>>    location_t loc = gimple_location (stmt);
>>
>> +  if (tree valid_len = only_valid_value (len))
>> +    {
>> +      /* LEN is in range whose only valid value is zero.  */
>> +      len = valid_len;
>> +    }
>> +
>>    /* If the LEN parameter is zero, return DEST.  */
>>    if (integer_zerop (len))
>>
>> just enhance this check to
>>
>>   if (integer_zerop (len)
>>       || size_must_be_zero_p (len))
>>
>> ?  'only_valid_value' looks too generic for this.
> 
> Sure.
> 
> FWIW, the reason I did it this was because my original patch
> returned error_mark_node for entirely invalid ranges and had
> the caller replace the call with a trap.  I decided not to
> include that part in this fix to keep it contained.
Seems reasonable.  Though I would suggest going forward with trap
replacement for clearly invalid ranges as a follow-up.  Once you do trap
replacement, the input operands all become dead as does all the code
after the trap and the outgoing edges in the CFG.

That often exposes a fair amount of cleanup.  Furthermore on targets
that have conditional traps, the controlling condition often turns into
a conditional trap.  In all, you get a lot of nice cascading effects
when you turn something that is clearly bogus into a trap.

gimple-ssa-isolate-erroneous-paths probably has the infrastructure you
need, including a code you can crib to detect PHI arguments which would
cause bogus behavior and allow you to isolate that specific path.



> 
>>
>> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
>> +    return NULL_TREE;
>> +
>>
>> why?
> 
> Only because I never remember what APIs are safe to use with
> what input.
> 
>> +  if (wi::eq_p (min, wone)
>> +      && wi::geu_p (max + 1, ssize_max))
>>
>>    if (wi::eq_p (min, 1)
>>       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>>
>> your ssize_max isn't signed size max, and max + 1 might overflow to zero.
> 
> You're right that the addition to max would be better done
> as subtraction from the result of (1 << N).  Thank you.
> 
> If (max + 1) overflowed then (max == TYPE_MAX) would have
> to hold which I thought could never be true for an anti
> range. (The patch includes tests for this case.)  Was I
> wrong?
Couldn't we have an anti range like ~[TYPE_MAX,TYPE_MAX]?  Or am I
misunderstanding something.

> 
> Attached is an updated version with the suggested changes
> plus an additional test to verify the absence of warnings.
The patch is OK.

I'll note this is another use of anti-ranges.  I'd really like to see
Aldy's work on the new range API move forward and get rid of anti-ranges.

THanks,
jeff

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Martin Sebor]

On 08/24/2017 03:52 PM, Jeff Law wrote:
> On 08/24/2017 09:03 AM, Martin Sebor wrote:
>> On 08/24/2017 08:03 AM, Richard Biener wrote:
>>> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>>>> test triggered by a recent VRP improvement.  A simple test case
>>>> that approximates the warning is:
>>>>
>>>>   void f (char *d, const char *s, size_t n)
>>>>   {
>>>>     if (n > 0 && n <= SIZE_MAX / 2)
>>>>       n = 0;
>>>>
>>>>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>>>   }
>>>>
>>>> Since the only valid value of n is zero the call to memcpy can
>>>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>>>> the maximum size of the largest object and would surely make
>>>> the call crash).
>>>>
>>>> The important difference between the test case and Bug 81908
>>>> is that in the latter, the code is emitted by GCC itself from
>>>> what appears to be correct source (looks like it's the result
>>>> of the loop distribution pass).  I believe the warning for
>>>> the test case above and for other human-written code like it
>>>> is helpful, but warning for code emitted by GCC, even if it's
>>>> dead or unreachable, is obviously not (at least not to users).
>>>>
>>>> The attached patch enhances the gimple_fold_builtin_memory_op
>>>> function to eliminate this patholohgical case by making use
>>>> of range information to fold into no-ops calls to memcpy whose
>>>> size argument is in a range where the only valid value is zero.
>>>> This gets rid of the warning and improves the emitted code.
>>>>
>>>> Tested on x86_64-linux.
>>>
>>> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op
>>> (gimple_stmt_iterator *gsi,
>>>    tree destvar, srcvar;
>>>    location_t loc = gimple_location (stmt);
>>>
>>> +  if (tree valid_len = only_valid_value (len))
>>> +    {
>>> +      /* LEN is in range whose only valid value is zero.  */
>>> +      len = valid_len;
>>> +    }
>>> +
>>>    /* If the LEN parameter is zero, return DEST.  */
>>>    if (integer_zerop (len))
>>>
>>> just enhance this check to
>>>
>>>   if (integer_zerop (len)
>>>       || size_must_be_zero_p (len))
>>>
>>> ?  'only_valid_value' looks too generic for this.
>>
>> Sure.
>>
>> FWIW, the reason I did it this was because my original patch
>> returned error_mark_node for entirely invalid ranges and had
>> the caller replace the call with a trap.  I decided not to
>> include that part in this fix to keep it contained.
> Seems reasonable.  Though I would suggest going forward with trap
> replacement for clearly invalid ranges as a follow-up.  Once you do trap
> replacement, the input operands all become dead as does all the code
> after the trap and the outgoing edges in the CFG.
>
> That often exposes a fair amount of cleanup.  Furthermore on targets
> that have conditional traps, the controlling condition often turns into
> a conditional trap.  In all, you get a lot of nice cascading effects
> when you turn something that is clearly bogus into a trap.
>
> gimple-ssa-isolate-erroneous-paths probably has the infrastructure you
> need, including a code you can crib to detect PHI arguments which would
> cause bogus behavior and allow you to isolate that specific path.

Thanks for the pointer.  I'll look into it.

>>> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
>>> +    return NULL_TREE;
>>> +
>>>
>>> why?
>>
>> Only because I never remember what APIs are safe to use with
>> what input.
>>
>>> +  if (wi::eq_p (min, wone)
>>> +      && wi::geu_p (max + 1, ssize_max))
>>>
>>>    if (wi::eq_p (min, 1)
>>>       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>>>
>>> your ssize_max isn't signed size max, and max + 1 might overflow to zero.
>>
>> You're right that the addition to max would be better done
>> as subtraction from the result of (1 << N).  Thank you.
>>
>> If (max + 1) overflowed then (max == TYPE_MAX) would have
>> to hold which I thought could never be true for an anti
>> range. (The patch includes tests for this case.)  Was I
>> wrong?
> Couldn't we have an anti range like ~[TYPE_MAX,TYPE_MAX]?  Or am I
> misunderstanding something.

I've never seen such an anti-range.  In my tests, a range one
of whose bounds is equal to one of the type's limits) ends up
canonicalized into an ordinary range.  If I'm reading the code
right it's done in set_and_canonicalize_value_range.

>> Attached is an updated version with the suggested changes
>> plus an additional test to verify the absence of warnings.
> The patch is OK.

Thanks.  Patch committed in r251347.

>
> I'll note this is another use of anti-ranges.  I'd really like to see
> Aldy's work on the new range API move forward and get rid of anti-ranges.

You and me both :)

Martin

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Richard Biener]

On August 24, 2017 11:52:41 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>On 08/24/2017 09:03 AM, Martin Sebor wrote:
>> On 08/24/2017 08:03 AM, Richard Biener wrote:
>>> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com>
>wrote:
>>>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>>>> test triggered by a recent VRP improvement.  A simple test case
>>>> that approximates the warning is:
>>>>
>>>>   void f (char *d, const char *s, size_t n)
>>>>   {
>>>>     if (n > 0 && n <= SIZE_MAX / 2)
>>>>       n = 0;
>>>>
>>>>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>>>   }
>>>>
>>>> Since the only valid value of n is zero the call to memcpy can
>>>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>>>> the maximum size of the largest object and would surely make
>>>> the call crash).
>>>>
>>>> The important difference between the test case and Bug 81908
>>>> is that in the latter, the code is emitted by GCC itself from
>>>> what appears to be correct source (looks like it's the result
>>>> of the loop distribution pass).  I believe the warning for
>>>> the test case above and for other human-written code like it
>>>> is helpful, but warning for code emitted by GCC, even if it's
>>>> dead or unreachable, is obviously not (at least not to users).
>>>>
>>>> The attached patch enhances the gimple_fold_builtin_memory_op
>>>> function to eliminate this patholohgical case by making use
>>>> of range information to fold into no-ops calls to memcpy whose
>>>> size argument is in a range where the only valid value is zero.
>>>> This gets rid of the warning and improves the emitted code.
>>>>
>>>> Tested on x86_64-linux.
>>>
>>> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op
>>> (gimple_stmt_iterator *gsi,
>>>    tree destvar, srcvar;
>>>    location_t loc = gimple_location (stmt);
>>>
>>> +  if (tree valid_len = only_valid_value (len))
>>> +    {
>>> +      /* LEN is in range whose only valid value is zero.  */
>>> +      len = valid_len;
>>> +    }
>>> +
>>>    /* If the LEN parameter is zero, return DEST.  */
>>>    if (integer_zerop (len))
>>>
>>> just enhance this check to
>>>
>>>   if (integer_zerop (len)
>>>       || size_must_be_zero_p (len))
>>>
>>> ?  'only_valid_value' looks too generic for this.
>> 
>> Sure.
>> 
>> FWIW, the reason I did it this was because my original patch
>> returned error_mark_node for entirely invalid ranges and had
>> the caller replace the call with a trap.  I decided not to
>> include that part in this fix to keep it contained.
>Seems reasonable.  Though I would suggest going forward with trap
>replacement for clearly invalid ranges as a follow-up.  Once you do
>trap
>replacement, the input operands all become dead as does all the code
>after the trap and the outgoing edges in the CFG.
>
>That often exposes a fair amount of cleanup.  Furthermore on targets
>that have conditional traps, the controlling condition often turns into
>a conditional trap.  In all, you get a lot of nice cascading effects
>when you turn something that is clearly bogus into a trap.
>
>gimple-ssa-isolate-erroneous-paths probably has the infrastructure you
>need, including a code you can crib to detect PHI arguments which would
>cause bogus behavior and allow you to isolate that specific path.
>
>
>
>> 
>>>
>>> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
>>> +    return NULL_TREE;
>>> +
>>>
>>> why?
>> 
>> Only because I never remember what APIs are safe to use with
>> what input.
>> 
>>> +  if (wi::eq_p (min, wone)
>>> +      && wi::geu_p (max + 1, ssize_max))
>>>
>>>    if (wi::eq_p (min, 1)
>>>       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>>>
>>> your ssize_max isn't signed size max, and max + 1 might overflow to
>zero.
>> 
>> You're right that the addition to max would be better done
>> as subtraction from the result of (1 << N).  Thank you.
>> 
>> If (max + 1) overflowed then (max == TYPE_MAX) would have
>> to hold which I thought could never be true for an anti
>> range. (The patch includes tests for this case.)  Was I
>> wrong?
>Couldn't we have an anti range like ~[TYPE_MAX,TYPE_MAX]?  Or am I
>misunderstanding something.
>
>> 
>> Attached is an updated version with the suggested changes
>> plus an additional test to verify the absence of warnings.
>The patch is OK.
>
>I'll note this is another use of anti-ranges.  I'd really like to see
>Aldy's work on the new range API move forward and get rid of
>anti-ranges.

Note that anti-ranges are not complicated to handle. In fact the current API could be trivially extended to return two ranges for them. The complication will be in the callers which will have to handle the unioning.

I'd like to get rid of anti-ranges in VRP and mainly for the reason that handling unioning of N ranges will increase precision. I do not see too many uses of the increased precision outside of VRP which is where Aldhys work is solely residing. 

Richard. 

>THanks,
>jeff

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Richard Sandiford]

Martin Sebor <msebor@gmail.com> writes:
> On 08/24/2017 08:03 AM, Richard Biener wrote:
>> On Wed, Aug 23, 2017 at 9:42 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> Bug 81908 is about a -Wstringop-overflow warning for a Fortran
>>> test triggered by a recent VRP improvement.  A simple test case
>>> that approximates the warning is:
>>>
>>>   void f (char *d, const char *s, size_t n)
>>>   {
>>>     if (n > 0 && n <= SIZE_MAX / 2)
>>>       n = 0;
>>>
>>>     memcpy (d, s, n);   // n in ~[1, SIZE_MAX / 2]
>>>   }
>>>
>>> Since the only valid value of n is zero the call to memcpy can
>>> be considered a no-op (a value of n > SIZE_MAX is in excess of
>>> the maximum size of the largest object and would surely make
>>> the call crash).
>>>
>>> The important difference between the test case and Bug 81908
>>> is that in the latter, the code is emitted by GCC itself from
>>> what appears to be correct source (looks like it's the result
>>> of the loop distribution pass).  I believe the warning for
>>> the test case above and for other human-written code like it
>>> is helpful, but warning for code emitted by GCC, even if it's
>>> dead or unreachable, is obviously not (at least not to users).
>>>
>>> The attached patch enhances the gimple_fold_builtin_memory_op
>>> function to eliminate this patholohgical case by making use
>>> of range information to fold into no-ops calls to memcpy whose
>>> size argument is in a range where the only valid value is zero.
>>> This gets rid of the warning and improves the emitted code.
>>>
>>> Tested on x86_64-linux.
>>
>> @@ -646,6 +677,12 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
>>    tree destvar, srcvar;
>>    location_t loc = gimple_location (stmt);
>>
>> +  if (tree valid_len = only_valid_value (len))
>> +    {
>> +      /* LEN is in range whose only valid value is zero.  */
>> +      len = valid_len;
>> +    }
>> +
>>    /* If the LEN parameter is zero, return DEST.  */
>>    if (integer_zerop (len))
>>
>> just enhance this check to
>>
>>   if (integer_zerop (len)
>>       || size_must_be_zero_p (len))
>>
>> ?  'only_valid_value' looks too generic for this.
>
> Sure.
>
> FWIW, the reason I did it this was because my original patch
> returned error_mark_node for entirely invalid ranges and had
> the caller replace the call with a trap.  I decided not to
> include that part in this fix to keep it contained.
>
>>
>> +  if (!wi::fits_uhwi_p (min) || !wi::fits_uhwi_p (max))
>> +    return NULL_TREE;
>> +
>>
>> why?
>
> Only because I never remember what APIs are safe to use with
> what input.
>
>> +  if (wi::eq_p (min, wone)
>> +      && wi::geu_p (max + 1, ssize_max))
>>
>>    if (wi::eq_p (min, 1)
>>       && wi::gtu_p (max, wi::max_value (prec, SIGNED)))
>>
>> your ssize_max isn't signed size max, and max + 1 might overflow to zero.
>
> You're right that the addition to max would be better done
> as subtraction from the result of (1 << N).  Thank you.
>
> If (max + 1) overflowed then (max == TYPE_MAX) would have
> to hold which I thought could never be true for an anti
> range. (The patch includes tests for this case.)  Was I
> wrong?
>
> Attached is an updated version with the suggested changes
> plus an additional test to verify the absence of warnings.
>
> Martin
>
> PR middle-end/81908 - FAIL: gfortran.dg/alloc_comp_auto_array_2.f90 -O3 -g -m32
>
> gcc/ChangeLog:
>
> 	PR middle-end/81908
> 	* gimple-fold.c (size_must_be_zero_p): New function.
> 	(gimple_fold_builtin_memory_op): Call it.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/81908
> 	* gcc.dg/tree-ssa/builtins-folding-gimple-2.c: New test.
> 	* gcc.dg/tree-ssa/builtins-folding-gimple-3.c: New test.
> 	* gcc.dg/tree-ssa/pr81908.c: New test.
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index 251446c..c4184fa 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -628,6 +628,36 @@ var_decl_component_p (tree var)
>    return SSA_VAR_P (inner);
>  }
>  
> +/* If the SIZE argument representing the size of an object is in a range
> +   of values of which exactly one is valid (and that is zero), return
> +   true, otherwise false.  */
> +
> +static bool
> +size_must_be_zero_p (tree size)
> +{
> +  if (integer_zerop (size))
> +    return true;
> +
> +  if (TREE_CODE (size) != SSA_NAME)
> +    return false;
> +
> +  wide_int min, max;
> +  enum value_range_type rtype = get_range_info (size, &min, &max);
> +  if (rtype != VR_ANTI_RANGE)
> +    return false;
> +
> +  tree type = TREE_TYPE (size);
> +  int prec = TYPE_PRECISION (type);
> +
> +  wide_int wone = wi::one (prec);
> +
> +  /* Compute the value of SSIZE_MAX, the largest positive value that
> +     can be stored in ssize_t, the signed counterpart of size_t .  */
> +  wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
> +
> +  return wi::eq_p (min, wone) && wi::geu_p (max, ssize_max);

Just a stylistic thing, but since the only use of "wone" is in
the eq_p, it'd be simpler just to use "1".  Also, the maximum
value is better calculated as "wi::max_value (prec, SIGNED)".  So:

  /* Compute the value of SSIZE_MAX, the largest positive value that
     can be stored in ssize_t, the signed counterpart of size_t .  */
  wide_int ssize_max = wi::max_value (prec, SIGNED);

  return wi::eq_p (min, 1) && wi::geu_p (max, ssize_max);

Thanks,
Richard

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Martin Sebor]

> Just a stylistic thing, but since the only use of "wone" is in
> the eq_p, it'd be simpler just to use "1".  Also, the maximum
> value is better calculated as "wi::max_value (prec, SIGNED)".  So:
>
>   /* Compute the value of SSIZE_MAX, the largest positive value that
>      can be stored in ssize_t, the signed counterpart of size_t .  */
>   wide_int ssize_max = wi::max_value (prec, SIGNED);
>
>   return wi::eq_p (min, 1) && wi::geu_p (max, ssize_max);

Thanks, I didn't know about the implicit conversion or the max_value
API.  I'll remember to use them the next time.

FWIW, going slightly off topic, and while I'm sure it's a matter
of getting used to the design, I can't say I find the wide_int
classes exactly intuitive.  It seems that the most natural way
to write the return statement in C++ is:

   return min == 1 && max >= ssize_max;

The first subexpression is accepted but the second one doesn't even
compile.  If it did, it would treat the operands as signed which
isn't what I need here.  One still has to resort to the clunky
predicate for it:

   return min == 1 && wi::geu_p (max, ssize_max);

It also doesn't help that the names of the predicates don't follow
the same convention as those that operate on trees (e.g., eq_p vs
tree_int_cst_equal).

Unless there's some inherent limitation that makes it impossible
to use the class the way I would like it might be worth investing
some time into making it more user-friendly.

Martin

Re: [PATCH] handle pathological anti-ranges in gimple_fold_builtin_memory_op (PR 81908)

[Richard Sandiford]

Martin Sebor <msebor@gmail.com> writes:
>> Just a stylistic thing, but since the only use of "wone" is in
>> the eq_p, it'd be simpler just to use "1".  Also, the maximum
>> value is better calculated as "wi::max_value (prec, SIGNED)".  So:
>>
>>   /* Compute the value of SSIZE_MAX, the largest positive value that
>>      can be stored in ssize_t, the signed counterpart of size_t .  */
>>   wide_int ssize_max = wi::max_value (prec, SIGNED);
>>
>>   return wi::eq_p (min, 1) && wi::geu_p (max, ssize_max);
>
> Thanks, I didn't know about the implicit conversion or the max_value
> API.  I'll remember to use them the next time.
>
> FWIW, going slightly off topic, and while I'm sure it's a matter
> of getting used to the design, I can't say I find the wide_int
> classes exactly intuitive.  It seems that the most natural way
> to write the return statement in C++ is:
>
>    return min == 1 && max >= ssize_max;
>
> The first subexpression is accepted but the second one doesn't even
> compile.  If it did, it would treat the operands as signed which
> isn't what I need here.  One still has to resort to the clunky
> predicate for it:
>
>    return min == 1 && wi::geu_p (max, ssize_max);

There are two main reasons for the wi:: stuff:

1. When we were adding wide-int initially, one of the requirements
   was that we could operate directly on rtx and tree representations
   of integers, without first converting them to wide_int etc.
   Since trees and rtxes are pointers, it wouldn't make sense
   to rely on operator overloading there.

2. wide_int itself has no inherent sign.  (So I disagree with
   "If it did, it would treat the operands as signed which isn't what
   I need here."  The reason for not having the overload is exactly
   that there is no obvious choice between signed and unsigned here.)

And yes, Richard has been giving me grief about the lack of a
wide_int-with-sign :-)

> It also doesn't help that the names of the predicates don't follow
> the same convention as those that operate on trees (e.g., eq_p vs
> tree_int_cst_equal).
>
> Unless there's some inherent limitation that makes it impossible
> to use the class the way I would like it might be worth investing
> some time into making it more user-friendly.

I think we already provide the overloads we can (i.e. those where
the sign is known or isn't relevant).  Let me know if we've missed
some though.

Thanks,
Richard