diff mbox series

[C] PR c/109618: ICE-after-error from error_mark_node.

Message ID 010401da9a89$d5a33150$80e993f0$@nextmovesoftware.com
State New
Headers show
Series [C] PR c/109618: ICE-after-error from error_mark_node. | expand

Commit Message

Roger Sayle April 29, 2024, 11:06 p.m. UTC
This patch solves another ICE-after-error problem in the C family
front-ends.  Upon a conflicting type redeclaration, the ambiguous
type is poisoned with an error_mark_node to indicate to the middle-end
that the type is suspect, but care has to be taken by the front-end to
avoid passing these malformed trees into the middle-end during error
recovery. In this case, a var_decl with a poisoned type appears within
a sizeof() expression (wrapped in NOP_EXPR) which causes problems.

This revision of the patch tests seen_error() to avoid tree traversal
(STRIP_NOPs) in the most common case that an error hasn't occurred.
Both this version (and an earlier revision that didn't test seen_error)
have survived bootstrap and regression testing on x86_64-pc-linux-gnu.
As a consolation, this code also contains a minor performance improvement,
by avoiding trying to create (and folding away) a CEIL_DIV_EXPR in the
common case that "char" is a single-byte.  The current code relies on
the middle-end's tree folding to recognize that CEIL_DIV_EXPR of
integer_one_node is a no-op, that can be optimized away.

Ok for mainline?


2024-04-30  Roger Sayle  <roger@nextmovesoftware.com>

gcc/c-family/ChangeLog
        PR c/109618
        * c-common.cc (c_sizeof_or_alignof_type): If seen_error() check
        whether value is (a VAR_DECL) of type error_mark_node, or a
        NOP_EXPR thereof.  Avoid folding CEIL_DIV_EXPR for the common
        case where char_type is a single byte.

gcc/testsuite/ChangeLog
        PR c/109618
        * gcc.dg/pr109618.c: New test case.


Thanks in advance,
Roger
--

Comments

Richard Biener April 30, 2024, 7:37 a.m. UTC | #1
On Tue, Apr 30, 2024 at 1:06 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch solves another ICE-after-error problem in the C family
> front-ends.  Upon a conflicting type redeclaration, the ambiguous
> type is poisoned with an error_mark_node to indicate to the middle-end
> that the type is suspect, but care has to be taken by the front-end to
> avoid passing these malformed trees into the middle-end during error
> recovery. In this case, a var_decl with a poisoned type appears within
> a sizeof() expression (wrapped in NOP_EXPR) which causes problems.
>
> This revision of the patch tests seen_error() to avoid tree traversal
> (STRIP_NOPs) in the most common case that an error hasn't occurred.
> Both this version (and an earlier revision that didn't test seen_error)
> have survived bootstrap and regression testing on x86_64-pc-linux-gnu.
> As a consolation, this code also contains a minor performance improvement,
> by avoiding trying to create (and folding away) a CEIL_DIV_EXPR in the
> common case that "char" is a single-byte.  The current code relies on
> the middle-end's tree folding to recognize that CEIL_DIV_EXPR of
> integer_one_node is a no-op, that can be optimized away.
>
> Ok for mainline?

Where does it end up ICEing?  I see size_binop_loc guards against
error_mark_node operands already, maybe it should use
error_operand_p instead?

>
> 2024-04-30  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/c-family/ChangeLog
>         PR c/109618
>         * c-common.cc (c_sizeof_or_alignof_type): If seen_error() check
>         whether value is (a VAR_DECL) of type error_mark_node, or a
>         NOP_EXPR thereof.  Avoid folding CEIL_DIV_EXPR for the common
>         case where char_type is a single byte.
>
> gcc/testsuite/ChangeLog
>         PR c/109618
>         * gcc.dg/pr109618.c: New test case.
>
>
> Thanks in advance,
> Roger
> --
>
Roger Sayle April 30, 2024, 8:23 a.m. UTC | #2
Hi Richard,
Thanks for looking into this.

It’s not the call to size_binop_loc (for CEIL_DIV_EXPR) that's problematic, but the
call to fold_convert_loc (loc, size_type_node, value) on line 4009 of c-common.cc.
At this point, value is (NOP_EXPR:sizetype (VAR_DECL:error_mark_node)).

Ultimately, it's the code in match.pd /* Handle cases of two conversions in a row.  */
with the problematic line being (match.pd:4748):
      unsigned int inside_prec = element_precision (inside_type); 

Here inside_type is error_mark_node, and so tree type checking in element_precision
throws an internal_error.

There doesn’t seem to be a good way to fix this in element_precision, and it's
complicated to reorganize the logic in match.pd's "with clause" inside the
(ocvt (icvt@1 @0)), but perhaps a (ocvt (icvt:non_error_type@1 @0))?

The last place/opportunity the front-end could sanitize this operand before
passing the dubious tree to the middle-end is c_sizeof_or_alignof_type (which
alas doesn't appear in the backtrace due to inlining).

#5  0x000000000227b0e9 in internal_error (
    gmsgid=gmsgid@entry=0x249c7b8 "tree check: expected class %qs, have %qs (%s) in %s, at %s:%d") at ../../gcc/gcc/diagnostic.cc:2232
#6  0x000000000081e32a in tree_class_check_failed (node=0x7ffff6c1ef30,
    cl=cl@entry=tcc_type, file=file@entry=0x2495f3f "../../gcc/gcc/tree.cc",
    line=line@entry=6795, function=function@entry=0x24961fe "element_precision")
    at ../../gcc/gcc/tree.cc:9005
#7  0x000000000081ef4c in tree_class_check (__t=<optimized out>, __class=tcc_type,
    __f=0x2495f3f "../../gcc/gcc/tree.cc", __l=6795,
    __g=0x24961fe "element_precision") at ../../gcc/gcc/tree.h:4067
