Re: [PATCH] c++: fix ICE with is_really_empty_class [PR110106]

[Jason Merrill <jason@redhat.com>]

On 7/20/23 14:13, Marek Polacek wrote:
> On Wed, Jul 19, 2023 at 10:11:27AM -0400, Patrick Palka wrote:
>> On Tue, 18 Jul 2023, Marek Polacek via Gcc-patches wrote:
>>
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk and branches?
>>
>> Looks reasonable to me.
> 
> Thanks.
>   
>> Though I wonder if we could also fix this by not checking potentiality
>> at all in this case?  The problematic call to is_rvalue_constant_expression
>> happens from cp_parser_constant_expression with 'allow_non_constant' != 0
>> and with 'non_constant_p' being a dummy out argument that comes from
>> cp_parser_functional_cast, so the result of is_rvalue_constant_expression
>> is effectively unused in this case, and we should be able to safely elide
>> it when 'allow_non_constant && non_constant_p == nullptr'.
> 
> Sounds plausible.  I think my patch could be applied first since it
> removes a tiny bit of code, then I can hopefully remove the flag below,
> then maybe go back and optimize the call to is_rvalue_constant_expression.
> Does that sound sensible?
> 
>> Relatedly, ISTM the member cp_parser::non_integral_constant_expression_p
>> is also effectively unused and could be removed?
> 
> It looks that way.  Seems it's only used in cp_parser_constant_expression:
> 10806   if (allow_non_constant_p)
> 10807     *non_constant_p = parser->non_integral_constant_expression_p;
> but that could be easily replaced by a local var.  I'd be happy to see if
> we can actually do away with it.  (I wonder why it was introduced and when
> it actually stopped being useful.)

It was for the C++98 notion of constant-expression, which was more of a 
parser-level notion, and has been supplanted by the C++11 version.  I'm 
happy to remove it, and therefore remove the 
is_rvalue_constant_expression call.

>>> -- >8 --
>>>
>>> is_really_empty_class is liable to crash when it gets an incomplete
>>> or dependent type.  Since r11-557, we pass the yet-uninstantiated
>>> class type S<0> of the PARM_DECL s to is_really_empty_class -- because
>>> of the potential_rvalue_constant_expression -> is_rvalue_constant_expression
>>> change in cp_parser_constant_expression.  Here we're not parsing
>>> a template so we did not check COMPLETE_TYPE_P as we should.
>>>
>>> 	PR c++/110106
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* constexpr.cc (potential_constant_expression_1): Check COMPLETE_TYPE_P
>>> 	even when !processing_template_decl.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/cpp0x/noexcept80.C: New test.
>>> ---
>>>   gcc/cp/constexpr.cc                     |  2 +-
>>>   gcc/testsuite/g++.dg/cpp0x/noexcept80.C | 12 ++++++++++++
>>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/noexcept80.C
>>>
>>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
>>> index 6e8f1c2b61e..1f59c5472fb 100644
>>> --- a/gcc/cp/constexpr.cc
>>> +++ b/gcc/cp/constexpr.cc
>>> @@ -9116,7 +9116,7 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>>>         if (now && want_rval)
>>>   	{
>>>   	  tree type = TREE_TYPE (t);
>>> -	  if ((processing_template_decl && !COMPLETE_TYPE_P (type))
>>> +	  if (!COMPLETE_TYPE_P (type)
>>>   	      || dependent_type_p (type)

There shouldn't be a problem completing the type here, so it seems to me 
that we're missing a call to complete_type_p, at least when 
!processing_template_decl.  Probably need to move the dependent_type_p 
check up as a result.

Jason