[lldb] correct inconsistent order of messages on process launch

[junior-jl]

Fixes #68035, where an inconsistency in the order of "Process launched" and "Process stopped" messages occurs during process launch.

The fix involves adjusting the message output sequence in CommandObjectProcessLaunch::DoExecute within source/Commands/CommandObjectProcess.cpp. This ensures "Process launched" consistently precedes "Process stopped" when executing commands with the '-o' flag, i.e., non-interactive mode.

Upon implementing this change, two tests failed: lldb/test/Shell/Breakpoint/jit-loader_jitlink_elf.test and lldb/test/Shell/Breakpoint/jit-loader_rtdyld_elf.test. These failures were expected as they relied on the previous, now-corrected message order. Updating these tests to align with the new message sequence is part of this PR's scope.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: José Lira Junior (junior-jl)

Changes

Overview

This pull request addresses the issue #68035, where an inconsistency in the order of "Process launched" and "Process stopped" messages occurs during process launch.

Impact

Upon implementing this change, two tests failed: lldb/test/Shell/Breakpoint/jit-loader_jitlink_elf.test and lldb/test/Shell/Breakpoint/jit-loader_rtdyld_elf.test. These failures are anticipated as the tests are designed to expect the "Process stopped" message before the "Process launched" message, which is the behavior this PR aims to correct.

---

Full diff: https://github.com/llvm/llvm-project/pull/73173.diff

1 Files Affected:

• (modified) lldb/source/Commands/CommandObjectProcess.cpp (+2-2)

diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index c7ce1b1258c196c..f601316a6f673ec 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -265,8 +265,6 @@ class CommandObjectProcessLaunch : public CommandObjectProcessLaunchOrAttach {
         process_sp->SyncIOHandler(0, std::chrono::seconds(2));
 
         llvm::StringRef data = stream.GetString();
-        if (!data.empty())
-          result.AppendMessage(data);
         // If we didn't have a local executable, then we wouldn't have had an
         // executable module before launch.
         if (!exe_module_sp)
@@ -282,6 +280,8 @@ class CommandObjectProcessLaunch : public CommandObjectProcessLaunchOrAttach {
               exe_module_sp->GetFileSpec().GetPath().c_str(), archname);
         }
         result.SetStatus(eReturnStatusSuccessFinishResult);
+        if (!data.empty())
+          result.AppendMessage(data);
         result.SetDidChangeProcessState(true);
       } else {
         result.AppendError(

[DavidSpickett]

I was about to tell you about the process launch -m shortcut, then I realised you added that yourself :) (I'm using it all the time now)

Upon implementing this change, two tests failed: lldb/test/Shell/Breakpoint/jit-loader_jitlink_elf.test and lldb/test/Shell/Breakpoint/jit-loader_rtdyld_elf.test.

This is actually good news then, we already have test coverage. Please re-order the expected output for those tests.

[DavidSpickett]

You can use a few specific terms to auto close an issue when a related PR lands: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests

Fixes #12345 for example.

[DavidSpickett]

Also could you include a bit more explanation of the problem in the commit message. The issue is great, but it'll save someone a few clicks if you just include a sentence or two more in the commit message.

[junior-jl]

I was about to tell you about the process launch -m shortcut, then I realised you added that yourself :) (I'm using it all the time now)

Haha, that's great to know! 🥹

This is actually good news then, we already have test coverage. Please re-order the expected output for those tests.

Done. ✅

[junior-jl]

Also could you include a bit more explanation of the problem in the commit message. The issue is great, but it'll save someone a few clicks if you just include a sentence or two more in the commit message.

I added more information. Now that you taught me that the PR description becomes the commit message in the end (in the other PR), I got a little worried of putting too much information, so please, let me know if I should add something else.

[DavidSpickett]

Opinions differ on what goes in a commit message but this seems fine to me, I'd just remove the sub-headings. My usual rule is that folks can choose not to read it if they don't want to, include as much information as you would want to see if you yourself had to come back to fix this change.

(which happens to me more than I'd like to admit :) )

[DavidSpickett]

This LGTM just fixup the PR message and you can merge it.

(if you don't have the permissions to do that, I can do it for you, let me know when you're ready)

[junior-jl]

Opinions differ on what goes in a commit message but this seems fine to me, I'd just remove the sub-headings. My usual rule is that folks can choose not to read it if they don't want to, include as much information as you would want to see if you yourself had to come back to fix this change.
(which happens to me more than I'd like to admit :) )

That's a good rule, thanks! Removed the sub-headings.

(if you don't have the permissions to do that, I can do it for you, let me know when you're ready)

I don't have write permissions here. Also, I tried using the --fixup flag on the second commit, does that mean the squashing is automatic or I need to squash the commits and push again?

[DavidSpickett]

Also, I tried using the --fixup flag on the second commit, does that mean the squashing is automatic or I need to squash the commits and push again?

Honestly I've never used that flag. I'm not sure it matters with the way llvm is setup. I guess if llvm-project used one of the commit's messages as the final message, it wouldn't choose a fixup commit, but we squash before merging anyway so it doesn't matter.