PR c/104506: Tolerate error_mark_node in useless_type_conversion_p.

  This simple fix to the middle-end, resolves PR c/104506, by adding an

explicit check for error_mark_node to useless_type_conversion_p.  I first

trying fixing this in the C front-end, but the type is valid at the point

that the NOP_EXPR is created, so the poisoned type leaks to the middle-end.

Returning either true or false from useless_type_conversion_p avoids the

ICE-after-error.  Apologies to Andrew Pinski, I hadn't noticed that he'd

assigned this PR to himself until after my regression testing had finished.

 

This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and

make -k check with no new failures.  Ok for mainline?

 

 

2022-02-14  Roger Sayle  <roger@nextmovesoftware.com>

 

gcc/ChangeLog

PR c/104506

* gimple-expr.cc (useless_type_conversion_p): Add a check for

error_mark_node.

 

gcc/testsuite/ChangeLog

PR c/104506

* gcc.dg/pr104506.c: New test case.

 

 

Roger

--
  

On Mon, Feb 14, 2022 at 1:54 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
>
> This simple fix to the middle-end, resolves PR c/104506, by adding an
>
> explicit check for error_mark_node to useless_type_conversion_p.  I first
>
> trying fixing this in the C front-end, but the type is valid at the point
>
> that the NOP_EXPR is created, so the poisoned type leaks to the middle-end.

Hmm, IMHO "fixing" something to error_mark after it has possibly be used
looks broken.

I don't like trying to paper over this in useless_type_conversion_p, the
predicate should not be called on an error_mark_node type.

Alternatively we might want to create an error_type_node that is at least
a type (with main variant error_type_node and TREE_CODE ERROR_TYPE, etc.).
But likely more complicated than avoiding to mess with the type of 'x' after
the fact?

Richard.

>
> Returning either true or false from useless_type_conversion_p avoids the
>
> ICE-after-error.  Apologies to Andrew Pinski, I hadn't noticed that he'd
>
> assigned this PR to himself until after my regression testing had finished.
>
>
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and
>
> make -k check with no new failures.  Ok for mainline?
>
>
>
>
>
> 2022-02-14  Roger Sayle  <roger@nextmovesoftware.com>
>
>
>
> gcc/ChangeLog
>
> PR c/104506
>
> * gimple-expr.cc (useless_type_conversion_p): Add a check for
>
> error_mark_node.
>
>
>
> gcc/testsuite/ChangeLog
>
> PR c/104506
>
> * gcc.dg/pr104506.c: New test case.
>
>
>
>
>
> Roger
>
> --
>
>
>
  

On Mon, Feb 14, 2022 at 4:31 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Feb 14, 2022 at 1:54 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
> >
> >
> >
> > This simple fix to the middle-end, resolves PR c/104506, by adding an
> >
> > explicit check for error_mark_node to useless_type_conversion_p.  I first
> >
> > trying fixing this in the C front-end, but the type is valid at the point
> >
> > that the NOP_EXPR is created, so the poisoned type leaks to the middle-end.
>
> Hmm, IMHO "fixing" something to error_mark after it has possibly be used
> looks broken.
>
> I don't like trying to paper over this in useless_type_conversion_p, the
> predicate should not be called on an error_mark_node type.
>
> Alternatively we might want to create an error_type_node that is at least
> a type (with main variant error_type_node and TREE_CODE ERROR_TYPE, etc.).
> But likely more complicated than avoiding to mess with the type of 'x' after
> the fact?

That is,

diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index c701f07befe..29658ade6d7 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -2964,14 +2964,12 @@ duplicate_decls (tree newdecl, tree olddecl)
     {
       /* Avoid `unused variable' and other warnings for OLDDECL.  */
       suppress_warning (olddecl, OPT_Wunused);
-      /* If the types are completely different, poison them both with
+      /* If the types are completely different, poison the new with
         error_mark_node.  */
       if (TREE_CODE (TREE_TYPE (newdecl)) != TREE_CODE (TREE_TYPE (olddecl))
          && olddecl != error_mark_node
          && seen_error ())
        {
-         if (TREE_CODE (olddecl) != FUNCTION_DECL)
-           TREE_TYPE (olddecl) = error_mark_node;
          if (TREE_CODE (newdecl) != FUNCTION_DECL)
            TREE_TYPE (newdecl) = error_mark_node;
        }

can you check what happens with that?

>
> Richard.
>
> >
> > Returning either true or false from useless_type_conversion_p avoids the
> >
> > ICE-after-error.  Apologies to Andrew Pinski, I hadn't noticed that he'd
> >
> > assigned this PR to himself until after my regression testing had finished.
> >
> >
> >
> > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and
> >
> > make -k check with no new failures.  Ok for mainline?
> >
> >
> >
> >
> >
> > 2022-02-14  Roger Sayle  <roger@nextmovesoftware.com>
> >
> >
> >
> > gcc/ChangeLog
> >
> > PR c/104506
> >
> > * gimple-expr.cc (useless_type_conversion_p): Add a check for
> >
> > error_mark_node.
> >
> >
> >
> > gcc/testsuite/ChangeLog
> >
> > PR c/104506
> >
> > * gcc.dg/pr104506.c: New test case.
> >
> >
> >
> >
> >
> > Roger
> >
> > --
> >
> >
> >
  

On Mon, Feb 14, 2022 at 4:54 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
>
> This simple fix to the middle-end, resolves PR c/104506, by adding an
>
> explicit check for error_mark_node to useless_type_conversion_p.  I first
>
> trying fixing this in the C front-end, but the type is valid at the point
>
> that the NOP_EXPR is created, so the poisoned type leaks to the middle-end.
>
> Returning either true or false from useless_type_conversion_p avoids the
>
> ICE-after-error.  Apologies to Andrew Pinski, I hadn't noticed that he'd
>
> assigned this PR to himself until after my regression testing had finished.
>
>
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and
>
> make -k check with no new failures.  Ok for mainline?
>
>
>
>
>
> 2022-02-14  Roger Sayle  <roger@nextmovesoftware.com>
>
>
>
> gcc/ChangeLog
>
> PR c/104506
>
> * gimple-expr.cc (useless_type_conversion_p): Add a check for
>
> error_mark_node.

I came up with a different patch (attached) which just changes
tree_ssa_useless_type_conversion rather than useless_type_conversion_p
which I was going to submit but had an issue with my build machine.
I did it this way as it was similar to how
STRIP_NOPS/tree_nop_conversion was done already.

Also from my description of the patch
    STRIP_USELESS_TYPE_CONVERSION is mostly used inside the gimplifier
    and the places where it is used outside of the gimplifier would not
    be adding too much overhead.

Though I think Richard Biener's patch is better really. It would be
interesting to see how the C++ front-end handles this case, I remember
it using integer_type_node in some locations after an error for a
type.

Thanks,
Andrew Pinski

>
>
>
> gcc/testsuite/ChangeLog
>
> PR c/104506
>
> * gcc.dg/pr104506.c: New test case.
>
>
>
>
>
> Roger
>
> --
>
>
From 43d418042efacbb7efe8664fbb1176608470474a Mon Sep 17 00:00:00 2001
From: Andrew Pinski <apinski@marvell.com>
Date: Sun, 13 Feb 2022 00:09:39 +0000
Subject: [PATCH] c: [PR104506] Fix ICE after error due to change of type to
 error_mark_node

The problem here is we end up with an error_mark_node when calling
useless_type_conversion_p and that ICEs. STRIP_NOPS/tree_nop_conversion
has had a check for the inner type being an error_mark_node since g9a6bb3f78c96
(2000). This just adds the check also to tree_ssa_useless_type_conversion.
STRIP_USELESS_TYPE_CONVERSION is mostly used inside the gimplifier
and the places where it is used outside of the gimplifier would not
be adding too much overhead.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

Thanks,
Andrew Pinski

	PR c/104506

gcc/ChangeLog:

	* tree-ssa.cc (tree_ssa_useless_type_conversion):
	Check the inner type before calling useless_type_conversion_p.

gcc/testsuite/ChangeLog:

	* gcc.dg/pr104506-1.c: New test.
	* gcc.dg/pr104506-2.c: New test.
	* gcc.dg/pr104506-3.c: New test.
---
 gcc/testsuite/gcc.dg/pr104506-1.c | 12 ++++++++++++
 gcc/testsuite/gcc.dg/pr104506-2.c | 11 +++++++++++
 gcc/testsuite/gcc.dg/pr104506-3.c | 11 +++++++++++
 gcc/tree-ssa.cc                   | 20 +++++++++++++-------
 4 files changed, 47 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr104506-1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr104506-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr104506-3.c

diff --git a/gcc/testsuite/gcc.dg/pr104506-1.c b/gcc/testsuite/gcc.dg/pr104506-1.c
new file mode 100644
index 00000000000..5eb71911b71
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr104506-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu11" } */
+/* PR c/104506: we used to ICE after the error of
+   changing the type.  */
+
+void
+foo (double x)
+/* { dg-message "note: previous definition" "previous definition" { target *-*-* } .-1 } */
+{
+  (void)x;
+  int x; /* { dg-error "redeclared as different kind of symbol" } */
+}
diff --git a/gcc/testsuite/gcc.dg/pr104506-2.c b/gcc/testsuite/gcc.dg/pr104506-2.c
new file mode 100644
index 00000000000..3c3aaaac4f8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr104506-2.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu11" } */
+/* PR c/104506: we used to ICE after the error of
+   changing the type.  */
+void
+foo (double x)
+/* { dg-message "note: previous definition" "previous definition" { target *-*-* } .-1 } */
+{
+  x;
+  int x; /* { dg-error "redeclared as different kind of symbol" } */
+}
diff --git a/gcc/testsuite/gcc.dg/pr104506-3.c b/gcc/testsuite/gcc.dg/pr104506-3.c
new file mode 100644
index 00000000000..b14deb5cf25
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr104506-3.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* PR c/104506: we used to ICE after the error of
+   changing the type.  */
+double x;
+/* { dg-message "note: previous declaration" "previous declaration" { target *-*-* } .-1 } */
+void
+foo (void)
+{
+  x;
+}
+int x; /* { dg-error "conflicting types" } */
diff --git a/gcc/tree-ssa.cc b/gcc/tree-ssa.cc
index 8fe0682981d..98dada09647 100644
--- a/gcc/tree-ssa.cc
+++ b/gcc/tree-ssa.cc
@@ -1256,18 +1256,24 @@ delete_tree_ssa (struct function *fn)
 bool
 tree_ssa_useless_type_conversion (tree expr)
 {
+  tree outer_type, inner_type;
+
   /* If we have an assignment that merely uses a NOP_EXPR to change
      the top of the RHS to the type of the LHS and the type conversion
      is "safe", then strip away the type conversion so that we can
      enter LHS = RHS into the const_and_copies table.  */
-  if (CONVERT_EXPR_P (expr)
-      || TREE_CODE (expr) == VIEW_CONVERT_EXPR
-      || TREE_CODE (expr) == NON_LVALUE_EXPR)
-    return useless_type_conversion_p
-      (TREE_TYPE (expr),
-       TREE_TYPE (TREE_OPERAND (expr, 0)));
+  if (!CONVERT_EXPR_P (expr)
+      && TREE_CODE (expr) != VIEW_CONVERT_EXPR
+      && TREE_CODE (expr) != NON_LVALUE_EXPR)
+    return false;
 
-  return false;
+  outer_type = TREE_TYPE (expr);
+  inner_type = TREE_TYPE (TREE_OPERAND (expr, 0));
+
+  if (!inner_type || inner_type == error_mark_node)
+    return false;
+
+  return useless_type_conversion_p (outer_type, inner_type);
 }
 
 /* Strip conversions from EXP according to
  

On Tue, Feb 15, 2022 at 12:58 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Mon, Feb 14, 2022 at 4:54 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
> >
> >
> >
> > This simple fix to the middle-end, resolves PR c/104506, by adding an
> >
> > explicit check for error_mark_node to useless_type_conversion_p.  I first
> >
> > trying fixing this in the C front-end, but the type is valid at the point
> >
> > that the NOP_EXPR is created, so the poisoned type leaks to the middle-end.
> >
> > Returning either true or false from useless_type_conversion_p avoids the
> >
> > ICE-after-error.  Apologies to Andrew Pinski, I hadn't noticed that he'd
> >
> > assigned this PR to himself until after my regression testing had finished.
> >
> >
> >
> > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and
> >
> > make -k check with no new failures.  Ok for mainline?
> >
> >
> >
> >
> >
> > 2022-02-14  Roger Sayle  <roger@nextmovesoftware.com>
> >
> >
> >
> > gcc/ChangeLog
> >
> > PR c/104506
> >
> > * gimple-expr.cc (useless_type_conversion_p): Add a check for
> >
> > error_mark_node.
>
> I came up with a different patch (attached) which just changes
> tree_ssa_useless_type_conversion rather than useless_type_conversion_p
> which I was going to submit but had an issue with my build machine.
> I did it this way as it was similar to how
> STRIP_NOPS/tree_nop_conversion was done already.
>
> Also from my description of the patch
>     STRIP_USELESS_TYPE_CONVERSION is mostly used inside the gimplifier
>     and the places where it is used outside of the gimplifier would not
>     be adding too much overhead.
>
> Though I think Richard Biener's patch is better really. It would be
> interesting to see how the C++ front-end handles this case, I remember
> it using integer_type_node in some locations after an error for a
> type.

If the fix to the C frontend doesn't work out I'd indeed prefer your variant.
Nit:

+  outer_type = TREE_TYPE (expr);
+  inner_type = TREE_TYPE (TREE_OPERAND (expr, 0));
+
+  if (!inner_type || inner_type == error_mark_node)
+    return false;

unless we get to a case where inner_type == NULL I would not bother
checking that.

As said, that TREE_TYPE (error_mark_node) is not a type is IMHO bad
for error recovery.  Maybe we really need ERROR_TYPE here.

> Thanks,
> Andrew Pinski
>
> >
> >
> >
> > gcc/testsuite/ChangeLog
> >
> > PR c/104506
> >
> > * gcc.dg/pr104506.c: New test case.
> >
> >
> >
> >
> >
> > Roger
> >
> > --
> >
> >
  

On Mon, Feb 14, 2022 at 11:33 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Feb 15, 2022 at 12:58 AM Andrew Pinski via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Mon, Feb 14, 2022 at 4:54 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
> > >
> > >
> > >
> > > This simple fix to the middle-end, resolves PR c/104506, by adding an
> > >
> > > explicit check for error_mark_node to useless_type_conversion_p.  I first
> > >
> > > trying fixing this in the C front-end, but the type is valid at the point
> > >
> > > that the NOP_EXPR is created, so the poisoned type leaks to the middle-end.
> > >
> > > Returning either true or false from useless_type_conversion_p avoids the
> > >
> > > ICE-after-error.  Apologies to Andrew Pinski, I hadn't noticed that he'd
> > >
> > > assigned this PR to himself until after my regression testing had finished.
> > >
> > >
> > >
> > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and
> > >
> > > make -k check with no new failures.  Ok for mainline?
> > >
> > >
> > >
> > >
> > >
> > > 2022-02-14  Roger Sayle  <roger@nextmovesoftware.com>
> > >
> > >
> > >
> > > gcc/ChangeLog
> > >
> > > PR c/104506
> > >
> > > * gimple-expr.cc (useless_type_conversion_p): Add a check for
> > >
> > > error_mark_node.
> >
> > I came up with a different patch (attached) which just changes
> > tree_ssa_useless_type_conversion rather than useless_type_conversion_p
> > which I was going to submit but had an issue with my build machine.
> > I did it this way as it was similar to how
> > STRIP_NOPS/tree_nop_conversion was done already.
> >
> > Also from my description of the patch
> >     STRIP_USELESS_TYPE_CONVERSION is mostly used inside the gimplifier
> >     and the places where it is used outside of the gimplifier would not
> >     be adding too much overhead.
> >
> > Though I think Richard Biener's patch is better really. It would be
> > interesting to see how the C++ front-end handles this case, I remember
> > it using integer_type_node in some locations after an error for a
> > type.
>
> If the fix to the C frontend doesn't work out I'd indeed prefer your variant.
> Nit:
>
> +  outer_type = TREE_TYPE (expr);
> +  inner_type = TREE_TYPE (TREE_OPERAND (expr, 0));
> +
> +  if (!inner_type || inner_type == error_mark_node)
> +    return false;
>
> unless we get to a case where inner_type == NULL I would not bother
> checking that.

Understood, I will remove it, I was just copying exactly what was done
in tree_nop_conversion really. The history on the null check seems to
date to 2000 without any explanation really.

Thanks,
Andrew Pinski

>
> As said, that TREE_TYPE (error_mark_node) is not a type is IMHO bad
> for error recovery.  Maybe we really need ERROR_TYPE here.
>
> > Thanks,
> > Andrew Pinski
> >
> > >
> > >
> > >
> > > gcc/testsuite/ChangeLog
> > >
> > > PR c/104506
> > >
> > > * gcc.dg/pr104506.c: New test case.
> > >
> > >
> > >
> > >
> > >
> > > Roger
> > >
> > > --
> > >
> > >
  

On Mon, Feb 14, 2022 at 11:39 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Mon, Feb 14, 2022 at 11:33 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Tue, Feb 15, 2022 at 12:58 AM Andrew Pinski via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > On Mon, Feb 14, 2022 at 4:54 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
> > > >
> > > >
> > > >
> > > > This simple fix to the middle-end, resolves PR c/104506, by adding an
> > > >
> > > > explicit check for error_mark_node to useless_type_conversion_p.  I first
> > > >
> > > > trying fixing this in the C front-end, but the type is valid at the point
> > > >
> > > > that the NOP_EXPR is created, so the poisoned type leaks to the middle-end.
> > > >
> > > > Returning either true or false from useless_type_conversion_p avoids the
> > > >
> > > > ICE-after-error.  Apologies to Andrew Pinski, I hadn't noticed that he'd
> > > >
> > > > assigned this PR to himself until after my regression testing had finished.
> > > >
> > > >
> > > >
> > > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap and
> > > >
> > > > make -k check with no new failures.  Ok for mainline?
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > 2022-02-14  Roger Sayle  <roger@nextmovesoftware.com>
> > > >
> > > >
> > > >
> > > > gcc/ChangeLog
> > > >
> > > > PR c/104506
> > > >
> > > > * gimple-expr.cc (useless_type_conversion_p): Add a check for
> > > >
> > > > error_mark_node.
> > >
> > > I came up with a different patch (attached) which just changes
> > > tree_ssa_useless_type_conversion rather than useless_type_conversion_p
> > > which I was going to submit but had an issue with my build machine.
> > > I did it this way as it was similar to how
> > > STRIP_NOPS/tree_nop_conversion was done already.
> > >
> > > Also from my description of the patch
> > >     STRIP_USELESS_TYPE_CONVERSION is mostly used inside the gimplifier
> > >     and the places where it is used outside of the gimplifier would not
> > >     be adding too much overhead.
> > >
> > > Though I think Richard Biener's patch is better really. It would be
> > > interesting to see how the C++ front-end handles this case, I remember
> > > it using integer_type_node in some locations after an error for a
> > > type.
> >
> > If the fix to the C frontend doesn't work out I'd indeed prefer your variant.
> > Nit:
> >
> > +  outer_type = TREE_TYPE (expr);
> > +  inner_type = TREE_TYPE (TREE_OPERAND (expr, 0));
> > +
> > +  if (!inner_type || inner_type == error_mark_node)
> > +    return false;
> >
> > unless we get to a case where inner_type == NULL I would not bother
> > checking that.
>
> Understood, I will remove it, I was just copying exactly what was done
> in tree_nop_conversion really. The history on the null check seems to
> date to 2000 without any explanation really.

Finally submitted it as
https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590595.html

Thanks,
Andrew


>
> Thanks,
> Andrew Pinski
>
> >
> > As said, that TREE_TYPE (error_mark_node) is not a type is IMHO bad
> > for error recovery.  Maybe we really need ERROR_TYPE here.
> >
> > > Thanks,
> > > Andrew Pinski
> > >
> > > >
> > > >
> > > >
> > > > gcc/testsuite/ChangeLog
> > > >
> > > > PR c/104506
> > > >
> > > > * gcc.dg/pr104506.c: New test case.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > Roger
> > > >
> > > > --
> > > >
> > > >