[C++] Fix value initialized decltype(nullptr) in constexpr (PR c++/85553)

Hi!

init = fold (convert (type, nullptr_node)); unfortunately creates
a NOP_EXPR around INTEGER_CST and constexpr.c doesn't consider that a valid
constant; fold (convert (type, integer_zero_node)) we used previously
on the other side emitted warnings.

The following patch just builds the INTEGER_CST directly.

Tested on x86_64-linux with make check-c++-all, ok for trunk and 8.1
if it passes full bootstrap/regtest on {x86_64,i686}-linux?

2018-04-27  Jakub Jelinek  <jakub@redhat.com>

	PR c++/85553
	* init.c (build_zero_init_1): For zero initialization of
	NULLPTR_TYPE_P type use build_int_cst directly.

	* g++.dg/cpp0x/Wzero-as-null-pointer-constant-3.C: Add dg-bogus
	directive.
	* g++.dg/cpp0x/constexpr-85553.C: New test.


	Jakub

On Fri, Apr 27, 2018 at 08:07:36PM +0200, Jakub Jelinek wrote:
> init = fold (convert (type, nullptr_node)); unfortunately creates
> a NOP_EXPR around INTEGER_CST and constexpr.c doesn't consider that a valid
> constant; fold (convert (type, integer_zero_node)) we used previously
> on the other side emitted warnings.
> 
> The following patch just builds the INTEGER_CST directly.
> 
> Tested on x86_64-linux with make check-c++-all, ok for trunk and 8.1
> if it passes full bootstrap/regtest on {x86_64,i686}-linux?

Now successfully bootstrapped/regtested on both.  Ok?

> 2018-04-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/85553
> 	* init.c (build_zero_init_1): For zero initialization of
> 	NULLPTR_TYPE_P type use build_int_cst directly.
> 
> 	* g++.dg/cpp0x/Wzero-as-null-pointer-constant-3.C: Add dg-bogus
> 	directive.
> 	* g++.dg/cpp0x/constexpr-85553.C: New test.

	Jakub

Ok

On Fri, Apr 27, 2018, 2:07 PM Jakub Jelinek <jakub@redhat.com> wrote:

