[lldb] Fix reading 32-bit signed integers

[igorkudrin]

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

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

Changes

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 --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 {

[igorkudrin]

Here is one possible manifestation of the problem, on an Arm32 Linux system:

$ cat test.cpp
class A {
public:
  A(int v) : value_a(v) {}
  virtual ~A() {}
private:
  int value_a;
};

class X {
public:
  X(int v) : value_x(v) {}
private:
  int value_x;
};

class B : public X, public virtual A {
public:
  B(int va, int vb, int vx) : X(vx), A(va), value_b(vb) {}
  virtual ~B() {}
private:
  int value_b;
};

void test(A *p, char *boom) {
  *boom = 43;
}

int main() {
  B b(5, 7, 9);
  test(&b, 0);
}
$ g++ test.cpp -o test -g -O0
$ ./test
Segmentation fault (core dumped)
$ lldb -c core
(lldb) var p -T -P1
(B *) p = 0xffe57164 {
  (X) X = {
    (int) value_x = <read memory from 0x1ffe57168 failed (0 of 4 bytes read)>

  }
  (A) A = {
    (int) value_a = <read memory from 0x1ffe57174 failed (0 of 4 bytes read)>

  }
  (int) value_b = <read memory from 0x1ffe5716c failed (0 of 4 bytes read)>

}

[DavidSpickett]

Overall looks good, the fixes are quite obvious in hindsight.

I know this seems redundant, but do we have coverage for 64-bit signed too?

Now would be a good time to add that. To show that reading 64-bit signed into int64_t gives -1, and reading 32-bit signed into int64_t also gives -1, because of the correctly applied sign extension.

Unlikely we'd ever break the 64-bit ones but it's cheap to check it here and it's a hint toward what the tests expect to see for 32-bit.

Your example is on Arm 32-bit but I think this applies to any target as long as the requested signed int is 32-bit. Did I understand correctly?

[igorkudrin]

I know this seems redundant, but do we have coverage for 64-bit signed too?

Now would be a good time to add that. To show that reading 64-bit signed into int64_t gives -1, and reading 32-bit signed into int64_t also gives -1, because of the correctly applied sign extension.

Unlikely we'd ever break the 64-bit ones but it's cheap to check it here and it's a hint toward what the tests expect to see for 32-bit.

I've added some basic tests for reading 64-bit values.

Your example is on Arm 32-bit but I think this applies to any target as long as the requested signed int is 32-bit. Did I understand correctly?

I've only checked on Arm32, but other 32-bit platforms are probably affected too.

[felipepiovezan]

David already did all the reviewing, this LGTM, but please let him give the final sign off.
Nice catch!

[DavidSpickett]

LGTM