Page MenuHomePhabricator

Added initial code completion support for the `expr` command

Authored by teemperor on Jun 21 2018, 4:44 PM.



This patch adds initial code completion support for the expr command.

We now have a completion handler in the expression CommandObject that
essentially just attempts to parse the given user expression with Clang with
an attached code completion consumer. We filter and prepare the
code completions provided by Clang and send them back to the completion

The current completion is limited to variables that are in the current scope.
This includes local variables and all types used by local variables. We however
don't do any completion of symbols that are not used in the local scope (or
in some other way already in the ASTContext).

This is partly because there is not yet any code that manually searches for additiona
information in the debug information. Another cause is that for some reason the existing
code for loading these additional symbols when requested by Clang doesn't seem to work.
This will be fixed in a future patch.

Diff Detail


Event Timeline

teemperor created this revision.Jun 21 2018, 4:44 PM

Note that there are two parent revisions. One is just refactoring because I needed to reuse some of the parsing setup code. The other patch (the AsyncPrint one) fixes a deadlock that affected this patch. The deadlock can be reproduces by starting lldb, going to a random breakpoint in a C++ program and then trying to complete a simple expression like expr some_local_var. lldb will fail to read some Objective C class information from the executable and attempts to print a warning, but this deadlocks everything as we hold the IO locks while completing arguments.

Very nice!

52 ↗(On Diff #152407)

Could you add a Doxygen comment?

333 ↗(On Diff #152407)

Even if it is less efficient, a version of this using cmd.split(' ') or a loop over cmd.ltrim(' ') would probably be much shorter and potentially easier to read. There is also llvm::StringExtras::getToken().

392 ↗(On Diff #152407)

Could you convert this to an early exit?

if (!target)
  return 0;
590 ↗(On Diff #152407)

Should this perhaps use some form of StringRef::dropWhile or similar?

696 ↗(On Diff #152407)


699 ↗(On Diff #152407)

no need for curly braces here

172 ↗(On Diff #152407)

Perhaps return an llvm::Error instead of an unsigned?

629 ↗(On Diff #152407)

You could also slice the StringRef at abs_pos, ::count('\n'), set column to slice.size-rfind('\n'). Your implementation is slightly faster, tough.

659 ↗(On Diff #152407)

We don't typically put single statements in curly braces in LLVM coding style.

labath added a subscriber: labath.Jun 22 2018, 3:19 AM

I think this would be a very nice feature for lldb. Thank you for working on this.

However, I am somewhat worried about how you're hooking the expression completer into the completion machinery. I think this should be cleaned up first.

1 ↗(On Diff #152407)

Maybe put the test under the expression_command sub-tree?

177–215 ↗(On Diff #152407)

I don't think a test like this has place in the test suite. It tests a different thing every time it's run and it will be impossible to debug if it starts to fail/be flaky on the build bots.

227 ↗(On Diff #152407)

We normally use the unittest2 functions to do assertions (self.assertEquals(num_matches, 0))

246–264 ↗(On Diff #152407)

We already have this function in We should move it so some place that we can reuse it instead of reimplementing it.

1 ↗(On Diff #152407)

The rest of this test is test is nicely self-contained. Could we avoid std::string here and make it fully hermetic?

419–423 ↗(On Diff #152407)

Will this work correctly if the expression command contains arguments (expr -i0 -- my_expression)? What about quoted strings (expr "string with spaces<TAB>)?

Have you looked at whether it would be possible to refactor the completion api to provide the absolute position (it has to exist at some point), instead of trying to reverse-engineer it here?

I think the problem here is that the completion api has this built-in assumption that you're only ever going to be completing "parsed" commands, which is why you get the input as an Args array and a two-dimensional cursor position. "expr" command takes "raw" input, which means it does not go through the usual word-splitting and getopt parsing. You can see that because CommandObjectExpression::DoExecute takes a const char *command, whereas for "parsed" commands (e.g. "frame variable") the DoExecute function takes the argument as Args &command.

I think it would make sense to start this by refactoring the completion api to behave the same way as the "DoExecute" api. For "raw" commands , the HandleCompletion function would take a raw string + absolute position in that string, and the parsed commands would get the an Args array plus the two-dimensional position. Without that, you're going to get subtle differences in how the expression is treated for completion purposes and for actual evaluation.

teemperor updated this revision to Diff 153748.Jul 2 2018, 10:03 AM
teemperor marked 13 inline comments as done.
  • Addresses the problems pointed out by Adrian and Pavel (Thanks!)
  • Now using the completion API patch to get rid of the code that rebuilds the command line string from the args.
177–215 ↗(On Diff #152407)

I don't see how a seeded PRNG is flaky, but I think that test is also not important enough that I really want to push for merging it in. I just removed it.

419–423 ↗(On Diff #152407)

I added another parent revision that gives us access to the raw command string, which removes all the cmd rebuilding code and fixes the cases you pointed out (which are now also in the test suite). I have to see how we can get rid of the string search for -- in both the completion/execute, but that's OOS.

590 ↗(On Diff #152407)

Most of the functions are for working on the start of the string, but I don't see anything like dropWhile for the end of the string. There is not even a reverse find_if or something like that :(

172 ↗(On Diff #152407)

Agreed. But I'll do that in a follow up revision because I didn't actually write the ParseInternal for this commit. It's just the renamed Parse function and added the missing documentation and completion parameters.

teemperor planned changes to this revision.Jul 2 2018, 11:11 AM

Actually, because both @labath and @jingham requested refactoring to get rid of the -- search (even though Phabricator couldn't parse Jim's comment), I'll first also refactor this one. Thanks for the feedback!

teemperor updated this revision to Diff 154885.Jul 10 2018, 2:36 PM
  • Now using the new API for extracting the raw string from the option list.
  • Test case for completing when we only have a forward decl in another file (as suggested by Fred).
teemperor set the repository for this revision to rLLDB LLDB.Aug 2 2018, 9:39 AM
teemperor updated this revision to Diff 159426.Aug 6 2018, 4:12 PM
  • Resolved merge conflicts.

Friendly ping on this (as the blockers for this patch are now all committed).

Yes, this is fine. Thanks for working on this!

  • Rebased patch.
teemperor accepted this revision.Aug 30 2018, 10:29 AM

(Just pressing the green button for Jim)

This revision is now accepted and ready to land.Aug 30 2018, 10:29 AM
This revision was automatically updated to reflect the committed changes.