#8  element_precision (type=<optimized out>, type@entry=0x7ffff6c1ef30)
    at ../../gcc/gcc/tree.cc:6795
#9  0x00000000017f66a4 in generic_simplify_CONVERT_EXPR (loc=201632,
    code=<optimized out>, type=0x7ffff6c3e7e0, _p0=0x7ffff6dc95c0)
    at generic-match-6.cc:3386
#10 0x0000000000c1b18c in fold_unary_loc (loc=201632, code=NOP_EXPR,
    type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at ../../gcc/gcc/fold-const.cc:9523
#11 0x0000000000c1d94a in fold_build1_loc (loc=201632, code=NOP_EXPR,
    type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at ../../gcc/gcc/fold-const.cc:14165
#12 0x000000000094068c in c_expr_sizeof_expr (loc=loc@entry=201632, expr=...)
    at ../../gcc/gcc/tree.h:3771
#13 0x000000000097f06c in c_parser_sizeof_expression (parser=<optimized out>)
    at ../../gcc/gcc/c/c-parser.cc:9932


I hope this explains what's happening.  The size_binop_loc call is a bit of a red
herring that returns the same tree it is given (as TYPE_PRECISION (char_type_node)
== BITS_PER_UNIT), so it's the "TYPE_SIZE_UNIT (type)" which needs to be checked
for the embedded VAR_DECL with a TREE_TYPE of error_mark_node.

As Andrew Pinski writes in comment #3, this one is trickier than average.

A more comprehensive fix might be to write deep_error_operand_p which does
more of a tree traversal checking error_operand_p within the unary and binary
operators of an expression tree.

Please let me know what you think/recommend.
Best regards,
Roger
--

> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: 30 April 2024 08:38
> To: Roger Sayle <roger@nextmovesoftware.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [C PATCH] PR c/109618: ICE-after-error from error_mark_node.
> 
> On Tue, Apr 30, 2024 at 1:06 AM Roger Sayle <roger@nextmovesoftware.com>
> wrote:
> >
> >
> > This patch solves another ICE-after-error problem in the C family
> > front-ends.  Upon a conflicting type redeclaration, the ambiguous type
> > is poisoned with an error_mark_node to indicate to the middle-end that
> > the type is suspect, but care has to be taken by the front-end to
> > avoid passing these malformed trees into the middle-end during error
> > recovery. In this case, a var_decl with a poisoned type appears within
> > a sizeof() expression (wrapped in NOP_EXPR) which causes problems.
> >
> > This revision of the patch tests seen_error() to avoid tree traversal
> > (STRIP_NOPs) in the most common case that an error hasn't occurred.
> > Both this version (and an earlier revision that didn't test
> > seen_error) have survived bootstrap and regression testing on x86_64-pc-linux-
> gnu.
> > As a consolation, this code also contains a minor performance
> > improvement, by avoiding trying to create (and folding away) a
> > CEIL_DIV_EXPR in the common case that "char" is a single-byte.  The
> > current code relies on the middle-end's tree folding to recognize that
> > CEIL_DIV_EXPR of integer_one_node is a no-op, that can be optimized away.
> >
> > Ok for mainline?
> 
> Where does it end up ICEing?  I see size_binop_loc guards against
> error_mark_node operands already, maybe it should use error_operand_p
> instead?
> 
> >
> > 2024-04-30  Roger Sayle  <roger@nextmovesoftware.com>
> >
> > gcc/c-family/ChangeLog
> >         PR c/109618
> >         * c-common.cc (c_sizeof_or_alignof_type): If seen_error() check
> >         whether value is (a VAR_DECL) of type error_mark_node, or a
> >         NOP_EXPR thereof.  Avoid folding CEIL_DIV_EXPR for the common
> >         case where char_type is a single byte.
> >
> > gcc/testsuite/ChangeLog
> >         PR c/109618
> >         * gcc.dg/pr109618.c: New test case.
> >
> >
> > Thanks in advance,
> > Roger
> > --
> >
Richard Biener April 30, 2024, 9:17 a.m. UTC | #3
On Tue, Apr 30, 2024 at 10:23 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> Hi Richard,
> Thanks for looking into this.
>
> It’s not the call to size_binop_loc (for CEIL_DIV_EXPR) that's problematic, but the
> call to fold_convert_loc (loc, size_type_node, value) on line 4009 of c-common.cc.
> At this point, value is (NOP_EXPR:sizetype (VAR_DECL:error_mark_node)).

I see.  Can we catch this when we build (NOP_EXPR:sizetype
(VAR_DECL:error_mark_node))
and instead have it "build" error_mark_node?

