[Lldb-commits] [lldb] 78b00c1 - Revert "[lldb] Improve maintainability and readability for ValueObject methods (#75865)"

Pavel Labath via lldb-commits lldb-commits at lists.llvm.org
Tue Jan 23 23:13:18 PST 2024


Author: Pavel Labath
Date: 2024-01-24T07:12:52Z
New Revision: 78b00c116be8b3b53ff13552e31eb305b11cb169

URL: https://github.com/llvm/llvm-project/commit/78b00c116be8b3b53ff13552e31eb305b11cb169
DIFF: https://github.com/llvm/llvm-project/commit/78b00c116be8b3b53ff13552e31eb305b11cb169.diff

LOG: Revert "[lldb] Improve maintainability and readability for ValueObject methods (#75865)"

This reverts commit d657519838e4b2310e13ec5ff52599e041860825 as it
breaks two dozen tests. The breakages are related to variable path
expression parsing and summary string parsing (possibly the same code).

Added: 
    

Modified: 
    lldb/source/Core/ValueObject.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index d58bf2ca763d9e2..9208047be36668d 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -1582,64 +1582,62 @@ bool ValueObject::IsUninitializedReference() {
 
 ValueObjectSP ValueObject::GetSyntheticArrayMember(size_t index,
                                                    bool can_create) {
-  if (!IsPointerType() && !IsArrayType())
-    return ValueObjectSP();
-
-  std::string index_str = llvm::formatv("[{0}]", index);
-  ConstString index_const_str(index_str);
-  // Check if we have already created a synthetic array member in this valid
-  // object. If we have we will re-use it.
-  if (auto existing_synthetic_child = GetSyntheticChild(index_const_str))
-    return existing_synthetic_child;
-
-  // We haven't made a synthetic array member for INDEX yet, so lets make
-  // one and cache it for any future reference.
-  ValueObject *synthetic_child = CreateChildAtIndex(0, true, index);
-
-  if (!synthetic_child)
-    return ValueObjectSP();
-
-  // Cache the synthetic child's value because it's valid.
-  AddSyntheticChild(index_const_str, synthetic_child);
-  auto synthetic_child_sp = synthetic_child->GetSP();
-  synthetic_child_sp->SetName(ConstString(index_str));
-  synthetic_child_sp->m_flags.m_is_array_item_for_pointer = true;
+  ValueObjectSP synthetic_child_sp;
+  if (IsPointerType() || IsArrayType()) {
+    std::string index_str = llvm::formatv("[{0}]", index);
+    ConstString index_const_str(index_str);
+    // Check if we have already created a synthetic array member in this valid
+    // object. If we have we will re-use it.
+    synthetic_child_sp = GetSyntheticChild(index_const_str);
+    if (!synthetic_child_sp) {
+      ValueObject *synthetic_child;
+      // We haven't made a synthetic array member for INDEX yet, so lets make
+      // one and cache it for any future reference.
+      synthetic_child = CreateChildAtIndex(0, true, index);
+
+      // Cache the value if we got one back...
+      if (synthetic_child) {
+        AddSyntheticChild(index_const_str, synthetic_child);
+        synthetic_child_sp = synthetic_child->GetSP();
+        synthetic_child_sp->SetName(ConstString(index_str));
+        synthetic_child_sp->m_flags.m_is_array_item_for_pointer = true;
+      }
+    }
+  }
   return synthetic_child_sp;
 }
 
 ValueObjectSP ValueObject::GetSyntheticBitFieldChild(uint32_t from, uint32_t to,
                                                      bool can_create) {
-  if (!IsScalarType())
-    return ValueObjectSP();
-
-  std::string index_str = llvm::formatv("[{0}-{1}]", from, to);
-  ConstString index_const_str(index_str);
-
-  // Check if we have already created a synthetic array member in this valid
-  // object. If we have we will re-use it.
-  if (auto existing_synthetic_child = GetSyntheticChild(index_const_str))
-    return existing_synthetic_child;
-
-  uint32_t bit_field_size = to - from + 1;
-  uint32_t bit_field_offset = from;
-  if (GetDataExtractor().GetByteOrder() == eByteOrderBig)
-    bit_field_offset =
-        GetByteSize().value_or(0) * 8 - bit_field_size - bit_field_offset;
-
-  // We haven't made a synthetic array member for INDEX yet, so lets make
-  // one and cache it for any future reference.
-  ValueObjectChild *synthetic_child = new ValueObjectChild(
-      *this, GetCompilerType(), index_const_str, GetByteSize().value_or(0), 0,
-      bit_field_size, bit_field_offset, false, false, eAddressTypeInvalid, 0);
-
-  if (!synthetic_child)
-    return ValueObjectSP();
-
-  // Cache the synthetic child's value because it's valid.
-  AddSyntheticChild(index_const_str, synthetic_child);
-  auto synthetic_child_sp = synthetic_child->GetSP();
-  synthetic_child_sp->SetName(ConstString(index_str));
-  synthetic_child_sp->m_flags.m_is_bitfield_for_scalar = true;
+  ValueObjectSP synthetic_child_sp;
+  if (IsScalarType()) {
+    std::string index_str = llvm::formatv("[{0}-{1}]", from, to);
+    ConstString index_const_str(index_str);
+    // Check if we have already created a synthetic array member in this valid
+    // object. If we have we will re-use it.
+    synthetic_child_sp = GetSyntheticChild(index_const_str);
+    if (!synthetic_child_sp) {
+      uint32_t bit_field_size = to - from + 1;
+      uint32_t bit_field_offset = from;
+      if (GetDataExtractor().GetByteOrder() == eByteOrderBig)
+        bit_field_offset =
+            GetByteSize().value_or(0) * 8 - bit_field_size - bit_field_offset;
+      // We haven't made a synthetic array member for INDEX yet, so lets make
+      // one and cache it for any future reference.
+      ValueObjectChild *synthetic_child = new ValueObjectChild(
+          *this, GetCompilerType(), index_const_str, GetByteSize().value_or(0),
+          0, bit_field_size, bit_field_offset, false, false,
+          eAddressTypeInvalid, 0);
+
+      // Cache the value if we got one back...
+      if (synthetic_child) {
+        AddSyntheticChild(index_const_str, synthetic_child);
+        synthetic_child_sp = synthetic_child->GetSP();
+        synthetic_child_sp->SetName(ConstString(index_str));
+        synthetic_child_sp->m_flags.m_is_bitfield_for_scalar = true;
+      }
+    }
+  }
   return synthetic_child_sp;
 }
 
@@ -1649,8 +1647,9 @@ ValueObjectSP ValueObject::GetSyntheticChildAtOffset(
 
   ValueObjectSP synthetic_child_sp;
 
-  if (name_const_str.IsEmpty())
+  if (name_const_str.IsEmpty()) {
     name_const_str.SetString("@" + std::to_string(offset));
+  }
 
   // Check if we have already created a synthetic array member in this valid
   // object. If we have we will re-use it.
@@ -1660,13 +1659,13 @@ ValueObjectSP ValueObject::GetSyntheticChildAtOffset(
     return synthetic_child_sp;
 
   if (!can_create)
-    return ValueObjectSP();
+    return {};
 
   ExecutionContext exe_ctx(GetExecutionContextRef());
   std::optional<uint64_t> size =
       type.GetByteSize(exe_ctx.GetBestExecutionContextScope());
   if (!size)
-    return ValueObjectSP();
+    return {};
   ValueObjectChild *synthetic_child =
       new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0,
                            false, false, eAddressTypeInvalid, 0);
@@ -1700,7 +1699,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset,
     return synthetic_child_sp;
 
   if (!can_create)
-    return ValueObjectSP();
+    return {};
 
   const bool is_base_class = true;
 
@@ -1708,7 +1707,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset,
   std::optional<uint64_t> size =
       type.GetByteSize(exe_ctx.GetBestExecutionContextScope());
   if (!size)
-    return ValueObjectSP();
+    return {};
   ValueObjectChild *synthetic_child =
       new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0,
                            is_base_class, false, eAddressTypeInvalid, 0);
@@ -1737,30 +1736,30 @@ static const char *SkipLeadingExpressionPathSeparators(const char *expression) {
 ValueObjectSP
 ValueObject::GetSyntheticExpressionPathChild(const char *expression,
                                              bool can_create) {
+  ValueObjectSP synthetic_child_sp;
   ConstString name_const_string(expression);
   // Check if we have already created a synthetic array member in this valid
   // object. If we have we will re-use it.
-  if (auto existing_synthetic_child = GetSyntheticChild(name_const_string))
-    return existing_synthetic_child;
-
-  // We haven't made a synthetic array member for expression yet, so lets
-  // make one and cache it for any future reference.
-  auto path_options = GetValueForExpressionPathOptions();
-  path_options.SetSyntheticChildrenTraversal(
-      GetValueForExpressionPathOptions::SyntheticChildrenTraversal::None);
-  auto synthetic_child =
-      GetValueForExpressionPath(expression, nullptr, nullptr, path_options);
-
-  if (!synthetic_child)
-    return ValueObjectSP();
-
-  // Cache the synthetic child's value because it's valid.
-  // FIXME: this causes a "real" child to end up with its name changed to
-  // the contents of expression
-  AddSyntheticChild(name_const_string, synthetic_child.get());
-  synthetic_child->SetName(
-      ConstString(SkipLeadingExpressionPathSeparators(expression)));
-  return synthetic_child;
+  synthetic_child_sp = GetSyntheticChild(name_const_string);
+  if (!synthetic_child_sp) {
+    // We haven't made a synthetic array member for expression yet, so lets
+    // make one and cache it for any future reference.
+    synthetic_child_sp = GetValueForExpressionPath(
+        expression, nullptr, nullptr,
+        GetValueForExpressionPathOptions().SetSyntheticChildrenTraversal(
+            GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
+                None));
+
+    // Cache the value if we got one back...
+    if (synthetic_child_sp.get()) {
+      // FIXME: this causes a "real" child to end up with its name changed to
+      // the contents of expression
+      AddSyntheticChild(name_const_string, synthetic_child_sp.get());
+      synthetic_child_sp->SetName(
+          ConstString(SkipLeadingExpressionPathSeparators(expression)));
+    }
+  }
+  return synthetic_child_sp;
 }
 
 void ValueObject::CalculateSyntheticValue() {
@@ -1957,55 +1956,66 @@ ValueObjectSP ValueObject::GetValueForExpressionPath(
     const GetValueForExpressionPathOptions &options,
     ExpressionPathAftermath *final_task_on_target) {
 
-  auto dummy_stop_reason = eExpressionPathScanEndReasonUnknown;
-  auto dummy_value_type = eExpressionPathEndResultTypeInvalid;
-  auto dummy_final_task = eExpressionPathAftermathNothing;
-
-  auto proxy_stop_reason = reason_to_stop ? reason_to_stop : &dummy_stop_reason;
-  auto proxy_value_type =
-      final_value_type ? final_value_type : &dummy_value_type;
-  auto proxy_final_task =
-      final_task_on_target ? final_task_on_target : &dummy_final_task;
-
-  auto ret_value = GetValueForExpressionPath_Impl(expression, proxy_stop_reason,
-                                                  proxy_value_type, options,
-                                                  proxy_final_task);
-
-  // The caller knows nothing happened if `final_task_on_target` doesn't change.
-  if (!ret_value || (*proxy_value_type) != eExpressionPathEndResultTypePlain ||
-      !final_task_on_target)
-    return ValueObjectSP();
-
-  ExpressionPathAftermath &final_task_on_target_ref = (*final_task_on_target);
-  ExpressionPathScanEndReason stop_reason_for_error;
-  Status error;
-  // The method can only dereference and take the address of plain objects.
-  switch (final_task_on_target_ref) {
-  case eExpressionPathAftermathNothing:
-    return ret_value;
-
-  case eExpressionPathAftermathDereference:
-    ret_value = ret_value->Dereference(error);
-    stop_reason_for_error = eExpressionPathScanEndReasonDereferencingFailed;
-    break;
-
-  case eExpressionPathAftermathTakeAddress:
-    ret_value = ret_value->AddressOf(error);
-    stop_reason_for_error = eExpressionPathScanEndReasonTakingAddressFailed;
-    break;
-  }
-
-  if (ret_value && error.Success()) {
-    final_task_on_target_ref = eExpressionPathAftermathNothing;
-    return ret_value;
+  ExpressionPathScanEndReason dummy_reason_to_stop =
+      ValueObject::eExpressionPathScanEndReasonUnknown;
+  ExpressionPathEndResultType dummy_final_value_type =
+      ValueObject::eExpressionPathEndResultTypeInvalid;
+  ExpressionPathAftermath dummy_final_task_on_target =
+      ValueObject::eExpressionPathAftermathNothing;
+
+  ValueObjectSP ret_val = GetValueForExpressionPath_Impl(
+      expression, reason_to_stop ? reason_to_stop : &dummy_reason_to_stop,
+      final_value_type ? final_value_type : &dummy_final_value_type, options,
+      final_task_on_target ? final_task_on_target
+                           : &dummy_final_task_on_target);
+
+  if (!final_task_on_target ||
+      *final_task_on_target == ValueObject::eExpressionPathAftermathNothing)
+    return ret_val;
+
+  if (ret_val.get() &&
+      ((final_value_type ? *final_value_type : dummy_final_value_type) ==
+       eExpressionPathEndResultTypePlain)) // I can only deref and takeaddress
+                                           // of plain objects
+  {
+    if ((final_task_on_target ? *final_task_on_target
+                              : dummy_final_task_on_target) ==
+        ValueObject::eExpressionPathAftermathDereference) {
+      Status error;
+      ValueObjectSP final_value = ret_val->Dereference(error);
+      if (error.Fail() || !final_value.get()) {
+        if (reason_to_stop)
+          *reason_to_stop =
+              ValueObject::eExpressionPathScanEndReasonDereferencingFailed;
+        if (final_value_type)
+          *final_value_type = ValueObject::eExpressionPathEndResultTypeInvalid;
+        return ValueObjectSP();
+      } else {
+        if (final_task_on_target)
+          *final_task_on_target = ValueObject::eExpressionPathAftermathNothing;
+        return final_value;
+      }
+    }
+    if (*final_task_on_target ==
+        ValueObject::eExpressionPathAftermathTakeAddress) {
+      Status error;
+      ValueObjectSP final_value = ret_val->AddressOf(error);
+      if (error.Fail() || !final_value.get()) {
+        if (reason_to_stop)
+          *reason_to_stop =
+              ValueObject::eExpressionPathScanEndReasonTakingAddressFailed;
+        if (final_value_type)
+          *final_value_type = ValueObject::eExpressionPathEndResultTypeInvalid;
+        return ValueObjectSP();
+      } else {
+        if (final_task_on_target)
+          *final_task_on_target = ValueObject::eExpressionPathAftermathNothing;
+        return final_value;
+      }
+    }
   }
-
-  if (reason_to_stop)
-    *reason_to_stop = stop_reason_for_error;
-
-  if (final_value_type)
-    *final_value_type = eExpressionPathEndResultTypeInvalid;
-  return ValueObjectSP();
+  return ret_val; // final_task_on_target will still have its original value, so
+                  // you know I did not do it
 }
 
 ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
@@ -2676,47 +2686,39 @@ ValueObjectSP ValueObject::AddressOf(Status &error) {
   const bool scalar_is_load_address = false;
   addr_t addr = GetAddressOf(scalar_is_load_address, &address_type);
   error.Clear();
+  if (addr != LLDB_INVALID_ADDRESS && address_type != eAddressTypeHost) {
+    switch (address_type) {
+    case eAddressTypeInvalid: {
+      StreamString expr_path_strm;
+      GetExpressionPath(expr_path_strm);
+      error.SetErrorStringWithFormat("'%s' is not in memory",
+                                     expr_path_strm.GetData());
+    } break;
 
-  StreamString expr_path_strm;
-  GetExpressionPath(expr_path_strm);
-  const char *expr_path_str = expr_path_strm.GetData();
-
-  ExecutionContext exe_ctx(GetExecutionContextRef());
-  auto scope = exe_ctx.GetBestExecutionContextScope();
-
-  if (addr == LLDB_INVALID_ADDRESS) {
+    case eAddressTypeFile:
+    case eAddressTypeLoad: {
+      CompilerType compiler_type = GetCompilerType();
+      if (compiler_type) {
+        std::string name(1, '&');
+        name.append(m_name.AsCString(""));
+        ExecutionContext exe_ctx(GetExecutionContextRef());
+        m_addr_of_valobj_sp = ValueObjectConstResult::Create(
+            exe_ctx.GetBestExecutionContextScope(),
+            compiler_type.GetPointerType(), ConstString(name.c_str()), addr,
+            eAddressTypeInvalid, m_data.GetAddressByteSize());
+      }
+    } break;
+    default:
+      break;
+    }
+  } else {
+    StreamString expr_path_strm;
+    GetExpressionPath(expr_path_strm);
     error.SetErrorStringWithFormat("'%s' doesn't have a valid address",
-                                   expr_path_str);
-    return ValueObjectSP();
+                                   expr_path_strm.GetData());
   }
 
-  switch (address_type) {
-  case eAddressTypeInvalid:
-    error.SetErrorStringWithFormat("'%s' is not in memory", expr_path_str);
-    return ValueObjectSP();
-
-  case eAddressTypeHost:
-    error.SetErrorStringWithFormat("'%s' is in host process (LLDB) memory",
-                                   expr_path_str);
-    return ValueObjectSP();
-
-  case eAddressTypeFile:
-  case eAddressTypeLoad: {
-    CompilerType compiler_type = GetCompilerType();
-    if (!compiler_type) {
-      error.SetErrorStringWithFormat("'%s' doesn't have a compiler type",
-                                     expr_path_str);
-      return ValueObjectSP();
-    }
-
-    std::string name(1, '&');
-    name.append(m_name.AsCString(""));
-    m_addr_of_valobj_sp = ValueObjectConstResult::Create(
-        scope, compiler_type.GetPointerType(), ConstString(name.c_str()), addr,
-        eAddressTypeInvalid, m_data.GetAddressByteSize());
-    return m_addr_of_valobj_sp;
-  }
-  }
+  return m_addr_of_valobj_sp;
 }
 
 ValueObjectSP ValueObject::DoCast(const CompilerType &compiler_type) {


        


More information about the lldb-commits mailing list