[lldb][ObjectFile] Relocate sections for in-memory objects (e.g. received via JITLoaderGDB)

[sgraenitz]

Summary

Part 2 of a fix for JITed code debugging. This has been a regression from 5.0 to 6.0 and it's still reproducible on current master: https://bugs.llvm.org/show_bug.cgi?id=36209 Part 1 was D61611 a while ago.

The in-memory object files we obtain from JITLoaderGDB are not yet relocated. It looks like this used to happen on the LLDB side and my guess is that it broke with D38142. (However, it's hard to tell because the whole thing was broken already due to the bug in part 1.) The patch moved relocation resolution to a later point in time and didn't apply it to in-memory objects. I am not aware of any reason why we wouldn't resolve relocations per-se, so I made it unconditional here. On Debian, it fixes the bug for me and all tests in check-lldb are still fine.

[labath]

Seems reasonable to me.

I do wonder though if the jit could be changed to avoid relocation. I don't know what's the behavior of other JITs, but given that the jitted object is not going to get "linked" in the normal sense of the word, wasting memory on debug info relocations seems suboptimal...

[sgraenitz]

Thanks for having a look!

The JIT implementations in LLVM operate on relocatable object files, so someone needs to resolve them. MCJIT has a flag ProcessAllSections to control which sections will be loaded and relocated. As it is off by default, it will usually ignore sections that it doesn't need for execution. LLDB does it the other way around in ObjectFileELF::RelocateSection(), it only processes sections with the ".debug" prefix. This seems to be a reasonable distribution of tasks. In general, relocations are resolved in place, so it doesn't rise memory consumption.

I do wonder though if the jit could be changed to avoid relocation.

Well, I don't think we are anywhere close to optimizations like this, but it would be nice indeed. If we compile from bitcode on the JIT side, we could lookup external symbols at compile-time and don't produce relocations for them in the first place. I guess it would heavily reduce the number of relocations and potentially save time. On the other hand, thinking about concurrent compile jobs and cross-dependencies.. I can imagine it gets hairy quickly. Plus: the way it is now, we can cache the object files and reuse them thanks to position-independent code.

[labath]

In D90769#2376557, @sgraenitz wrote:

Thanks for having a look!

The JIT implementations in LLVM operate on relocatable object files, so someone needs to resolve them. MCJIT has a flag ProcessAllSections to control which sections will be loaded and relocated. As it is off by default, it will usually ignore sections that it doesn't need for execution. LLDB does it the other way around in ObjectFileELF::RelocateSection(), it only processes sections with the ".debug" prefix. This seems to be a reasonable distribution of tasks. In general, relocations are resolved in place, so it doesn't rise memory consumption.

But you still have to allocate memory to store the relocations. And that memory is not freed after the relocations are resolved, right?

I do wonder though if the jit could be changed to avoid relocation.

Well, I don't think we are anywhere close to optimizations like this, but it would be nice indeed. If we compile from bitcode on the JIT side, we could lookup external symbols at compile-time and don't produce relocations for them in the first place. I guess it would heavily reduce the number of relocations and potentially save time. On the other hand, thinking about concurrent compile jobs and cross-dependencies.. I can imagine it gets hairy quickly. Plus: the way it is now, we can cache the object files and reuse them thanks to position-independent code.

In a "normal" compilation, there are two kinds of relocations. The ones that are resolved by the normal (static) linker, and the ones that are resolved by the dynamic linker (loader). to make a module relocatable and have it interface with external code, you only need the second kind. The debug info relocations are all of the first kind, so they could be resolved (or, not emitted in the first place -- like what happens with MachO object files) without impacting this ability.

But that's for another discussion...

[sgraenitz]

That's right, but I don't see how even the static ones could be resolved. Wouldn't it hard-wire the objects together? In the next session we want to load some object files from cache while others have changed.. Maybe one day I should check how ccache and the likes handle it.

IIRC OSO entries in MachO files point to the object file on disk, which allows LLDB to load them lazily? The DWARF parsing for ELF should follow the same laziness: each section is only relocated once we need it. It's just not loading the object lazily right? However, we couldn't do that even with OSO-style entries, because there is no "pull mechanism" in JITLoaderGDB :) Well, for now it just works.

I will leave the review here for another day or so, in case there's more feedback.

