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

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:268-269
+          exe_module_sp = target->GetExecutableModule();
+        if (!exe_module_sp) {
+          result.AppendError("Could not get executable module after launch.");
+          return false;
----------------
clayborg wrote:
> I agree with Pavel here, unless you have a case where you need this to work this way?
If we couldn't get the executable module, that would mean that we queried a process stopped in the remote server after launch, and the dynamic loader wasn't able to determine the main executable.  That's something we do all the time, so it would be weird if we couldn't do that.  I don't know why that would happen or how well the debug session is going to go on from here.  But leaving it around in the hopes that it's useful is fine by me.


================
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"
----------------
labath wrote:
> 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.
Got it.  I just preserve the file name from the A packet and test it later.


================
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()
----------------
labath wrote:
> 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.
Good catch.


================
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.")
----------------
labath wrote:
> we have `lldbutil.expect_state_changes` for this
I hadn't noticed that, nice...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113521