hang bug in lldb-mi -var-update

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

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

The spec says that's what it should do. From
GDB/MI Variable Objects (Debugging with GDB) :

"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 :slight_smile:

Ted

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.

Jim

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