C++: Fix ICE in warn_for_memset within templates (PR c++/83814)

Message ID 1515794964-35447-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • C++: Fix ICE in warn_for_memset within templates (PR c++/83814)
Related show

Commit Message

David Malcolm Jan. 12, 2018, 10:09 p.m.
PR c++/83814 reports an ICE introduced by the location wrapper patch
(r256448), affecting certain memset calls within templates.

The issue occurs when fold_for_warn is called on the arguments to the
memset before they've been type-checked, leading to e.g. this assertion
failing within fold_binary_loc:

9739      gcc_assert (TYPE_PRECISION (atype) == TYPE_PRECISION (type));

when folding a PLUS_EXPR of an int and a char.

I added these fold_for_warn calls in:
  [v3 of 05/14] C++: handle locations wrappers when calling warn_for_memset
    https://gcc.gnu.org/ml/gcc-patches/2017-12/msg01378.html

Jason: looking back at the discussion, I added them in response to
your comment:
  "warn_for_memset may be missing some calls to fold_for_warn."
in:
     https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00672.html

It seemed that you wanted me to use fold_for_warn for stripping
location wrappers at warnings, but fold_for_warn (currently) assumes
that the expression it's folding has sane types.

This patch fixes the issue by updating the C++ implementation of
fold_for_warn so that it doesn't fold when processing a template.
Doing so fixes the ICE.  I made this case strip location wrappers,
rather than just being a no-op, which ensures that we do still warn
within a template for e.g. -Wmemset-transposed-args.  Similarly, I
retained the:
  if (TREE_CODE (x) == CONST_DECL)
    x = DECL_INITIAL (x);
so that we can continue to look through enum values (as we did prior
to r256448).

Given that processing_template_decl is C++-specific I had to move
fold_for_warn from c-common.c, splitting it out into separate C and
C++ implementations.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
OK for trunk?

gcc/c-family/ChangeLog:
	PR c++/83814
	* c-common.c (fold_for_warn): Move to c/c-fold.c and cp/expr.c.

gcc/c/ChangeLog:
	PR c++/83814
	* c-fold.c (fold_for_warn): Move from c-common.c, reducing to just
	the C part.

gcc/cp/ChangeLog:
	PR c++/83814
	* expr.c (fold_for_warn): Move from c-common.c, reducing to just
	the C++ part.  If processing a template, merely strip location
	wrappers rather than fully folding.

gcc/testsuite/ChangeLog:
	PR c++/83814
	* g++.dg/wrappers/pr83814.C: New test case.
---
 gcc/c-family/c-common.c                 | 13 -------
 gcc/c/c-fold.c                          | 10 ++++++
 gcc/cp/expr.c                           | 22 ++++++++++++
 gcc/testsuite/g++.dg/wrappers/pr83814.C | 63 +++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/wrappers/pr83814.C

Comments

Jakub Jelinek Jan. 17, 2018, 10:34 a.m. | #1
On Fri, Jan 12, 2018 at 05:09:24PM -0500, David Malcolm wrote:
> PR c++/83814 reports an ICE introduced by the location wrapper patch
> (r256448), affecting certain memset calls within templates.

Note, I think this issue sadly affects a lot of code, so it is quite urgent.

That said, wonder if we really can't do any folding when
processing_template_decl, could we e.g. do at least maybe_constant_value,
or fold if the expression is not type nor value dependent?

BTW, never know if cp_fold_rvalue is a superset of maybe_constant_value or not.

> --- a/gcc/cp/expr.c
> +++ b/gcc/cp/expr.c
> @@ -315,3 +315,25 @@ mark_exp_read (tree exp)
>      }
>  }
>  
> +/* Fold X for consideration by one of the warning functions when checking
> +   whether an expression has a constant value.  */
> +
> +tree
> +fold_for_warn (tree x)
> +{
> +  /* C++ implementation.  */
> +
> +  /* It's not generally safe to fold inside of a template, so
> +     merely strip any location wrapper and read through enum values.  */
> +  if (processing_template_decl)
> +    {
> +      STRIP_ANY_LOCATION_WRAPPER (x);
> +
> +      if (TREE_CODE (x) == CONST_DECL)
> +	x = DECL_INITIAL (x);
> +
> +      return x;
> +    }
> +
> +  return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL);
> +}

	Jakub
