[v2] C++: Fix crash in warn_for_memset within templates (PR c++/83814)

Message ID 1516210020-13394-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • [v2] C++: Fix crash in warn_for_memset within templates (PR c++/83814)
Related show

Commit Message

David Malcolm Jan. 17, 2018, 5:27 p.m.
On Wed, 2018-01-17 at 09:28 -0500, Jason Merrill wrote:
> 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


Thanks.  Here's an updated version of the patch.

Changed in v2:
* use fold_non_dependent_expr in the C++ impl of fold_for_warn
* add some test coverage of folding to g++.dg/wrappers/pr83814.C
* added another testcase (from PR c++/83902)

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, call
	fold_non_dependent_expr rather than fully folding.

gcc/testsuite/ChangeLog:
	PR c++/83814
	PR c++/83902
	* g++.dg/wrappers/pr83814.C: New test case.
	* g++.dg/wrappers/pr83902.C: New test case.
---
 gcc/c-family/c-common.c                 | 13 ------
 gcc/c/c-fold.c                          | 10 +++++
 gcc/cp/expr.c                           | 15 +++++++
 gcc/testsuite/g++.dg/wrappers/pr83814.C | 70 +++++++++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/wrappers/pr83902.C |  9 +++++
 5 files changed, 104 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/wrappers/pr83814.C
 create mode 100644 gcc/testsuite/g++.dg/wrappers/pr83902.C

-- 
1.8.5.3

Comments

Jason Merrill Jan. 17, 2018, 5:43 p.m. | #1
OK.

On Wed, Jan 17, 2018 at 12:27 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Wed, 2018-01-17 at 09:28 -0500, Jason Merrill wrote:

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

>

> Thanks.  Here's an updated version of the patch.

>

> Changed in v2:

> * use fold_non_dependent_expr in the C++ impl of fold_for_warn

> * add some test coverage of folding to g++.dg/wrappers/pr83814.C

> * added another testcase (from PR c++/83902)

>

> 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, call

>         fold_non_dependent_expr rather than fully folding.

>

> gcc/testsuite/ChangeLog:

>         PR c++/83814

>         PR c++/83902

>         * g++.dg/wrappers/pr83814.C: New test case.

>         * g++.dg/wrappers/pr83902.C: New test case.

> ---

>  gcc/c-family/c-common.c                 | 13 ------

>  gcc/c/c-fold.c                          | 10 +++++

>  gcc/cp/expr.c                           | 15 +++++++

>  gcc/testsuite/g++.dg/wrappers/pr83814.C | 70 +++++++++++++++++++++++++++++++++

>  gcc/testsuite/g++.dg/wrappers/pr83902.C |  9 +++++

>  5 files changed, 104 insertions(+), 13 deletions(-)

>  create mode 100644 gcc/testsuite/g++.dg/wrappers/pr83814.C

>  create mode 100644 gcc/testsuite/g++.dg/wrappers/pr83902.C

>

> 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..b1ab453 100644

> --- a/gcc/cp/expr.c

> +++ b/gcc/cp/expr.c

> @@ -315,3 +315,18 @@ 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 fully fold inside of a template, so

> +     call fold_non_dependent_expr instead.  */

> +  if (processing_template_decl)

> +    return fold_non_dependent_expr (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..b9f8faa

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/wrappers/pr83814.C

> @@ -0,0 +1,70 @@

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

> +

> +  // This case requires reading through an enum value:

> +  __builtin_memset (t2, 0, a_n); // { dg-warning "element size" }

> +

> +  __builtin_memset (s->t, 0, 20); // { dg-warning "element size" }

> +

> +  // These cases require folding of arg2 within a template:

> +  __builtin_memset (t2, 0, a_n + 0); // { dg-warning "element size" }

> +  __builtin_memset (t2, 0, a_n * 1); // { dg-warning "element size" }

> +}

> diff --git a/gcc/testsuite/g++.dg/wrappers/pr83902.C b/gcc/testsuite/g++.dg/wrappers/pr83902.C

> new file mode 100644

> index 0000000..3d334e7

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/wrappers/pr83902.C

> @@ -0,0 +1,9 @@

> +extern "C" void *memset (void *, int, __SIZE_TYPE__);

> +void *p;

> +

> +template <int T>

> +struct B

> +{

> +  void foo () { memset (p, 0, 4 * T * sizeof(float)); }

> +};

> +

> --

> 1.8.5.3

>

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..b1ab453 100644
--- a/gcc/cp/expr.c
+++ b/gcc/cp/expr.c
@@ -315,3 +315,18 @@  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 fully fold inside of a template, so
+     call fold_non_dependent_expr instead.  */
+  if (processing_template_decl)
+    return fold_non_dependent_expr (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..b9f8faa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/wrappers/pr83814.C
@@ -0,0 +1,70 @@ 
+/* 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" }
+
+  // This case requires reading through an enum value:
+  __builtin_memset (t2, 0, a_n); // { dg-warning "element size" }
+
+  __builtin_memset (s->t, 0, 20); // { dg-warning "element size" }
+
+  // These cases require folding of arg2 within a template:
+  __builtin_memset (t2, 0, a_n + 0); // { dg-warning "element size" }
+  __builtin_memset (t2, 0, a_n * 1); // { dg-warning "element size" }
+}
diff --git a/gcc/testsuite/g++.dg/wrappers/pr83902.C b/gcc/testsuite/g++.dg/wrappers/pr83902.C
new file mode 100644
index 0000000..3d334e7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/wrappers/pr83902.C
@@ -0,0 +1,9 @@ 
+extern "C" void *memset (void *, int, __SIZE_TYPE__);
+void *p;
+
+template <int T>
+struct B
+{
+  void foo () { memset (p, 0, 4 * T * sizeof(float)); }
+};
+