>
> Ultimately, it's the code in match.pd /* Handle cases of two conversions in a row.  */
> with the problematic line being (match.pd:4748):
>       unsigned int inside_prec = element_precision (inside_type);
>
> Here inside_type is error_mark_node, and so tree type checking in element_precision
> throws an internal_error.
>
> There doesn’t seem to be a good way to fix this in element_precision, and it's
> complicated to reorganize the logic in match.pd's "with clause" inside the
> (ocvt (icvt@1 @0)), but perhaps a (ocvt (icvt:non_error_type@1 @0))?
>
> The last place/opportunity the front-end could sanitize this operand before
> passing the dubious tree to the middle-end is c_sizeof_or_alignof_type (which
> alas doesn't appear in the backtrace due to inlining).
>
> #5  0x000000000227b0e9 in internal_error (
>     gmsgid=gmsgid@entry=0x249c7b8 "tree check: expected class %qs, have %qs (%s) in %s, at %s:%d") at ../../gcc/gcc/diagnostic.cc:2232
> #6  0x000000000081e32a in tree_class_check_failed (node=0x7ffff6c1ef30,
>     cl=cl@entry=tcc_type, file=file@entry=0x2495f3f "../../gcc/gcc/tree.cc",
>     line=line@entry=6795, function=function@entry=0x24961fe "element_precision")
>     at ../../gcc/gcc/tree.cc:9005
> #7  0x000000000081ef4c in tree_class_check (__t=<optimized out>, __class=tcc_type,
>     __f=0x2495f3f "../../gcc/gcc/tree.cc", __l=6795,
>     __g=0x24961fe "element_precision") at ../../gcc/gcc/tree.h:4067
> #8  element_precision (type=<optimized out>, type@entry=0x7ffff6c1ef30)
>     at ../../gcc/gcc/tree.cc:6795
> #9  0x00000000017f66a4 in generic_simplify_CONVERT_EXPR (loc=201632,
>     code=<optimized out>, type=0x7ffff6c3e7e0, _p0=0x7ffff6dc95c0)
>     at generic-match-6.cc:3386
> #10 0x0000000000c1b18c in fold_unary_loc (loc=201632, code=NOP_EXPR,
>     type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at ../../gcc/gcc/fold-const.cc:9523
> #11 0x0000000000c1d94a in fold_build1_loc (loc=201632, code=NOP_EXPR,
>     type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at ../../gcc/gcc/fold-const.cc:14165
> #12 0x000000000094068c in c_expr_sizeof_expr (loc=loc@entry=201632, expr=...)
>     at ../../gcc/gcc/tree.h:3771
> #13 0x000000000097f06c in c_parser_sizeof_expression (parser=<optimized out>)
>     at ../../gcc/gcc/c/c-parser.cc:9932
>
>
> I hope this explains what's happening.  The size_binop_loc call is a bit of a red
> herring that returns the same tree it is given (as TYPE_PRECISION (char_type_node)
> == BITS_PER_UNIT), so it's the "TYPE_SIZE_UNIT (type)" which needs to be checked
> for the embedded VAR_DECL with a TREE_TYPE of error_mark_node.
>
> As Andrew Pinski writes in comment #3, this one is trickier than average.
>
> A more comprehensive fix might be to write deep_error_operand_p which does
> more of a tree traversal checking error_operand_p within the unary and binary
> operators of an expression tree.
>
> Please let me know what you think/recommend.
> Best regards,
> Roger
> --
>
> > -----Original Message-----
> > From: Richard Biener <richard.guenther@gmail.com>
> > Sent: 30 April 2024 08:38
> > To: Roger Sayle <roger@nextmovesoftware.com>
> > Cc: gcc-patches@gcc.gnu.org
> > Subject: Re: [C PATCH] PR c/109618: ICE-after-error from error_mark_node.
> >
> > On Tue, Apr 30, 2024 at 1:06 AM Roger Sayle <roger@nextmovesoftware.com>
> > wrote:
> > >
> > >
> > > This patch solves another ICE-after-error problem in the C family
> > > front-ends.  Upon a conflicting type redeclaration, the ambiguous type
> > > is poisoned with an error_mark_node to indicate to the middle-end that
> > > the type is suspect, but care has to be taken by the front-end to
> > > avoid passing these malformed trees into the middle-end during error
> > > recovery. In this case, a var_decl with a poisoned type appears within
> > > a sizeof() expression (wrapped in NOP_EXPR) which causes problems.
> > >
> > > This revision of the patch tests seen_error() to avoid tree traversal
> > > (STRIP_NOPs) in the most common case that an error hasn't occurred.
> > > Both this version (and an earlier revision that didn't test
> > > seen_error) have survived bootstrap and regression testing on x86_64-pc-linux-
> > gnu.
> > > As a consolation, this code also contains a minor performance
> > > improvement, by avoiding trying to create (and folding away) a
> > > CEIL_DIV_EXPR in the common case that "char" is a single-byte.  The
> > > current code relies on the middle-end's tree folding to recognize that
> > > CEIL_DIV_EXPR of integer_one_node is a no-op, that can be optimized away.
> > >
> > > Ok for mainline?
> >
> > Where does it end up ICEing?  I see size_binop_loc guards against
> > error_mark_node operands already, maybe it should use error_operand_p
> > instead?
> >
> > >
> > > 2024-04-30  Roger Sayle  <roger@nextmovesoftware.com>
> > >
> > > gcc/c-family/ChangeLog
> > >         PR c/109618
> > >         * c-common.cc (c_sizeof_or_alignof_type): If seen_error() check
> > >         whether value is (a VAR_DECL) of type error_mark_node, or a
> > >         NOP_EXPR thereof.  Avoid folding CEIL_DIV_EXPR for the common
> > >         case where char_type is a single byte.
> > >
> > > gcc/testsuite/ChangeLog
> > >         PR c/109618
> > >         * gcc.dg/pr109618.c: New test case.
> > >
> > >
> > > Thanks in advance,
> > > Roger
> > > --
> > >
>
Roger Sayle April 30, 2024, 3:14 p.m. UTC | #4
> On Tue, Apr 30, 2024 at 10:23 AM Roger Sayle <roger@nextmovesoftware.com>
> wrote:
> > Hi Richard,
> > Thanks for looking into this.
> >
> > It’s not the call to size_binop_loc (for CEIL_DIV_EXPR) that's
> > problematic, but the call to fold_convert_loc (loc, size_type_node, value) on line
> 4009 of c-common.cc.
> > At this point, value is (NOP_EXPR:sizetype (VAR_DECL:error_mark_node)).
> 
> I see.  Can we catch this when we build (NOP_EXPR:sizetype
> (VAR_DECL:error_mark_node))
> and instead have it "build" error_mark_node?

