Page MenuHomePhabricator

Inject only relevant local variables in the expression evaluation context
ClosedPublic

Authored by teemperor on May 7 2018, 2:56 PM.

Details

Summary

In r259902, LLDB started injecting all the locals in every expression
evaluation. This fixed a bunch of issues, but also caused others, mostly
performance regressions on some codebases. The regressions were bad
enough that we added a setting in r274783 to control the behavior and
we have been shipping with the setting off to avoid the perf regressions.

This patch changes the logic injecting the local variables to only inject
the ones present in the expression typed by the user. The approach is
fairly simple and just scans the typed expression for every local name.
Hopefully this gives us the best of both world as it just realizes the
types of the variables really used by the expression.

Landing this requires the 2 other issues I pointed out today to be addressed
but I wanted to gather comments right away.

Original patch by Frédéric Riss!

Diff Detail

Repository
rLLDB LLDB

Event Timeline

friss created this revision.May 7 2018, 2:56 PM
clayborg added inline comments.May 7 2018, 3:32 PM
source/Expression/ExpressionSourceCode.cpp
174–179 ↗(On Diff #145553)

Might be clearer as:

const int prev = from-1;
if (prev >= 0 && clang::isIdentifierBody(body[prev]))
  continue;
const int next = from + var.size()
if (next == body.size() || clang::isIdentifierBody(body[next]))
  continue;
clayborg accepted this revision.May 7 2018, 3:34 PM

I forgot to increment from before continuing in my example. No need to update code if you don't feel it is clearer

This revision is now accepted and ready to land.May 7 2018, 3:34 PM
jingham accepted this revision.May 7 2018, 3:37 PM

I'm still a little sad we can't get this to happen correctly in clang's lookup, but this is a clever way to get the benefit of this workaround without paying all the cost, and is a fine temporary solution.

The implementation looks okay to me either way.

labath added a subscriber: labath.May 8 2018, 1:32 AM

One could probably concoct an example (using macros, token pasting and such) where this would get it wrong, but that's probably not worth supporting.

source/Expression/ExpressionSourceCode.cpp
193 ↗(On Diff #145553)

s/AsCString/GetStringRef (saves a strlen operation).

teemperor commandeered this revision.Apr 27 2019, 5:29 AM
teemperor added a reviewer: friss.
teemperor added a subscriber: teemperor.

Shafik asked me to take a look at this and see if I can fix the completion tests.

teemperor updated this revision to Diff 196965.Apr 27 2019, 6:29 AM
  • Disable the variable filtering when during tab completion for now, as it seems our Clang lookup doesn't pull in external types which breaks the completion tests (and others too). This is only a temporary fix to unblock D59960.
  • Some minor cleanup and documentation.
teemperor edited the summary of this revision. (Show Details)
teemperor set the repository for this revision to rLLDB LLDB.
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2019, 6:31 AM
This revision was automatically updated to reflect the committed changes.