[Lldb-commits] [PATCH] D74598: [lldb/gdb-remote] Add support for the qOffsets packet

Mon Feb 24 06:31:57 PST 2020

labath added a comment.

Thanks for the quick response, and sorry it took me a while to get back to this.

In D74598#1877316 <https://reviews.llvm.org/D74598#1877316>, @clayborg wrote:

> While Module::SetLoadAddress doesn't, the Target interfaces do in fact support this fully. You specify the section and the section load address. Look at the macOS dynamic loader, it does this. All shared libraries in the macOS dyld shared cache have all the the __TEXT segments put next to each other so that the OS can load one large read + execute mapping, all the modifiable data sections in __DATA are relocated as well along with other segments. The Module::SetLoadAddress() interface was put in Module to make it easy to slide full binaries.

Ah, thanks for explaing that. My understanding was that `Module::SetLoadAddress` was the "main" interface, and that the other functions were just there to support it. Having `Module::SetLoadAddress` be a utility function on top of those makes perfect sense now.

================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:429
+  /// qOffsets
+  llvm::Optional<lldb::addr_t> GetExecutableOffset();
+
----------------
clayborg wrote:
> It would be nice to have two functions here:
> 1 - "llvm::SmallVector<llvm::StringRef> GetQOffsets();" which will return raw qOffsets calls results
> 2 - "llvm::Optional<lldb::addr_t> GetExecutableOffset();" which calls above function and then does it thing where it tries to get a single rigid slide for the entire executable binary or fails.
I don't think that is really worth it -- supporting the full qOffsets packet is is somewhat tricky, and if anyone sets out to do that, the lack of a preexisting query function will be the least of his concerns.

The main thing I found tricky is knowing what exactly should the individual offset fields apply to. The packet response doesn't just list sections or anything. It specifically prescribes three fields: "Text", "Data", and "Bss". Looking at gdb sources, I got the impression that it maps "Text" to the ".text" section, but that seems too narrow to me -- I would expect that it should apply to all executable sections, but as I don't know of any system which would make use of this option, I cannot validate my hypothesis.

The second complication comes from the supposed difference in handling of "Text" vs "TextSeg" field. To implement that absolutely correctly, I'd need to know whether the object file uses segments or not. But knowing that is again tricky because our ObjectFile interface abstracts those differences away. I could again make some guesses at what would be a reasonable behavior, but I also could not check that this is right.

So I think it's better to leave this to someone who actually needs this behavior.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74598/new/

https://reviews.llvm.org/D74598