Improve ValueObject::GetValueDidChange test; Add a comment for it
ClosedPublic

Authored by ki.stfu on Mar 6 2015, 5:57 AM.

Details

Summary

This patch adds a few comments for GetValueDidChange and contains improvements for TestValueVarUpdate.py test which checks ValueObject::GetValueDidChange for complex types.

Diff Detail

Repository
rL LLVM
ki.stfu retitled this revision from to Fix ValueObject::GetValueDidChange; Improve test for it.Mar 6 2015, 5:57 AM
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added subscribers: clayborg, zturner, granata.enrico, Unknown Object (MLST).

I have noticed 2 new failures:

======================================================================
FAIL: test_with_dsym_and_run_command (TestTypeCompletion.TypeCompletionTestCase)
   Check that types only get completed when necessary.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/IliaK/p/llvm_delphi/tools/lldb/test/lldbtest.py", line 456, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/IliaK/p/llvm_delphi/tools/lldb/test/functionalities/type_completion/TestTypeCompletion.py", line 20, in test_with_dsym_and_run_command
    self.type_completion_commands()
  File "/Users/IliaK/p/llvm_delphi/tools/lldb/test/functionalities/type_completion/TestTypeCompletion.py", line 61, in type_completion_commands
    self.assertFalse(p_type.IsTypeComplete(), 'vector<T> complete but it should not be')
AssertionError: True is not False : vector<T> complete but it should not be
Config=x86_64-clang
======================================================================
FAIL: test_with_dwarf_and_run_command (TestTypeCompletion.TypeCompletionTestCase)
   Check that types only get completed when necessary.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/IliaK/p/llvm_delphi/tools/lldb/test/lldbtest.py", line 473, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/IliaK/p/llvm_delphi/tools/lldb/test/lldbtest.py", line 537, in wrapper
    func(*args, **kwargs)
  File "/Users/IliaK/p/llvm_delphi/tools/lldb/test/functionalities/type_completion/TestTypeCompletion.py", line 28, in test_with_dwarf_and_run_command
    self.type_completion_commands()
  File "/Users/IliaK/p/llvm_delphi/tools/lldb/test/functionalities/type_completion/TestTypeCompletion.py", line 61, in type_completion_commands
    self.assertFalse(p_type.IsTypeComplete(), 'vector<T> complete but it should not be')
AssertionError: True is not False : vector<T> complete but it should not be
Config=x86_64-clang
----------------------------------------------------------------------

Therefore seems that my patch is wrong because this it works as expected.

But what should I do to get ValueObject::GetValueDidChange work? If we don't want to complete type on create should we do "ValueObject::Complete" method to complete it, before we will be interested in use of ValueObject::GetValueDidChange, right?

clayborg requested changes to this revision.Mar 6 2015, 9:35 AM

I believe this will cause serious performance issues. GetValueDidChange() is not trying to figure out if any children have changed, but if the current SBValue or ValueObject has changed. A structure with 1000000 items doesn't have a value itself, and there is no way we should be checking all 1000000 children and asking them if anything changed.

To clarify a few things that we consider when using ValueObjects/SBValue:
1 - an aggregate type has no value. These include structs, unions, classes, arrays, anything that contains multiple children. ValueDidChange should never say true for any of these as computing them based on visiting all children is too expensive. They might have summaries like "123 objects" for a std::vector<int>.
2 - pointers to aggregate types do have a value: the pointer itself. This can say it changed because it is very easy to calculate.
3 - simple types have values and can easily claim if they change or not
4 - synthetic types might also be very expensive and are a form of aggregate types. Imagine a synthetic child provider for std::map with a map that has 1000000 items inside it. We can not pay the cost of realizing all 1000000 children so we can set the value did change bit correctly. Stepping through a frame with an IDE that displays a variable whose type is std::map<A, B> would slow down stepping by a few minutes

So please do not commit this. I would suggest aborting this change.

This revision now requires changes to proceed.Mar 6 2015, 9:35 AM

I believe this will cause serious performance issues. GetValueDidChange() is not trying to figure out if any children have changed, but if the current SBValue or ValueObject has changed. A structure with 1000000 items doesn't have a value itself, and there is no way we should be checking all 1000000 children and asking them if anything changed.

It seems that we do not understand each other. I don't check children on GetValueDidChange(). As I said in Summary, the GetValueDidChange() checks previous checksum and if it is empty the GetValueDidChange() can't determine whether the value was changed or not. Because of this reason, the -var-update doesn't work even if I recursively check all children using the GetValueDidChange().

I expanded a test (see changes made in test/python_api/value_var_update) and now it doesn't work without these changes.

Also this bug happens in the following example:

  1. If we consider the following variable:
struct complex_type {
  struct { int i; } inner;
} c = { { 1 } };
  1. then this will not work (but should):
c = self.frame().FindVariable("c")
self.frame().EvaluateExpression("c.inner.i=3")
assertTrue(c.GetChildAtIndex(0).GetChildAtIndex(0).GetValueDidChange()) # ERROR
  1. BUT if I update this child before EvaluateExpression() it will work:
