Page MenuHomePhabricator

Handle typeof() expressions
ClosedPublic

Authored by JDevlieghere on Feb 19 2018, 9:03 AM.

Details

Summary

Before this patch, LLDB was not able to evaluate expressions that
resulted in a value with a typeof-type.

(lldb) p int i; __typeof__(i) j = 1; j
(typeof (i)) $0 =

This fixes that. The type is still printed as (typeof (i)) but at least
we get a value now.

(lldb) p int i; __typeof__(i) j = 1; j
(typeof (i)) $0 = 1

I'm looking into printing this as int but will keep that for a follow-up commit either way.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Feb 19 2018, 9:03 AM

Seems straight-forward enough, but technically Jim is the owner of the expression evaluator these days, so I'll leave the honours to him.

packages/Python/lldbsuite/test/expression_command/test/TestExprs.py
270 ↗(On Diff #134935)

Are you sure this command is actually correct? The "p" here seems odd...

source/Symbol/ClangASTContext.cpp
5264 ↗(On Diff #134935)

I'm guessing this means decltype doesn't work either ?

davide requested changes to this revision.Feb 19 2018, 10:15 AM

Jonas, this looks a good use case for using lit.
Is it possible to reuse the machinery we use in lldb/lit/Expr ?
If not, well, we know there's something we can improve :)

This revision now requires changes to proceed.Feb 19 2018, 10:15 AM

Change to lit test

JDevlieghere marked 2 inline comments as done.Feb 19 2018, 10:48 AM
JDevlieghere added inline comments.
source/Symbol/ClangASTContext.cpp
5264 ↗(On Diff #134935)

Indeed, I was thinking about doing this in a separate diff but since it's so similar I'll update this patch.

davide added inline comments.Feb 19 2018, 10:51 AM
lit/Expr/TestTypeOfExpr.test
2 ↗(On Diff #134942)

I really really love how concise and clear the new test is!

JDevlieghere marked an inline comment as done.

Do this for decltype too

JDevlieghere marked an inline comment as done.

Make sure the lit test actually checks the lldb output instead of the # CHECK lines.

Without the check-next, CHECK matches the # CHECK lines, as they are printed by lldb:

(lldb) # RUN: %lldb -b -s %s | FileCheck %s
(lldb) expression int i; __typeof__(i) j = 1; j
(typeof (i)) $0 = 1
(lldb) # CHECK: (lldb) expression int i; __typeof__(i) j = 1; j
(lldb) # CHECK-NEXT: (typeof (i)) $0 = 1
(lldb) expression int i; typeof(i) j = 1; j
(typeof (i)) $1 = 1
(lldb) # CHECK: (lldb) expression int i; typeof(i) j = 1; j
(lldb) # CHECK-NEXT: (typeof (i)) $1 = 1
(lldb) expression int i; decltype(i) j = 1; j
(decltype(i)) $2 = 1
(lldb) # CHECK: (lldb) expression int i; decltype(i) j = 1; j
(lldb) # CHECK-NEXT: (decltype(i)) $2 = 1
jingham requested changes to this revision.Feb 19 2018, 11:46 AM

The code part of this looks fine. I had a few quibbles with the test, see inline.

packages/Python/lldbsuite/test/expression_command/test/TestExprs.py
270 ↗(On Diff #134935)

Yes, all the other commands in this test that run expressions either get the stopped frame and run SBFrame.EvaluateExpression, or they spell out "expression" completely. Unless you are actually testing that an alias works, it's better not to use aliases in tests.

Also, was there a reason why you didn't use TestEBasicExprCommandsTestCase.build_and_run? It looks like everything you do is done there. I was going to say you should make sure that run stopped at the breakpoint - otherwise if that fails you'll get some weird error from the expression that may be harder to diagnose. But then I noticed the build_and_run function which ALSO doesn't test that the run stopped at your breakpoint, but if all the tests used that then we could just add the check there.

This revision now requires changes to proceed.Feb 19 2018, 11:46 AM

The code part of this looks fine. I had a few quibbles with the test, see inline.

Jim, it looks like you're commenting on an older version of the diff. I've since switched to checking this with lit.

jingham added a comment.EditedFeb 19 2018, 11:58 AM

Oh, somehow my browser hadn't updated to show the lit test. Ignore the previous comment.

This is fine, except can you make the test check not depend on the particular result variable number ($0, $1, $2). You aren't testing here that the result variable numbers are monotonically increasing integers that start at zero, and it will make adding to the test annoying over time. This command output type of test is fragile because it depends on irrelevant details of command output. For instance TestCallStopAndContinue.test knows that the text we output for a stop is exactly "Execution was interrupted, reason: breakpoint" seems like more detail than a test should rely on. Similarly the exact format of the result variable seems more detail than this test needs.

Other than that it's fine.

Thanks for the review Jim! I've updated the test accordingly.

jingham accepted this revision.Feb 19 2018, 1:26 PM

Looks good

davide accepted this revision.Feb 19 2018, 1:55 PM

LGTM

This revision is now accepted and ready to land.Feb 19 2018, 1:55 PM
This revision was automatically updated to reflect the committed changes.

One issue I can see with this is that if you check the documentation of -b it says this:

-b
--batch
     Tells the debugger to run the commands from -s, -S, -o & -O, and
     then quit.  However if any run command stopped due to a signal or
     crash, the debugger will return to the interactive prompt at the
     place of the crash.

So, this should work fine as long as none of these commands crash, but if they do crash it will cause lit to hang. It might be useful to add a -B option to LLDB that will *not* return to the interactive prompt in the case of a signal or crash, and instead will just terminate. This way we'd see the crash as just another form of test failure rather than a timeout.

Rather than adding another flag, can't you just put -o quit at the end of your RUN line?

If your commands are A, B, C, and D but A crashes and returns to the interactive prompt, will it still try to execute B, C, and D? If so then I guess that would work (I haven't tried). OTOH, there's a risk of people forgetting to do this (but I'm not sure if the risk would be higher than people using -b instead of -B).

Actually I may have misunderstood the help text. It sounds like it may be referring to a crash of the inferior, not a crash of LLDB itself. If the test contains no run commands (as this one doesn't), then it doesn't seem like there's any risk of this happening, and this is a non issue.

The point of --batch is that if the program crashes, control is transferred from the script to the user so that they can inspect the crash. lit would just hang at that point, right?

I worry that people will use this RUN example as an template, and then make a test that does cause the target to execute, which test would then hang up if the target crashed. So I still think ending with an explicit quit rather than using --batch is safer.