[Sema] Remove location from implicit capture init expr

[vsk]

Summary

A lambda's closure is initialized when the lambda is declared. For
implicit captures, the initialization code emitted from EmitLambdaExpr
references source locations *within the lambda body* in the function
containing the lambda. This results in a poor debugging experience: we
step to the line containing the lambda, then into lambda, out again,
over and over, until every capture's field is initialized.

To improve stepping behavior, assign an empty location to expressions
which initialize an implicit capture within a closure. This prevents the
debugger from stepping into a lambda when single-stepping in its parent
function.

rdar://39807527

[vsk]

• Here is a GIF that might help illustrate the bug: http://net.vedantk.com/static/llvm/lambda-implicit-capture-bug.gif
• Update test/SemaCXX/uninitialized.cpp to highlight the behavior change which comes from using getBeginOrDeclLoc().

[vsk]

Ping.

[rsmith]

I would prefer that we use the location of the capture-default as the location of all the implicitly captured fields, rather than using an invalid location.

clang/lib/Sema/SemaDecl.cpp

10689 ↗

(On Diff #161536)

I'm a bit surprised you updated this call to DeclRefExpr::getBeginLoc() (and no others). I think this should be unreachable for a DeclRefExpr that refers to an implicit lambda capture, because such a lambda-capture cannot refer to itself from its own (implicit) initializer.

[vsk]

• Partially address some of @rsmith's feedback. Instead of using the capture default location, I used the beginning location of the capture list. This results in smoother single-stepping when those two locations are on different lines.

clang/lib/Sema/SemaDecl.cpp

10689 ↗

(On Diff #161536)

IIUC, that's exactly what this visitor (SelfReferenceChecker) diagnoses. The only test I found which exercises this code path is SemaCXX/uninitialized.cpp. The relevant portion is updated in this diff, but to summarize, there's a DeclRefExpr in the CXXConstructExpr initializer generated for a1 which self-refers to a1.

[rsmith]

clang/lib/Sema/SemaLambda.cpp

1422–1424 ↗

(On Diff #162764)

Should these locations also be updated to InitLoc? If we're modeling the initialization as happening at the capture-default, we should do that consistently.

1612 ↗

(On Diff #162764)

Use CaptureDefaultLoc here instead of the start of the introducer range.

clang/unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp

78–84 ↗

(On Diff #162764)

Please describe what the status quo is and why rather than explaining what it used to be and why it's different. The history stops being interesting very soon after you commit the new behavior.

[vsk]

Address the latest round of review feedback.

clang/lib/Sema/SemaLambda.cpp

1422–1424 ↗

(On Diff #162764)

I've revised the patch to use one location consistently here. The tradeoff is that a few diagnostics now point to CaptureDefaultLoc instead of within the lambda body, but I'm happy to defer to more experienced Sema hands.

1612 ↗

(On Diff #162764)

Sure. This does seem better for diagnostic reporting purposes. I'll just note that it may make the line table look awkward in the (admittedly unlikely) event that 'IntroducerRange.getBegin()' and 'CaptureDefaultLoc' are on different lines.

[rsmith]

clang/lib/Sema/SemaLambda.cpp

1422–1424 ↗

(On Diff #162764)

Yes, the changed diagnostics are confusing, but the old diagnostics were also not great. I think the right thing to do is to keep the "used here" diagnostic in its current location, and to add a separate note "implicitly capturing local variable 'x' first used here" (or similar) at the point of capture.

The way to do that is to push a CodeSynthesisContext for the duration where you're synthesizing code to initialize the lambda capture. (That'll insert a note at the right place in the note stack.) You'll need to add a new value to the CodeSynthesisContext enumeration and update Sema::PrintInstantiationStack to produce the note.

I'm fine with this being done in a follow-up commit (and I'm approving this on that basis), but I don't think that the new diagnostic is really good enough for the long term (while the old one was bad, the new one no longer gives any clue as to why we're calling a copy constructor or for what).

lib/Sema/SemaLambda.cpp

CXXConversionDecl *Conversion = CXXConversionDecl::Create(
       S.Context.getTrivialTypeSourceInfo(ConvTy, Loc),
       /*isInline=*/true, /*isExplicit=*/false,
       /*isConstexpr=*/false, CallOperator->getBody()->getEndLoc());
   Conversion->setAccess(AS_public);
   Conversion->setImplicit(true);
   Class->addDecl(Conversion);
 }
 
-static ExprResult performLambdaVarCaptureInitialization(Sema &S,
-                                                        const Capture &Capture,
-                                                        FieldDecl *Field) {
+static ExprResult performLambdaVarCaptureInitialization(
+    Sema &S, const Capture &Capture, FieldDecl *Field,
+    SourceLocation ImplicitCaptureLoc, bool IsImplicitCapture) {
   assert(Capture.isVariableCapture() && "not a variable capture");
 
   auto *Var = Capture.getVariable();
-  SourceLocation Loc = Capture.getLocation();
+  SourceLocation Loc =
+      IsImplicitCapture ? ImplicitCaptureLoc : Capture.getLocation();
 
   // C++11 [expr.prim.lambda]p21:
   //   When the lambda-expression is evaluated, the entities that
   //   are captured by copy are used to direct-initialize each
   //   corresponding non-static data member of the resulting closure
   //   object. (For array members, the array elements are
   //   direct-initialized in increasing subscript order.) These
   //   initializations are performed in the (unspecified) order in
for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
       }
 
       VarDecl *Var = From.getVariable();
       LambdaCaptureKind Kind = From.isCopyCapture() ? LCK_ByCopy : LCK_ByRef;
       Captures.push_back(LambdaCapture(From.getLocation(), IsImplicit, Kind,
                                        Var, From.getEllipsisLoc()));
       Expr *Init = From.getInitExpr();
       if (!Init) {
-        auto InitResult =
-            performLambdaVarCaptureInitialization(*this, From, *CurField);
+        auto InitResult = performLambdaVarCaptureInitialization(
+            *this, From, *CurField, CaptureDefaultLoc, IsImplicit);
         if (InitResult.isInvalid())
           return ExprError();
         Init = InitResult.get();
       }
       CaptureInits.push_back(Init);
     }
 
     // C++11 [expr.prim.lambda]p6:

test/CXX/expr/expr.prim/expr.prim.lambda/p14.cpp

 
 class NonConstCopy {
 public:
   NonConstCopy(NonConstCopy&); // expected-note{{would lose const}}
 };
 
 void capture_by_copy(NonCopyable nc, NonCopyable &ncr, const NonConstCopy nco) {
   (void)[nc] { }; // expected-error{{capture of variable 'nc' as type 'NonCopyable' calls private copy constructor}}
-  (void)[=] {
-    ncr.foo(); // expected-error{{capture of variable 'ncr' as type 'NonCopyable' calls private copy constructor}} 
+  (void)[=] { // expected-error{{capture of variable 'ncr' as type 'NonCopyable' calls private copy constructor}}
+    ncr.foo();
   }();
 
   [nco] {}(); // expected-error{{no matching constructor for initialization of 'const NonConstCopy'}}
 }
 
 struct NonTrivial {
   NonTrivial();
   NonTrivial(const NonTrivial &);

test/CXX/expr/expr.prim/expr.prim.lambda/templates.cpp

const std::type_info &ti
         }()); 
   }
 
   template void odr_used(int&, Boom<int>); // expected-note{{in instantiation of function template specialization}}
 
   template<typename R, typename T>
   void odr_used2(R &r, Boom<T> boom) {
     const std::type_info &ti
-      = typeid([=,&r] () -> R& {
-          boom.tickle(); // expected-note{{in instantiation of member function}}
+      = typeid([=,&r] () -> R& { // expected-note{{in instantiation of member function 'p2::Boom<float>::Boom' requested here}}
+          boom.tickle();
           return r; 
         }()); 
   }
 
   template void odr_used2(P&, Boom<float>);
 }
 
 namespace p5 {

test/CodeGenCXX/debug-info-lambda.cpp

+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm \
+// RUN:   -debug-info-kind=line-tables-only -dwarf-column-info -std=c++11 %s -o - | FileCheck %s
+
+// CHECK-LABEL: define{{.*}}lambda_in_func
+void lambda_in_func(int &ref) {
+  // CHECK: [[ref_slot:%.*]] = getelementptr inbounds %class.anon, %class.anon* {{.*}}, i32 0, i32 0, !dbg [[lambda_decl_loc:![0-9]+]]
+  // CHECK-NEXT: %1 = load i32*, i32** %ref.addr, align 8, !dbg [[capture_init_loc:![0-9]+]]
+  // CHECK-NEXT: store i32* %1, i32** %0, align 8, !dbg [[lambda_decl_loc]]
+  // CHECK-NEXT: call void {{.*}}, !dbg [[lambda_call_loc:![0-9]+]]
+
+  auto helper = [       // CHECK: [[lambda_decl_loc]] = !DILocation(line: [[@LINE]], column: 17
+                 &]() { // CHECK: [[capture_init_loc]] = !DILocation(line: [[@LINE]], column: 18
+    ++ref;
+  };
+  helper();             // CHECK: [[lambda_call_loc]] = !DILocation(line: [[@LINE]]
+}

test/SemaCXX/uninitialized.cpp

 }
 
 namespace lambdas {
   struct A {
     template<typename T> A(T) {}
     int x;
   };
   A a0([] { return a0.x; }); // ok
-  void f() { 
-    A a1([=] { return a1.x; }); // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+  void f() {
+    A a1([=] { // expected-warning{{variable 'a1' is uninitialized when used within its own initialization}}
+      return a1.x;
+    });
     A a2([&] { return a2.x; }); // ok
   }
 }
 
 namespace record_fields {
   bool x;
   struct A {
     A() {}

unittests/Tooling/RecursiveASTVisitorTests/DeclRefExpr.cpp

TEST(RecursiveASTVisitor, VisitsUseOfImplicitLambdaCapture) {
   EXPECT_TRUE(Visitor.runOver(
     "void f() { int i; [=]{ i; }; }",
     DeclRefExprVisitor::Lang_CXX11));
 }
 
 TEST(RecursiveASTVisitor, VisitsImplicitLambdaCaptureInit) {
   DeclRefExprVisitor Visitor;
   Visitor.setShouldVisitImplicitCode(true);
-  // We're expecting the "i" in the lambda to be visited twice:
-  // - Once for the DeclRefExpr in the lambda capture initialization (whose
-  //   source code location is set to the first use of the variable).
-  // - Once for the DeclRefExpr for the use of "i" inside the lambda.
-  Visitor.ExpectMatch("i", 1, 24, /*Times=*/2);
+  // We're expecting "i" to be visited twice: once for the initialization expr
+  // for the captured variable "i" outside of the lambda body, and again for
+  // the use of "i" inside the lambda.
+  Visitor.ExpectMatch("i", 1, 20, /*Times=*/1);
+  Visitor.ExpectMatch("i", 1, 24, /*Times=*/1);
   EXPECT_TRUE(Visitor.runOver(
     "void f() { int i; [=]{ i; }; }",
     DeclRefExprVisitor::Lang_CXX11));
 }
 
 TEST(RecursiveASTVisitor, VisitsLambdaInitCaptureInit) {
   DeclRefExprVisitor Visitor;
   Visitor.ExpectMatch("i", 1, 24);