c = self.frame().FindVariable("c")
c.GetChildAtIndex(0).GetChildAtIndex(0).GetNumChildren() # It always will return 0, but the main goal of this line is to
                                                         # update the "c.inner.i" VariableObject using the UpdateValueIfNeeded() 
self.frame().EvaluateExpression("c.inner.i=3")
assertTrue(c.GetChildAtIndex(0).GetChildAtIndex(0).GetValueDidChange()) # OK

How I should fix it by another way?

In my previous message I supposed that we can create ValueObject::Complete() method to complete it. The Complete() will be called once at the creation of the variable for which we will call GetValueDidChange() in the future. It will help us to fix -var-update and the described test above.

jingham added a subscriber: jingham.Mar 6 2015, 1:20 PM

The way to understand this is since "value did change" really means "value did change since I last looked at it" you have to ask the question "what constitutes looking at something." We have made the decision that when you make the value for a struct, you have only looked at the struct, NOT at its children. It isn't till you poke at the children that you know what their values are - and then whether they change later on.

This is a fairly artificial definition of "when you look at a struct's children" but it is an important one, as Greg said. If you imagine the Locals view of a debugger, which is going to show the top level view of all the locals, until the user actually chooses to disclose a struct (by some UI gesture that amounts to calling GetNumChildren & GetChildAtIndex) we don't want to pay the cost of realizing all its types, and getting all its potentially many children.

So the answers is that the "failure" you see in #1 is actually by design. If you want to make the MI -var-update work correctly then when you hand out the variable you should get all the children internally at first. Then it will behave as you expect it to. But the fact that until you call GetNumChildren we don't do ANY work to fetch the children is by design, not a bug.

Jim

I believe the test is flawed. GetValueDidChange() will only work if you have gotten the value and then later ask if it changed. See my inline comments above.

We can document that we GetValueDidChange() is only valid if GetValue() was previously called on a SBValue. So fix the test and remove all other code.

source/Core/ValueObjectVariable.cpp
46–60 ↗(On Diff #21345)

We can not do this. An array with 1000000 entries will cause this to slow ways down. What if you have:

struct foo
{

// Define a very large structure here

};

struct foo[1000][1000][1000] foo_vector;

Now you single step and we delay for a few minutes when this variable is in your function.

This can't go in.

test/python_api/value_var_update/TestValueVarUpdate.py
55 ↗(On Diff #21345)

You will need to get the value you want to check for changes before you can ask if it has changed below. Add the following code here:

# Get any values from the SBValue objecst so we can ask them if they changed after a continue
value = i.GetValue()
value = c.GetChildAtIndex(1).GetValue()
value = c.GetChildAtIndex(0).GetChildAtIndex(0).GetValue()

Fix the test case as noted in inline comment.

include/lldb/Core/ValueObjectVariable.h
85–88 ↗(On Diff #21345)

Remove this.

source/Core/ValueObjectVariable.cpp
41–59 ↗(On Diff #21345)

revert this.

Ok. Thanks for answer. I think we really should add this comment to avoid such questions in the future. I'll fix this test and remove the rest of the changes.

ki.stfu updated this revision to Diff 21394.Mar 6 2015, 2:04 PM

Update test; Add a comment; Remove ValueObjectVariable::UpdateValueObjectAndChildren

ki.stfu retitled this revision from Fix ValueObject::GetValueDidChange; Improve test for it to Improve ValueObject::GetValueDidChange test; Add a comment for it.Mar 6 2015, 2:07 PM
clayborg accepted this revision.Mar 6 2015, 2:12 PM
This revision is now accepted and ready to land.Mar 6 2015, 2:12 PM
ki.stfu updated this revision to Diff 21400.Mar 6 2015, 2:33 PM

Add the same comment to SBValue::GetValueDidChange

ki.stfu updated this object.Mar 6 2015, 2:36 PM
ki.stfu closed this revision.Mar 6 2015, 2:37 PM
This revision was automatically updated to reflect the committed changes.

I don't see how the "It will only be valid the second time" comments help anything. ValueDidChange can only mean "is it different from the last time you looked at it" since we aren't actually tracking all variable values in the program... So by definition, the return from ValueDidChange is arbitrary the first time you look at an object.

Since we have to choose something, we set it to "false" when you first look at a value. That is actually the correct choice because then a UI that is using ValueDidChange to drive the choice to color variables that have changed since the last stop in this frame won't set them all to the changed color the first time we hit the frame - which wouldn't be very useful.

That might be a better comment, since it says what actually happens, if you think a comment is needed here.

Hello Jim,

I don't see how the "It will only be valid the second time" comments help

anything. ValueDidChange can only mean "is it different from the last time
you looked at it" since we aren't actually tracking all variable values in
the program... So by definition, the return from ValueDidChange is
arbitrary the first time you look at an object.

I assumed that ValueDidChange is tracking value since it was created.It
isn't obvious for me when I can use GetValueDidChange. I think this comment
will help other people that think like me.

That might be a better comment, since it says what actually happens, if you

think a comment is needed here.

If you think that it's obvious I can remove it. Now I'm familiar with that
and I don't need them.

Thanks,
Ilia