Jason Merrill Jan. 17, 2018, 2:28 p.m. | #2
On Wed, Jan 17, 2018 at 5:34 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 12, 2018 at 05:09:24PM -0500, David Malcolm wrote:
>> PR c++/83814 reports an ICE introduced by the location wrapper patch
>> (r256448), affecting certain memset calls within templates.
>
> Note, I think this issue sadly affects a lot of code, so it is quite urgent.
>
> That said, wonder if we really can't do any folding when
> processing_template_decl, could we e.g. do at least maybe_constant_value,
> or fold if the expression is not type nor value dependent?

Yes, in a template we should call fold_non_dependent_expr.

> BTW, never know if cp_fold_rvalue is a superset of maybe_constant_value or not.

It is.

Jason

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 097d192..858ed68 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -868,19 +868,6 @@  c_get_substring_location (const substring_loc &substr_loc,
 }
 
 
-/* Fold X for consideration by one of the warning functions when checking
-   whether an expression has a constant value.  */
-
-tree
-fold_for_warn (tree x)
-{
-  if (c_dialect_cxx ())
-    return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL);
-  else
-    /* The C front-end has already folded X appropriately.  */
-    return x;
-}
-
 /* Return true iff T is a boolean promoted to int.  */
 
 bool
diff --git a/gcc/c/c-fold.c b/gcc/c/c-fold.c
index 5776f1b..be6a0fc 100644
--- a/gcc/c/c-fold.c
+++ b/gcc/c/c-fold.c
@@ -668,3 +668,13 @@  c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands,
     }
   return ret;
 }
+
+/* Fold X for consideration by one of the warning functions when checking
+   whether an expression has a constant value.  */
+
+tree
+fold_for_warn (tree x)
+{
+  /* The C front-end has already folded X appropriately.  */
+  return x;
+}
diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c
index 7d79215..3b20c9b 100644
--- a/gcc/cp/expr.c
+++ b/gcc/cp/expr.c
@@ -315,3 +315,25 @@  mark_exp_read (tree exp)
     }
 }
 
+/* Fold X for consideration by one of the warning functions when checking
+   whether an expression has a constant value.  */
+
+tree
+fold_for_warn (tree x)
+{
+  /* C++ implementation.  */
+
+  /* It's not generally safe to fold inside of a template, so
+     merely strip any location wrapper and read through enum values.  */
+  if (processing_template_decl)
+    {
+      STRIP_ANY_LOCATION_WRAPPER (x);
+
+      if (TREE_CODE (x) == CONST_DECL)
+	x = DECL_INITIAL (x);
+
+      return x;
+    }
+
+  return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL);
+}
diff --git a/gcc/testsuite/g++.dg/wrappers/pr83814.C b/gcc/testsuite/g++.dg/wrappers/pr83814.C
new file mode 100644
index 0000000..5bbae7b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/wrappers/pr83814.C
@@ -0,0 +1,63 @@ 
+/* Verify that our memset warnings don't crash when folding
+   arguments within a template (PR c++/83814).  */
+
+// { dg-options "-Wno-int-to-pointer-cast -Wmemset-transposed-args -Wmemset-elt-size" }
+
+template <class>
+void test_1()
+{
+  __builtin_memset (int() - char(), 0, 0);
+}
+
+template <class>
+void test_2()
+{
+  __builtin_memset (0, 0, int() - char());
+}
+
+template <class>
+void test_3 (unsigned a, int c)
+{
+  __builtin_memset((char *)c + a, 0, a);
+}
+
+template <class>
+void test_4 (unsigned a, int c)
+{
+  __builtin_memset(0, 0, (char *)c + a);
+}
+
+/* Verify that we warn for -Wmemset-transposed-args inside
+   a template.  */
+
+char buf[1024];
+
+template <class>
+void test_5 ()
+{
+  __builtin_memset (buf, sizeof buf, 0); // { dg-warning "transposed parameters" }
+}
+
+/* Adapted from c-c++-common/memset-array.c; verify that 
+   -Wmemset-elt-size works within a template.  */
+
+enum a {
+  a_1,
+  a_2,
+  a_n
+};
+int t1[20];
+int t2[a_n];
+
+struct s
+{
+  int t[20];
+};
+
+template<class>
+void foo (struct s *s)
+{
+  __builtin_memset (t1, 0, 20); /* { dg-warning "element size" } */
+  __builtin_memset (t2, 0, a_n); /* { dg-warning "element size" } */
+  __builtin_memset (s->t, 0, 20); /* { dg-warning "element size" } */
+}