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

[Serge Pavlov]

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.


>
> 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/ec57062f/attachment.html>