Page MenuHomePhabricator

[Expressions] Add support of expressions evaluation in some object's context
ClosedPublic

Authored by aleksandr.urakov on Dec 5 2018, 5:55 AM.

Details

Summary

This patch adds support of expression evaluation in a context of some object. Consider the following example:

struct S {
  int a = 11;
  int b = 12;
};

int main() {
  S s;
  int a = 1;
  int b = 2;
  // We have stopped here
  return 0;
}

This patch allows to do something like that:

lldb.frame.FindVariable("s").EvaluateExpression("a + b")

and the result will be 33 (not 3) because fields a and b of s will be used (not locals a and b).

This is achieved by replacing of this type and object for the expression. This has some limitations: an expression can be evaluated only for values located in the debuggee process memory (they must have an address of eAddressTypeLoad type).

Our company needs this functionality to implement some UI visualization of variables in our IDEs. I understand that the community may be not interested in this functionality, and I'll just abandon it if so. But if the community is interested in this, we can save some time on merging in the future :)

Diff Detail

Repository
rL LLVM

Event Timeline

Btw, it can be useful if there ever would be a declarative format for pretty printers in LLDB.

jingham requested changes to this revision.Dec 5 2018, 12:42 PM

This is a little bit odd, but it does make it easy to call methods on a value you got from FindVariable without having to cons up an expression. That seems worthwhile.

It would be good to add a little more explanation of what this is doing in the docs: "in the context of the object" is a little vague.

You should add some tests where the operation doesn't make sense, for instance what if the SBValue is a scalar? Or what if it is an expression result so it's not in memory. You have some error handling in the patch but your test doesn't exercise it. Be good to make sure that actually works.

You also only test the case where the value is a struct, can you test a pointer to a struct and an ObjC class? They both should work but you don't have tests for them.

As an implementation detail, the fact that lldb has to get local variable lookup right by textually injecting the local variables declarations into the expression is a bug, these should be provided on demand from the ASTSource. But that doesn't work correctly at present - the AST source gets asked AFTER looking in the class context which is to late - and we have do the local variable insertion trick instead. So if we ever fix the bug in clang lookup, your implementation will break, and you will have to interfere in that lookup instead. But that's for the future.

This revision now requires changes to proceed.Dec 5 2018, 12:42 PM

Sounds like an interesting feature to me.

Thanks for the interest to the feature! I've updated the patch due to the comments.

The only problem is that I'm not familiar with Objective C and have no Mac OS to test it, so I haven't written the test for Objective C case. I've added the "support" of Objective C blindly, so I even not sure that it works correctly. I can remove it from the patch (or, may be some person more familiar with the theme can check if it is ok?). I'm not sure, is it better to leave a preparation of the feature implementation for Objective C or to completely remove it?

zturner added inline comments.Dec 6 2018, 4:33 PM
include/lldb/API/SBValue.h
310–312 ↗(On Diff #176980)

Can you use StringRef instead of const char * here?

include/lldb/Expression/UserExpression.h
296 ↗(On Diff #176980)

A reference to a shared_ptr seems odd. Why don't we just say const ValueObject* ctx_obj = nullptr?

include/lldb/Symbol/ClangASTContext.h
1057 ↗(On Diff #176980)

Same thing here.

source/API/SBValue.cpp
1325–1327 ↗(On Diff #176980)

Instead of the lines like if (log) log->Printf(...) you can instead write:

LLDB_LOG(log, "SBValue::EvaluateExpression called with an empty expression");
1367–1371 ↗(On Diff #176980)

If you decide to make that change, note that the macros call Format, not Printf, so in this case you would have to change your format string to something like:

LLDB_LOG(log, "SBValue({0}::EvaluateExpression (expr=\"{1}\") => SBValue({2}) (execution result = {3})",
    value_sp.get(), expr, res_val_sp.get(), expr_res);

BTW, I would discourage logging pointer values, as it makes log output non-deterministic and doesn't often help when reading log files. That said, it wouldn't be the first time we logged pointer values.

jingham added inline comments.Dec 6 2018, 4:52 PM
include/lldb/API/SBValue.h
310–312 ↗(On Diff #176980)

This is an SB API. We don't use StringRef's or any other llvm data types in the SB API's.

jingham added inline comments.Dec 6 2018, 4:57 PM
source/API/SBValue.cpp
1367–1371 ↗(On Diff #176980)

Logging the pointer value of something long-lived can often be really helpful when you are actually debugging lldb, since you can notice some oddity by looking at the log and then get directly to the object that was logged. Logging the values of short-lived pointers is not so useful. I think this is more of the latter case, and maybe logging the incoming value_sp's name and whether the result was successful might be more useful.

zturner added inline comments.Dec 6 2018, 4:57 PM
include/lldb/API/SBValue.h
310–312 ↗(On Diff #176980)

Ahh, you are right of course. My bad.

aleksandr.urakov marked 6 inline comments as done.Dec 7 2018, 12:55 AM
aleksandr.urakov added inline comments.
include/lldb/Expression/UserExpression.h
296 ↗(On Diff #176980)

Yes, I agree. I wasn't sure about the lifetime of the context object, that's why I've used a shared pointer here. But now it is obvious that there should be no problems with the lifetime - an expression evaluation with a context object is called from SBValue only, so it is guaranteed that the required value exists during the evaluation.

The only thing is that the pointer points to a non-const value object - non-const methods are used.

aleksandr.urakov marked an inline comment as done.

Thanks for the comments, I've updated the patch.

Ping! What do you think about this?

The only reservation I have is that this really ought to work correctly for ObjC as well as C++, and there's code to support that, but it isn't tested. I don't have time to write ObjC tests before the new year for sure. Is it possible to rope somebody else into that? If that's not possible, maybe check it in with the ObjC support off and a big FIXME and file a bug to re-enable that support with tests.

Sorry for the long delay with reply. My colleague with a Mac (and Obj-C knowledge) have created the test for the Obj-C case. Can you take a look, please?

Ping! Can you take a look, please?

jingham accepted this revision.Feb 4 2019, 3:13 PM

Thanks for adding the ObjC tests! I had two trivial requests for the test comments to be a little clearer, but even without that this is fine.

packages/Python/lldbsuite/test/expression_command/context-object-objc/TestContextObjectObjc.py
42–52 ↗(On Diff #183816)

IIUC, these last two are testing that evaluating in the context of an object doesn't effect computations that don't refer to that object. Is that right? After all "self.property" doesn't need any help from the objcClass variable to look up the result. Can you make the comment a little clearer.

54 ↗(On Diff #183816)

This one is testing that when the reference object is an lldb result object, we can still use it correctly. "Test computation result" is a little terse. Can you make this a little clearer.

This revision is now accepted and ready to land.Feb 4 2019, 3:13 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 1:14 AM

Thank you! I've updated the comments in the commit.