> Hi!
>
> init = fold (convert (type, nullptr_node)); unfortunately creates
> a NOP_EXPR around INTEGER_CST and constexpr.c doesn't consider that a valid
> constant; fold (convert (type, integer_zero_node)) we used previously
> on the other side emitted warnings.
>
> The following patch just builds the INTEGER_CST directly.
>
> Tested on x86_64-linux with make check-c++-all, ok for trunk and 8.1
> if it passes full bootstrap/regtest on {x86_64,i686}-linux?
>
> 2018-04-27  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/85553
>         * init.c (build_zero_init_1): For zero initialization of
>         NULLPTR_TYPE_P type use build_int_cst directly.
>
>         * g++.dg/cpp0x/Wzero-as-null-pointer-constant-3.C: Add dg-bogus
>         directive.
>         * g++.dg/cpp0x/constexpr-85553.C: New test.
>
> --- gcc/cp/init.c.jj    2018-04-27 19:11:56.613549524 +0200
> +++ gcc/cp/init.c       2018-04-27 19:20:50.102839130 +0200
> @@ -180,8 +180,10 @@ build_zero_init_1 (tree type, tree nelts
>         items with static storage duration that are not otherwise
>         initialized are initialized to zero.  */
>      ;
> -  else if (TYPE_PTR_OR_PTRMEM_P (type) || NULLPTR_TYPE_P (type))
> +  else if (TYPE_PTR_OR_PTRMEM_P (type))
>      init = fold (convert (type, nullptr_node));
> +  else if (NULLPTR_TYPE_P (type))
> +    init = build_int_cst (type, 0);
>    else if (SCALAR_TYPE_P (type))
>      init = fold (convert (type, integer_zero_node));
>    else if (RECORD_OR_UNION_CODE_P (TREE_CODE (type)))
> --- gcc/testsuite/g++.dg/cpp0x/Wzero-as-null-pointer-constant-3.C.jj
> 2018-04-12 10:22:56.640162364 +0200
> +++ gcc/testsuite/g++.dg/cpp0x/Wzero-as-null-pointer-constant-3.C
>  2018-04-27 19:23:58.349941329 +0200
> @@ -3,4 +3,4 @@
>  // { dg-options "-Wzero-as-null-pointer-constant" }
>
>  int* no_warn = {};
> -decltype( nullptr ) warn = {};
> +decltype( nullptr ) warn = {}; // { dg-bogus "zero as null pointer
> constant" }
> --- gcc/testsuite/g++.dg/cpp0x/constexpr-85553.C.jj     2018-04-27
> 19:24:33.547960437 +0200
> +++ gcc/testsuite/g++.dg/cpp0x/constexpr-85553.C        2018-04-27
> 19:24:51.456970160 +0200
> @@ -0,0 +1,4 @@
> +// PR c++/85553
> +// { dg-do compile { target c++11 } }
> +using T = decltype(nullptr);
> +const constexpr T foo{};
>
>         Jakub
>

Hi,

On 27/04/2018 22:13, Jason Merrill wrote:
> Ok
Maybe for 8.2.0 or whatever, would it make sense to special case convert 
itself? See what we do in, say, decay_conversion, we aren't afraid to 
immediately return nullptr_node...

Paolo.

On 27/04/2018 22:25, Paolo Carlini wrote:
> Hi,
>
> On 27/04/2018 22:13, Jason Merrill wrote:
>> Ok
> Maybe for 8.2.0 or whatever, would it make sense to special case 
> convert itself? See what we do in, say, decay_conversion, we aren't 
> afraid to immediately return nullptr_node...
... or more generally ocp_convert which already handles explicitly 
NULLPTR_TYPE, I'm coming to the conclusion that something is wrong or 
missing in it.

Paolo.

Hi again,

I'm now pretty sure that we have a latent issue in ocp_convert. The bug 
fixed by Jakub shows that we used to not have issues with 
integer_zero_node. That's easy to explain: at the beginning of 
ocp_convert there is code which handles first some special / simple 
cases when same_type_ignoring_top_level_qualifiers_p is true. That code 
isn't of course used for integer_zero_node as source expression, which 
therefore is handled by:

   if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
     {
       if (complain & tf_warning)
     maybe_warn_zero_as_null_pointer_constant (e, loc);
       return nullptr_node;
     }

which does the right thing. On the other hand the code at the beginning 
of ocp_covert *is* used for nullptr_node but it doesn't notice that the 
conversion is really trivial and wraps type and expr in a NOP_EXPR. 
Therefore, I think we should handle the special case there, like we 
handle it at the beginning of decay_conversion (careful with side 
effects, as we (I) learned the hard way with a bug!). Tested 
x86_64-linux, of course I also separately checked that the below would 
fix 85553 in a different way.

Thanks,
Paolo.

//////////////////
2018-04-28  Paolo Carlini  <paolo.carlini@oracle.com>

	* cvt.c (ocp_convert): Early handle the special case of
	nullptr converted to nullptr.
Index: cvt.c
===================================================================
--- cvt.c	(revision 259731)
+++ cvt.c	(working copy)
@@ -736,6 +736,8 @@ ocp_convert (tree type, tree expr, int convtype, i
 	  TREE_TYPE (e) = TREE_TYPE (TARGET_EXPR_SLOT (e)) = type;
 	  return e;
 	}
+      else if (NULLPTR_TYPE_P (type) && !TREE_SIDE_EFFECTS (e))
+	return nullptr_node;
       else
 	{
 	  /* We shouldn't be treating objects of ADDRESSABLE type as

On Fri, Apr 27, 2018 at 7:26 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi again,
>
> I'm now pretty sure that we have a latent issue in ocp_convert. The bug
> fixed by Jakub shows that we used to not have issues with integer_zero_node.
> That's easy to explain: at the beginning of ocp_convert there is code which
> handles first some special / simple cases when
> same_type_ignoring_top_level_qualifiers_p is true. That code isn't of course
> used for integer_zero_node as source expression, which therefore is handled
> by:
>
>   if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
>     {
>       if (complain & tf_warning)
>     maybe_warn_zero_as_null_pointer_constant (e, loc);
>       return nullptr_node;
>     }

Maybe we should move this code up, then.

Jason

Hi,

On 28/04/2018 18:41, Jason Merrill wrote:
> On Fri, Apr 27, 2018 at 7:26 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Hi again,
>>
>> I'm now pretty sure that we have a latent issue in ocp_convert. The bug
>> fixed by Jakub shows that we used to not have issues with integer_zero_node.
>> That's easy to explain: at the beginning of ocp_convert there is code which
>> handles first some special / simple cases when
>> same_type_ignoring_top_level_qualifiers_p is true. That code isn't of course
>> used for integer_zero_node as source expression, which therefore is handled
>> by:
>>
>>    if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
>>      {
>>        if (complain & tf_warning)
>>      maybe_warn_zero_as_null_pointer_constant (e, loc);
>>        return nullptr_node;
>>      }
> Maybe we should move this code up, then.
You are totally right. Yesterday I realized that and tested on 
x86_64-linux the below, both with and without Jakub's fix.

Thanks!
Paolo.

////////////////////
Index: cvt.c
===================================================================
--- cvt.c	(revision 259742)
+++ cvt.c	(working copy)
@@ -711,6 +711,14 @@ ocp_convert (tree type, tree expr, int convtype, i
   if (error_operand_p (e))
     return error_mark_node;
 
+  if (NULLPTR_TYPE_P (type) && null_ptr_cst_p (e))
+    {
+      if (complain & tf_warning)
+	maybe_warn_zero_as_null_pointer_constant (e, loc);
+      if (!TREE_SIDE_EFFECTS (e))
+	return nullptr_node;
+    }
+
   if (MAYBE_CLASS_TYPE_P (type) && (convtype & CONV_FORCE_TEMP))
     /* We need a new temporary; don't take this shortcut.  */;
   else if (same_type_ignoring_top_level_qualifiers_p (type, TREE_TYPE (e)))
@@ -832,12 +840,6 @@ ocp_convert (tree type, tree expr, int convtype, i
       /* Ignore any integer overflow caused by the conversion.  */
       return ignore_overflows (converted, e);
     }
-  if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
-    {
-      if (complain & tf_warning)
-	maybe_warn_zero_as_null_pointer_constant (e, loc);
-      return nullptr_node;
-    }
   if (POINTER_TYPE_P (type) || TYPE_PTRMEM_P (type))
     return cp_convert_to_pointer (type, e, dofold, complain);
   if (code == VECTOR_TYPE)

Hi all, Jason,

On 29/04/2018 09:23, Paolo Carlini wrote:
> Hi,
>
> On 28/04/2018 18:41, Jason Merrill wrote:
>> On Fri, Apr 27, 2018 at 7:26 PM, Paolo Carlini 
>> <paolo.carlini@oracle.com> wrote:
>>> Hi again,
>>>
>>> I'm now pretty sure that we have a latent issue in ocp_convert. The bug
>>> fixed by Jakub shows that we used to not have issues with 
>>> integer_zero_node.
>>> That's easy to explain: at the beginning of ocp_convert there is 
>>> code which
>>> handles first some special / simple cases when
>>> same_type_ignoring_top_level_qualifiers_p is true. That code isn't 
>>> of course
>>> used for integer_zero_node as source expression, which therefore is 
>>> handled
>>> by:
>>>
>>>    if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
>>>      {
>>>        if (complain & tf_warning)
>>>      maybe_warn_zero_as_null_pointer_constant (e, loc);
>>>        return nullptr_node;
>>>      }
>> Maybe we should move this code up, then.
> You are totally right. Yesterday I realized that and tested on 
> x86_64-linux the below, both with and without Jakub's fix.
In trunk shall I go ahead with this, then?

     https://gcc.gnu.org/ml/gcc-patches/2018-04/msg01290.html

Thanks!
Paolo.

On Sun, Apr 29, 2018 at 3:23 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 28/04/2018 18:41, Jason Merrill wrote:
>>
>> On Fri, Apr 27, 2018 at 7:26 PM, Paolo Carlini <paolo.carlini@oracle.com>
>> wrote:
>>>
>>> Hi again,
>>>
>>> I'm now pretty sure that we have a latent issue in ocp_convert. The bug
>>> fixed by Jakub shows that we used to not have issues with
>>> integer_zero_node.
>>> That's easy to explain: at the beginning of ocp_convert there is code
>>> which
>>> handles first some special / simple cases when
>>> same_type_ignoring_top_level_qualifiers_p is true. That code isn't of
>>> course
>>> used for integer_zero_node as source expression, which therefore is
>>> handled
>>> by:
>>>
>>>    if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
>>>      {
>>>        if (complain & tf_warning)
>>>      maybe_warn_zero_as_null_pointer_constant (e, loc);
>>>        return nullptr_node;
>>>      }
>>
>> Maybe we should move this code up, then.
>
> You are totally right. Yesterday I realized that and tested on x86_64-linux
> the below, both with and without Jakub's fix.

+      if (!TREE_SIDE_EFFECTS (e))
+        return nullptr_node;

So what happens if e has side-effects?

Jason

Hi,

On 04/05/2018 19:45, Jason Merrill wrote:
> On Sun, Apr 29, 2018 at 3:23 AM, Paolo Carlini<paolo.carlini@oracle.com>  wrote:
>> Hi,
>>
>> On 28/04/2018 18:41, Jason Merrill wrote:
>>> On Fri, Apr 27, 2018 at 7:26 PM, Paolo Carlini<paolo.carlini@oracle.com>
>>> wrote:
>>>> Hi again,
>>>>
>>>> I'm now pretty sure that we have a latent issue in ocp_convert. The bug
>>>> fixed by Jakub shows that we used to not have issues with
>>>> integer_zero_node.
>>>> That's easy to explain: at the beginning of ocp_convert there is code
>>>> which
>>>> handles first some special / simple cases when
>>>> same_type_ignoring_top_level_qualifiers_p is true. That code isn't of
>>>> course
>>>> used for integer_zero_node as source expression, which therefore is
>>>> handled
>>>> by:
>>>>
>>>>     if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
>>>>       {
>>>>         if (complain & tf_warning)
>>>>       maybe_warn_zero_as_null_pointer_constant (e, loc);
>>>>         return nullptr_node;
>>>>       }
>>> Maybe we should move this code up, then.
>> You are totally right. Yesterday I realized that and tested on x86_64-linux
>> the below, both with and without Jakub's fix.
> +      if (!TREE_SIDE_EFFECTS (e))
> +        return nullptr_node;
>
> So what happens if e has side-effects?
In that case nothing should change wrt the status quo, that is, the 
"fast path" wrapping the thing in a NOP_EXPR. That only for 
NULLPTR_TYPE_P nodes, I don't think that can happen for 
integer_zero_nodes. I must say, if I take out the check there are no 
regressions, but using it seems consistent with decay_conversion, were 
not having the check caused a real wrong code bug. What do you think? 
Maybe an alternative would be returning immediately e as-is?!?

Paolo.

... thinking more about the issue, probably the best thing to do would 
be wrapping instead in a COMPOUND_EXPR, which would be also consistent 
with cp_convert_to_pointer. Thus I'm finishing testing the below (past 
g++.dg). How about it?

Thanks!
Paolo.

/////////////////////
Index: cvt.c
===================================================================
--- cvt.c	(revision 259926)
+++ cvt.c	(working copy)
@@ -711,6 +711,15 @@ ocp_convert (tree type, tree expr, int convtype, i
   if (error_operand_p (e))
     return error_mark_node;
 
+  if (NULLPTR_TYPE_P (type) && null_ptr_cst_p (e))
+    {
+      if (complain & tf_warning)
+	maybe_warn_zero_as_null_pointer_constant (e, loc);
+
+      return (TREE_SIDE_EFFECTS (e)
+	      ? build2 (COMPOUND_EXPR, type, e, nullptr_node) : nullptr_node);
+    }
+
   if (MAYBE_CLASS_TYPE_P (type) && (convtype & CONV_FORCE_TEMP))
     /* We need a new temporary; don't take this shortcut.  */;
   else if (same_type_ignoring_top_level_qualifiers_p (type, TREE_TYPE (e)))
@@ -832,12 +841,6 @@ ocp_convert (tree type, tree expr, int convtype, i
       /* Ignore any integer overflow caused by the conversion.  */
       return ignore_overflows (converted, e);
     }
-  if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
-    {
-      if (complain & tf_warning)
-	maybe_warn_zero_as_null_pointer_constant (e, loc);
-      return nullptr_node;
-    }
   if (POINTER_TYPE_P (type) || TYPE_PTRMEM_P (type))
     return cp_convert_to_pointer (type, e, dofold, complain);
   if (code == VECTOR_TYPE)

On Fri, May 4, 2018 at 2:06 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
>
> On 04/05/2018 19:45, Jason Merrill wrote:
>>
>> On Sun, Apr 29, 2018 at 3:23 AM, Paolo Carlini<paolo.carlini@oracle.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 28/04/2018 18:41, Jason Merrill wrote:
>>>>
>>>> On Fri, Apr 27, 2018 at 7:26 PM, Paolo Carlini<paolo.carlini@oracle.com>
>>>> wrote:
>>>>>
>>>>> Hi again,
>>>>>
>>>>> I'm now pretty sure that we have a latent issue in ocp_convert. The bug
>>>>> fixed by Jakub shows that we used to not have issues with
>>>>> integer_zero_node.
>>>>> That's easy to explain: at the beginning of ocp_convert there is code
>>>>> which
>>>>> handles first some special / simple cases when
>>>>> same_type_ignoring_top_level_qualifiers_p is true. That code isn't of
>>>>> course
>>>>> used for integer_zero_node as source expression, which therefore is
>>>>> handled
>>>>> by:
>>>>>
>>>>>     if (NULLPTR_TYPE_P (type) && e && null_ptr_cst_p (e))
>>>>>       {
>>>>>         if (complain & tf_warning)
>>>>>       maybe_warn_zero_as_null_pointer_constant (e, loc);
>>>>>         return nullptr_node;
>>>>>       }
>>>>
>>>> Maybe we should move this code up, then.
>>>
>>> You are totally right. Yesterday I realized that and tested on
>>> x86_64-linux
>>> the below, both with and without Jakub's fix.
>>
>> +      if (!TREE_SIDE_EFFECTS (e))
>> +        return nullptr_node;
>>
>> So what happens if e has side-effects?
>
> In that case nothing should change wrt the status quo, that is, the "fast
> path" wrapping the thing in a NOP_EXPR. That only for NULLPTR_TYPE_P nodes,
> I don't think that can happen for integer_zero_nodes. I must say, if I take
> out the check there are no regressions, but using it seems consistent with
> decay_conversion, were not having the check caused a real wrong code bug.
> What do you think? Maybe an alternative would be returning immediately e
> as-is?!?

The patch is OK as is.

Jason