lldb/source/Symbol/ObjectFile.cpp

size_t ObjectFile::ReadSectionData(Section *section,
   assert(section);
   section_offset *= section->GetTargetByteSize();
 
   // If some other objectfile owns this data, pass this to them.
   if (section->GetObjectFile() != this)
     return section->GetObjectFile()->ReadSectionData(section, section_offset,
                                                      dst, dst_len);
 
+  if (!section->IsRelocated())
+    RelocateSection(section);
+
   if (IsInMemory()) {
     ProcessSP process_sp(m_process_wp.lock());
     if (process_sp) {
       Status error;
       const addr_t base_load_addr =
           section->GetLoadBaseAddress(&process_sp->GetTarget());
       if (base_load_addr != LLDB_INVALID_ADDRESS)
         return process_sp->ReadMemory(base_load_addr + section_offset, dst,
                                       dst_len, error);
     }
   } else {
-    if (!section->IsRelocated())
-      RelocateSection(section);
-
     const lldb::offset_t section_file_size = section->GetFileSize();
     if (section_offset < section_file_size) {
       const size_t section_bytes_left = section_file_size - section_offset;
       size_t section_dst_len = dst_len;
       if (section_dst_len > section_bytes_left)
         section_dst_len = section_bytes_left;
       return CopyData(section->GetFileOffset() + section_offset,
                       section_dst_len, dst);

 
 // Get the section data the file on disk
 size_t ObjectFile::ReadSectionData(Section *section,
                                    DataExtractor &section_data) {
   // If some other objectfile owns this data, pass this to them.
   if (section->GetObjectFile() != this)
     return section->GetObjectFile()->ReadSectionData(section, section_data);
 
+  if (!section->IsRelocated())
+    RelocateSection(section);
+
   if (IsInMemory()) {
     ProcessSP process_sp(m_process_wp.lock());
     if (process_sp) {
       const addr_t base_load_addr =
           section->GetLoadBaseAddress(&process_sp->GetTarget());
       if (base_load_addr != LLDB_INVALID_ADDRESS) {
         DataBufferSP data_sp(
             ReadMemory(process_sp, base_load_addr, section->GetByteSize()));
         if (data_sp) {
           section_data.SetData(data_sp, 0, data_sp->GetByteSize());
           section_data.SetByteOrder(process_sp->GetByteOrder());
           section_data.SetAddressByteSize(process_sp->GetAddressByteSize());
           return section_data.GetByteSize();
         }
       }
     }
-    return GetData(section->GetFileOffset(), section->GetFileSize(),
-                   section_data);
-  } else {
-    // The object file now contains a full mmap'ed copy of the object file
-    // data, so just use this
-    if (!section->IsRelocated())
-      RelocateSection(section);
-
-    return GetData(section->GetFileOffset(), section->GetFileSize(),
-                   section_data);
-  }
-}
+  }
+
 
 bool ObjectFile::SplitArchivePathWithObject(llvm::StringRef path_with_object,
                                             FileSpec &archive_file,
                                             ConstString &archive_object,
                                             bool must_exist) {
   size_t len = path_with_object.size();
   if (len < 2 || path_with_object.back() != ')')
     return false;

lldb/test/Shell/Breakpoint/jitbp_elf.test

 # REQUIRES: target-x86_64
 # XFAIL: system-windows
 
 # RUN: %clang -g -S -emit-llvm --target=x86_64-unknown-unknown-elf -o %t.ll %p/Inputs/jitbp.cpp
 # RUN: %lldb -b -o 'settings set plugin.jit-loader.gdb.enable on' -o 'b jitbp' -o 'run -jit-kind=mcjit %t.ll' lli | FileCheck %s
 
 # CHECK: Breakpoint 1: no locations (pending).
 # CHECK: (lldb) run -jit-kind=mcjit {{.*}}/jitbp_elf.test.tmp.ll
 # CHECK: Process {{.*}} stopped
-# CHECK: JIT(0x{{.*}})`jitbp:
+# CHECK: JIT(0x{{.*}})`jitbp() at jitbp.cpp:1:15
+# CHECK: -> 1    int jitbp() { return 0; }
+# CHECK:                       ^
+# CHECK:    2    int main() { return jitbp(); }
 # CHECK: Process {{.*}} launched: {{.*}}