On Tue, Apr 30, 2024 at 5:14 PM Roger Sayle <ro...@nextmovesoftware.com> wrote: > > > On Tue, Apr 30, 2024 at 10:23 AM Roger Sayle <ro...@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.guent...@gmail.com> > > > > Sent: 30 April 2024 08:38 > > > > To: Roger Sayle <ro...@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 > > > > <ro...@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 <ro...@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 > > > > > -- > > > > > > > > >
