Page MenuHomePhabricator

TestMultilineExpr: validate evaluation for expressions spread over multiple lines
ClosedPublic

Authored by sgraenitz on Sep 19 2018, 10:48 AM.

Details

Summary

When LLDB successfully parses a command (like "expression" in this case) and determines incomplete input, the user can continue typing on multiple lines (in this case "2+3"). This should provide the correct result.
Note that LLDB reverts input from the additional lines, so they are not present in the output.

Diff Detail

Repository
rL LLVM

Event Timeline

sgraenitz created this revision.Sep 19 2018, 10:48 AM
sgraenitz added a subscriber: Restricted Project.Sep 19 2018, 10:50 AM
aprantl added inline comments.Sep 19 2018, 10:55 AM
lit/Expr/TestMultilineExpr.test
3 ↗(On Diff #166145)

"reverts" -> "joins"?

3 ↗(On Diff #166145)

or did you mean "hides"

11 ↗(On Diff #166145)

There's no newline at end of file :-)

Addressing Adrian's comments

Does that mean we can remove ./packages/Python/lldbsuite/test/expression_command/multiline/TestMultilineExpressions.py ?

sgraenitz marked 3 inline comments as done.Sep 19 2018, 11:25 AM
sgraenitz added inline comments.
lit/Expr/TestMultilineExpr.test
9 ↗(On Diff #166145)

Maybe it's nitpicking, but I'd actually prefer not to match this specific string. It could be any human-readable instruction. However, if I replace it with {{.*}} lit will complain that the match doesn't start on the next line:

TestMultilineExpr.test:9:15: error: CHECK-NEXT: is on the same line as previous match
# CHECK-NEXT: {{.*}}
              ^
<stdin>:9:18: note: 'next' match was here
(lldb) expression
                 ^
<stdin>:9:18: note: previous match ended here
(lldb) expression
                 ^

Any ideas how to get it right?

Can't you just check for '5', as this is the only information we actually actually care about here?

Remove old Python test in ./packages/Python/lldbsuite/test/expression_command/multiline/

For those following at home: the point of this exercise is to get rid of the -expect-based TestMultilineExpressions.py testcase that kept failing on build bots.

aprantl added inline comments.Sep 19 2018, 11:37 AM
lit/Expr/TestMultilineExpr.test
9 ↗(On Diff #166145)

why not:

# CHECK: (lldb) expression
# CHECK: (int) {{.*}} = 5

?

Can't you just check for '5', as this is the only information we actually actually care about here?

Yes that would be the simplest way, but as LLDB still echoes all commands including comments, all CHECK lines are self-fulfilling prophecies.
This is what FileCheck will see in this case:

(lldb) command source -s 0 '/Users/sgranitz/Develop/lldb-llvm/git-svn/llvm/tools/lldb/lit/lit-lldb-init'
Executing commands in '/Users/sgranitz/Develop/lldb-llvm/git-svn/llvm/tools/lldb/lit/lit-lldb-init'.
(lldb) # LLDB init file for the LIT tests.
(lldb) settings set symbols.enable-external-lookup false
(lldb) command source -s 0 '/Users/sgranitz/Develop/lldb-llvm/git-svn/llvm/tools/lldb/lit/Expr/TestMultilineExpr.test'
Executing commands in '/Users/sgranitz/Develop/lldb-llvm/git-svn/llvm/tools/lldb/lit/Expr/TestMultilineExpr.test'.
(lldb) # RUN: %lldb -b -s %s | FileCheck %s
(lldb) # In terminal sessions LLDB hides input from subsequent lines so it's not visible in the output we check below.
(lldb) expression
Enter expressions, then terminate with an empty line to evaluate:
(int) $0 = 5
(lldb) # CHECK: (lldb) expression
(lldb) # CHECK-NEXT: Enter expressions, then terminate with an empty line to evaluate:
(lldb) # CHECK-NEXT: (int) {{.*}} = 5

LLDB still echoes all commands including comments

What do you think about fixing that before landing this patch? Then we don't need to work around it.

What do you think about fixing that before landing this patch? Then we don't need to work around it.

Hm, we didn't finally decide for the fix and I don't want to raise the pressure on it artificially.
I think it will anyway reveal more places, where we forgot about this little detail and thus provide opportunity to improve also this test.

Of course, this also means that I can keep matching the specific string for this (hopefully short) foreseeable future.

lit/Expr/TestMultilineExpr.test
9 ↗(On Diff #166145)

I think we can keep it like this for now.

sgraenitz updated this revision to Diff 168466.Oct 5 2018, 6:38 AM

Simplify test as proposed in review

sgraenitz marked an inline comment as done.Oct 5 2018, 6:40 AM

Requires: https://reviews.llvm.org/D52788 (Add EchoCommentCommands to CommandInterpreterRunOptions in addition to the existing EchoCommands and expose both as interpreter settings)

aprantl accepted this revision.Oct 5 2018, 9:08 AM
This revision is now accepted and ready to land.Oct 5 2018, 9:08 AM
This revision was automatically updated to reflect the committed changes.