Re: [PATCH] c++: build_new_1 and non-dep array size [PR111929]
On Tue, 24 Oct 2023, Jason Merrill wrote:
> On 10/24/23 13:03, Patrick Palka wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> > like the right approach?
> >
> > -- >8 --
> >
> > This PR is another instance of NON_DEPENDENT_EXPR having acted as an
> > "analysis barrier" for middle-end routines, and now that it's gone we
> > may end up passing weird templated trees (that have a generic tree code)
> > to the middle-end which leads to an ICE. In the testcase below the
> > non-dependent array size 'var + 42' is expressed as an ordinary
> > PLUS_EXPR, but whose operand types have different precisions -- long and
> > int respectively -- naturally because templated trees encode only the
> > syntactic form of an expression devoid of e.g. implicit conversions
> > (typically). This type incoherency triggers a wide_int assert during
> > the call to size_binop in build_new_1 which requires the operand types
> > have the same precision.
> >
> > This patch fixes this by replacing our incremental folding of 'size'
> > within build_new_1 with a single call to cp_fully_fold (which is a no-op
> > in template context) once 'size' is fully built.
>
> This is OK, but we could probably also entirely skip a lot of the calculation
> in a template, since we don't care about any values. Can we skip the entire
> if (array_p) block?
That seems to be safe correctness-wise, but QOI-wise it'd mean we'd no
longer diagnose a too large array size ahead of time:
template
void f() {
new int[__SIZE_MAX__ / sizeof(int)];
}
: In function ‘void f()’:
:3:37: error: size ‘(((sizetype)(18446744073709551615 / sizeof (int)))
* 4)’ of array exceeds maximum object size ‘9223372036854775807’
(That we diagnose this ahead of time is thanks to the NON_DEPENDENT_EXPR
removal; previously 'nelts' was wrapped in NON_DEPENDENT_EXPR which
ironically prevented fold_non_dependent_expr from folding it to a
constant...)
>
> > PR c++/111929
> >
> > gcc/cp/ChangeLog:
> >
> > * init.cc (build_new_1): Use convert, build2, build3 instead of
> > fold_convert, size_binop and fold_build3 when building 'size'.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/template/non-dependent28.C: New test.
> > ---
> > gcc/cp/init.cc | 9 +
> > gcc/testsuite/g++.dg/template/non-dependent28.C | 6 ++
> > 2 files changed, 11 insertions(+), 4 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent28.C
> >
> > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
> > index d48bb16c7c5..56c1b5e9f5e 100644
> > --- a/gcc/cp/init.cc
> > +++ b/gcc/cp/init.cc
> > @@ -3261,7 +3261,7 @@ build_new_1 (vec **placement, tree type,
> > tree nelts,
> > max_outer_nelts = wi::udiv_trunc (max_size, inner_size);
> > max_outer_nelts_tree = wide_int_to_tree (sizetype, max_outer_nelts);
> > - size = size_binop (MULT_EXPR, size, fold_convert (sizetype,
> > nelts));
> > + size = build2 (MULT_EXPR, sizetype, size, convert (sizetype, nelts));
> > if (TREE_CODE (cst_outer_nelts) == INTEGER_CST)
> > {
> > @@ -3344,7 +3344,7 @@ build_new_1 (vec **placement, tree type,
> > tree nelts,
> > /* Use a class-specific operator new. */
> > /* If a cookie is required, add some extra space. */
> > if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
> > - size = size_binop (PLUS_EXPR, size, cookie_size);
> > + size = build2 (PLUS_EXPR, sizetype, size, cookie_size);
> > else
> > {
> > cookie_size = NULL_TREE;
> > @@ -3358,8 +3358,8 @@ build_new_1 (vec **placement, tree type,
> > tree nelts,
> > if (cxx_dialect >= cxx11 && flag_exceptions)
> > errval = throw_bad_array_new_length ();
> > if (outer_nelts_check != NULL_TREE)
> > - size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check,
> > - size, errval);
> > + size = build3 (COND_EXPR, sizetype, outer_nelts_check, size, errval);
> > + size = cp_fully_fold (size);
> > /* Create the argument list. */
> > vec_safe_insert (*placement, 0, size);
> > /* Do name-lookup to find the appropriate operator. */
> > @@ -3418,6 +3418,7 @@ build_new_1 (vec **placement, tree type,
> > tree nelts,
> > /* If size is zero e.g. due to type having zero size, try to
> > preserve outer_nelts for constant expression evaluation
> > purposes. */
> > + size = cp_fully_fold (size);
> > if (integer_zerop (size) && outer_nelts)
> > size = build2 (MULT_EXPR, TREE_TYPE (size), size, outer_nelts);
> > diff --git a/gcc/testsuite/g++.dg/template/non-dependent28.C
> > b/gcc/testsuite/g++.dg/template/non-dependent28.C
> > new file mode 100644
> > index 000..3e45154f61d
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/non-dependent28.C
> > @@ -0,0 +1,6 @@
> > +// PR c++/111929
> > +
> > +template
> > +void f(long var) {
> > + new int[var + 42];
> > +}
>
Re: [PATCH] c++: build_new_1 and non-dep array size [PR111929]
On 10/24/23 13:03, Patrick Palka wrote:
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
like the right approach?
-- >8 --
This PR is another instance of NON_DEPENDENT_EXPR having acted as an
"analysis barrier" for middle-end routines, and now that it's gone we
may end up passing weird templated trees (that have a generic tree code)
to the middle-end which leads to an ICE. In the testcase below the
non-dependent array size 'var + 42' is expressed as an ordinary
PLUS_EXPR, but whose operand types have different precisions -- long and
int respectively -- naturally because templated trees encode only the
syntactic form of an expression devoid of e.g. implicit conversions
(typically). This type incoherency triggers a wide_int assert during
the call to size_binop in build_new_1 which requires the operand types
have the same precision.
This patch fixes this by replacing our incremental folding of 'size'
within build_new_1 with a single call to cp_fully_fold (which is a no-op
in template context) once 'size' is fully built.
This is OK, but we could probably also entirely skip a lot of the
calculation in a template, since we don't care about any values. Can we
skip the entire if (array_p) block?
PR c++/111929
gcc/cp/ChangeLog:
* init.cc (build_new_1): Use convert, build2, build3 instead of
fold_convert, size_binop and fold_build3 when building 'size'.
gcc/testsuite/ChangeLog:
* g++.dg/template/non-dependent28.C: New test.
---
gcc/cp/init.cc | 9 +
gcc/testsuite/g++.dg/template/non-dependent28.C | 6 ++
2 files changed, 11 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/template/non-dependent28.C
diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
index d48bb16c7c5..56c1b5e9f5e 100644
--- a/gcc/cp/init.cc
+++ b/gcc/cp/init.cc
@@ -3261,7 +3261,7 @@ build_new_1 (vec **placement, tree type,
tree nelts,
max_outer_nelts = wi::udiv_trunc (max_size, inner_size);
max_outer_nelts_tree = wide_int_to_tree (sizetype, max_outer_nelts);
- size = size_binop (MULT_EXPR, size, fold_convert (sizetype, nelts));
+ size = build2 (MULT_EXPR, sizetype, size, convert (sizetype, nelts));
if (TREE_CODE (cst_outer_nelts) == INTEGER_CST)
{
@@ -3344,7 +3344,7 @@ build_new_1 (vec **placement, tree type,
tree nelts,
/* Use a class-specific operator new. */
/* If a cookie is required, add some extra space. */
if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
- size = size_binop (PLUS_EXPR, size, cookie_size);
+ size = build2 (PLUS_EXPR, sizetype, size, cookie_size);
else
{
cookie_size = NULL_TREE;
@@ -3358,8 +3358,8 @@ build_new_1 (vec **placement, tree type,
tree nelts,
if (cxx_dialect >= cxx11 && flag_exceptions)
errval = throw_bad_array_new_length ();
if (outer_nelts_check != NULL_TREE)
- size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check,
- size, errval);
+ size = build3 (COND_EXPR, sizetype, outer_nelts_check, size, errval);
+ size = cp_fully_fold (size);
/* Create the argument list. */
vec_safe_insert (*placement, 0, size);
/* Do name-lookup to find the appropriate operator. */
@@ -3418,6 +3418,7 @@ build_new_1 (vec **placement, tree type,
tree nelts,
/* If size is zero e.g. due to type having zero size, try to
preserve outer_nelts for constant expression evaluation
purposes. */
+ size = cp_fully_fold (size);
if (integer_zerop (size) && outer_nelts)
size = build2 (MULT_EXPR, TREE_TYPE (size), size, outer_nelts);
diff --git a/gcc/testsuite/g++.dg/template/non-dependent28.C b/gcc/testsuite/g++.dg/template/non-dependent28.C
new file mode 100644
index 000..3e45154f61d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/non-dependent28.C
@@ -0,0 +1,6 @@
+// PR c++/111929
+
+template
+void f(long var) {
+ new int[var + 42];
+}
[PATCH] c++: build_new_1 and non-dep array size [PR111929]
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
like the right approach?
-- >8 --
This PR is another instance of NON_DEPENDENT_EXPR having acted as an
"analysis barrier" for middle-end routines, and now that it's gone we
may end up passing weird templated trees (that have a generic tree code)
to the middle-end which leads to an ICE. In the testcase below the
non-dependent array size 'var + 42' is expressed as an ordinary
PLUS_EXPR, but whose operand types have different precisions -- long and
int respectively -- naturally because templated trees encode only the
syntactic form of an expression devoid of e.g. implicit conversions
(typically). This type incoherency triggers a wide_int assert during
the call to size_binop in build_new_1 which requires the operand types
have the same precision.
This patch fixes this by replacing our incremental folding of 'size'
within build_new_1 with a single call to cp_fully_fold (which is a no-op
in template context) once 'size' is fully built.
PR c++/111929
gcc/cp/ChangeLog:
* init.cc (build_new_1): Use convert, build2, build3 instead of
fold_convert, size_binop and fold_build3 when building 'size'.
gcc/testsuite/ChangeLog:
* g++.dg/template/non-dependent28.C: New test.
---
gcc/cp/init.cc | 9 +
gcc/testsuite/g++.dg/template/non-dependent28.C | 6 ++
2 files changed, 11 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/template/non-dependent28.C
diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
index d48bb16c7c5..56c1b5e9f5e 100644
--- a/gcc/cp/init.cc
+++ b/gcc/cp/init.cc
@@ -3261,7 +3261,7 @@ build_new_1 (vec **placement, tree type,
tree nelts,
max_outer_nelts = wi::udiv_trunc (max_size, inner_size);
max_outer_nelts_tree = wide_int_to_tree (sizetype, max_outer_nelts);
- size = size_binop (MULT_EXPR, size, fold_convert (sizetype, nelts));
+ size = build2 (MULT_EXPR, sizetype, size, convert (sizetype, nelts));
if (TREE_CODE (cst_outer_nelts) == INTEGER_CST)
{
@@ -3344,7 +3344,7 @@ build_new_1 (vec **placement, tree type,
tree nelts,
/* Use a class-specific operator new. */
/* If a cookie is required, add some extra space. */
if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
- size = size_binop (PLUS_EXPR, size, cookie_size);
+ size = build2 (PLUS_EXPR, sizetype, size, cookie_size);
else
{
cookie_size = NULL_TREE;
@@ -3358,8 +3358,8 @@ build_new_1 (vec **placement, tree type,
tree nelts,
if (cxx_dialect >= cxx11 && flag_exceptions)
errval = throw_bad_array_new_length ();
if (outer_nelts_check != NULL_TREE)
- size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check,
- size, errval);
+ size = build3 (COND_EXPR, sizetype, outer_nelts_check, size, errval);
+ size = cp_fully_fold (size);
/* Create the argument list. */
vec_safe_insert (*placement, 0, size);
/* Do name-lookup to find the appropriate operator. */
@@ -3418,6 +3418,7 @@ build_new_1 (vec **placement, tree type,
tree nelts,
/* If size is zero e.g. due to type having zero size, try to
preserve outer_nelts for constant expression evaluation
purposes. */
+ size = cp_fully_fold (size);
if (integer_zerop (size) && outer_nelts)
size = build2 (MULT_EXPR, TREE_TYPE (size), size, outer_nelts);
diff --git a/gcc/testsuite/g++.dg/template/non-dependent28.C
b/gcc/testsuite/g++.dg/template/non-dependent28.C
new file mode 100644
index 000..3e45154f61d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/non-dependent28.C
@@ -0,0 +1,6 @@
+// PR c++/111929
+
+template
+void f(long var) {
+ new int[var + 42];
+}
--
2.42.0.424.gceadf0f3cf
