[lldb-dev] hang bug in lldb-mi -var-update

[Greg Clayton via lldb-dev]

> On Aug 25, 2017, at 4:15 PM, Jim Ingham <jingham at apple.com> wrote:
> 
> If that’s the expectation, you have to obey it...   Xcode is pretty careful to only act on the elements that were visible in a view, which made the locals view much less heavy-weight.  But that took some work on their end.
> 
> More to the point, this doesn’t seem to be code that should be in the MI layer.  Shouldn’t this be a method on the SBValue?  If there was something tricky that you could get wrong, it would be better to centralize it.
> 
> As Greg says, this shouldn’t be the default “has changed” behavior, but still it seems like it should be an SBValue method.

We could possibly add:

  bool SBValue::GetValueDidChange(bool check_children);

as a new API. I still say this is a potentially very expensive operation that most if not all debuggers should avoid, but if they want to debug really really slowly under some circumstances, I don't really mind. There should be a strong warning on this method that things can get expensive.


Your fix of not recursing through pointers needs to also do the same for any references (l value, r value, and C++ 11 &&). So your fix is correct if you wish to keep the lldb-mi paradigm of checking all children, just add references to the list of things to not check. 

Just for fun, make an std::vector<int> with a million entries, change the last one, and single step over it and wait to see how long this code takes just to make sure you really want to check that many items.

Greg


> 
> Jim
> 
>> On Aug 25, 2017, at 3:49 PM, Ted Woodward via lldb-dev <lldb-dev at lists.llvm.org> wrote:
>> 
>> The spec says that's what it should do. From
>> https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Variable-Objects.html :
>> 
>> "Reevaluate the expressions corresponding to the variable object name and
>> all its direct and indirect children, and return the list of variable
>> objects whose values have changed;"
>> 
>> Also, our Eclipse guy gets grumpy when it doesn't :-)
>> 
>> Ted
>> 
>> --
>> Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
>> Linux Foundation Collaborative Project
>> 
>>> -----Original Message-----
>>> From: Greg Clayton [mailto:clayborg at gmail.com]
>>> Sent: Friday, August 25, 2017 5:00 PM
>>> To: Ted Woodward <ted.woodward at codeaurora.org>
>>> Cc: lldb-dev at lists.llvm.org
>>> Subject: Re: [lldb-dev] hang bug in lldb-mi -var-update
>>> 
>>> lldb-mi should never be checking the children. This is never a good idea
>> due to
>>> performance. What happens when you have an array with a million entries?
>>> Long delay. Aggregate types should never say they changed. Only SBValue
>>> objects that have values should claim to change.
>>> 
>>> Greg
>>> 
>>> 
>>>> On Aug 25, 2017, at 10:42 AM, Ted Woodward via lldb-dev <lldb-
>>> dev at lists.llvm.org> wrote:
>>>> 
>>>> I found a hang in lldb-mi's -var-update. It checks to see if a var
>>>> changed, then it checks each of the children recursively. If a child
>>>> is a pointer back to a parent, as in this case:
>>>> 
>>>> struct complex_type
>>>> {
>>>>  int i;
>>>>  struct { long l; } inner;
>>>>  struct complex_type *complex_ptr;
>>>> };
>>>> 
>>>> void
>>>> var_update_test(void)
>>>> {
>>>>  struct complex_type complx_array[2];
>>>> 
>>>>  complx_array[0].i = 4;
>>>>  complx_array[0].inner.l = 4;
>>>>  complx_array[0].complex_ptr = &complx_array[1];
>>>>  complx_array[1].i = 5;
>>>>  complx_array[1].inner.l = 5;
>>>>  complx_array[1].complex_ptr = &complx_array[0];
>>>> 
>>>> 
>>>> 
>>>> the code in CMICmdCmdVarUpdate::ExamineSBValueForChange will get into
>>>> an infinite loop.
>>>> 
>>>> const MIuint nChildren = vrwValue.GetNumChildren();  for (MIuint i =
>>>> 0; i < nChildren; ++i) {
>>>>  lldb::SBValue member = vrwValue.GetChildAtIndex(i);
>>>>  if (!member.IsValid())
>>>>    continue;
>>>> 
>>>>  if (member.GetValueDidChange()) {
>>>>    vrwbChanged = true;
>>>>    return MIstatus::success;
>>>>  } else if (ExamineSBValueForChange(member, vrwbChanged) &&
>>> vrwbChanged)
>>>>    // Handle composite types (i.e. struct or arrays)
>>>>    return MIstatus::success;
>>>> }
>>>> 
>>>> I've got a patch that disables checking a pointer's children. I'll put
>>>> it up on phabricator today.
>>>> 
>>>> Ted
>>>> 
>>>> --
>>>> Qualcomm Innovation Center, Inc.
>>>> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>>>> a Linux Foundation Collaborative Project
>>>> 
>>>> 
>>>> _______________________________________________
>>>> lldb-dev mailing list
>>>> lldb-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>> 
>> 
>> _______________________________________________
>> lldb-dev mailing list
>> lldb-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20170828/0f7fb819/attachment.html>