That's the tricky part.  At the point the NOP_EXPR is built the VAR_DECL's type
is valid.  It's later when this variable gets redefined with a conflicting type that
the shared VAR_DECL gets modified, setting its type to error_mark_node.
Mutating this shared node, then potentially introduces error_operand_p at
arbitrary places deep within an expression.  Fortunately, we only have to 
worry about this in the unusual/exceptional case that seen_error() is true.


> > Ultimately, it's the code in match.pd /* Handle cases of two
> > conversions in a row.  */ with the problematic line being (match.pd:4748):
> >       unsigned int inside_prec = element_precision (inside_type);
> >
> > Here inside_type is error_mark_node, and so tree type checking in
> > element_precision throws an internal_error.
> >
> > There doesn’t seem to be a good way to fix this in element_precision,
> > and it's complicated to reorganize the logic in match.pd's "with
> > clause" inside the (ocvt (icvt@1 @0)), but perhaps a (ocvt
> (icvt:non_error_type@1 @0))?
> >
> > The last place/opportunity the front-end could sanitize this operand
> > before passing the dubious tree to the middle-end is
> > c_sizeof_or_alignof_type (which alas doesn't appear in the backtrace due to
> inlining).
> >
> > #5  0x000000000227b0e9 in internal_error (
> >     gmsgid=gmsgid@entry=0x249c7b8 "tree check: expected class %qs,
> > have %qs (%s) in %s, at %s:%d") at ../../gcc/gcc/diagnostic.cc:2232
> > #6  0x000000000081e32a in tree_class_check_failed (node=0x7ffff6c1ef30,
> >     cl=cl@entry=tcc_type, file=file@entry=0x2495f3f "../../gcc/gcc/tree.cc",
> >     line=line@entry=6795, function=function@entry=0x24961fe
> "element_precision")
> >     at ../../gcc/gcc/tree.cc:9005
> > #7  0x000000000081ef4c in tree_class_check (__t=<optimized out>,
> __class=tcc_type,
> >     __f=0x2495f3f "../../gcc/gcc/tree.cc", __l=6795,
> >     __g=0x24961fe "element_precision") at ../../gcc/gcc/tree.h:4067
> > #8  element_precision (type=<optimized out>, type@entry=0x7ffff6c1ef30)
> >     at ../../gcc/gcc/tree.cc:6795
> > #9  0x00000000017f66a4 in generic_simplify_CONVERT_EXPR (loc=201632,
> >     code=<optimized out>, type=0x7ffff6c3e7e0, _p0=0x7ffff6dc95c0)
> >     at generic-match-6.cc:3386
> > #10 0x0000000000c1b18c in fold_unary_loc (loc=201632, code=NOP_EXPR,
> >     type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at
> > ../../gcc/gcc/fold-const.cc:9523
> > #11 0x0000000000c1d94a in fold_build1_loc (loc=201632, code=NOP_EXPR,
> >     type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at
> > ../../gcc/gcc/fold-const.cc:14165
> > #12 0x000000000094068c in c_expr_sizeof_expr (loc=loc@entry=201632,
> expr=...)
> >     at ../../gcc/gcc/tree.h:3771
> > #13 0x000000000097f06c in c_parser_sizeof_expression (parser=<optimized
> out>)
> >     at ../../gcc/gcc/c/c-parser.cc:9932
> >
> >
> > I hope this explains what's happening.  The size_binop_loc call is a
> > bit of a red herring that returns the same tree it is given (as
> > TYPE_PRECISION (char_type_node) == BITS_PER_UNIT), so it's the
> > "TYPE_SIZE_UNIT (type)" which needs to be checked for the embedded
> VAR_DECL with a TREE_TYPE of error_mark_node.
> >
> > As Andrew Pinski writes in comment #3, this one is trickier than average.
> >
> > A more comprehensive fix might be to write deep_error_operand_p which
> > does more of a tree traversal checking error_operand_p within the
> > unary and binary operators of an expression tree.
> >
> > Please let me know what you think/recommend.
> > Best regards,
> > Roger
> > --
> >
> > > -----Original Message-----
> > > From: Richard Biener <richard.guenther@gmail.com>
> > > Sent: 30 April 2024 08:38
> > > To: Roger Sayle <roger@nextmovesoftware.com>
> > > Cc: gcc-patches@gcc.gnu.org
> > > Subject: Re: [C PATCH] PR c/109618: ICE-after-error from error_mark_node.
> > >
> > > On Tue, Apr 30, 2024 at 1:06 AM Roger Sayle
> > > <roger@nextmovesoftware.com>
> > > wrote:
> > > >
> > > >
> > > > This patch solves another ICE-after-error problem in the C family
> > > > front-ends.  Upon a conflicting type redeclaration, the ambiguous
> > > > type is poisoned with an error_mark_node to indicate to the
> > > > middle-end that the type is suspect, but care has to be taken by
> > > > the front-end to avoid passing these malformed trees into the
> > > > middle-end during error recovery. In this case, a var_decl with a
> > > > poisoned type appears within a sizeof() expression (wrapped in NOP_EXPR)
> which causes problems.
> > > >
> > > > This revision of the patch tests seen_error() to avoid tree
> > > > traversal
> > > > (STRIP_NOPs) in the most common case that an error hasn't occurred.
> > > > Both this version (and an earlier revision that didn't test
> > > > seen_error) have survived bootstrap and regression testing on
> > > > x86_64-pc-linux-
> > > gnu.
> > > > As a consolation, this code also contains a minor performance
> > > > improvement, by avoiding trying to create (and folding away) a
> > > > CEIL_DIV_EXPR in the common case that "char" is a single-byte.
> > > > The current code relies on the middle-end's tree folding to
> > > > recognize that CEIL_DIV_EXPR of integer_one_node is a no-op, that can be
> optimized away.
> > > >
> > > > Ok for mainline?
> > >
> > > Where does it end up ICEing?  I see size_binop_loc guards against
> > > error_mark_node operands already, maybe it should use
> > > error_operand_p instead?
> > >
> > > >
> > > > 2024-04-30  Roger Sayle  <roger@nextmovesoftware.com>
> > > >
> > > > gcc/c-family/ChangeLog
> > > >         PR c/109618
> > > >         * c-common.cc (c_sizeof_or_alignof_type): If seen_error() check
> > > >         whether value is (a VAR_DECL) of type error_mark_node, or a
> > > >         NOP_EXPR thereof.  Avoid folding CEIL_DIV_EXPR for the common
> > > >         case where char_type is a single byte.
> > > >
> > > > gcc/testsuite/ChangeLog
> > > >         PR c/109618
> > > >         * gcc.dg/pr109618.c: New test case.
> > > >
> > > >
> > > > Thanks in advance,
> > > > Roger
> > > > --
> > > >
> >
Richard Biener May 2, 2024, 9:57 a.m. UTC | #5
On Tue, Apr 30, 2024 at 5:14 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
> > On Tue, Apr 30, 2024 at 10:23 AM Roger Sayle <roger@nextmovesoftware.com>
> > wrote:
> > > Hi Richard,
> > > Thanks for looking into this.
> > >
> > > It’s not the call to size_binop_loc (for CEIL_DIV_EXPR) that's
> > > problematic, but the call to fold_convert_loc (loc, size_type_node, value) on line
> > 4009 of c-common.cc.
> > > At this point, value is (NOP_EXPR:sizetype (VAR_DECL:error_mark_node)).
> >
> > I see.  Can we catch this when we build (NOP_EXPR:sizetype
> > (VAR_DECL:error_mark_node))
> > and instead have it "build" error_mark_node?
>
> That's the tricky part.  At the point the NOP_EXPR is built the VAR_DECL's type
> is valid.  It's later when this variable gets redefined with a conflicting type that
> the shared VAR_DECL gets modified, setting its type to error_mark_node.
> Mutating this shared node, then potentially introduces error_operand_p at
> arbitrary places deep within an expression.

