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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

> This patch does not implement full scope of the qOffsets packet (which supports having different biases for code, data and bss sections). This is because the relevant lldb interfaces (e.g., Module::SetLoadAddress) do not support passing different values for different sections, and it's not clear how would such a thing apply to typical object files.

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.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3552
+      return llvm::None;
+    if (offset && *offset != cur_offset)
+      return llvm::None;
----------------
Too a bit to understand what this was doing. Might be worth a comment here or in the header that says something like:

```
We only handle offsets where all sections in a file are offset by the same amount. If any sections is slid by a different amount, we return no value.
```


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:428
 
+  /// qOffsets
+  llvm::Optional<lldb::addr_t> GetExecutableOffset();
----------------
Might be nice to document that we only handle when a binary has all its sections slid by the same amount somewhere. 


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:429
+  /// qOffsets
+  llvm::Optional<lldb::addr_t> GetExecutableOffset();
+
----------------
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.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1116
+    }
+  }
+
----------------
We could fall back eventually to try and parse the full results of qOffsets directly here and try to slide. Doesn't need to be done now, but a TODO comment might be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74598