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

[Richard Smith]

On Wed, Jan 8, 2014 at 10:36 PM, Serge Pavlov <sepavloff at gmail.com> wrote:

>
> 2014/1/9 Richard Smith <metafoo at gmail.com>
>
>> On Thu Dec 12 2013 at 9:20:37 PM, Serge Pavlov <sepavloff at gmail.com>
>> wrote:
>>
>>> 2013/12/13 Richard Smith <richard at metafoo.co.uk>
>>>
>>>
>>>   Have you checked that IR generation does the right thing in these
>>> cases? A CodeGen test for this would be useful.
>>>
>>> Attached is a test that can be compiled and run to check particular
>>> binding in clang and gcc.
>>>
>>>
>>>
>>> ================
>>> Comment at: test/Parser/bad-control.c:22-30
>>> @@ +21,11 @@
>>> +
>>> +// GCC rejects this code
>>> +void pr8880_3(int first) {
>>> +  for ( ; ; (void)({ if (first) { first = 0; continue; } 0; })) {}
>>> +}
>>> +
>>> +// GCC rejects this code (see also tests/Analysis/dead-stores.c
>>> rdar8014335()
>>> +void pr8880_4(int first) {
>>> +  for ( ; ; (void)({ if (first) { first = 0; break; } 0; })) {}
>>> +}
>>> +
>>> ----------------
>>> Serge Pavlov wrote:
>>> > Richard Smith wrote:
>>> > > Does this mean we're misinterpreting pr8880_10 and pr8880_11 below?
>>> > No, this is a feature of GCC. It uses different interpretation
>>> depending on whether code is compiled as C or as C++. In C++ mode break and
>>> continue in the second or third expression of for statement refer to the
>>> inner loop, in C mode - to the outer. However in both modes GCC reject
>>> using break/continue if for statement is not inside another loop. Clang is
>>> more consistent, it considers that the third expression refers to the inner
>>> loop and the second - to outer, the interpretation is the same in C and C++
>>> mode.
>>> Interesting. I agree that it makes little sense for C and C++ to differ
>>> here, other than GCC compatibility.
>>>
>>> Why does break/continue in the second expression bind to the inner scope
>>> and the third expression bind to the outer? It seems strange that we would
>>> (deliberately) differ from both GCC's C mode and its C++ mode here.
>>>
>>> Doug Gregor filed a bug against GCC (http://gcc.gnu.org/bugzilla/
>>> show_bug.cgi?id=44715), but after 3 years GCC still keeps inconsistent
>>> behavior. Maybe he has more information on this subject.
>>>
>>
>> I don't think you've answered my question.
>>
>> GCC says that control in the second and third expression bind to the
>> outer loop in C and to the inner loop in C++. You've picked another option
>> that's not even self-consistent (binding to the outer loop for the second
>> expression and to the inner loop for the third). Why are you doing that?
>>
>
> There is nothing in GCC documentation that says which expression binds to
> which context. This correspondence was revealed experimentally. And there
> is PR44715 in GCC bug database that states GCC behavior is buggy.
> The dominating opinion in GCC community was that such constructs should be
> prohibited, but QT already uses 'break' in the 3rd expression of 'for' loop.
> This patch does not chose particular binding to inner or outer scope, it
> only adds message generation if the corresponding scope is not a loop or
> switch. The binding is already implemented in clang. There must be reasons
> why the implementation is such, maybe Doug or Ted know them. Changing
> current implementation requires special care.
>
>
>> I think what we should do here is to always bind to the inner loop in the
>> second and third expression, and we should additionally produce a
>> -Wgcc-compat diagnostic if:
>> 1) we're in C, and
>> 2) we've got control flow in the second or third expression, and
>> 3) there is an outer loop
>> (that is, in the case where both we and GCC would accept, and where we
>> would do something different from what GCC does).
>>
>
> As a variant, we could emit an error in all cases but break in the 3rd
> expression, just to allow QT code compile. These constructs can create
> problems for OpenMP or loop transformations if someone starts really using
> them.
>

I'd be OK with that (or perhaps with allowing both 'break' and 'continue'
in the third expression) -- disallowing the cases where we currently have
different behavior from GCC seems unlikely to break much existing code. I'd
still want the warning if this happens in C code, though.


> Since GCC rejects this if the loop is non-nested, it seems more reasonable
>>> to me for them to always bind to the surrounding scope. Is there a reason
>>> to not do that?
>>>
>>>
>>> In C++ mode break/continue bind to inner scope. Surprisingly this
>>> construct is used in QT macro 'foreach'. The following code:
>>>
>>>   foreach(QString name, names)
>>>     qDebug() << name;
>>>
>>> after preprocessing produces:
>>>
>>> for (QForeachContainer<__typeof__(names)> _container_(names);
>>>      !_container_.brk && _container_.i != _container_.e;
>>>      __extension__ ({ ++_container_.brk; ++_container_.i; }))
>>> for (QString name = *_container_.i;; __extension__ ({--_container_.brk;
>>> break;}))
>>>     qDebug() << name;
>>>
>>> So clang must bind break in the third expression to inner loop.
>>>
>>> There is also a test in Analysis/dead-stores.c :
>>>
>>> // The FOREACH macro in QT uses 'break' statements within statement
>>> expressions
>>> // placed within the increment code of for loops.
>>> void rdar8014335() {
>>>   for (int i = 0 ; i != 10 ; ({ break; })) {
>>>     for ( ; ; ({ ++i; break; })) ;
>>>     // Note that the next value stored to 'i' is never executed
>>>     // because the next statement to be executed is the 'break'
>>>     // in the increment code of the first loop.
>>>     i = i * 3; // expected-warning{{Value stored to 'i' is never read}}
>>> expected-warning{{The left operand to '*' is always 1}}
>>>   }
>>> }
>>>
>>> This code cannot be compiled by GCC (I tried 4.6.3 and ToT). The test
>>> was added by Ted Kremenek in r104375. Maybe he can recall if it was
>>> obtained from real application. If so, this is the second usage case clang
>>> should support.
>>>
>>>
>>>
>>>
>>> http://llvm-reviews.chandlerc.com/D2018
>>>
>>>
>>>
>>>
>>> --
>>> Thanks,
>>> --Serge
>>>
>>
>
>
> --
> Thanks,
> --Serge
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/6353b1f4/attachment.html>