[elf-core] Improve reading memory from core file

[djtodoro]

Summary

This patch tries to improve memory-read from core files (in order to improve disassembly functionality).
I am using RHEL 7.7 (linux kernel 3.10) and for a lot of cases, I was not able to disassemble some functions from backtrace when debugging crashes from core files. It outputs some dummy code as following:

(lldb) disassemble
example`fn:
    0x55deb51866b0 <+0>:   addb   %al, (%rax)
    0x55deb51866b2 <+2>:   addb   %al, (%rax)
    0x55deb51866b4 <+4>:   addb   %al, (%rax)
    0x55deb51866b6 <+6>:   addb   %al, (%rax)
    ....

The cause of the problem was the fact we are returning all the zeros from ProcessElfCore::ReadMemory() that is being called within Disassembler::ParseInstructions() and it disassembles some dummy opcodes from the buffer returned. If we are about to fill the buffer with all zeros, I guess it is safe to just return zero, and to proceed with reading from file itself. Therefore, we are removing zero bytes filling (padding) completely.

Before this patch (simple example that has been attached into the test):

$ lldb lldb/test/API/functionalities/postmortem/elf-core/linux-x86_64-for-disassemble.out -c lldb/test/API/functionalities/postmortem/elf-core/linux-x86_64-for-disassemble.core
(lldb) f 4
frame #4: 0x00007f1d2f862545 libc.so.6`__libc_start_main + 245
libc.so.6`__libc_start_main:
->  0x7f1d2f862545 <+245>: addb   %al, (%rax)
    0x7f1d2f862547 <+247>: addb   %al, (%rax)
    0x7f1d2f862549 <+249>: addb   %al, (%rax)
    0x7f1d2f86254b <+251>: addb   %al, (%rax)

After this patch applied:

(lldb) f 4
frame #4: 0x00007f1d2f862545 libc.so.6`__libc_start_main + 245
libc.so.6`__libc_start_main:
->  0x7f1d2f862545 <+245>: movl   %eax, %edi
    0x7f1d2f862547 <+247>: callq  0x39d10                   ; exit
    0x7f1d2f86254c <+252>: xorl   %edx, %edx
    0x7f1d2f86254e <+254>: jmp    0x22489                   ; <+57>

GDB was able to disassemble all the functions from core file.

[labath]

Have you by any chance learned why are we zero-filling this buffer in the first place? Seems like an odd thing to do... Maybe we should just stop zero-filling completely?

[djtodoro]

(@labath Sorry for late response, I've been away from keyboard for some time.)

Have you by any chance learned why are we zero-filling this buffer in the first place? Seems like an odd thing to do... Maybe we should just stop zero-filling completely?

Hmm... not sure why. It was there since the original commit that introduced the file (c037383aff81a61ed956858353ec003e970fb2ce). I don't see the point actually, and when I removed the zero_fill_size and zero filling with memset(), there was no regression in the lldb test suite. I guess we can remove it completely.

[djtodoro]

nfc

[labath]

The fact that the test suite passes is not too surprising since we have very few core file tests. However, I agree that we should just remove this zero filling completely.

Regarding the test, would it be possible to reuse one of the existing core files? (The reason we have so few core tests is because we used to not allow checked in core files at all -- now we kinda do, but it's still not ideal.) I'm guessing you don't even need to disassemble a function to reproduce this -- it should be sufficient to run disassemble --start-address X --end-address Y, where the region X--Y crosses a core segment boundary..

[djtodoro]

Regarding the test, would it be possible to reuse one of the existing core files? (The reason we have so few core tests is because we used to not allow checked in core files at all -- now we kinda do, but it's still not ideal.) I'm guessing you don't even need to disassemble a function to reproduce this -- it should be sufficient to run disassemble --start-address X --end-address Y, where the region X--Y crosses a core segment boundary..

We can disassemble a region that crosses core segment boundary, e.g.:
(let's use lldb/test/API/functionalities/postmortem/elf-core/linux-x86_64.core)

(lldb) disassemble --start 0x400161 --end 0x40100c
  0x400161: addb   %al, (%rax)
  0x400163: addb   %al, (%rax)
  0x400165: addb   %al, (%rax)
  0x400167: addb   %dl, (%rax,%rax)
  0x40016a: addb   %al, (%rax)
  0x40016c: addb   %al, (%rax)
  0x40016e: addb   %al, (%rax)
  0x400170: addl   %edi, 0x52(%rdx)
  0x400173: addb   %al, (%rcx)
  0x400175: js     0x400187
  0x400177: addl   %ebx, (%rbx)
  0x400179: orb    $0x7, %al
  0x40017b: orb    %dl, 0x1c000001(%rax)
  0x400181: addb   %al, (%rax)
  0x400183: addb   %bl, (%rax,%rax)
  0x400186: addb   %al, (%rax)
  0x400188: testb  %bh, %bh

and this triggers the code for zero-bytes-filling. After applying this patch, the output of the command is exactly the same.
I am not sure what should we check in the test if we use this existing core-file, any idea?

[labath]

I think that's because lldb's dissassembler currently just stops when it encounters an unknown/invalid instruction :(, so it doesn't even get to the interesting part. If I skip over the random bytes I get:

(lldb) disassemble --start 0x400ff0 --end 0x40100c
    0x400ff0: addb   %al, (%rax)
    0x400ff2: addb   %al, (%rax)
    0x400ff4: addb   %al, (%rax)
    0x400ff6: addb   %al, (%rax)
    0x400ff8: addb   %al, (%rax)
    0x400ffa: addb   %al, (%rax)
    0x400ffc: addb   %al, (%rax)
    0x400ffe: addb   %al, (%rax)
    0x401000: addb   %al, (%rax)
    0x401002: addb   %al, (%rax)
    0x401004: addb   %al, (%rax)
    0x401006: addb   %al, (%rax)
    0x401008: addb   %al, (%rax)
    0x40100a: addb   %al, (%rax)

With your patch I guess this would stop at 0x400ffe.

Another option would be to ditch disassembling, and check this via memory reads, as that is what you are actually fixing:

(lldb) memory read 0x400ff0 -c 20
0x00400ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
0x00401000: 00 00 00 00                                      ....

[djtodoro]

Another option would be to ditch disassembling, and check this via memory reads, as that is what you are actually fixing

+1, nice! thanks!

[djtodoro]

• Test update

[labath]

cool

[djtodoro]

@labath Thanks for the review! I'll land this tomorrow.

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

size_t ProcessElfCore::DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   }
 
   // Convert the address into core file offset
   const lldb::addr_t offset = addr - address_range->GetRangeBase();
   const lldb::addr_t file_start = address_range->data.GetRangeBase();
   const lldb::addr_t file_end = address_range->data.GetRangeEnd();
   size_t bytes_to_read = size; // Number of bytes to read from the core file
   size_t bytes_copied = 0;   // Number of bytes actually read from the core file
-  size_t zero_fill_size = 0; // Padding
   lldb::addr_t bytes_left =
       0; // Number of bytes available in the core file from the given address
 
   // Don't proceed if core file doesn't contain the actual data for this
   // address range.
   if (file_start == file_end)
     return 0;
 
   // Figure out how many on-disk bytes remain in this segment starting at the
   // given offset
   if (file_end > file_start + offset)
     bytes_left = file_end - (file_start + offset);
 
-  // Figure out how many bytes we need to zero-fill if we are reading more
-  // bytes than available in the on-disk segment
-  if (bytes_to_read > bytes_left) {
-    zero_fill_size = bytes_to_read - bytes_left;
+  if (bytes_to_read > bytes_left)
     bytes_to_read = bytes_left;
-  }
 
   // If there is data available on the core file read it
   if (bytes_to_read)
     bytes_copied =
         core_objfile->CopyData(offset + file_start, bytes_to_read, buf);
 
-  assert(zero_fill_size <= size);
-  // Pad remaining bytes
-  if (zero_fill_size)
-    memset(((char *)buf) + bytes_copied, 0, zero_fill_size);
-
-  return bytes_copied + zero_fill_size;
+  return bytes_copied;
 }
 
 void ProcessElfCore::Clear() {
   m_thread_list.Clear();
 
   SetUnixSignals(std::make_shared<UnixSignals>());
 }
 

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

def test_two_cores_same_pid(self):
         self.assertTrue(error.Success())
         self.assertEqual(F, "_start")
 
         # without destroying this process, run the test which opens another core file with the
         # same pid
         self.do_test("linux-x86_64", self._x86_64_pid, self._x86_64_regions,
                      "a.out")
 
+
+    @skipIf(triple='^mips')
+    @skipIfLLVMTargetMissing("X86")
+    @skipIfWindows
+    @skipIfReproducer
+    def test_read_memory(self):
+        """Test that we are able to read as many bytes as available"""
+        target = self.dbg.CreateTarget("linux-x86_64.out")
+        process = target.LoadCore("linux-x86_64.core")
+        self.assertTrue(process, PROCESS_IS_VALID)
+
+        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)
+
     @skipIf(triple='^mips')
     @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")