| Message ID | 4CDEBE2F.8080406@gnu.org |
|---|---|
| State | New |
| Headers | show |
On Sat, 13 Nov 2010, Paolo Bonzini wrote: > On 11/13/2010 03:15 PM, H.J. Lu wrote: > > On Sat, Oct 30, 2010 at 6:24 AM, Paolo Bonzini<bonzini@gnu.org> wrote: > > > This patch improves GCC error detection so that some cases of > > > declarations with unknown type names are detected. This also > > > allows GCC to do better on cascading errors, because the variables > > > that are declared enter the symbol table. > > > > This caused: > > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46462 > > Here is the fix. > > Bootstrapped (together with the semicolon patch), regtest in progress and > fixes all remaining failures (except the one caused by PR45062, ok for > mainline assuming it passes? OK in the absence of any objections from ObjC maintainers within 24 hours.
On 13 Nov 2010, at 17:26, Joseph S. Myers wrote: > On Sat, 13 Nov 2010, Paolo Bonzini wrote: > >> On 11/13/2010 03:15 PM, H.J. Lu wrote: >>> On Sat, Oct 30, 2010 at 6:24 AM, Paolo Bonzini<bonzini@gnu.org> >>> wrote: >>>> This patch improves GCC error detection so that some cases of >>>> declarations with unknown type names are detected. This also >>>> allows GCC to do better on cascading errors, because the variables >>>> that are declared enter the symbol table. >>> >>> This caused: >>> >>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46462 >> >> Here is the fix. >> >> Bootstrapped (together with the semicolon patch), regtest in >> progress and >> fixes all remaining failures (except the one caused by PR45062, ok >> for >> mainline assuming it passes? > > OK in the absence of any objections from ObjC maintainers within 24 > hours. works for me (GNU & NeXT runtimes on i686-darwin9) ... .. although I can neither object nor approve ;) cheers Iain
>> Bootstrapped (together with the semicolon patch), regtest in progress and >> fixes all remaining failures (except the one caused by PR45062, ok for >> mainline assuming it passes? > > OK in the absence of any objections from ObjC maintainers within 24 hours. I'm not an ObjC maintainer, but I wrote the ObjC fast enumeration parsing code that is being patched and I had a look at the patch - it looks Ok to me. :-) I have one suggestion though -- > @@ -4653,14 +4658,15 @@ c_parser_for_statement (c_parser *parser > + parser->objc_could_be_foreach_context = c_dialect_objc (); I would change that to always set objc_could_be_foreach_context to 'true'; in C, it has not effect, so it does no matter what you set it to. But if you always set it to 'true', then the nice assert that you added a few lines later to check that objc_could_be_foreach_context is always set back to 'false' -- > + gcc_assert(!parser->objc_could_be_foreach_context); gets always exercised when you run the C testsuite, which means that we are confident that objc_could_be_foreach_context is always restored to 'false' after parsing a C "for" loop (which is also a valid ObjC "for" loop). :-) Thanks
On 11/13/2010 07:38 PM, Nicola Pero wrote: > I would change that to always set objc_could_be_foreach_context to 'true'; in C, > it has not effect, so it does no matter what you set it to. But if you always set it to > 'true', then the nice assert that you added a few lines later to check that > objc_could_be_foreach_context is always set back to 'false' -- > >> + gcc_assert(!parser->objc_could_be_foreach_context); > > gets always exercised when you run the C testsuite, which means that we are confident > that objc_could_be_foreach_context is always restored to 'false' after parsing a C "for" > loop (which is also a valid ObjC "for" loop). :-) You are right. However, there would be a subtle change, in that the shiny new unknown type name error would be disabled in C 'for' loop initial declarations, same as it is disabled for Objective C: $ cat f.c int f() { for (intt i = 1; i < 10; i++) ; } $ ./cc1 f.c -std=c99 -quiet f.c: In function ‘f’: f.c:3:7: error: unknown type name ‘intt’ $ ./cc1obj f.c -std=c99 -quiet f.c:3:7: error: ‘intt’ undeclared (first use in this function) f.c:3:12: error: expected ‘;’ before ‘i’ f.c:3:19: error: ‘i’ undeclared (first use in this function) This can be fixed by adding a c_dialect_objc check to c_parser_next_tokens_start_declaration (after which you can do what you suggest). You can even get the new error to work for ObjC if you check for RID_IN. However, I'll leave it for a separate patch, possibly written by somebody with more interest in Objective-C. :) Paolo
> You are right. However, there would be a subtle change, in that the > shiny new unknown type name error would be disabled in C 'for' loop > initial declarations, same as it is disabled for Objective C: Thanks - you are right - I hadn't noticed that. The C and Objective-C compilers should really behave identically when parsing non-Objective-C things (such as a standard 'for' loop). If your patch intentionally breaks that, it's a problem. ;-) > This can be fixed by adding a c_dialect_objc check to > c_parser_next_tokens_start_declaration (after which you can do what you > suggest). You can even get the new error to work for ObjC if you check > for RID_IN. However, I'll leave it for a separate patch, possibly > written by somebody with more interest in Objective-C. :) I think you should add the missing checks that you describe to restore having the same behaviour in C and Objective-C; it probably would have taken you less time to fix your patch than to write the changes in an email to get me to fix it for you ;-) But you're actually worrying me as I'm now fearful that many more differences between C and ObjC may have been introduced. Maybe we should run the C testsuite with the compiler in ObjC mode ? Thanks
On 11/13/2010 08:43 PM, Nicola Pero wrote: > >> You are right. However, there would be a subtle change, in that the >> shiny new unknown type name error would be disabled in C 'for' loop >> initial declarations, same as it is disabled for Objective C: > > Thanks - you are right - I hadn't noticed that. > > The C and Objective-C compilers should really behave identically when > parsing non-Objective-C things (such as a standard 'for' loop). If > your patch intentionally breaks that, it's a problem. ;-) I accepted to deviate a bit from this good rule for "for" loops, since Objective-C is not a superset of C there: int f() { for (int in = 0; ; ) ; } gives f.c:3:11: error: expected identifier or ‘(’ before ‘in’ in Objective-C. I think foreach syntax is a very bad idea, though not your fault of course. "for (x : a)" would have been much better. > I think you should add the missing checks that you describe to restore > having the same behaviour in C and Objective-C; it probably would have > taken you less time to fix your patch than to write the changes in an > email to get me to fix it for you ;-) I can do that. However, not that it's only a matter of which errors you get. My patch does not affect which valid C code is invalid Objective-C. (Part of the idea of the mail was also to document all this for teh Google, otherwise I might as well have written it in Italian ;). > But you're actually worrying me as I'm now fearful that many more differences > between C and ObjC may have been introduced. Not by me (this one was intentional, and I didn't touch Objective-C very much), but... > Maybe we should run the C > testsuite with the compiler in ObjC mode ? ... this is a good idea. Paolo
On 13 Nov 2010, at 20:13, Paolo Bonzini wrote: > On 11/13/2010 08:43 PM, Nicola Pero wrote: >> >>> You are right. However, there would be a subtle change, in that the >>> shiny new unknown type name error would be disabled in C 'for' loop >>> initial declarations, same as it is disabled for Objective C: >> >> Thanks - you are right - I hadn't noticed that. >> >> The C and Objective-C compilers should really behave identically when >> parsing non-Objective-C things (such as a standard 'for' loop). If >> your patch intentionally breaks that, it's a problem. ;-) > > I accepted to deviate a bit from this good rule for "for" loops, > since Objective-C is not a superset of C there: > > int f() > { > for (int in = 0; ; ) > ; > } > > gives > > f.c:3:11: error: expected identifier or ‘(’ before ‘in’ > > in Objective-C. I think foreach syntax is a very bad idea, though > not your fault of course. "for (x : a)" would have been much better. This probably reflects that we've still some bugs to shake out of the (new) implementation. on Darwin, both "gcc-4.2 -x objective-c" and "clang -ObjC " warn (a) about the c99 requirement and (b) that the var "in" is unused. (which seems like the correct behavior) >> Maybe we should run the C >> testsuite with the compiler in ObjC mode ? > > ... this is a good idea. This is kinda on my TODO - with the idea to make it an optional add-on to the ObjC test-suite... .. one would not want to adopt the additional burden of running the entire 'c' testsuite twice every test-cycle. cheers Iain
>> I accepted to deviate a bit from this good rule for "for" loops, >> since Objective-C is not a superset of C there: >> >> int f() >> { >> for (int in = 0; ; ) >> ; >> } >> >> gives >> >> f.c:3:11: error: expected identifier or ‘(’ before ‘in’ >> >> in Objective-C. I think foreach syntax is a very bad idea, though >> not your fault of course. "for (x : a)" would have been much better. > > This probably reflects that we've still some bugs to shake out of the > (new) implementation. > on Darwin, both "gcc-4.2 -x objective-c" and "clang -ObjC " > > warn (a) about the c99 requirement and (b) that the var "in" is unused. > (which seems like the correct behavior) You may want to try a more interesting case -- int in = 0; for (in = in + 1; in < 10; in++) printf ("%d\n", in); try it in C and Objective-C. Do you get the same result ? In our implementation, I took the approach of simply not allowing 'in' as a variable in a 'for' loop in Objective-C. That may be simplistic and we may want to be more sophisticated; but if we follow a more complicated logic, we need to document it clearly. It is ugly to have some hidden heuristics inside the compiler arbitrarily decide what 'in' means, as users have no way of knowing when 'in' can be used as a variable and when it can not. ;-) Thanks
On 13 Nov 2010, at 21:03, Nicola Pero wrote: > >>> I accepted to deviate a bit from this good rule for "for" loops, >>> since Objective-C is not a superset of C there: >>> >>> int f() >>> { >>> for (int in = 0; ; ) >>> ; >>> } >>> >>> gives >>> >>> f.c:3:11: error: expected identifier or ‘(’ before ‘in’ >>> >>> in Objective-C. I think foreach syntax is a very bad idea, though >>> not your fault of course. "for (x : a)" would have been much >>> better. >> >> This probably reflects that we've still some bugs to shake out of the >> (new) implementation. >> on Darwin, both "gcc-4.2 -x objective-c" and "clang -ObjC " >> >> warn (a) about the c99 requirement and (b) that the var "in" is >> unused. >> (which seems like the correct behavior) > > You may want to try a more interesting case -- > > int in = 0; > > for (in = in + 1; in < 10; in++) > printf ("%d\n", in); > > try it in C and Objective-C. Do you get the same result ? OK Darwin, (OSX 10.6.5 Latest XCode) clang gets it right (same answer with/without -ObjC) gcc-4.2(.1) gets it right for 'c' and bails out 'confused by earlier errors' for -x objective-c > In our implementation, I took the approach of simply not allowing > 'in' as a variable > in a 'for' loop in Objective-C. That may be simplistic and we may > want to be more > sophisticated; but if we follow a more complicated logic, we need to > document it clearly. > It is ugly to have some hidden heuristics inside the compiler > arbitrarily decide what > 'in' means, as users have no way of knowing when 'in' can be used as > a variable and > when it can not. ;-) indeed, we should not be doing that.... ... clang is the de-facto 'standard' I guess ... Iain
On Nov 13, 2010, at 10:02 AM, IainS <developer@sandoe-acoustics.co.uk> wrote: > On 13 Nov 2010, at 17:26, Joseph S. Myers wrote: > >> On Sat, 13 Nov 2010, Paolo Bonzini wrote: >> >>> On 11/13/2010 03:15 PM, H.J. Lu wrote: >>>> On Sat, Oct 30, 2010 at 6:24 AM, Paolo Bonzini<bonzini@gnu.org> wrote: >>>>> This patch improves GCC error detection so that some cases of >>>>> declarations with unknown type names are detected. This also >>>>> allows GCC to do better on cascading errors, because the variables >>>>> that are declared enter the symbol table. >>>> >>>> This caused: >>>> >>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46462 >>> >>> Here is the fix. >>> >>> Bootstrapped (together with the semicolon patch), regtest in progress and >>> fixes all remaining failures (except the one caused by PR45062, ok for >>> mainline assuming it passes? >> >> OK in the absence of any objections from ObjC maintainers within 24 hours. > > works for me (GNU & NeXT runtimes on i686-darwin9) ... > .. although I can neither object nor approve ;) Ok.
On Nov 13, 2010, at 8:34 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
> Bootstrapped (together with the semicolon patch), regtest in progress and fixes all remaining failures (except the one caused by PR45062, ok for mainline assuming it passes?
Ok.
On Nov 13, 2010, at 1:27 PM, IainS <developer@sandoe-acoustics.co.uk> wrote: > indeed, we should not be doing that.... > ... clang is the de-facto 'standard' I guess ... Yes, but we don't really want to be bug compatible with them, if we think they got it wrong. One can always file a bug report against clang, if they want to know for sure.
Hi Mike, On 14 Nov 2010, at 05:37, Mike Stump wrote: > On Nov 13, 2010, at 1:27 PM, IainS <developer@sandoe- > acoustics.co.uk> wrote: >> indeed, we should not be doing that.... >> ... clang is the de-facto 'standard' I guess ... > > Yes, but we don't really want to be bug compatible with them, if we > think they got it wrong. One can always file a bug report against > clang, if they want to know for sure. In this instance, it seems to me that clang is doing it right ... Nicola's example: > int in = 0; > > for (in = in + 1; in < 10; in++) > printf ("%d\n", in); I would expect that to compile properly for both 'c' and 'objc' -- -- unless somewhere there is a statement that 'in' is a reserved word in that context. (in which case we could file a bug against clang because it allows it, OSX's gcc-4.2.1 fails - but _not_ with an error that says 'wrong use of keyword' or similar). what do you think is the proper behavior? cheers, Iain
>>> indeed, we should not be doing that.... >>> ... clang is the de-facto 'standard' I guess ... > >> Yes, but we don't really want to be bug compatible with them, if we >> think they got it wrong. One can always file a bug report against >> clang, if they want to know for sure. > > In this instance, it seems to me that clang is doing it right ... I agree that clang is doing it right in this instance [and our compiler needs more work ;-)]; I think Mike was not commenting on this particular case, but just giving general guidelines on how to approach differences. And I agree with his guidelines; we obviously want to be very compatible, but if they get something wrong (not in this case) we don't necessarily need to be faithfully reproducing obvious bugs. :-) Anyway, I created a PR for improving our compiler: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46473 Thanks
On 14 Nov 2010, at 11:08, Nicola Pero wrote: > >>>> indeed, we should not be doing that.... >>>> ... clang is the de-facto 'standard' I guess ... >> >>> Yes, but we don't really want to be bug compatible with them, if we >>> think they got it wrong. One can always file a bug report against >>> clang, if they want to know for sure. >> >> In this instance, it seems to me that clang is doing it right ... > > I agree that clang is doing it right in this instance [and our > compiler > needs more work ;-)]; I think Mike was not commenting on this > particular case, > but just giving general guidelines on how to approach differences. I read it that way - but also am interested in his view on this specific case, in case we missed something... > And I agree with his guidelines; we obviously want to be very > compatible, > but if they get something wrong (not in this case) we don't > necessarily need > to be faithfully reproducing obvious bugs. :-) completely agreed ... (although Darwin might end up needing a '- mcompat=apple' :/ ) Where does this leave us with the 'fix' to the current FE? cheers Iain
On Sat, Nov 13, 2010 at 8:34 AM, Paolo Bonzini <bonzini@gnu.org> wrote: > On 11/13/2010 03:15 PM, H.J. Lu wrote: >> >> On Sat, Oct 30, 2010 at 6:24 AM, Paolo Bonzini<bonzini@gnu.org> wrote: >>> >>> This patch improves GCC error detection so that some cases of >>> declarations with unknown type names are detected. This also >>> allows GCC to do better on cascading errors, because the variables >>> that are declared enter the symbol table. >> >> This caused: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46462 > > Here is the fix. > > Bootstrapped (together with the semicolon patch), regtest in progress and > fixes all remaining failures (except the one caused by PR45062, ok for > mainline assuming it passes? > This may have caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46475
On Nov 14, 2010, at 1:59 AM, IainS <developer@sandoe-acoustics.co.uk> wrote: > what do you think is the proper behavior? When you, Nicola and clang all agree, I'd not even entertain a different answer. :) >
On Nov 14, 2010, at 3:08 AM, "Nicola Pero" <nicola.pero@meta-innovation.com> wrote: > I think Mike was not commenting on this particular case, > but just giving general guidelines on how to approach differences. Yes, just to head off any idea that we blindly follow clang without question. > And I agree with his guidelines; we obviously want to be very compatible, > but if they get something wrong (not in this case) we don't necessarily need > to be faithfully reproducing obvious bugs. :-) Nicely stated. If some import apple header used a bug, I'm of course fine with implementing a bug.... >
On Nov 14, 2010, at 3:22 AM, IainS <developer@sandoe-acoustics.co.uk> wrote: >>> >> > completely agreed ... (although Darwin might end up needing a '-mcompat=apple' :/ ) Ick, no, please. >
On Sat, Nov 13, 2010 at 8:34 AM, Paolo Bonzini <bonzini@gnu.org> wrote: > On 11/13/2010 03:15 PM, H.J. Lu wrote: >> >> On Sat, Oct 30, 2010 at 6:24 AM, Paolo Bonzini<bonzini@gnu.org> wrote: >>> >>> This patch improves GCC error detection so that some cases of >>> declarations with unknown type names are detected. This also >>> allows GCC to do better on cascading errors, because the variables >>> that are declared enter the symbol table. >> >> This caused: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46462 > > Here is the fix. > > Bootstrapped (together with the semicolon patch), regtest in progress and > fixes all remaining failures (except the one caused by PR45062, ok for > mainline assuming it passes? > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47435
Index: c-decl.c =================================================================== --- c-decl.c (revision 166700) +++ c-decl.c (working copy) @@ -9324,6 +9324,11 @@ declspecs_add_type (location_t loc, stru } specs->type = type; } + else + { + /* Set a dummy type here to avoid warning about implicit 'int'. */ + specs->type = integer_type_node; + } return specs; } Index: c-parser.c =================================================================== --- c-parser.c (revision 166700) +++ c-parser.c (working copy) @@ -641,7 +641,10 @@ c_parser_next_tokens_start_declaration ( && token->id_kind == C_ID_ID && (c_parser_peek_2nd_token (parser)->type == CPP_NAME || c_parser_peek_2nd_token (parser)->type == CPP_MULT) - && !lookup_name (token->value)) + && !lookup_name (token->value) + + /* Do not try too hard when we could have "object in array". */ + && !parser->objc_could_be_foreach_context) return true; return false; @@ -1373,10 +1376,12 @@ c_parser_declaration_or_fndef (c_parser c_parser_peek_token (parser)->value); /* Parse declspecs normally to get a correct pointer type, but avoid - a further "fails to be a type name" error. */ + a further "fails to be a type name" error. Refuse nested functions + since it is not how the user likely wants us to recover. */ c_parser_peek_token (parser)->type = CPP_KEYWORD; c_parser_peek_token (parser)->keyword = RID_VOID; c_parser_peek_token (parser)->value = error_mark_node; + fndef_ok = !nested; } c_parser_declspecs (parser, specs, true, true, start_attr_ok); @@ -4653,14 +4658,15 @@ c_parser_for_statement (c_parser *parser { /* Parse the initialization declaration or expression. */ object_expression = error_mark_node; + parser->objc_could_be_foreach_context = c_dialect_objc (); if (c_parser_next_token_is (parser, CPP_SEMICOLON)) { + parser->objc_could_be_foreach_context = false; c_parser_consume_token (parser); c_finish_expr_stmt (loc, NULL_TREE); } else if (c_parser_next_tokens_start_declaration (parser)) { - parser->objc_could_be_foreach_context = true; c_parser_declaration_or_fndef (parser, true, true, true, true, true, &object_expression); parser->objc_could_be_foreach_context = false; @@ -4690,7 +4696,6 @@ c_parser_for_statement (c_parser *parser int ext; ext = disable_extension_diagnostics (); c_parser_consume_token (parser); - parser->objc_could_be_foreach_context = true; c_parser_declaration_or_fndef (parser, true, true, true, true, true, &object_expression); parser->objc_could_be_foreach_context = false; @@ -4714,7 +4719,6 @@ c_parser_for_statement (c_parser *parser init_expr: { tree init_expression; - parser->objc_could_be_foreach_context = true; init_expression = c_parser_expression (parser).value; parser->objc_could_be_foreach_context = false; if (c_parser_next_token_is_keyword (parser, RID_IN)) @@ -4735,6 +4739,7 @@ c_parser_for_statement (c_parser *parser } /* Parse the loop condition. In the case of a foreach statement, there is no loop condition. */ + gcc_assert (!parser->objc_could_be_foreach_context); if (!is_foreach_statement) { if (c_parser_next_token_is (parser, CPP_SEMICOLON))