[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

Sun Jun 9 23:27:33 PDT 2019

labath added a subscriber: mgorny.
labath added a comment.

This looks mostly good. The thing I'm wondering about is whether this part of the code is really linux specific (in which case we should rename ELFLinkMap into LinuxLinkMap), or if it can be useful on other platforms (in which case this should go into the newly-created NativeProcessELF). @mgorny , @krytarowski, what do you think?

================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py:21-29
+        self.test_sequence.add_log_lines(
+            [
+                # Start the inferior...
+                "read packet: $c#63"
+            ],
+            True,
+        )
----------------
This countinue-and-immediatelly-interrupt sequence seems very dodgy. What's the purpose of that? Given that the inferior forces a break with the null dereference, I would expect you don't need to send any interrupt packets here, just simply wait for the inferior to stop.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2181
+template <typename T>
+Status NativeProcessLinux::ReadSVR4LibraryInfo(lldb::addr_t link_map_addr,
+                                               SVR4LibraryInfo &info,
----------------
labath wrote:
> This too could return an `llvm::Error`. `Expected<std::pair<SVR4LibraryInfo, addr_t>>` is a bit of a mouthful, but I'd consider that instead of by-ref returns too..
What about that llvm::Error?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502