This is an archive of the discontinued LLVM Phabricator instance.

lldb-mi: -var-update can hang when traversing complex types with pointers
ClosedPublic

Authored by ted on Aug 25 2017, 10:58 AM.

Details

Summary

-var-update calls CMICmdCmdVarUpdate::ExamineSBValueForChange to check if a varObj has been updated. It checks that the varObj is updated, then recurses on all of its children. If a child is a pointer pointing back to a parent node, this will result in an infinite loop, and lldb-mi hanging.

The problem is exposed by packages/Python/lldbsuite/test/tools/lldb-mi/variable/TestMiVar.py, but this test is skipped everywhere.

This patch changes ExamineSBValueForChange to not traverse children of varObjs that are pointers.

Diff Detail

Event Timeline

ted created this revision.Aug 25 2017, 10:58 AM
abidh added a subscriber: abidh.Aug 30 2017, 1:50 AM
abidh added a comment.Aug 30 2017, 2:08 AM

This check used to be there above the loop and was removed when you reported that changes in pointers are not being tracked in
http://lists.llvm.org/pipermail/lldb-dev/2017-May/012428.html

I think putting it on individual child is probably good enough compromise.

ted updated this revision to Diff 113310.Aug 30 2017, 2:02 PM
ted added reviewers: clayborg, abidh.
ted removed a subscriber: abidh.

Added check for reference types, as Greg suggested.

Simplified change.

clayborg added inline comments.Aug 30 2017, 2:17 PM
tools/lldb-mi/MICmdCmdVar.cpp
529–530

Better to keep this to 1 API call by using "uint32_t SBValue::GetTypeFlags()". That call returns a bunch of type info bits that will be set from lldb::TypeFlags and it allows you to get all of the info you need on a type pretty quickly:

// Skip pointers and references to avoid infinite loop
if (member.GetType().GetTypeFlags() & (lldb::eTypeIsPointer | lldb::eTypeIsReference))
  continue;
ted updated this revision to Diff 113317.Aug 30 2017, 2:44 PM
ted marked an inline comment as done.

Updated with Greg's suggestion.

Removed second call to GetValueDidChange() because it's handled at the top of the recursive call. This way we get 1 extra call to ExamineSBValueForChange() on a changed child, but avoid 2 calls to GetValueDidChange() for each child.

clayborg requested changes to this revision.Aug 30 2017, 2:55 PM
clayborg added inline comments.
tools/lldb-mi/MICmdCmdVar.cpp
518–522

This code needs to go before the children loop, but after the "if (vrwValue.GetValueDidChange()) {...}". We want to know if a pointer or reference has changed, just not their children.

bool CMICmdCmdVarUpdate::ExamineSBValueForChange(lldb::SBValue &vrwValue,
                                                 bool &vrwbChanged) {
  if (vrwValue.GetValueDidChange()) {
    vrwbChanged = true;
    return MIstatus::success;
  }
  vrwbChanged = false;

  // Skip children of pointers and references to avoid infinite loop
  if (vrwValue.GetType().GetTypeFlags() & 
      (lldb::eTypeIsPointer | lldb::eTypeIsReference)) {
    return MIstatus::success;
  }

  const MIuint nChildren = vrwValue.GetNumChildren();
  for (MIuint i = 0; i < nChildren; ++i) {
    auto member = vrwValue.GetChildAtIndex(i);
    if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
      break;
  }
  return MIstatus::success;
}
This revision now requires changes to proceed.Aug 30 2017, 2:55 PM
clayborg added inline comments.Aug 30 2017, 2:59 PM
tools/lldb-mi/MICmdCmdVar.cpp
522

Or even cleaner:

bool CMICmdCmdVarUpdate::ExamineSBValueForChange(lldb::SBValue &vrwValue,
                                                 bool &vrwbChanged) {
  vrwbChanged = vrwValue.GetValueDidChange();
  if (vrwbChanged)
    return MIstatus::success;

  // Skip children of pointers and references to avoid infinite loop
  if (vrwValue.GetType().GetTypeFlags() & 
      (lldb::eTypeIsPointer | lldb::eTypeIsReference)) {
    return MIstatus::success;
  }

  const MIuint nChildren = vrwValue.GetNumChildren();
  for (MIuint i = 0; i < nChildren && !vrwbChanged; ++i) {
    ExamineSBValueForChange(vrwValue.GetChildAtIndex(i), vrwbChanged);
  }
  return MIstatus::success;
}
ted added a comment.Aug 31 2017, 9:42 AM

We want to go down into a pointer if it's the original parent, but not into a child that's a pointer. To avoid skipping children of a top-level pointer/reference, I think we need to do the type check in the loop.

clayborg accepted this revision.Aug 31 2017, 10:32 AM

Makes sense even though this can be quite expensive.

This revision is now accepted and ready to land.Aug 31 2017, 10:32 AM
ted closed this revision.Aug 31 2017, 12:23 PM