Page MenuHomePhabricator

[lldb] Print better diagnostics for user expressions and modules
ClosedPublic

Authored by teemperor on Aug 2 2019, 4:55 AM.

Details

Summary

Currently our expression evaluators only prints very basic errors that are not very useful when writing complex expressions.

For example, in the expression below the user made a type error, but it's not clear from the diagnostic what went wrong:

(lldb) expr printf("Modulos are:", foobar%mo1, foobar%mo2, foobar%mo3)
error: invalid operands to binary expression ('int' and 'double')

This patch enables full Clang diagnostics in our expression evaluator. After this patch the diagnostics for the expression look like this:

(lldb) expr printf("Modulos are:", foobar%mo1, foobar%mo2, foobar%mo3)
error: <user expression 1>:1:54: invalid operands to binary expression ('int' and 'float')
printf("Modulos are:", foobar%mo1, foobar%mo2, foobar%mo3)
                                               ~~~~~~^~~~

To make this possible, we now emulate a user expression file within our diagnostics. This prevents that the user is exposed to
our internal wrapper code we inject.

Note that the diagnostics that refer to declarations from the debug information (e.g. 'note' diagnostics pointing to a called function)
will not be improved by this as they don't have any source locations associated with them, so caret or line printing isn't possible.
We instead just suppress these diagnostics as we already do with warnings as they would otherwise just be a context message
without any context (and the original diagnostic in the user expression should be enough to explain the issue).

Fixes rdar://24306342

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Aug 2 2019, 4:55 AM
teemperor edited reviewers, added: JDevlieghere, aprantl, shafik; removed: Restricted Project.Aug 2 2019, 4:57 AM

I have a question based on my half-knowledge about the expression evaluator: I thought that we already wrote out a temporary file for the expression in order to support expr -g? Is this orthogonal or do we now have to ways of pretending there is a source file backing up the expression?

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
73 ↗(On Diff #213018)

The comment seems asymmetric now.

I have a question based on my half-knowledge about the expression evaluator: I thought that we already wrote out a temporary file for the expression in order to support expr -g? Is this orthogonal or do we now have to ways of pretending there is a source file backing up the expression?

The file creation happens a little above the change to pass in a memory file:

// We also want a real file on disk if we generate full debug info.
should_create_file |= m_compiler->getCodeGenOpts().getDebugInfo() ==
                      codegenoptions::FullDebugInfo;

around line 891. Raphael's change doesn't change whether we emit this source file or not.

But it seem weird that this will make every expression seem to come from the same file and line. It would be really nice to be able to set breakpoints in compiled expressions, but if all the files think they have the same source file & starting line, this will be harder to get working.

Note, however, the creation of the source file & tracing back from the debug_line that we insert when we JIT the expression to this source file seems broken, however. If I do:

(lldb) expr --top-level -- int $foo(int) { (int) printf("I am here.\n"); }
(lldb) b s -n printf
Breakpoint 3: where = libsystem_c.dylib`printf, address = 0x00007fff616588ec
(lldb) expr -i 0 -- $foo(10)
1 location added to breakpoint 2
error: Execution was interrupted, reason: breakpoint 2.1.
The process has been left at the point where it was interrupted, use "thread return -x" to return to the state before expression evaluation.
Process 3708 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
    frame #0: 0x0000000100104b1b $__lldb_expr1`$foo(int) at Parse:1
Target 0: (something) stopped.

I should have seen the source code for $foo, but we don't seem to have found the file. So something is broken in this process.

Is this orthogonal or do we now have to ways of pretending there is a source file backing up the expression?

Jim already answered this, but this patch only changes the file names of our expression and pretends for the sake of diagnostics that we are in a separate file for the user code.

But it seem weird that this will make every expression seem to come from the same file and line.

What about enumerating the expression file names we print? E.g. <user expression 1>, <user expression 2>?

That would be fine. In the swift REPL, we increment the line number so that it appears that all the expressions are one contiguous source file. But I think for the expression parser treating each expression as a separate file is a better fiction.

teemperor updated this revision to Diff 213587.Aug 6 2019, 5:29 AM
teemperor edited the summary of this revision. (Show Details)
  • We now enumerate the fake expression files. E.g. <user expression 3>.
JDevlieghere added inline comments.Aug 6 2019, 8:45 AM
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
267 ↗(On Diff #213587)

std::move(filename)

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
29 ↗(On Diff #213587)

Can we use StringRefs for prefix and body?

31 ↗(On Diff #213587)

Can you use an enum to name the boolean parameter?

enum Wrapping : bool {
    Wrap = true,
    NoWrap = false,
  };
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
88 ↗(On Diff #213587)

///

teemperor updated this revision to Diff 213703.Aug 6 2019, 1:42 PM
  • Addressed feedback.
teemperor updated this revision to Diff 213705.Aug 6 2019, 1:44 PM
  • Addressed some feedback I forgot.
teemperor marked 4 inline comments as done.Aug 6 2019, 1:45 PM

Fixed some of the unrelated comments in a separate commit (that is already upstreamed).

LGTM, but you might want to have someone with more functional knowledge give the go-ahead.

teemperor added a reviewer: Restricted Project.Aug 26 2019, 6:23 AM

ping :)

davide added a subscriber: davide.Sep 13 2019, 11:16 AM
teemperor retitled this revision from [lldb] Print better diagnostics for user expressions. to [lldb] Print better diagnostics for user expressions and modules.
  • Rebased test.
  • Also testing Objective-C modules now (as the ClangModulesDeclVendor provides valid SourceLocations, so this patch also improves diagnostics from modules).
JDevlieghere accepted this revision.Sep 17 2019, 9:22 AM
This revision is now accepted and ready to land.Sep 17 2019, 9:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 1:54 AM