[PATCH] Fix to PR8880 (clang dies processing a for loop).

[Richard Smith]

On Wed, Dec 11, 2013 at 9:55 AM, Serge Pavlov <sepavloff at gmail.com> wrote:

>
> 2013/12/10 Richard Smith <richard at metafoo.co.uk>
>
>> @@ -1291,6 +1289,7 @@
>>    // See comments in ParseIfStatement for why we create a scope for the
>>    // condition and a new scope for substatement in C++.
>>    //
>> +  getCurScope()->AddFlags(Scope::BreakScope | Scope::ContinueScope);
>>    ParseScope InnerScope(this, Scope::DeclScope,
>>                          C99orCXX && Tok.isNot(tok::l_brace));
>>
>> @@ -1342,6 +1341,7 @@
>>
>>    // Pop the body scope if needed.
>>    InnerScope.Exit();
>> +  getCurScope()->RemoveFlags(Scope::BreakScope | Scope::ContinueScope);
>>
>> These changes look redundant: you're adding and removing the flags to the
>> outer scope for exactly the period where the outer scope is not active.
>>
>> The outer scope defines BreakParent and ContinueParent for the inner
> scope. Setting flags to the inner scope is not possible in some cases. If
> the loop body is represented by compound statement, inner scope is made by
> the latter and break/continue flags wouldn't be set.
>

OK, I see. In C89, declarations from within the controlled statement *are*
visible in the while condition, but 'break' and 'continue' affect the outer
loop, in hideous contortions like this:

  extern void *p;
  void f() {
    while(1) {
      do
        (struct S { int n; }*)0;
      while (
        ((struct S*)p)->n && // use struct definition from within loop
        ({ break; 0; }) // break out of containing 'while' loop
      );
    }
  }

... so we do need to RemoveFlags. Yuck.

+  assert ((Flags & ControlScope) != 0 || "Must be control scope");
>>
>> No space after 'assert'.
>>
>> Fixed.
>
>>
>> +void Scope::RemoveFlags(unsigned FlagsToClear) {
>> +  assert((FlagsToClear & ~(BreakScope | ContinueScope)) == 0 ||
>> +         "Unsupported scope flags");
>> +  if (FlagsToClear & BreakScope) {
>> +    assert((Flags & ControlScope) != 0 || "Must be control scope");
>> +    assert((Flags & BreakScope) != 0 || "Already cleared");
>> +    BreakParent = 0;
>> + }
>> +  if (FlagsToClear & ContinueScope) {
>> +    assert ((Flags & ControlScope) != 0 || "Must be control scope");
>> +    assert((Flags & ContinueScope) != 0 || "Already cleared");
>> +    ContinueParent = 0;
>> +  }
>> +  Flags &= ~FlagsToClear;
>> +}
>>
>> These are incorrect: the BreakParent and ContinueParent should be
>> inherited from the parent here, rather than set to zero. However, with the
>> first change above, RemoveFlags will be unused and can just be deleted.
>>
>> You're missing test cases for 'break' and 'continue' in the condition of
>> a do-while, which would have caught the above bug.
>>
>> Sadly but you are absolutely right. Thank you for the catch. I tried
> making better test coverage.
>
>>
>> On Mon, Dec 9, 2013 at 4:06 AM, Serge Pavlov <sepavloff at gmail.com> wrote:
>>
>>> If there is no objections, I'll commit this fix.
>>>
>>
>> See http://llvm.org/docs/DeveloperPolicy.html#code-reviews bullet 5: "Do
>> not assume silent approval".
>>
> Yes, sure. Please forgive me my impatience.
>
>
> --
> Thanks,
> --Serge
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131211/9ecfb819/attachment.html>