Ugh, I see.  I wonder if we can avoid setting the VAR_DECL type to
error_mark_node,
it's bad do change existing IL to some random invalid state.  Can we
instead make
future name lookup on the then not merged decl fail?  I suppose we made it
error_mark_node to improve error recovery but as can be seen here the way we
do this leads to issues.

> Fortunately, we only have to
> worry about this in the unusual/exceptional case that seen_error() is true.

But unfortunately there might be many places this would be necessary, so
it doesn't look very maintainable to me.

Marek?

Thanks,
Richard.

>
> > > Ultimately, it's the code in match.pd /* Handle cases of two
> > > conversions in a row.  */ with the problematic line being (match.pd:4748):
> > >       unsigned int inside_prec = element_precision (inside_type);
> > >
> > > Here inside_type is error_mark_node, and so tree type checking in
> > > element_precision throws an internal_error.
> > >
> > > There doesn’t seem to be a good way to fix this in element_precision,
> > > and it's complicated to reorganize the logic in match.pd's "with
> > > clause" inside the (ocvt (icvt@1 @0)), but perhaps a (ocvt
> > (icvt:non_error_type@1 @0))?
> > >
> > > The last place/opportunity the front-end could sanitize this operand
> > > before passing the dubious tree to the middle-end is
> > > c_sizeof_or_alignof_type (which alas doesn't appear in the backtrace due to
> > inlining).
> > >
> > > #5  0x000000000227b0e9 in internal_error (
> > >     gmsgid=gmsgid@entry=0x249c7b8 "tree check: expected class %qs,
> > > have %qs (%s) in %s, at %s:%d") at ../../gcc/gcc/diagnostic.cc:2232
> > > #6  0x000000000081e32a in tree_class_check_failed (node=0x7ffff6c1ef30,
> > >     cl=cl@entry=tcc_type, file=file@entry=0x2495f3f "../../gcc/gcc/tree.cc",
> > >     line=line@entry=6795, function=function@entry=0x24961fe
> > "element_precision")
> > >     at ../../gcc/gcc/tree.cc:9005
> > > #7  0x000000000081ef4c in tree_class_check (__t=<optimized out>,
> > __class=tcc_type,
> > >     __f=0x2495f3f "../../gcc/gcc/tree.cc", __l=6795,
> > >     __g=0x24961fe "element_precision") at ../../gcc/gcc/tree.h:4067
> > > #8  element_precision (type=<optimized out>, type@entry=0x7ffff6c1ef30)
> > >     at ../../gcc/gcc/tree.cc:6795
> > > #9  0x00000000017f66a4 in generic_simplify_CONVERT_EXPR (loc=201632,
> > >     code=<optimized out>, type=0x7ffff6c3e7e0, _p0=0x7ffff6dc95c0)
> > >     at generic-match-6.cc:3386
> > > #10 0x0000000000c1b18c in fold_unary_loc (loc=201632, code=NOP_EXPR,
> > >     type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at
> > > ../../gcc/gcc/fold-const.cc:9523
> > > #11 0x0000000000c1d94a in fold_build1_loc (loc=201632, code=NOP_EXPR,
> > >     type=0x7ffff6c3e7e0, op0=0x7ffff6dc95c0) at
> > > ../../gcc/gcc/fold-const.cc:14165
> > > #12 0x000000000094068c in c_expr_sizeof_expr (loc=loc@entry=201632,
> > expr=...)
> > >     at ../../gcc/gcc/tree.h:3771
> > > #13 0x000000000097f06c in c_parser_sizeof_expression (parser=<optimized
> > out>)
> > >     at ../../gcc/gcc/c/c-parser.cc:9932
> > >
> > >
> > > I hope this explains what's happening.  The size_binop_loc call is a
> > > bit of a red herring that returns the same tree it is given (as
> > > TYPE_PRECISION (char_type_node) == BITS_PER_UNIT), so it's the
> > > "TYPE_SIZE_UNIT (type)" which needs to be checked for the embedded
> > VAR_DECL with a TREE_TYPE of error_mark_node.
> > >
> > > As Andrew Pinski writes in comment #3, this one is trickier than average.
> > >
> > > A more comprehensive fix might be to write deep_error_operand_p which
> > > does more of a tree traversal checking error_operand_p within the
> > > unary and binary operators of an expression tree.
> > >
> > > Please let me know what you think/recommend.
> > > Best regards,
> > > Roger
> > > --
> > >
> > > > -----Original Message-----
> > > > From: Richard Biener <richard.guenther@gmail.com>
> > > > Sent: 30 April 2024 08:38
> > > > To: Roger Sayle <roger@nextmovesoftware.com>
> > > > Cc: gcc-patches@gcc.gnu.org
> > > > Subject: Re: [C PATCH] PR c/109618: ICE-after-error from error_mark_node.
> > > >
> > > > On Tue, Apr 30, 2024 at 1:06 AM Roger Sayle
> > > > <roger@nextmovesoftware.com>
> > > > wrote:
> > > > >
> > > > >
> > > > > This patch solves another ICE-after-error problem in the C family
> > > > > front-ends.  Upon a conflicting type redeclaration, the ambiguous
> > > > > type is poisoned with an error_mark_node to indicate to the
> > > > > middle-end that the type is suspect, but care has to be taken by
> > > > > the front-end to avoid passing these malformed trees into the
> > > > > middle-end during error recovery. In this case, a var_decl with a
> > > > > poisoned type appears within a sizeof() expression (wrapped in NOP_EXPR)
> > which causes problems.
> > > > >
> > > > > This revision of the patch tests seen_error() to avoid tree
> > > > > traversal
> > > > > (STRIP_NOPs) in the most common case that an error hasn't occurred.
> > > > > Both this version (and an earlier revision that didn't test
> > > > > seen_error) have survived bootstrap and regression testing on
> > > > > x86_64-pc-linux-
> > > > gnu.
> > > > > As a consolation, this code also contains a minor performance
> > > > > improvement, by avoiding trying to create (and folding away) a
> > > > > CEIL_DIV_EXPR in the common case that "char" is a single-byte.
> > > > > The current code relies on the middle-end's tree folding to
> > > > > recognize that CEIL_DIV_EXPR of integer_one_node is a no-op, that can be
> > optimized away.
> > > > >
> > > > > Ok for mainline?
> > > >
> > > > Where does it end up ICEing?  I see size_binop_loc guards against
> > > > error_mark_node operands already, maybe it should use
> > > > error_operand_p instead?
> > > >
> > > > >
> > > > > 2024-04-30  Roger Sayle  <roger@nextmovesoftware.com>
> > > > >
> > > > > gcc/c-family/ChangeLog
> > > > >         PR c/109618
> > > > >         * c-common.cc (c_sizeof_or_alignof_type): If seen_error() check
> > > > >         whether value is (a VAR_DECL) of type error_mark_node, or a
> > > > >         NOP_EXPR thereof.  Avoid folding CEIL_DIV_EXPR for the common
> > > > >         case where char_type is a single byte.
> > > > >
> > > > > gcc/testsuite/ChangeLog
> > > > >         PR c/109618
> > > > >         * gcc.dg/pr109618.c: New test case.
> > > > >
> > > > >
> > > > > Thanks in advance,
> > > > > Roger
> > > > > --
> > > > >
> > >
>
diff mbox series

