[clang] Fix ternary operator in the second for loop statement

[danlark]

Summary

Fix ternary operator in for loop argument, it was by mistake not set as CanBeForRangeDecl and led to incorrect codegen. It fixes https://bugs.llvm.org/show_bug.cgi?id=50038. I don't have commit rights. Danila Kutenin. [e-mail]

[lebedev.ri]

This should have codegen, and maybe AST, tests.

clang/test/Parser/cxx2a-init-statement.cpp

18

This isn't cxx2a-specific isn't it?
Isn't this valid even in C99?

[danlark]

In D102502#2759776, @lebedev.ri wrote:

This should have codegen, and maybe AST, tests.

Done

clang/test/Parser/cxx2a-init-statement.cpp

18

clang even in C++11 versions go to this branch and produce warning that this feature is c++2a, so it fails currently for everything but the culprit was https://github.com/llvm/llvm-project/commit/8baa50013c86c34a58d8327c5d1a043898b86398

[danlark]

• Add codegen test

[danlark]

• Add codegen and AST tests

[danlark]

Done, please, take a look

[rsmith]

Thanks, nice catch!

Can we also add an assert when parsing a for statement that we actually find a range if the tentative parse said we were expecting one?

clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp

2

We don't use -O3 tests for this kind of thing, to avoid depending on the behaviour of the optimizer; instead you should test that the (unoptimized) IR generated by clang is correct.

clang/test/PCH/for-loop-init-ternary-operator-statement.cpp

6

This ast-print output looks wrong; I assume that's an unrelated bug?

[danlark]

• Add codegen and AST tests

clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp

2

Done.

clang/test/PCH/for-loop-init-ternary-operator-statement.cpp

6

Yes, I think so, in a buggy version x as a second parameter is missing so at least it tests correctly

[danlark]

• Add assert of finding a for range declaration

[danlark]

In D102502#2759900, @rsmith wrote:

Thanks, nice catch!

Can we also add an assert when parsing a for statement that we actually find a range if the tentative parse said we were expecting one?

Done.

[rsmith]

clang/lib/Parse/ParseExprCXX.cpp

2036

If parsing failed, I don't think this assertion should be expected to hold: error recovery after the parse error might not have interpreted the : as the start of the range expression.

[danlark]

• Address comment from Richard

[danlark]

Failure looks unrelated, please submit as I don't have commit rights

clang/lib/Parse/ParseExprCXX.cpp

case ConditionOrInitStatement::ForRangeDecl: {
     // This is 'for (init-stmt; for-range-decl : range-expr)'.
     // We're not actually in a for loop yet, so 'break' and 'continue' aren't
     // permitted here.
     assert(FRI && "should not parse a for range declaration here");
     SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
     DeclGroupPtrTy DG = ParseSimpleDeclaration(DeclaratorContext::ForInit,
                                                DeclEnd, attrs, false, FRI);
     FRI->LoopVar = Actions.ActOnDeclStmt(DG, DeclStart, Tok.getLocation());
+    assert((FRI->ColonLoc.isValid() || !DG) &&
+           "cannot find for range declaration");
     return Sema::ConditionResult();
   }
 
   case ConditionOrInitStatement::ConditionDecl:
   case ConditionOrInitStatement::Error:
     break;
   }
 

clang/lib/Parse/ParseTentative.cpp

if (!resolved()) {
       // FIXME: Unify the parsing codepaths for condition variables and
       // simple-declarations so that we don't need to eagerly figure out which
       // kind we have here. (Just parse init-declarators until we reach a
       // semicolon or right paren.)
       RevertingTentativeParsingAction PA(P);
       if (CanBeForRangeDecl) {
         // Skip until we hit a ')', ';', or a ':' with no matching '?'.
         // The final case is a for range declaration, the rest are not.
-        while (true) {
-          unsigned QuestionColonDepth = 0;
+        while (true) {
           P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
                       StopBeforeMatch);
           if (P.Tok.is(tok::question))
             ++QuestionColonDepth;
           else if (P.Tok.is(tok::colon)) {
             if (QuestionColonDepth)
               --QuestionColonDepth;
             else {

clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: @_Z1fv(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[RETVAL:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    [[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    [[X:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i32 0, i32* [[I]], align 4
+// CHECK-NEXT:    br label [[FOR_COND:%.*]]
+// CHECK:       for.cond:
+// CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[TMP0]], 2
+// CHECK-NEXT:    [[TMP1:%.*]] = zext i1 [[CMP]] to i64
+// CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 1, i32 0
+// CHECK-NEXT:    store i32 [[COND]], i32* [[X]], align 4
+// CHECK-NEXT:    [[TMP2:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[TMP2]], 0
+// CHECK-NEXT:    br i1 [[TOBOOL]], label [[FOR_BODY:%.*]], label [[FOR_END:%.*]]
+// CHECK:       for.body:
+// CHECK-NEXT:    [[TMP3:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:    store i32 [[TMP3]], i32* [[RETVAL]], align 4
+// CHECK-NEXT:    br label [[RETURN:%.*]]
+// CHECK:       for.inc:
+// CHECK-NEXT:    [[TMP4:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:    [[INC:%.*]] = add nsw i32 [[TMP4]], 1
+// CHECK-NEXT:    store i32 [[INC]], i32* [[I]], align 4
+// CHECK-NEXT:    br label [[FOR_COND]], !llvm.loop [[LOOP2:![0-9]+]]
+// CHECK:       for.end:
+// CHECK-NEXT:    store i32 0, i32* [[RETVAL]], align 4
+// CHECK-NEXT:    br label [[RETURN]]
+// CHECK:       return:
+// CHECK-NEXT:    [[TMP5:%.*]] = load i32, i32* [[RETVAL]], align 4
+// CHECK-NEXT:    ret i32 [[TMP5]]
+//
+int f() {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+    return x;
+  }
+  return 0;
+}
+

clang/test/PCH/for-loop-init-ternary-operator-statement.cpp

+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x ast -ast-print %t | FileCheck %s
+
+int f() {
+  // CHECK: for (int i = 0; x; i++) {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+    return x;
+  }
+  return 0;
+}
+

clang/test/Parser/cxx2a-init-statement.cpp

 
 void f() {
   int arr1[3];
   for (int n = 5; int x : arr1) {}
 
   int A<0>::*arr2[3];
   for (int n = 5; int A<true ? 0 : 1>::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
 
   // Here, we have a declaration rather than an expression.
   for (int n = 5; F (*p)(int (n)); ++n) {}
 
   // We detect whether we have a for-range-declaration before parsing so that