diff mbox series

[C++] Fix FIX_TRUNC_EXPR instantiation (PR c++/84942)

Message ID 20180319195041.GR8577@tucnak
State New
Headers show
Series [C++] Fix FIX_TRUNC_EXPR instantiation (PR c++/84942) | expand

Commit Message

Jakub Jelinek March 19, 2018, 7:50 p.m. UTC 
Hi!

We instantiate FIX_TRUNC_EXPR using cp_build_unary_op, but that function
doesn't look like a good match for this tree, what it really does for it is:
1) handle error_operand_p
2) on ia64 only diagnose __fpreg uses (I believe a cast of __fpreg to int is
   fine, so we shouldn't warn)
3) and then it does:
        argtype = TREE_TYPE (arg);
      return build1 (code, argtype, arg);
   which creates FIX_TRUNC_EXPR with the type of the argument rather than
   the integral type it should have.
The following patch thus bypasses that function and just builds the
FIX_TRUNC_EXPR with the right type.  I think we don't need to tsubst the
type, because if it is type dependent, we wouldn't be creating
FIX_TRUNC_EXPR, but record it just as a some kind of conversion.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-03-19  Jakub Jelinek  <jakub@redhat.com>

	PR c++/84942
	* pt.c (tsubst_copy_and_build) <case FIX_TRUNC_EXPR>: Don't call
	cp_build_unary_op, just use build1.

	* g++.dg/cpp1y/pr84942.C: New test.


	Jakub

Comments

Jason Merrill March 20, 2018, 9 p.m. UTC | #1 
On Mon, Mar 19, 2018 at 3:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> +int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));

This seems like another situation like 84610 and 84642 that Alex sent
a patch for, of 'auto' in an attribute wrongly being treated as an
implicit template parameter.  As I suggested in response to his patch,
we ought to disable auto_is_implicit_function_parm_p within an
attribute-specifier.

Jason
Jakub Jelinek March 21, 2018, 8:42 a.m. UTC | #2 
On Tue, Mar 20, 2018 at 05:00:34PM -0400, Jason Merrill wrote:
> On Mon, Mar 19, 2018 at 3:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > +int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
> 
> This seems like another situation like 84610 and 84642 that Alex sent
> a patch for, of 'auto' in an attribute wrongly being treated as an
> implicit template parameter.  As I suggested in response to his patch,
> we ought to disable auto_is_implicit_function_parm_p within an
> attribute-specifier.

Ok, for this specific testcase I'll wait for Alex.  That doesn't mean
the FIX_TRUNC_EXPR handling isn't completely wrong though.
So, shall we fix it anyway, or remove or just gcc_unreachable ()?
It was added in PR51150 (it couldn't work even at that point,
cp_build_unary_op would use wrong return type even back then), but the
testcase never invokes this anymore, uses CAST_EXPR, STATIC_CAST_EXPR etc.
instead.

	Jakub
Jason Merrill March 21, 2018, 4:01 p.m. UTC | #3 
On Wed, Mar 21, 2018 at 4:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Mar 20, 2018 at 05:00:34PM -0400, Jason Merrill wrote:
>> On Mon, Mar 19, 2018 at 3:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > +int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
>>
>> This seems like another situation like 84610 and 84642 that Alex sent
>> a patch for, of 'auto' in an attribute wrongly being treated as an
>> implicit template parameter.  As I suggested in response to his patch,
>> we ought to disable auto_is_implicit_function_parm_p within an
>> attribute-specifier.
>
> Ok, for this specific testcase I'll wait for Alex.  That doesn't mean
> the FIX_TRUNC_EXPR handling isn't completely wrong though.
> So, shall we fix it anyway, or remove or just gcc_unreachable ()?
> It was added in PR51150 (it couldn't work even at that point,
> cp_build_unary_op would use wrong return type even back then), but the
> testcase never invokes this anymore, uses CAST_EXPR, STATIC_CAST_EXPR etc.
> instead.

Hmm, I think remove.  It should then hit the gcc_unreachable() in tsubst_copy.