Patch

diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
index 6fa8243..be8ff09 100644
--- a/gcc/c-family/c-common.cc
+++ b/gcc/c-family/c-common.cc
@@ -3993,10 +3993,31 @@  c_sizeof_or_alignof_type (location_t loc,
   else
     {
       if (is_sizeof)
-	/* Convert in case a char is more than one unit.  */
-	value = size_binop_loc (loc, CEIL_DIV_EXPR, TYPE_SIZE_UNIT (type),
-				size_int (TYPE_PRECISION (char_type_node)
-					  / BITS_PER_UNIT));
+	{
+	  value = TYPE_SIZE_UNIT (type);
+
+	  /* PR 109618: Check for erroneous types, stripping NOPs.  */
+	  if (seen_error ())
+	    {
+	      tree tmp = value;
+	      while (CONVERT_EXPR_P (tmp)
+		     || TREE_CODE (tmp) == NON_LVALUE_EXPR)
+		{
+		  if (TREE_TYPE (tmp) == error_mark_node)
+		    return error_mark_node;
+		  tmp = TREE_OPERAND (tmp, 0);
+		}
+	      if (tmp == error_mark_node
+		  || TREE_TYPE (tmp) == error_mark_node)
+		return error_mark_node;
+	    }
+
+	  /* Convert in case a char is more than one unit.  */
+	  if (TYPE_PRECISION (char_type_node) != BITS_PER_UNIT)
+	    value = size_binop_loc (loc, CEIL_DIV_EXPR, value,
+				    size_int (TYPE_PRECISION (char_type_node)
+					      / BITS_PER_UNIT));
+	}
       else if (min_alignof)
 	value = size_int (min_align_of_type (type));
       else
diff --git a/gcc/testsuite/gcc.dg/pr109618.c b/gcc/testsuite/gcc.dg/pr109618.c
new file mode 100644
index 0000000..f240907
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr109618.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O0" } */
+int foo()
+{
+  const unsigned int var_1 = 2;
+  
+  char var_5[var_1];
+  
+  int var_1[10];  /* { dg-error "conflicting type" } */
+  
+  return sizeof(var_5);
+}
+