[Lldb-commits] [PATCH] D120319: Set error message if ValueObjectRegister fails to write back to register

[Ilya Nozhkin via Phabricator via lldb-commits]

ilya-nozhkin created this revision.
ilya-nozhkin added reviewers: labath, clayborg.
Herald added a subscriber: pengfei.
ilya-nozhkin requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

`SetValueFromCString` and `SetData` methods return false if register can't
be written but they don't set a error message. It sometimes confuses callers of
these methods because they try to get the error message in case of failure but
`Status::AsCString` returns `nullptr`.

For example, `lldb-vscode` crashes due to this bug if some register can't be
written. It invokes `SBError::GetCString` in case of error and doesn't check
whether the result is `nullptr` (see `request_setVariable` implementation in
`lldb-vscode.cpp` for more info).

I couldn't write any test for this issue on x86 architecture because all x86
registers are writable. It might be possible to add such test for some other
testable architecture but I don't know much about them and I don't have a
machine with any architecture other than x86 to actually run the test.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120319

Files:
  lldb/source/Core/ValueObjectRegister.cpp


Index: lldb/source/Core/ValueObjectRegister.cpp
===================================================================
--- lldb/source/Core/ValueObjectRegister.cpp
+++ lldb/source/Core/ValueObjectRegister.cpp
@@ -269,26 +269,30 @@
   // The new value will be in the m_data.  Copy that into our register value.
   error =
       m_reg_value.SetValueFromString(&m_reg_info, llvm::StringRef(value_str));
-  if (error.Success()) {
-    if (m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
-      SetNeedsUpdate();
-      return true;
-    } else
-      return false;
-  } else
+  if (!error.Success())
     return false;
+
+  if (!m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
+    error.SetErrorString("unable to write back to register");
+    return false;
+  }
+
+  SetNeedsUpdate();
+  return true;
 }
 
 bool ValueObjectRegister::SetData(DataExtractor &data, Status &error) {
   error = m_reg_value.SetValueFromData(&m_reg_info, data, 0, false);
-  if (error.Success()) {
-    if (m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
-      SetNeedsUpdate();
-      return true;
-    } else
-      return false;
-  } else
+  if (!error.Success())
     return false;
+
+  if (!m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
+    error.SetErrorString("unable to write back to register");
+    return false;
+  }
+
+  SetNeedsUpdate();
+  return true;
 }
 
 bool ValueObjectRegister::ResolveValue(Scalar &scalar) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120319.410491.patch
Type: text/x-patch
Size: 1443 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20220222/65d803aa/attachment.bin>