[lldb] Correct address calculation for reading segment data

[GeorgeHuyubo]

This commit addresses a bug introduced in commit bcf654c, which prevented LLDB from parsing the GNU build ID for the main executable from a core file. The fix finds the p_vaddr of the first PT_LOAD segment as the base_addr and subtract this base_addr from the virtual address being read.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (GeorgeHuyubo)

Changes

Previous commit bcf654c
introduced a bug which makes lldb unable to parse gnu build id for the main executable from a core file.

This PR fix it by replace p_vaddr with p_offset.

Tested with core files with ELF headers off all modules. LLDB is able to parse the gnu build id for main executable as well as other shared libs.

---

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

1 Files Affected:

• (modified) lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp (+2-2)

diff --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
index b3916cc913f7db..cf3e056a20a049 100644
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -1047,9 +1047,9 @@ UUID ProcessElfCore::FindBuidIdInCoreMemory(lldb::addr_t address) {
     std::vector<uint8_t> note_bytes;
     note_bytes.resize(program_header.p_memsz);
 
-    // We need to slide the address of the p_vaddr as these values don't get
+    // We need to slide the address of the p_offset as these values don't get
     // relocated in memory.
-    const lldb::addr_t vaddr = program_header.p_vaddr + address;
+    const lldb::addr_t vaddr = program_header.p_offset + address;
     byte_read =
         ReadMemory(vaddr, note_bytes.data(), program_header.p_memsz, error);
     if (byte_read != program_header.p_memsz)

[labath]

I'm pretty sure this is not correct. p_offset is an offset in the file. It tells you nothing about how the file is mapped into memory.

What you're probably running into is an executable whose base address (p_vaddr of the first PT_LOAD segment) is not zero. This is true for all non-PIE executables, but it can be true for other kinds of files as well.

Since the value you get for address here is the actual address of the first load segment (rather than the delta between the virtual and actual addresses, which is used in some other places), what you need to do here is to subtract the address of the first segment from the result. This way you get zero for the first segment, and then a delta for all the others. Take a look at this code doing something similar.

[GeorgeHuyubo]

Updated the commit to address @labath's comment.

[labath]

Updated the commit to address @labath's comment.

Can you explain how you did that? The new implementation does something completely different (and wrong on several levels) from the one I pointed you to. Here is a description of elf loading from a linker POV that might be helpful.

[GeorgeHuyubo]

Updated the commit to address @labath's comment.

Can you explain how you did that? The new implementation does something completely different (and wrong on several levels) from the one I pointed you to. Here is a description of elf loading from a linker POV that might be helpful.

Hope I got this right this time. Now it's trying to find the p_vaddr of the first PT_LOAD segment as the base_addr.
And then subtract this base_addr from the vaddr I am trying to read from.

[labath]

Yes, that looks about right, though it's unfortunate that this does not have any test case. I would really recommend you figure out a testing strategy for this -- for your benefit (less chance of other changes breaking this use case) if nothing else.

Be sure to update the commit message before committing this.