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

[Richard Smith]

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?

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).

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140108/99fc3d13/attachment.html>