Fix error handling after [<index>] in 'frame variable'

[jarin]

Summary

This fixes a bug where

frame var a[0]+5

returns the value a[0] without any warning because the current logic simply ignores everything after ']' as long as there is no '.', '-' or '[' in the rest of the string.

The fix simplifies the termination condition of the expression path parsing loop to check if have a non-empty remaining string to parse. Previously, the condition checked if a separator was found. That condition coincided with the remaining string-to-parse condition except for the buggy indexed case where non-empty string was left ("+5" in the example above), but the separator index was 'npos'.

[labath]

Makes sense to me.

[jarin]

Do you think Raphael would want to review this as well? If you think it is not necessary, could you land the patch for me?

[teemperor]

I don't think I know this code better than Pavel, but this also LGTM.

lldb/source/Target/StackFrame.cpp

ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
   }
   if (!valobj_sp) {
     error.SetErrorStringWithFormat("no variable named '%s' found in this frame",
                                    name_const_string.GetCString());
     return ValueObjectSP();
   }
 
   // We are dumping at least one child
-  while (separator_idx != std::string::npos) {
+  while (!var_expr.empty()) {
     // Calculate the next separator index ahead of time
     ValueObjectSP child_valobj_sp;
     const char separator_type = var_expr[0];
     bool expr_is_ptr = false;
     switch (separator_type) {
     case '-':
       expr_is_ptr = true;
       if (var_expr.size() >= 2 && var_expr[1] != '>')
case '[': {
           }
         }
 
         if (!child_valobj_sp) {
           // Invalid array index...
           return ValueObjectSP();
         }
 
-        separator_idx = var_expr.find_first_of(".-[");
         if (use_dynamic != eNoDynamicValues) {
           ValueObjectSP dynamic_value_sp(
               child_valobj_sp->GetDynamicValue(use_dynamic));
           if (dynamic_value_sp)
             child_valobj_sp = dynamic_value_sp;
         }
         // Break out early from the switch since we were able to find the child
         // member
case '[': {
             var_expr_path_strm.GetData());
       }
 
       if (!child_valobj_sp) {
         // Invalid bitfield range...
         return ValueObjectSP();
       }
 
-      separator_idx = var_expr.find_first_of(".-[");
       if (use_dynamic != eNoDynamicValues) {
         ValueObjectSP dynamic_value_sp(
             child_valobj_sp->GetDynamicValue(use_dynamic));
         if (dynamic_value_sp)
           child_valobj_sp = dynamic_value_sp;
       }
       // Break out early from the switch since we were able to find the child
       // member
default:
             var_expr.str().c_str());
 
         return ValueObjectSP();
       }
     }
 
     if (child_valobj_sp)
       valobj_sp = child_valobj_sp;
-
-    if (var_expr.empty())
-      break;
   }
   if (valobj_sp) {
     if (deref) {
       ValueObjectSP deref_valobj_sp(valobj_sp->Dereference(error));
       valobj_sp = deref_valobj_sp;
     } else if (address_of) {
       ValueObjectSP address_of_valobj_sp(valobj_sp->AddressOf(error));
       valobj_sp = address_of_valobj_sp;

lldb/test/API/functionalities/var_path/TestVarPath.py

class TestVarPath(TestBase):
 
     def test_frame_var(self):
         self.build()
         self.do_test()
 
     def verify_point(self, frame, var_name, var_typename, x_value, y_value):
         v = frame.GetValueForVariablePath(var_name)
         self.assertTrue(v.GetError().Success(), "Make sure we find '%s'" % (var_name))
-        self.assertEquals(v.GetType().GetName(), var_typename, 
+        self.assertEquals(v.GetType().GetName(), var_typename,
                         "Make sure '%s' has type '%s'" % (var_name, var_typename))
 
         if '*' in var_typename:
             valid_prefix = var_name + '->'
             invalid_prefix = var_name + '.'
         else:
             valid_prefix = var_name + '.'
             invalid_prefix = var_name + '->'
def do_test(self):
         # Test a pointer
         self.verify_point(frame, 'pt_ptr', 'Point *', 3030, 4040)
         # Test using a pointer as an array
         self.verify_point(frame, 'pt_ptr[-1]', 'Point', 1010, 2020)
         self.verify_point(frame, 'pt_ptr[0]', 'Point', 3030, 4040)
         self.verify_point(frame, 'pt_ptr[1]', 'Point', 5050, 6060)
         # Test arrays
         v = frame.GetValueForVariablePath('points')
-        self.assertTrue(v.GetError().Success(), 
+        self.assertTrue(v.GetError().Success(),
                         "Make sure we find 'points'")
         self.verify_point(frame, 'points[0]', 'Point', 1010, 2020)
         self.verify_point(frame, 'points[1]', 'Point', 3030, 4040)
         self.verify_point(frame, 'points[2]', 'Point', 5050, 6060)
+        v = frame.GetValueForVariablePath('points[0]+5')
+        self.assertTrue(v.GetError().Fail(),
+                        "Make sure we do not ignore characters between ']' and the end")
         # Test a reference
         self.verify_point(frame, 'pt_ref', 'Point &', 1, 2)
         v = frame.GetValueForVariablePath('pt_sp')
         self.assertTrue(v.GetError().Success(), "Make sure we find 'pt_sp'")
         # Make sure we don't crash when looking for non existant child
         # in type with synthetic children. This used to cause a crash.
         v = frame.GetValueForVariablePath('pt_sp->not_valid_child')
-        self.assertTrue(v.GetError().Fail(), 
+        self.assertTrue(v.GetError().Fail(),
                         "Make sure we don't find 'pt_sp->not_valid_child'")