[C] PR c/109618: ICE-after-error from error_mark_node.
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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
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
> > > > > --
> > > > >
> > >
>
@@ -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
new file mode 100644
@@ -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);
+}
+