Jason
Jakub Jelinek March 22, 2018, 8:14 a.m. UTC | #4 
On Wed, Mar 21, 2018 at 12:01:58PM -0400, Jason Merrill wrote:
> On Wed, Mar 21, 2018 at 4:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Tue, Mar 20, 2018 at 05:00:34PM -0400, Jason Merrill wrote:
> >> On Mon, Mar 19, 2018 at 3:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> >> > +int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
> >>
> >> This seems like another situation like 84610 and 84642 that Alex sent
> >> a patch for, of 'auto' in an attribute wrongly being treated as an
> >> implicit template parameter.  As I suggested in response to his patch,
> >> we ought to disable auto_is_implicit_function_parm_p within an
> >> attribute-specifier.
> >
> > Ok, for this specific testcase I'll wait for Alex.  That doesn't mean
> > the FIX_TRUNC_EXPR handling isn't completely wrong though.
> > So, shall we fix it anyway, or remove or just gcc_unreachable ()?
> > It was added in PR51150 (it couldn't work even at that point,
> > cp_build_unary_op would use wrong return type even back then), but the
> > testcase never invokes this anymore, uses CAST_EXPR, STATIC_CAST_EXPR etc.
> > instead.
> 
> Hmm, I think remove.  It should then hit the gcc_unreachable() in tsubst_copy.

To my surprise, with latest Alex' changes tsubst_copy_and_build is still
called with FIX_TRUNC_EXPR (the (int) __builtin_inf () expression), but with
the following patch we don't ICE, because args is NULL and
tsubst_copy starts with:
14945	  if (t == NULL_TREE || t == error_mark_node || args == NULL_TREE)
14946	    return t;
So, do you still want the FIX_TRUNC_EXPR handling removed or fixed (as done
in the first patch)?

2018-03-22  Jakub Jelinek  <jakub@redhat.com>

	PR c++/84942
	* pt.c (tsubst_copy_and_build): Remove FIX_TRUNC_EXPR handling.

	* g++.dg/cpp1y/pr84942.C: New test.

--- gcc/cp/pt.c.jj	2018-03-20 13:53:32.265938570 +0100
+++ gcc/cp/pt.c	2018-03-21 17:25:36.155977417 +0100
@@ -17513,10 +17513,6 @@ tsubst_copy_and_build (tree t,
 				RECUR (TREE_OPERAND (t, 0)),
 				complain|decltype_flag));
 
-    case FIX_TRUNC_EXPR:
-      RETURN (cp_build_unary_op (FIX_TRUNC_EXPR, RECUR (TREE_OPERAND (t, 0)),
-				 false, complain));
-
     case ADDR_EXPR:
       op1 = TREE_OPERAND (t, 0);
       if (TREE_CODE (op1) == LABEL_DECL)
--- gcc/testsuite/g++.dg/cpp1y/pr84942.C.jj	2018-03-19 11:04:16.201588211 +0100
+++ gcc/testsuite/g++.dg/cpp1y/pr84942.C	2018-03-19 11:02:37.950609950 +0100
@@ -0,0 +1,7 @@
+// PR c++/84942
+// { dg-do compile { target c++14 } }
+// { dg-options "-w" }
+
+int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
+// { dg-error "expected primary-expression before" "" { target *-*-* } .-1 }
+// { dg-error "declared as implicit template" "" { target *-*-* } .-2 }


	Jakub
Jason Merrill March 22, 2018, 6:02 p.m. UTC | #5 
On Thu, Mar 22, 2018 at 4:14 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 21, 2018 at 12:01:58PM -0400, Jason Merrill wrote:
>> On Wed, Mar 21, 2018 at 4:42 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Tue, Mar 20, 2018 at 05:00:34PM -0400, Jason Merrill wrote:
>> >> On Mon, Mar 19, 2018 at 3:50 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> >> > +int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
>> >>
>> >> This seems like another situation like 84610 and 84642 that Alex sent
>> >> a patch for, of 'auto' in an attribute wrongly being treated as an
>> >> implicit template parameter.  As I suggested in response to his patch,
>> >> we ought to disable auto_is_implicit_function_parm_p within an
>> >> attribute-specifier.
>> >
>> > Ok, for this specific testcase I'll wait for Alex.  That doesn't mean
>> > the FIX_TRUNC_EXPR handling isn't completely wrong though.
>> > So, shall we fix it anyway, or remove or just gcc_unreachable ()?
>> > It was added in PR51150 (it couldn't work even at that point,
>> > cp_build_unary_op would use wrong return type even back then), but the
>> > testcase never invokes this anymore, uses CAST_EXPR, STATIC_CAST_EXPR etc.
>> > instead.
>>
>> Hmm, I think remove.  It should then hit the gcc_unreachable() in tsubst_copy.
>
> To my surprise, with latest Alex' changes tsubst_copy_and_build is still
> called with FIX_TRUNC_EXPR (the (int) __builtin_inf () expression)

