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

[ki.stfu]

Summary

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

[ki.stfu]

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]

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.

[ki.stfu]

In D8103#135594, @clayborg wrote:

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 } };

2. 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

3. 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]

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

[clayborg]

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()

[clayborg]

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.

[ki.stfu]

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]

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

[ki.stfu]

Add the same comment to SBValue::GetValueDidChange

[jingham]

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.

[ki.stfu]

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

lldb/trunk/include/lldb/API/SBValue.h

public:
     GetValueAsSigned(int64_t fail_value=0);
     
     uint64_t
     GetValueAsUnsigned(uint64_t fail_value=0);
 
     ValueType
     GetValueType ();
 
+    // It will be only valid starting from the second time.
     bool
     GetValueDidChange ();
 
     const char *
     GetSummary ();
     
     const char *
     GetSummary (lldb::SBStream& stream,

lldb/trunk/include/lldb/Core/ValueObject.h

//        SetContext (ExecutionContextScope *exe_scope);
     DumpPrintableRepresentation (Stream& s,
                                  ValueObjectRepresentationStyle val_obj_display = eValueObjectRepresentationStyleSummary,
                                  lldb::Format custom_format = lldb::eFormatInvalid,
                                  PrintableRepresentationSpecialCases special = ePrintableRepresentationSpecialCasesAllow,
                                  bool do_dump_error = true);
     bool
     GetValueIsValid () const;
 
+    // It will be only valid starting from the second time.
     bool
     GetValueDidChange ();
 
     bool
     UpdateValueIfNeeded (bool update_format = true);
     
     bool
     UpdateFormatsIfNeeded();

lldb/trunk/test/python_api/value_var_update/TestValueVarUpdate.py

def do_test(self):
         
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
             substrs = ['stopped',
                        'stop reason = breakpoint'])
 
         i = self.frame().FindVariable("i")
         i_val = i.GetValueAsUnsigned(0)
+        c = self.frame().FindVariable("c")
+
+        # Update any values from the SBValue objects so we can ask them if they changed after a continue
+        i.GetValueDidChange()
+        c.GetChildAtIndex(1).GetValueDidChange()
+        c.GetChildAtIndex(0).GetChildAtIndex(0).GetValueDidChange()
         
         if self.TraceOn(): self.runCmd("frame variable")
         
         self.runCmd("continue")
 
         if self.TraceOn(): self.runCmd("frame variable")
         
         self.assertTrue(i_val != i.GetValueAsUnsigned(0), "GetValue() is saying a lie")
         self.assertTrue(i.GetValueDidChange(), "GetValueDidChange() is saying a lie")
 
+        # Check complex type
+        self.assertTrue(c.GetChildAtIndex(0).GetChildAtIndex(0).GetValueDidChange() and
+                        not c.GetChildAtIndex(1).GetValueDidChange(), "GetValueDidChange() is saying a lie")
 
 if __name__ == '__main__':
     import atexit
     lldb.SBDebugger.Initialize()
     atexit.register(lambda: lldb.SBDebugger.Terminate())
     unittest2.main()

lldb/trunk/test/python_api/value_var_update/main.c

+struct complex_type {
+    struct { long l; } inner;
+    struct complex_type *complex_ptr;
+};
+
 int main() {
     int i = 0;
+    struct complex_type c = { { 1L }, &c };
     for (int j = 3; j < 20; j++)
     {
-        i += j;
+        c.inner.l += (i += j);
         i = i - 1; // break here
     }
     return i;
 }