-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[lldb-dap] Add: show return value on step out #106907
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
[lldb-dap] Add: show return value on step out #106907
Conversation
@llvm/pr-subscribers-lldb Author: None (Da-Viper) Changesshow_return_values.mp4Full diff: https://github.com/llvm/llvm-project/pull/106907.diff 1 Files Affected:
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index c5c4b09f15622b..c9116c62c46b5e 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -3801,6 +3801,17 @@ void request_variables(const llvm::json::Object &request) {
variable_name_counts[GetNonNullVariableName(variable)]++;
}
+ // Show return value if there is any ( in the top frame )
+ auto process = g_dap.target.GetProcess();
+ auto selectedThread = process.GetSelectedThread();
+ lldb::SBValue stopReturnValue = selectedThread.GetStopReturnValue();
+ if (stopReturnValue.IsValid() &&
+ (selectedThread.GetSelectedFrame().GetFrameID() == 0)) {
+ auto renamedReturnValue = stopReturnValue.Clone("(Return Value)");
+ variables.emplace_back(
+ CreateVariable(renamedReturnValue,0, UINT64_MAX, hex, false));
+ }
+
// Now we construct the result with unique display variable names
for (auto i = start_idx; i < end_idx; ++i) {
lldb::SBValue variable = top_scope->GetValueAtIndex(i);
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
This looks much better!
Could you please write some tests?
@JDevlieghere was away for sometime could you please review this. thanks |
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
a5e336a
to
42e3bd5
Compare
Is there any that needs to be done one the PR? |
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.
Looks good as long as this is well tested!
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 🙂
@da-viper Can you rebase and merge this PR by yourself? Or do you need somebody to merge it on your behalf? |
Explicitly check for the return value variable
04db61f
to
4453e42
Compare
I rebased but I do not have the necessary permissions to merge |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/13574 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/12219 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/6707 Here is the relevant piece of the build log for the reference
|
…es (llvm#129409) amd64 and aarch64 are treated differently Follows up llvm#106907
show_return_values.mp4