He hadn't yet checked in the relevant change, "Disable
auto_is_implicit_function_template_parm_p while parsing attributes".
That should happen soon.

> but with
> the following patch we don't ICE, because args is NULL and
> tsubst_copy starts with:
> 14945     if (t == NULL_TREE || t == error_mark_node || args == NULL_TREE)
> 14946       return t;
> So, do you still want the FIX_TRUNC_EXPR handling removed or fixed (as done
> in the first patch)?

Hmm, let's make it gcc_unreachable then.

Jason
Jakub Jelinek March 22, 2018, 6:04 p.m. UTC | #6 
On Thu, Mar 22, 2018 at 02:02:01PM -0400, Jason Merrill wrote:
> > To my surprise, with latest Alex' changes tsubst_copy_and_build is still
> > called with FIX_TRUNC_EXPR (the (int) __builtin_inf () expression)
> 
> He hadn't yet checked in the relevant change, "Disable
> auto_is_implicit_function_template_parm_p while parsing attributes".
> That should happen soon.

I thought it was r258748, but perhaps there is some other patch.

	Jakub
Alexandre Oliva March 22, 2018, 11:05 p.m. UTC | #7 
On Mar 22, 2018, Jakub Jelinek <jakub@redhat.com> wrote:

> On Thu, Mar 22, 2018 at 02:02:01PM -0400, Jason Merrill wrote:
>> > To my surprise, with latest Alex' changes tsubst_copy_and_build is still
>> > called with FIX_TRUNC_EXPR (the (int) __builtin_inf () expression)
>> 
>> He hadn't yet checked in the relevant change, "Disable
>> auto_is_implicit_function_template_parm_p while parsing attributes".
>> That should happen soon.

> I thought it was r258748, but perhaps there is some other patch.

No, there were two different patches related with this, and only the
latter one (linked to from PR84942), that I'm going to install tonight,
affects this case.  I didn't include the testcase in the patch assuming
your testcase adn patch would go in, but it will certainly require
adjustments for the expected error messages.
Jakub Jelinek March 23, 2018, 2:18 p.m. UTC | #8 
On Thu, Mar 22, 2018 at 02:02:01PM -0400, Jason Merrill wrote:
> He hadn't yet checked in the relevant change, "Disable
> auto_is_implicit_function_template_parm_p while parsing attributes".
> That should happen soon.
> 
> > but with
> > the following patch we don't ICE, because args is NULL and
> > tsubst_copy starts with:
> > 14945     if (t == NULL_TREE || t == error_mark_node || args == NULL_TREE)
> > 14946       return t;
> > So, do you still want the FIX_TRUNC_EXPR handling removed or fixed (as done
> > in the first patch)?
> 
> Hmm, let's make it gcc_unreachable then.

So like this?  Passes make check-c++-all on current trunk, where the above
patch from Alex is already in.

2018-03-23  Jakub Jelinek  <jakub@redhat.com>

	PR c++/84942
	* pt.c (tsubst_copy_and_build) <case FIX_TRUNC_EXPR>: Replace
	cp_build_unary_op call with gcc_unreachable ().

	* g++.dg/cpp1y/pr84942.C: New test.

--- gcc/cp/pt.c.jj	2018-03-23 09:49:38.063519286 +0100
+++ gcc/cp/pt.c	2018-03-23 09:51:24.232501064 +0100
@@ -17514,8 +17514,7 @@ tsubst_copy_and_build (tree t,
 				complain|decltype_flag));
 
     case FIX_TRUNC_EXPR:
-      RETURN (cp_build_unary_op (FIX_TRUNC_EXPR, RECUR (TREE_OPERAND (t, 0)),
-				 false, complain));
+      gcc_unreachable ();    
 
     case ADDR_EXPR:
       op1 = TREE_OPERAND (t, 0);
