[C++] PR 84333 ("[6/7/8 Regression] ICE with ternary operator in template function")

Hi,

this ICE on valid happens only with checking enabled - that explains why 
we didn't notice it so far - but I think points to a minor but 
substantive correctness issue. In short, we ICE when 
build_conditional_expr calls save_expr, which in turn calls 
contain_placeholder_p, which doesn't handle correctly the sizeof(int), 
and tries to use TREE_CONSTANT on the INTEGER_TYPE. I think that in 
general we simply have to explicitly handle both kinds of sizeof in 
contains_placeholder_p: even for a type as simple as INTEGER_TYPE the 
result may not be trivial, ie, type_contains_placeholder_1 checks the 
bounds:

    case INTEGER_TYPE:
     case REAL_TYPE:
     case FIXED_POINT_TYPE:
       /* Here we just check the bounds.  */
       return (CONTAINS_PLACEHOLDER_P (TYPE_MIN_VALUE (type))
           || CONTAINS_PLACEHOLDER_P (TYPE_MAX_VALUE (type)));

I'm finishing testing the below on x86_64-linux, all good so far.

Thanks, Paolo.

//////////////////
2018-02-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84333
	* tree.c (contains_placeholder_p): Explicitly handle both kinds
	of SIZEOF_EXPR, ie, type and expr operand.

/testsuite
2018-02-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/84333
	* g++.dg/template/sizeof16.C: New.

OK.

On Mon, Feb 12, 2018 at 2:18 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> this ICE on valid happens only with checking enabled - that explains why we
> didn't notice it so far - but I think points to a minor but substantive
> correctness issue. In short, we ICE when build_conditional_expr calls
> save_expr, which in turn calls contain_placeholder_p, which doesn't handle
> correctly the sizeof(int), and tries to use TREE_CONSTANT on the
> INTEGER_TYPE. I think that in general we simply have to explicitly handle
> both kinds of sizeof in contains_placeholder_p: even for a type as simple as
> INTEGER_TYPE the result may not be trivial, ie, type_contains_placeholder_1
> checks the bounds:
>
>    case INTEGER_TYPE:
>     case REAL_TYPE:
>     case FIXED_POINT_TYPE:
>       /* Here we just check the bounds.  */
>       return (CONTAINS_PLACEHOLDER_P (TYPE_MIN_VALUE (type))
>           || CONTAINS_PLACEHOLDER_P (TYPE_MAX_VALUE (type)));
>
> I'm finishing testing the below on x86_64-linux, all good so far.
>
> Thanks, Paolo.
>
> //////////////////
>

Hi Jason,

On 13/02/2018 15:35, Jason Merrill wrote:
> OK.
Thanks. Unfortunately, I don't think I sent a complete solution for 
these issues. In fact we would still ICE on:

template<typename T> int foo()
{
   return sizeof(T) > 1 ? : 1;
}

because type_contains_placeholder_p doesn't know how to handle a 
TEMPLATE_TYPE_PARM. In fact, cp_save_expr teaches me something:

tree
cp_save_expr (tree expr)
{
   /* There is no reason to create a SAVE_EXPR within a template; if
      needed, we can create the SAVE_EXPR when instantiating the
      template.  Furthermore, the middle-end cannot handle C++-specific
      tree codes.  */
   if (processing_template_decl)
     return expr;
   return save_expr (expr);
}

would it be correct to just use it? Or we have to do something more 
complex? Note the issue only affects the GNU-extension with omitted 
middle operand, thus I believe we have *some* leeway...

Thanks!
Paolo.

/////////////////////
Index: cp/call.c
===================================================================
--- cp/call.c	(revision 257627)
+++ cp/call.c	(working copy)
@@ -4805,7 +4805,7 @@ build_conditional_expr_1 (location_t loc, tree arg
       if (lvalue_p (arg1))
 	arg2 = arg1 = cp_stabilize_reference (arg1);
       else
-	arg2 = arg1 = save_expr (arg1);
+	arg2 = arg1 = cp_save_expr (arg1);
     }
 
   /* If something has already gone wrong, just pass that fact up the
Index: testsuite/g++.dg/template/sizeof16.C
===================================================================
--- testsuite/g++.dg/template/sizeof16.C	(nonexistent)
+++ testsuite/g++.dg/template/sizeof16.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/84333
+// { dg-options -Wno-pedantic }
+
+template<typename> int foo()
+{
+  return sizeof(int) > 1 ? : 1;
+}
Index: testsuite/g++.dg/template/sizeof17.C
===================================================================
--- testsuite/g++.dg/template/sizeof17.C	(nonexistent)
+++ testsuite/g++.dg/template/sizeof17.C	(working copy)
@@ -0,0 +1,7 @@
+// PR c++/84333
+// { dg-options -Wno-pedantic }
+
+template<typename T> int foo()
+{
+  return sizeof(T) > 1 ? : 1;
+}

.. I should honestly add that now I don't really believe - as stated by 
Volker - that the issue, in any substantive way, is a recent, post 
6.1.0, regression: for the GNU extension build_conditional_expr was 
definitely copying arg2 = arg1 before that release and using save_expr 
on arg1 and the comment in cp_save_expr was already true.

Paolo.

On Tue, Feb 13, 2018 at 10:43 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> On 13/02/2018 15:35, Jason Merrill wrote:
> Thanks. Unfortunately, I don't think I sent a complete solution for these
> issues. In fact we would still ICE on:
>
> template<typename T> int foo()
> {
>   return sizeof(T) > 1 ? : 1;
> }
>
> because type_contains_placeholder_p doesn't know how to handle a
> TEMPLATE_TYPE_PARM. In fact, cp_save_expr teaches me something:
>
> tree
> cp_save_expr (tree expr)
> {
>   /* There is no reason to create a SAVE_EXPR within a template; if
>      needed, we can create the SAVE_EXPR when instantiating the
>      template.  Furthermore, the middle-end cannot handle C++-specific
>      tree codes.  */
>   if (processing_template_decl)
>     return expr;
>   return save_expr (expr);
> }
>
> would it be correct to just use it? Or we have to do something more complex?
> Note the issue only affects the GNU-extension with omitted middle operand,
> thus I believe we have *some* leeway...

Hmm, yes, that should work.  In a template we're just figuring out the
result type, at instantiation time we'll see the missing middle op
again and can do the right thing.  OK.

Jason