-
Notifications
You must be signed in to change notification settings - Fork 14.1k
[lldb-dap] Report exit status message in lldb-dap, same as lldb cli #89405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-lldb Author: Miro Bucko (mbucko) ChangesSummary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Full diff: https://github.com/llvm/llvm-project/pull/89405.diff 1 Files Affected:
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 25c5ad56e3d6fe..76c7fb17a793cb 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -503,6 +503,13 @@ void EventThreadFunction() {
SendContinuedEvent();
break;
case lldb::eStateExited:
+ const int exit_status = process.GetExitStatus();
+ const char * const exit_description = process.GetExitDescription();
+ g_dap.SendFormattedOutput(OutputType::Console,
+ "Process %" PRIu64 " exited with status = %i (0x%8.8x) %s\n",
+ process.GetProcessID(), exit_status, exit_status,
+ exit_description ? exit_description : "");
+
// When restarting, we can get an "exited" event for the process we
// just killed with the old PID, or even with no PID. In that case
// we don't have to terminate the session.
|
You generally have to tag people with an at sign to get their attention :) @jeffreytan81 @clayborg @kusmour |
Is it possible to test this, for example by explicitly killing the debug stub? |
Yep, please write a python test. A possible idea is to have a target that does nothing for 10 seconds, during which you kill the debug server (lldb-server or debugserver) and then you assert on the final message sent by lldb-vscode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thank you!
I've wanted this feature for a while, tbh, because the user doesn't know if the program terminated successfully or if the debugger crashed.
✅ With the latest revision this PR passed the Python code formatter. |
"debugserver" if platform.platform() == "MacOS" else "lldb-server" | ||
) | ||
process = get_subprocess(process_name) | ||
process.terminate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we can create a crash packet so that we can initiate lldb-server/debugserver crash for testing purpose without this kind of hack...
4c22c23
to
d0ed0c6
Compare
Summary: When the target inferior process that is being debugged exits in lldb command line, it emits following message: Process 4049526 exited with status = -1 (0xffffffff) debugserver died with signal SIGTERM lldb-dap on the other hand does not emit a similar message. This PR adds the same status message to lldb-dap. Test Plan: In VSCode debug any target and hit stop mode, kill lldb-server and observe an exit status message similar to the following: Process 2167677 exited with status = -1 (0xffffffff) debugserver died with signal SIGTERM Reviewers: Subscribers: Tasks: lldb-dap Tags:
@mbucko Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Hi, I think we're seeing some breakage in our CI after this patch. We see the lldb-api :: tools/lldb-dap/console/TestDAP_console.py test failing.
would you mind taking a look. CC: @mysterymath since he's the most familiar w/ our lldb build |
Summary: 'test_exit_status_message_sigterm' is failing due to 'psutil' dependency introduced in PR llvm#89405. This fix removes 'deque' dependency and checks if 'psutil' can be imported before running the test. If 'psutil' cannot be imported, it emits a warning and skips the test. Test Plan: ./bin/llvm-lit -sv /path-to-llvm-project/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py --filter=tools/lldb-dap/console/TestDAP_console.py Reviewers: @jeffreytan81,@clayborg,@kusmour, @JDevlieghere,@walter-erquinigo Subscribers: Tasks: lldb-dap Tags:
Summary: 'test_exit_status_message_sigterm' is failing due to 'psutil' dependency introduced in PR llvm#89405. This fix removes 'deque' dependency and checks if 'psutil' can be imported before running the test. If 'psutil' cannot be imported, it emits a warning and skips the test. Test Plan: ./bin/llvm-lit -sv /path-to-llvm-project/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py --filter=tools/lldb-dap/console/TestDAP_console.py Reviewers: @jeffreytan81,@clayborg,@kusmour, @JDevlieghere,@walter-erquinigo Subscribers: Tasks: lldb-dap Tags:
Summary: 'test_exit_status_message_sigterm' is failing due to 'psutil' dependency introduced in PR llvm#89405. This fix removes 'deque' dependency and checks if 'psutil' can be imported before running the test. If 'psutil' cannot be imported, it emits a warning and skips the test. Test Plan: ./bin/llvm-lit -sv /path-to-llvm-project/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py --filter=tools/lldb-dap/console/TestDAP_console.py Reviewers: @jeffreytan81,@clayborg,@kusmour, @JDevlieghere,@walter-erquinigo Subscribers: Tasks: lldb-dap Tags:
Summary: 'test_exit_status_message_sigterm' is failing due to 'psutil' dependency introduced in PR #89405. This fix removes 'deque' dependency and checks if 'psutil' can be imported before running the test. If 'psutil' cannot be imported, it emits a warning and skips the test. Test Plan: ./bin/llvm-lit -sv /path-to-llvm-project/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py --filter=tools/lldb-dap/console/TestDAP_console.py Reviewers: @jeffreytan81,@clayborg,@kusmour, @JDevlieghere,@walter-erquinigo Subscribers: Tasks: lldb-dap Tags:
Summary:
When the target inferior process that is being debugged exits in lldb command line, it emits following message:
Process 4049526 exited with status = -1 (0xffffffff) debugserver died with signal SIGTERM
lldb-dap on the other hand does not emit a similar message. This PR adds the same status message to lldb-dap.
Test Plan:
In VSCode debug any target and hit stop mode, kill lldb-server and observe an exit status message similar to the following: Process 2167677 exited with status = -1 (0xffffffff) debugserver died with signal SIGTERM
Reviewers:
@jeffreytan81,@clayborg,@kusmour,
Subscribers:
Tasks:
lldb-dap
Tags: