| Message ID | 20160226144336.GC3017@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/26/2016 07:43 AM, Jakub Jelinek wrote: > Hi! > > Already PR69483 and these two further PRs show that it is really a bad idea > to set TREE_TYPE of decls with incomplete types to error_mark_node, there > are lots of places in the middle-end that don't expect error_mark_nodes > appearing so late. > > I've bootstrapped/regtested on x86_64-linux and i686-linux following patch > which just doesn't do anything with the type, we don't emit the error > multiple times, because if the FE emits errors, cgraphunit doesn't attempt > to assemble variables or compile functions (just gimplifies them) if > seen_error (). Attached is then untested alternative, set the type to > something that should hopefully not cause ICEs nor further errors/warnings > during the error-recovery. > > Ok for trunk, or shall I test the other patch instead? > > 2016-02-26 Jakub Jelinek <jakub@redhat.com> > > PR c/69796 > PR c/69974 > * c-parser.c (c_parser_translation_unit): Don't change TREE_TYPE > of incomplete decls to error_mark_node. > > * gcc.dg/pr69796.c: New test. > * gcc.dg/pr69974.c: New test. This one leaves the type incomplete, right? So ISTM it's somewhat more likely than the second to expose other errors later with code that doesn't expect the type to be incomplete (much like other code doesn't expect to find error_mark_node in here). The second patch at least puts a real type in there. I suspect that's less likely to cause problems downstream, except perhaps with diagnostics. I could argue for either. I almost asked for the latter to be tested, but the more I think about it, I don't like slamming in another type like that. I'll conditionally approve -- if nobody objects in 72hrs, consider the first patch OK for the trunk. jeff
On Fri, Feb 26, 2016 at 02:45:38PM -0700, Jeff Law wrote: > >2016-02-26 Jakub Jelinek <jakub@redhat.com> > > > > PR c/69796 > > PR c/69974 > > * c-parser.c (c_parser_translation_unit): Don't change TREE_TYPE > > of incomplete decls to error_mark_node. > > > > * gcc.dg/pr69796.c: New test. > > * gcc.dg/pr69974.c: New test. > This one leaves the type incomplete, right? So ISTM it's somewhat more Yes. > likely than the second to expose other errors later with code that doesn't > expect the type to be incomplete (much like other code doesn't expect to > find error_mark_node in here). Well, we were without the error in the FE for many years, the error has been diagnosed only very late (during assemble_variable) and I'm not aware of ICEs because of that. The c-parser.c change to add the error has been done only to error also in -fsyntax-only mode. So while there is some risk, I think it is very low. > The second patch at least puts a real type in there. I suspect that's less > likely to cause problems downstream, except perhaps with diagnostics. Yeah, I'm worried some code might be upset when it compares the types and gets upset that they aren't compatible or something similar. > I'll conditionally approve -- if nobody objects in 72hrs, consider the first > patch OK for the trunk. Ok, will wait. Jakub
On Fri, Feb 26, 2016 at 02:45:38PM -0700, Jeff Law wrote: > This one leaves the type incomplete, right? So ISTM it's somewhat more > likely than the second to expose other errors later with code that doesn't > expect the type to be incomplete (much like other code doesn't expect to > find error_mark_node in here). > > The second patch at least puts a real type in there. I suspect that's less > likely to cause problems downstream, except perhaps with diagnostics. > > I could argue for either. I almost asked for the latter to be tested, but > the more I think about it, I don't like slamming in another type like that. > > I'll conditionally approve -- if nobody objects in 72hrs, consider the first > patch OK for the trunk. I'm leaning towards the first patch, i.e. the one without setting the type to char_type_node. If it causes some issues (I hope not), we'll probably have to add some COMPLETE_TYPE_P checks. Marek
--- gcc/c/c-parser.c.jj 2016-02-24 09:33:37.000000000 +0100 +++ gcc/c/c-parser.c 2016-02-26 13:31:00.947133760 +0100 @@ -1436,10 +1436,7 @@ c_parser_translation_unit (c_parser *par tree decl; FOR_EACH_VEC_ELT (incomplete_record_decls, i, decl) if (DECL_SIZE (decl) == NULL_TREE && TREE_TYPE (decl) != error_mark_node) - { - error ("storage size of %q+D isn%'t known", decl); - TREE_TYPE (decl) = error_mark_node; - } + error ("storage size of %q+D isn%'t known", decl); } /* Parse an external declaration (C90 6.7, C99 6.9). --- gcc/testsuite/gcc.dg/pr69796.c.jj 2016-02-26 13:16:32.352323768 +0100 +++ gcc/testsuite/gcc.dg/pr69796.c 2016-02-26 13:16:16.000000000 +0100 @@ -0,0 +1,10 @@ +/* PR c/69796 */ +/* { dg-do compile } */ + +struct S s; /* { dg-error "storage size of 's' isn't known" } */ + +void +foo () +{ + s a; /* { dg-error "expression statement has incomplete type|expected" } */ +} --- gcc/testsuite/gcc.dg/pr69974.c.jj 2016-02-26 13:13:40.239740046 +0100 +++ gcc/testsuite/gcc.dg/pr69974.c 2016-02-26 13:13:29.000000000 +0100 @@ -0,0 +1,13 @@ +/* PR c/69974 */ +/* { dg-do compile } */ + +struct S; +char foo (struct S *); +struct S a; /* { dg-error "storage size of 'a' isn't known" } */ +int b; + +void +bar () +{ + b &= foo (&a); +}
Hi! Already PR69483 and these two further PRs show that it is really a bad idea to set TREE_TYPE of decls with incomplete types to error_mark_node, there are lots of places in the middle-end that don't expect error_mark_nodes appearing so late. I've bootstrapped/regtested on x86_64-linux and i686-linux following patch which just doesn't do anything with the type, we don't emit the error multiple times, because if the FE emits errors, cgraphunit doesn't attempt to assemble variables or compile functions (just gimplifies them) if seen_error (). Attached is then untested alternative, set the type to something that should hopefully not cause ICEs nor further errors/warnings during the error-recovery. Ok for trunk, or shall I test the other patch instead? 2016-02-26 Jakub Jelinek <jakub@redhat.com> PR c/69796 PR c/69974 * c-parser.c (c_parser_translation_unit): Don't change TREE_TYPE of incomplete decls to error_mark_node. * gcc.dg/pr69796.c: New test. * gcc.dg/pr69974.c: New test. Jakub 2016-02-26 Jakub Jelinek <jakub@redhat.com> PR c/69796 PR c/69974 * c-parser.c (c_parser_translation_unit): For incomplete decls set type to char_type_node instead of error_mark_node. * gcc.dg/pr69796.c: New test. * gcc.dg/pr69974.c: New test. --- gcc/c/c-parser.c.jj 2016-02-24 09:33:37.000000000 +0100 +++ gcc/c/c-parser.c 2016-02-26 13:04:16.416657335 +0100 @@ -1438,7 +1438,10 @@ c_parser_translation_unit (c_parser *par if (DECL_SIZE (decl) == NULL_TREE && TREE_TYPE (decl) != error_mark_node) { error ("storage size of %q+D isn%'t known", decl); - TREE_TYPE (decl) = error_mark_node; + /* Don't set the type to error_mark_node, the middle-end is unprepared + to see error_mark_node type appearing so late in the IL, + so using some other type is better for error recovery. */ + TREE_TYPE (decl) = char_type_node; } } --- gcc/testsuite/gcc.dg/pr69796.c.jj 2016-02-26 13:16:32.352323768 +0100 +++ gcc/testsuite/gcc.dg/pr69796.c 2016-02-26 13:16:16.000000000 +0100 @@ -0,0 +1,10 @@ +/* PR c/69796 */ +/* { dg-do compile } */ + +struct S s; /* { dg-error "storage size of 's' isn't known" } */ + +void +foo () +{ + s a; /* { dg-error "expression statement has incomplete type|expected" } */ +} --- gcc/testsuite/gcc.dg/pr69974.c.jj 2016-02-26 13:13:40.239740046 +0100 +++ gcc/testsuite/gcc.dg/pr69974.c 2016-02-26 13:13:29.000000000 +0100 @@ -0,0 +1,13 @@ +/* PR c/69974 */ +/* { dg-do compile } */ + +struct S; +char foo (struct S *); +struct S a; /* { dg-error "storage size of 'a' isn't known" } */ +int b; + +void +bar () +{ + b &= foo (&a); +}