[lldb][core] Fix getting summary of a variable pointing to r/o memory

[igorkudrin]

Motivation example:

> lldb -c altmain2.core
...
(lldb) var F
(const char *) F = 0x0804a000 ""

The variable F points to a read-only memory page not dumped to the core file, so Process::ReadMemory() cannot read the data. The patch switches to Target::ReadMemory(), which can read data both from the process memory and the application binary.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

Changes

Motivation example:

> lldb -c altmain2.core
...
(lldb) var F
(const char *) F = 0x0804a000 ""

The variable F points to a read-only memory page not dumped to the core file, so Process::ReadMemory() cannot read the data. The patch switches to Target::ReadMemory(), which can read data both from the process memory and the application binary.

---

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

4 Files Affected:

• (modified) lldb/source/ValueObject/ValueObject.cpp (+12-1)
• (modified) lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py (+27)
• (added) lldb/test/API/functionalities/postmortem/elf-core/altmain2.core ()
• (added) lldb/test/API/functionalities/postmortem/elf-core/altmain2.out ()

diff --git a/lldb/source/ValueObject/ValueObject.cpp b/lldb/source/ValueObject/ValueObject.cpp
index e1c66763ff0b8..aab78428d9103 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -735,7 +735,7 @@ size_t ValueObject::GetPointeeData(DataExtractor &data, uint32_t item_idx,
     case eAddressTypeLoad: {
       ExecutionContext exe_ctx(GetExecutionContextRef());
       Process *process = exe_ctx.GetProcessPtr();
-      if (process) {
+      if (process && process->IsLiveDebugSession()) {
         heap_buf_ptr->SetByteSize(bytes);
         size_t bytes_read = process->ReadMemory(
             addr + offset, heap_buf_ptr->GetBytes(), bytes, error);
@@ -743,6 +743,17 @@ size_t ValueObject::GetPointeeData(DataExtractor &data, uint32_t item_idx,
           data.SetData(data_sp);
           return bytes_read;
         }
+      } else if (Target *target = exe_ctx.GetTargetPtr()) {
+        Address target_addr;
+        target_addr.SetLoadAddress(addr + offset, target);
+        heap_buf_ptr->SetByteSize(bytes);
+        size_t bytes_read =
+            target->ReadMemory(target_addr, heap_buf_ptr->GetBytes(), bytes,
+                               error, /*force_live_memory=*/true);
+        if (error.Success() || bytes_read > 0) {
+          data.SetData(data_sp);
+          return bytes_read;
+        }
       }
     } break;
     case eAddressTypeHost: {
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py b/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
index a287fd19ba352..d1e065a32efdc 100644
--- a/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ b/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -977,6 +977,33 @@ def test_get_core_file_api(self):
         self.assertEqual(process.GetCoreFile().GetFilename(), core_file_name)
         self.dbg.DeleteTarget(target)
 
+    @skipIfLLVMTargetMissing("X86")
+    def test_ro_cstring(self):
+        """
+        Test that we can show the summary for a cstring variable that points
+        to a r/o memory page which is not dumped to a core file.
+        """
+        target = self.dbg.CreateTarget("altmain2.out")
+        process = target.LoadCore("altmain2.core")
+        self.assertTrue(process, PROCESS_IS_VALID)
+
+        frame = process.GetSelectedThread().GetFrameAtIndex(0)
+        self.assertEqual(frame.GetFunctionName(), "_start")
+
+        var = frame.FindVariable("F")
+
+        # The variable points to a RO segment that is not dumped to the core
+        # file and thus process.ReadCStringFromMemory() cannot get the value.
+        error = lldb.SBError()
+        cstr = process.ReadCStringFromMemory(var.GetValueAsUnsigned(), 256, error)
+        self.assertFailure(error, error_str="core file does not contain 0x804a000")
+        self.assertEqual(cstr, "")
+
+        # Nevertheless, when getting the summary, the value can be read from the
+        # application binary.
+        cstr = var.GetSummary()
+        self.assertEqual(cstr, '"_start"')
+
     def check_memory_regions(self, process, region_count):
         region_list = process.GetMemoryRegions()
         self.assertEqual(region_list.GetSize(), region_count)
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/altmain2.core b/lldb/test/API/functionalities/postmortem/elf-core/altmain2.core
new file mode 100755
index 0000000000000..b9dd8de08b813
Binary files /dev/null and b/lldb/test/API/functionalities/postmortem/elf-core/altmain2.core differ
diff --git a/lldb/test/API/functionalities/postmortem/elf-core/altmain2.out b/lldb/test/API/functionalities/postmortem/elf-core/altmain2.out
new file mode 100755
index 0000000000000..e930c3d8055e1
Binary files /dev/null and b/lldb/test/API/functionalities/postmortem/elf-core/altmain2.out differ

[DavidSpickett]

Surprised no one found this sooner, thanks for contributing.

[DavidSpickett]

Could already checked in core files be used for this test? I assume not but please confirm. We like to avoid checked in binaries if we can.

I think we need the core and the program file because the program file tells us the address of F, correct?

[DavidSpickett]

And these were made using source code that's already checked in I assume?

[igorkudrin]

Yes, the program file contains the debug information and the read-only section with the data, so both the core and program files are required. And there are no core files with the same properties already committed. The program is built from the existing altmain.c.