--- gcc/testsuite/g++.dg/cpp1y/pr84942.C.jj	2018-03-23 09:50:16.320512716 +0100
+++ gcc/testsuite/g++.dg/cpp1y/pr84942.C	2018-03-23 09:52:33.408489183 +0100
@@ -0,0 +1,6 @@
+// PR c++/84942
+// { dg-do compile { target c++14 } }
+// { dg-options "-w" }
+
+int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
+// { dg-error "expected primary-expression before" "" { target *-*-* } .-1 }


	Jakub
Jason Merrill March 23, 2018, 4:34 p.m. UTC | #9 
OK.

On Fri, Mar 23, 2018 at 10:18 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 22, 2018 at 02:02:01PM -0400, Jason Merrill wrote:
>> He hadn't yet checked in the relevant change, "Disable
>> auto_is_implicit_function_template_parm_p while parsing attributes".
>> That should happen soon.
>>
>> > but with
>> > the following patch we don't ICE, because args is NULL and
>> > tsubst_copy starts with:
>> > 14945     if (t == NULL_TREE || t == error_mark_node || args == NULL_TREE)
>> > 14946       return t;
>> > So, do you still want the FIX_TRUNC_EXPR handling removed or fixed (as done
>> > in the first patch)?
>>
>> Hmm, let's make it gcc_unreachable then.
>
> So like this?  Passes make check-c++-all on current trunk, where the above
> patch from Alex is already in.
>
> 2018-03-23  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/84942
>         * pt.c (tsubst_copy_and_build) <case FIX_TRUNC_EXPR>: Replace
>         cp_build_unary_op call with gcc_unreachable ().
>
>         * g++.dg/cpp1y/pr84942.C: New test.
>
> --- gcc/cp/pt.c.jj      2018-03-23 09:49:38.063519286 +0100
> +++ gcc/cp/pt.c 2018-03-23 09:51:24.232501064 +0100
> @@ -17514,8 +17514,7 @@ tsubst_copy_and_build (tree t,
>                                 complain|decltype_flag));
>
>      case FIX_TRUNC_EXPR:
> -      RETURN (cp_build_unary_op (FIX_TRUNC_EXPR, RECUR (TREE_OPERAND (t, 0)),
> -                                false, complain));
> +      gcc_unreachable ();
>
>      case ADDR_EXPR:
>        op1 = TREE_OPERAND (t, 0);
> --- gcc/testsuite/g++.dg/cpp1y/pr84942.C.jj     2018-03-23 09:50:16.320512716 +0100
> +++ gcc/testsuite/g++.dg/cpp1y/pr84942.C        2018-03-23 09:52:33.408489183 +0100
> @@ -0,0 +1,6 @@
> +// PR c++/84942
> +// { dg-do compile { target c++14 } }
> +// { dg-options "-w" }
> +
> +int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
> +// { dg-error "expected primary-expression before" "" { target *-*-* } .-1 }
>
>
>         Jakub
diff mbox series

Patch

--- gcc/cp/pt.c.jj	2018-03-16 21:11:04.440773108 +0100
+++ gcc/cp/pt.c	2018-03-19 10:58:39.803657613 +0100
@@ -17495,8 +17495,10 @@  tsubst_copy_and_build (tree t,
 				complain|decltype_flag));
 
     case FIX_TRUNC_EXPR:
-      RETURN (cp_build_unary_op (FIX_TRUNC_EXPR, RECUR (TREE_OPERAND (t, 0)),
-				 false, complain));
+      op1 = RECUR (TREE_OPERAND (t, 0));
+      if (error_operand_p (op1))
+        RETURN (error_mark_node);
+      RETURN (build1 (FIX_TRUNC_EXPR, TREE_TYPE (t), op1));
 
     case ADDR_EXPR:
       op1 = TREE_OPERAND (t, 0);
--- gcc/testsuite/g++.dg/cpp1y/pr84942.C.jj	2018-03-19 11:04:16.201588211 +0100
+++ gcc/testsuite/g++.dg/cpp1y/pr84942.C	2018-03-19 11:02:37.950609950 +0100
@@ -0,0 +1,7 @@ 
+// PR c++/84942
+// { dg-do compile { target c++14 } }
+// { dg-options "-w" }
+
+int a(__attribute__((b((int)__builtin_inf() * 1ULL / auto))));
+// { dg-error "expected primary-expression before" "" { target *-*-* } .-1 }
+// { dg-error "declared as implicit template" "" { target *-*-* } .-2 }