| Message ID | 010401da9a89$d5a33150$80e993f0$@nextmovesoftware.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C] PR c/109618: ICE-after-error from error_mark_node. | expand |
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 > -- >
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
> > --
> >
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 > > > -- > > > >
> 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 > > > > -- > > > > > >
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 --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); +} +
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 --