[C++] PR 68087

Hi,

for this small 5/6 regression (an ICE) Markus suggests in the audit 
trail to protect the tree_to_shwi call with tree_fits_shwi_p. I tweaked 
a bit his suggestion adopting a pattern already used, eg, in the parser, 
and successfully tested it in trunk. I suppose it could make sense to 
also fix the issue in the branch, if we can do that safely, and I also 
propose below a variant for that (cxx_eval_array_reference is slightly 
different in the branch). Ok?

Thanks,
Paolo.

////////////////////////////
/cp
2015-11-25  Markus Trippelsdorf  <markus@trippelsdorf.de>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/68087
	* constexpr.c (cxx_eval_array_reference): Use tree_fits_shwi_p before
	tree_to_shwi to avoid ICEs.

/testsuite
2015-11-25  Markus Trippelsdorf  <markus@trippelsdorf.de>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/68087
	* g++.dg/cpp0x/constexpr-array13.C: New.

Index: cp/constexpr.c
===================================================================
--- cp/constexpr.c	(revision 230866)
+++ cp/constexpr.c	(working copy)
@@ -1751,7 +1751,7 @@ cxx_eval_array_reference (const constexpr_ctx *ctx
       gcc_unreachable ();
     }
 
-  i = tree_to_shwi (index);
+  i = tree_fits_shwi_p (index) ? tree_to_shwi (index) : -1;
   bool found = true;
   if (TREE_CODE (ary) == CONSTRUCTOR && len
       && (TREE_CODE (CONSTRUCTOR_ELT (ary, len-1)->index) == RANGE_EXPR
Index: testsuite/g++.dg/cpp0x/constexpr-array13.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-array13.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/constexpr-array13.C	(working copy)
@@ -0,0 +1,6 @@
+// PR c++/68087
+// { dg-do compile { target c++11 } }
+
+constexpr char c[] = "hello";
+constexpr const char *p = c;
+constexpr char ch = *(p-1);  // { dg-error "negative array subscript" }

On 2015.11.25 at 16:43 +0100, Paolo Carlini wrote:
> Hi,
> 
> for this small 5/6 regression (an ICE) Markus suggests in the audit trail to
> protect the tree_to_shwi call with tree_fits_shwi_p. I tweaked a bit his
> suggestion adopting a pattern already used, eg, in the parser, and
> successfully tested it in trunk. I suppose it could make sense to also fix
> the issue in the branch, if we can do that safely, and I also propose below
> a variant for that (cxx_eval_array_reference is slightly different in the
> branch). Ok?
> 
> ////////////////////////////

> /cp
> 2015-11-25  Markus Trippelsdorf  <markus@trippelsdorf.de>
> 	    Paolo Carlini  <paolo.carlini@oracle.com>
> 
> 	PR c++/68087
> 	* constexpr.c (cxx_eval_array_reference): Use tree_fits_shwi_p before
> 	tree_to_shwi to avoid ICEs.
> 
> /testsuite
> 2015-11-25  Markus Trippelsdorf  <markus@trippelsdorf.de>
> 	    Paolo Carlini  <paolo.carlini@oracle.com>
> 
> 	PR c++/68087
> 	* g++.dg/cpp0x/constexpr-array13.C: New.

> Index: cp/constexpr.c
> ===================================================================
> --- cp/constexpr.c	(revision 230865)
> +++ cp/constexpr.c	(working copy)
> @@ -1799,8 +1799,8 @@ cxx_eval_array_reference (const constexpr_ctx *ctx
>        gcc_unreachable ();
>      }
>  
> -  i = tree_to_shwi (index);
> -  if (i < 0)
> +  if (!tree_fits_shwi_p (index)
> +      || (i = tree_to_shwi (index)) < 0)

Last time Richard pointed out that:
    if (wi::lts_p (index, 0))  
is more idiomatic.

Hi,

On 11/25/2015 04:59 PM, Markus Trippelsdorf wrote:
>> Index: cp/constexpr.c
>> ===================================================================
>> --- cp/constexpr.c	(revision 230865)
>> +++ cp/constexpr.c	(working copy)
>> @@ -1799,8 +1799,8 @@ cxx_eval_array_reference (const constexpr_ctx *ctx
>>         gcc_unreachable ();
>>       }
>>   
>> -  i = tree_to_shwi (index);
>> -  if (i < 0)
>> +  if (!tree_fits_shwi_p (index)
>> +      || (i = tree_to_shwi (index)) < 0)
> Last time Richard pointed out that:
>      if (wi::lts_p (index, 0))
> is more idiomatic.
I see, but isn't used anywhere else in the whole cp/ and in the case at 
issue we still need to assign to 'i', thus I would rather follow the 
existing practice in the front-end...

Paolo.

Both are OK.

Jason

On Wed, Nov 25, 2015 at 5:09 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 11/25/2015 04:59 PM, Markus Trippelsdorf wrote:
>>>
>>> Index: cp/constexpr.c
>>> ===================================================================
>>> --- cp/constexpr.c      (revision 230865)
>>> +++ cp/constexpr.c      (working copy)
>>> @@ -1799,8 +1799,8 @@ cxx_eval_array_reference (const constexpr_ctx *ctx
>>>         gcc_unreachable ();
>>>       }
>>>   -  i = tree_to_shwi (index);
>>> -  if (i < 0)
>>> +  if (!tree_fits_shwi_p (index)
>>> +      || (i = tree_to_shwi (index)) < 0)
>>
>> Last time Richard pointed out that:
>>      if (wi::lts_p (index, 0))
>> is more idiomatic.
>
> I see, but isn't used anywhere else in the whole cp/ and in the case at
> issue we still need to assign to 'i', thus I would rather follow the
> existing practice in the front-end...

You can also use tree_int_cst_sgn (index) == -1 (which uses wi::neg_p
(index) internally).

Richard.

> Paolo.