C++ PATCH for c++/91740 - ICE with constexpr call and ?: in ARRAY_REF

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

C++ PATCH for c++/91740 - ICE with constexpr call and ?: in ARRAY_REF

Marek Polacek-3
This ICEs since r267272 - more location wrapper nodes, but not because we can't
cope with new location wrappers, but because that commit introduced a call to
maybe_constant_value in cp_build_array_ref.  In this testcase we call it with

  f (VIEW_CONVERT_EXPR<const char[4]>("BAR")) ? 1 : 0

argument and that crashes in fold_convert because we end up trying to convert
"BAR" of type const char[4] to const char * when evaluating the call.  At this
point, decay_conversion hasn't turned the argument into (const char *) "BAR"
yet.

The ICE doesn't occur without :?, because then the call will be wrapped in
NON_DEPENDENT_EXPR and constexpr throws its hands (I'm anthropomorphizing) up
when it encounters such an expression.

I noticed that build_non_dependent_expr doesn't wrap op0 of ?: in N_D_E.  This
is so since r70606 -- Nathan, is there a reason not to do it?  Doing it fixes
this problem.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-09-11  Marek Polacek  <[hidden email]>

        PR c++/91740 - ICE with constexpr call and ?: in ARRAY_REF.
        * pt.c (build_non_dependent_expr): Call build_non_dependent_expr for
        the first operand.

        * g++.dg/cpp1y/var-templ63.C: New test.

diff --git gcc/cp/pt.c gcc/cp/pt.c
index c5915a5ecd0..775389d8245 100644
--- gcc/cp/pt.c
+++ gcc/cp/pt.c
@@ -26709,7 +26709,7 @@ build_non_dependent_expr (tree expr)
   if (TREE_CODE (expr) == COND_EXPR)
     return build3 (COND_EXPR,
    TREE_TYPE (expr),
-   TREE_OPERAND (expr, 0),
+   build_non_dependent_expr (TREE_OPERAND (expr, 0)),
    (TREE_OPERAND (expr, 1)
     ? build_non_dependent_expr (TREE_OPERAND (expr, 1))
     : build_non_dependent_expr (TREE_OPERAND (expr, 0))),
diff --git gcc/testsuite/g++.dg/cpp1y/var-templ63.C gcc/testsuite/g++.dg/cpp1y/var-templ63.C
new file mode 100644
index 00000000000..a65f53b2963
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/var-templ63.C
@@ -0,0 +1,5 @@
+// PR c++/91740 - ICE with constexpr call and ?: in ARRAY_REF.
+// { dg-do compile { target c++14 } }
+
+constexpr bool f(const char*) { return true; }
+template<typename T> const char c = "FOO"[f("BAR") ? 1 : 0];
Reply | Threaded
Open this post in threaded view
|

Re: C++ PATCH for c++/91740 - ICE with constexpr call and ?: in ARRAY_REF

Paolo Carlini-3
Hi,

On 11/09/19 23:15, Marek Polacek wrote:

> --- gcc/cp/pt.c
> +++ gcc/cp/pt.c
> @@ -26709,7 +26709,7 @@ build_non_dependent_expr (tree expr)
>     if (TREE_CODE (expr) == COND_EXPR)
>       return build3 (COND_EXPR,
>     TREE_TYPE (expr),
> -   TREE_OPERAND (expr, 0),
> +   build_non_dependent_expr (TREE_OPERAND (expr, 0)),
>     (TREE_OPERAND (expr, 1)
>      ? build_non_dependent_expr (TREE_OPERAND (expr, 1))
>      : build_non_dependent_expr (TREE_OPERAND (expr, 0))),

Looks like we would end up unnecessarily calling
build_non_dependent_expr three times instead of two: probably is very
cheap, probably the code is cleaner this way but I'm a little annoyed at
this anyway, for the record ;)

Cheers, Paolo.

Reply | Threaded
Open this post in threaded view
|

Re: C++ PATCH for c++/91740 - ICE with constexpr call and ?: in ARRAY_REF

Paolo Carlini-3
Hi again,

On 12/09/19 11:03, Paolo Carlini wrote:

> Hi,
>
> On 11/09/19 23:15, Marek Polacek wrote:
>> --- gcc/cp/pt.c
>> +++ gcc/cp/pt.c
>> @@ -26709,7 +26709,7 @@ build_non_dependent_expr (tree expr)
>>     if (TREE_CODE (expr) == COND_EXPR)
>>       return build3 (COND_EXPR,
>>              TREE_TYPE (expr),
>> -           TREE_OPERAND (expr, 0),
>> +           build_non_dependent_expr (TREE_OPERAND (expr, 0)),
>>              (TREE_OPERAND (expr, 1)
>>               ? build_non_dependent_expr (TREE_OPERAND (expr, 1))
>>               : build_non_dependent_expr (TREE_OPERAND (expr, 0))),
>
> Looks like we would end up unnecessarily calling
> build_non_dependent_expr three times instead of two: probably is very
> cheap, probably the code is cleaner this way but I'm a little annoyed
> at this anyway, for the record ;)

Sorry, I misread the code: normally TREE_OPERAND (expr, 1) isn't
NULL_TREE thus we are fine.

Paolo.

Reply | Threaded
Open this post in threaded view
|

Re: C++ PATCH for c++/91740 - ICE with constexpr call and ?: in ARRAY_REF

Marek Polacek-3
On Thu, Sep 12, 2019 at 11:08:43AM +0200, Paolo Carlini wrote:

> Hi again,
>
> On 12/09/19 11:03, Paolo Carlini wrote:
> > Hi,
> >
> > On 11/09/19 23:15, Marek Polacek wrote:
> > > --- gcc/cp/pt.c
> > > +++ gcc/cp/pt.c
> > > @@ -26709,7 +26709,7 @@ build_non_dependent_expr (tree expr)
> > >     if (TREE_CODE (expr) == COND_EXPR)
> > >       return build3 (COND_EXPR,
> > >              TREE_TYPE (expr),
> > > -           TREE_OPERAND (expr, 0),
> > > +           build_non_dependent_expr (TREE_OPERAND (expr, 0)),
> > >              (TREE_OPERAND (expr, 1)
> > >               ? build_non_dependent_expr (TREE_OPERAND (expr, 1))
> > >               : build_non_dependent_expr (TREE_OPERAND (expr, 0))),
> >
> > Looks like we would end up unnecessarily calling
> > build_non_dependent_expr three times instead of two: probably is very
> > cheap, probably the code is cleaner this way but I'm a little annoyed at
> > this anyway, for the record ;)
>
> Sorry, I misread the code: normally TREE_OPERAND (expr, 1) isn't NULL_TREE
> thus we are fine.

And I forgot to mention that build_x_conditional_expr has

 6743       ifexp = build_non_dependent_expr (ifexp);
 6744       if (op1)
 6745         op1 = build_non_dependent_expr (op1);
 6746       op2 = build_non_dependent_expr (op2);

which means my fix should make more sense.

--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Reply | Threaded
Open this post in threaded view
|

Re: C++ PATCH for c++/91740 - ICE with constexpr call and ?: in ARRAY_REF

Jason Merrill
In reply to this post by Marek Polacek-3
On 9/11/19 4:15 PM, Marek Polacek wrote:

> This ICEs since r267272 - more location wrapper nodes, but not because we can't
> cope with new location wrappers, but because that commit introduced a call to
> maybe_constant_value in cp_build_array_ref.  In this testcase we call it with
>
>    f (VIEW_CONVERT_EXPR<const char[4]>("BAR")) ? 1 : 0
>
> argument and that crashes in fold_convert because we end up trying to convert
> "BAR" of type const char[4] to const char * when evaluating the call.  At this
> point, decay_conversion hasn't turned the argument into (const char *) "BAR"
> yet.
>
> The ICE doesn't occur without :?, because then the call will be wrapped in
> NON_DEPENDENT_EXPR and constexpr throws its hands (I'm anthropomorphizing) up
> when it encounters such an expression.
>
> I noticed that build_non_dependent_expr doesn't wrap op0 of ?: in N_D_E.  This
> is so since r70606 -- Nathan, is there a reason not to do it?  Doing it fixes
> this problem.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2019-09-11  Marek Polacek  <[hidden email]>
>
> PR c++/91740 - ICE with constexpr call and ?: in ARRAY_REF.
> * pt.c (build_non_dependent_expr): Call build_non_dependent_expr for
> the first operand.
>
> * g++.dg/cpp1y/var-templ63.C: New test.
>
> diff --git gcc/cp/pt.c gcc/cp/pt.c
> index c5915a5ecd0..775389d8245 100644
> --- gcc/cp/pt.c
> +++ gcc/cp/pt.c
> @@ -26709,7 +26709,7 @@ build_non_dependent_expr (tree expr)
>     if (TREE_CODE (expr) == COND_EXPR)
>       return build3 (COND_EXPR,
>     TREE_TYPE (expr),
> -   TREE_OPERAND (expr, 0),
> +   build_non_dependent_expr (TREE_OPERAND (expr, 0)),
>     (TREE_OPERAND (expr, 1)
>      ? build_non_dependent_expr (TREE_OPERAND (expr, 1))
>      : build_non_dependent_expr (TREE_OPERAND (expr, 0))),

OK.  I wonder why this code copies op0 for the ?: extension rather than
leave it null?

Jason