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

2018-02-13 Thread Jason Merrill 
On Tue, Feb 13, 2018 at 10:43 AM, Paolo Carlini
 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 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

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

2018-02-13 Thread Paolo Carlini 
.. 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.

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

2018-02-13 Thread Paolo Carlini 

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 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 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 int foo()
+{
+  return sizeof(T) > 1 ? : 1;
+}

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

2018-02-13 Thread Jason Merrill 
OK.

On Mon, Feb 12, 2018 at 2:18 PM, Paolo Carlini  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.
>
> //
>

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

2018-02-12 Thread Paolo Carlini 

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  

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  

PR c++/84333
* g++.dg/template/sizeof16.C: New.
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,6 @@
+// PR c++/84333
+
+template int foo()
+{
+  return sizeof(int) > 1 ? : 1;
+}
Index: tree.c
===
--- tree.c  (revision 257588)
+++ tree.c  (working copy)
@@ -3733,6 +3733,12 @@ contains_placeholder_p (const_tree exp)
 a PLACEHOLDER_EXPR. */
  return 0;
 
+   case SIZEOF_EXPR:
+ if (TYPE_P (TREE_OPERAND (exp, 0)))
+   return type_contains_placeholder_p (TREE_OPERAND (exp, 0));
+ else
+   return CONTAINS_PLACEHOLDER_P (TREE_OPERAND (exp, 0));
+
default:
  break;
}