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

[ted]

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.

[abidh]

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]

Added check for reference types, as Greg suggested.

Simplified change.

[clayborg]

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

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

[clayborg]

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]

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]

Makes sense even though this can be quite expensive.

tools/lldb-mi/MICmdCmdVar.cpp

 //--
 bool CMICmdCmdVarUpdate::ExamineSBValueForChange(lldb::SBValue &vrwValue,
                                                  bool &vrwbChanged) {
   if (vrwValue.GetValueDidChange()) {
     vrwbChanged = true;
     return MIstatus::success;
   }
 
-  lldb::SBType valueType = vrwValue.GetType();
-
   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)
+    // skip pointers and references to avoid infinite loop
+    if (member.GetType().GetTypeFlags() &
+        (lldb::eTypeIsPointer | lldb::eTypeIsReference))
+      continue;
+
+    if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
       return MIstatus::success;
   }
   vrwbChanged = false;
   return MIstatus::success;
 }
 
 //---------------------------------------------------------------------------------------
 //---------------------------------------------------------------------------------------
 //---------------------------------------------------------------------------------------
 
 //++
 //------------------------------------------------------------------------------------
 // Details: CMICmdCmdVarDelete constructor.
 // Type:    Method.