[Lldb-commits] [lldb] [lldb] Fix reading 32-bit signed integers (PR #169150)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

<details>
<summary>Changes</summary>

Both `Target::ReadSignedIntegerFromMemory()` and
`Process::ReadSignedIntegerFromMemory()` internally created an unsigned scalar, so extending the value later did not duplicate the sign bit.

---
Full diff: https://github.com/llvm/llvm-project/pull/169150.diff


3 Files Affected:

- (modified) lldb/source/Target/Process.cpp (+3-1) 
- (modified) lldb/source/Target/Target.cpp (+4-2) 
- (modified) lldb/unittests/Target/MemoryTest.cpp (+61-1) 


``````````diff
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 69edea503002e..5879b8f4795ab 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2452,8 +2452,10 @@ size_t Process::ReadScalarIntegerFromMemory(addr_t addr, uint32_t byte_size,
         scalar = data.GetMaxU32(&offset, byte_size);
       else
         scalar = data.GetMaxU64(&offset, byte_size);
-      if (is_signed)
+      if (is_signed) {
+        scalar.MakeSigned();
         scalar.SignExtend(byte_size * 8);
+      }
       return bytes_read;
     }
   } else {
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 6fc8053038d10..a06626e4e3044 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2282,8 +2282,10 @@ size_t Target::ReadScalarIntegerFromMemory(const Address &addr, uint32_t byte_si
       else
         scalar = data.GetMaxU64(&offset, byte_size);
 
-      if (is_signed)
+      if (is_signed) {
+        scalar.MakeSigned();
         scalar.SignExtend(byte_size * 8);
+      }
       return bytes_read;
     }
   } else {
@@ -2298,7 +2300,7 @@ int64_t Target::ReadSignedIntegerFromMemory(const Address &addr,
                                             int64_t fail_value, Status &error,
                                             bool force_live_memory) {
   Scalar scalar;
-  if (ReadScalarIntegerFromMemory(addr, integer_byte_size, false, scalar, error,
+  if (ReadScalarIntegerFromMemory(addr, integer_byte_size, true, scalar, error,
                                   force_live_memory))
     return scalar.SLongLong(fail_value);
   return fail_value;
diff --git a/lldb/unittests/Target/MemoryTest.cpp b/lldb/unittests/Target/MemoryTest.cpp
index e444f68dc4871..c095e7c5753f0 100644
--- a/lldb/unittests/Target/MemoryTest.cpp
+++ b/lldb/unittests/Target/MemoryTest.cpp
@@ -61,7 +61,7 @@ class DummyProcess : public Process {
       m_bytes_left -= size;
     }
 
-    memset(buf, 'B', num_bytes_to_write);
+    memset(buf, m_filler, num_bytes_to_write);
     return num_bytes_to_write;
   }
   bool DoUpdateThreadList(ThreadList &old_thread_list,
@@ -72,8 +72,11 @@ class DummyProcess : public Process {
 
   // Test-specific additions
   size_t m_bytes_left;
+  int m_filler = 'B';
   MemoryCache &GetMemoryCache() { return m_memory_cache; }
   void SetMaxReadSize(size_t size) { m_bytes_left = size; }
+  void SetFiller(int filler) { m_filler = filler; }
+  using Process::SetPrivateState;
 };
 } // namespace
 
@@ -85,6 +88,13 @@ TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
   return target_sp;
 }
 
+static void OverrideTargetProcess(Target *target, lldb::ProcessSP process) {
+  struct TargetHack : public Target {
+    void SetProcess(lldb::ProcessSP process) { m_process_sp = process; }
+  };
+  static_cast<TargetHack *>(target)->SetProcess(process);
+}
+
 TEST_F(MemoryTest, TesetMemoryCacheRead) {
   ArchSpec arch("x86_64-apple-macosx-");
 
@@ -227,6 +237,56 @@ TEST_F(MemoryTest, TesetMemoryCacheRead) {
                                                        // old cache
 }
 
+TEST_F(MemoryTest, TestProcessReadSignedInteger) {
+  ArchSpec arch("x86_64-apple-macosx-");
+
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast<DummyProcess *>(process_sp.get());
+  process->SetFiller(0xff);
+  process->SetMaxReadSize(4);
+
+  Status error;
+  int64_t val = process->ReadSignedIntegerFromMemory(0, 4, 0, error);
+  EXPECT_EQ(val, -1);
+}
+
+TEST_F(MemoryTest, TestTargetReadSignedInteger) {
+  ArchSpec arch("x86_64-apple-macosx-");
+
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+  OverrideTargetProcess(target_sp.get(), process_sp);
+
+  DummyProcess *process = static_cast<DummyProcess *>(process_sp.get());
+  process->SetFiller(0xff);
+  process->SetMaxReadSize(4);
+  process->SetPrivateState(eStateStopped);
+
+  Status error;
+  int64_t val = target_sp->ReadSignedIntegerFromMemory(Address(0), 4, 0, error);
+  EXPECT_EQ(val, -1);
+}
+
 /// A process class that, when asked to read memory from some address X, returns
 /// the least significant byte of X.
 class DummyReaderProcess : public Process {

``````````

</details>


https://github.com/llvm/llvm-project/pull/169150