This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Remove location from implicit capture init expr
ClosedPublic

Authored by vsk on Aug 17 2018, 2:31 PM.

Details

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

Diff Detail

Repository
rC Clang

Event Timeline

vsk created this revision.Aug 17 2018, 2:31 PM
vsk updated this revision to Diff 161536.Aug 20 2018, 12:28 PM

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 updated this revision to Diff 162764.Aug 27 2018, 3:36 PM
  • 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 added inline comments.Aug 27 2018, 4:56 PM
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 updated this revision to Diff 162918.Aug 28 2018, 11:39 AM
vsk marked 2 inline comments as done.

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 accepted this revision.Aug 28 2018, 3:16 PM
rsmith added inline comments.
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).

This revision is now accepted and ready to land.Aug 28 2018, 3:16 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.