[Lldb-commits] [PATCH] D113521: Allow lldb to launch a remote executable when there isn't a local copy

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I have two high-level questions about this:

- what should be the relative priority of executable ModuleSP vs the launch info? IIUC, in the current implementation the module takes precedence, but it seems to me it would be more natural if the launch info came out on top. One of the reasons I'm thinking about this is because you currently cannot re-run executables that use exec(2) (I mean, you can, but it will rarely produce the desired results). This happens because we use the post-exec executable, but the rest of the launch info refers to the main one. If the executable module was not the primary source of truth here, then maybe the we could somehow use this mechanism to improve the exec story as well (by storing the original executable in the launch info or something).
- what happens if the launch_info path happens to refer to a host executable? Will we still launch the remote one or will we try to copy the local one instead? I'm not sure of the current behavior, but I would like to avoid the behavior depending on the presence of local files.



================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:269
+        if (!exe_module_sp) {
+          result.AppendError("Could not get executable module after launch.");
+          return false;
----------------
This probably shouldn't be a hard error. If we fail to obtain the executable module, I'd think we should still be able to continue with asm debugging. (In fact, since you don't clean up the process here, that's probably what will happen regardless of the error state.)


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestNoLocalFile.py:30
+                ascii = bytearray.fromhex(a_arr[2]).decode()
+                self.testcase.assertEqual(ascii, self.testcase.absent_file, "We sent the right path")
+                return "OK"
----------------
the responder will run on a separate thread, so a failing assertion will not immediately terminate the test, but rather confuse the hell out of everyone. It be better to just record the data here, and do the assertion on the main thread.


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestNoLocalFile.py:69
+        self.server.responder = MyResponder(self)
+        target = self.dbg.CreateTargetWithFileAndArch(None, "x86_64")
+        launch_info = target.GetLaunchInfo()
----------------
I would use the CreateTarget overload which takes a platform name argument. Otherwise, this will pick a random platform on non-darwin hosts and then it may get confused by the darwin-specific responses.


================
Comment at: lldb/test/API/functionalities/gdb_remote_client/TestNoLocalFile.py:79-83
+        # We need to fetch the connected event:
+        event = lldb.SBEvent()
+        got_event = self.dbg.GetListener().WaitForEventForBroadcaster(30, process.GetBroadcaster(), event)
+        self.assertTrue(got_event, "Got an initial event after connect")
+        self.assertEqual(process.GetState(), lldb.eStateConnected, "Unexpected state.")
----------------
we have `lldbutil.expect_state_changes` for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113521