Set error message if ValueObjectRegister fails to write back to register

[ilya-nozhkin]

Summary

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).

[labath]

Core files have read-only registers. Would that help? (you could just throw in a SetValueFromCString call into one of the existing core file tests)

[clayborg]

In D120319#3340298, @labath wrote:

Core files have read-only registers. Would that help? (you could just throw in a SetValueFromCString call into one of the existing core file tests)

Actually I believe we allow writing registers for core files. This was requested so that people could fix the registers like SP or FP and the get a real backtrace. Since core files contain crashes, people wanted to be able to make sure they could get a backtrace.

[clayborg]

Code looks good. Lets see if we can test it with a core file. I am guessing we can't from my previous comment, but it would be worth checking

[ilya-nozhkin]

Added a test based on core files debugging. RegisterContextCorePOSIX_x86_64 indeed forbids writing registers.

[labath]

So, it seems we were all correct. The mach-o core file implementation does permit writing registers, while the elf one doesn't. I guess we never got that feature request for elf.

I don't think that's a blocker for writing this test though. If we decide to support writing registers in for elf too, we can always change/remove the test.

Another possibility is to use the gdb-client test infrastructure to simulate a server which refuses to write to a register, but those tests are slightly tricky to write, so I don't think that's worth it here (unless you're interested in getting to know that infra, that is).

lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py

151–152

I don't think there's a valid reason for why should this ever fail.

[ilya-nozhkin]

Replaced redundant check in the test with assert. Also, I've removed type annotations to be consistent with the code around.

[ilya-nozhkin]

In D120319#3342657, @labath wrote:

Another possibility is to use the gdb-client test infrastructure to simulate a server which refuses to write to a register

It would be the most proper way, I agree, but I think that it is an overkill for such a simple case.

[clayborg]

Great, thanks for adding the test.

[ilya-nozhkin]

Could someone, please, commit these changes to the main repository? I still don't have commit access. I'll apply for it right after merging these changes (LLVM Developer Policy states that it'd be better to have several patches already submitted before applying for commit access).

[ChuanqiXu]

In D120319#3348265, @ilya-nozhkin wrote:

Could someone, please, commit these changes to the main repository? I still don't have commit access. I'll apply for it right after merging these changes (LLVM Developer Policy states that it'd be better to have several patches already submitted before applying for commit access).

What's your name and email address for this patch? I'd love to commit it.

[ilya-nozhkin]

In D120319#3348267, @ChuanqiXu wrote:

What's your name and email address for this patch?

Ilya Nozhkin
[e-mail]

[ChuanqiXu]

In D120319#3348268, @ilya-nozhkin wrote:

In D120319#3348267, @ChuanqiXu wrote:

What's your name and email address for this patch?

Ilya Nozhkin
[e-mail]

Done~

lldb/source/Core/ValueObjectRegister.cpp

bool ValueObjectRegister::UpdateValue() {
   return false;
 }
 
 bool ValueObjectRegister::SetValueFromCString(const char *value_str,
                                               Status &error) {
   // 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())
+
+  if (!m_reg_ctx_sp->WriteRegister(&m_reg_info, m_reg_value)) {
+    error.SetErrorString("unable to write back to register");
     return false;
   }
 
-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
+}
+
+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)) {
+    error.SetErrorString("unable to write back to register");
     return false;
   }
 
+  SetNeedsUpdate();
+  return true;
+}
+
 bool ValueObjectRegister::ResolveValue(Scalar &scalar) {
   if (UpdateValueIfNeeded(
           false)) // make sure that you are up to date before returning anything
     return m_reg_value.GetScalarValue(scalar);
   return false;
 }
 
 void ValueObjectRegister::GetExpressionPath(Stream &s,
                                             GetExpressionPathFormat epformat) {
   s.Printf("$%s", m_reg_info.name);
 }

lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py

def test_read_memory(self):
         error = lldb.SBError()
         bytesread = process.ReadMemory(0x400ff0, 20, error)
         
         # read only 16 bytes without zero bytes filling
         self.assertEqual(len(bytesread), 16)
         self.dbg.DeleteTarget(target)
 
     @skipIfLLVMTargetMissing("X86")
+    def test_write_register(self):
+        """Test that writing to register results in an error and that error
+           message is set."""
+        target = self.dbg.CreateTarget("linux-x86_64.out")
+        process = target.LoadCore("linux-x86_64.core")
+        self.assertTrue(process, PROCESS_IS_VALID)
+
+        thread = process.GetSelectedThread()
+        self.assertTrue(thread)
+
+        frame = thread.GetSelectedFrame()
+        self.assertTrue(frame)
+
+        reg_value = frame.FindRegister('eax')
+        self.assertTrue(reg_value)
+
+        error = lldb.SBError()
+        success = reg_value.SetValueFromCString('10', error)
+        self.assertFalse(success)
+        self.assertTrue(error.Fail())
+        self.assertIsNotNone(error.GetCString())
+
+    @skipIfLLVMTargetMissing("X86")
     def test_FPR_SSE(self):
         # check x86_64 core file
         target = self.dbg.CreateTarget(None)
         self.assertTrue(target, VALID_TARGET)
         process = target.LoadCore("linux-fpr_sse_x86_64.core")
 
         values = {}
         values["fctrl"] = "0x037f"