Skip to content
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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Da-Viper
Copy link
Contributor

@Da-Viper Da-Viper commented Sep 1, 2024

show_return_values.mp4

@llvmbot
Copy link
Member

llvmbot commented Sep 1, 2024

@llvm/pr-subscribers-lldb

Author: None (Da-Viper)

Changes
show_return_values.mp4

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

1 Files Affected:

  • (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+11)
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);

Copy link

github-actions bot commented Sep 1, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@walter-erquinigo walter-erquinigo left a 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?

@Da-Viper
Copy link
Contributor Author

@JDevlieghere was away for sometime could you please review this.

thanks

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -663,6 +663,41 @@ def do_test_indexedVariables(self, enableSyntheticChildDebugging: bool):
]["variables"]
self.verify_variables(verify_children, children)

def test_return_variables(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arm64 ABI does not require restoring the memory return register value (x8) on exit from a function, so lldb can't reliably fetch memory return values. You'll need to skip this test on Darwin running on arm64.

Copy link
Collaborator

@jimingham jimingham Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to split this test into a "scalar return" test, and a "memory return" test. Except on systems that never return in registers (x86) we can always get scalar returns. It's on ly memory returns that require the ABI specify restoring the memory return address register.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little bit confused about something just need some clarification.

lldb provides SBThread::GetStopReturnValue(). I am assuming on arm64, checking isValid() will be false?.

when you say scalar return test is it the one return a value in register and the memory returns a memory address to the variable ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lldb will do almost the right thing if it can't figure out the return value of a function. The SBValue that SBThread::GetStopReturnValue() returns won't be valid. I say that's "almost the right thing" because a better return would be a valid SBValue with an error that tells you why we couldn't fetch the return value.

In general, lldb will always be able to get the return value of functions that return their values in registers, since that information has to be available at the moment you step out of the function. It doesn't matter whether the return is in one or multiple registers, so long as lldb can figure out from the ABI and the return type that it is returned in one or more registers, the information is available for lldb to figure out the value.

For return values that are bigger than will fit in registers for that ABI, the function gets passed the address that the caller expects the return value to be written to. In the Itanium ABI - which is what for instance x86_64 uses, the ABI mandates that the return address be written back to the "return address register" specifically so that analysis tools like debuggers could reconstruct it. But for the arm64 ABI someone decided that benefit wasn't worth the extra store & write, so the function is free to reuse that address-passing register. That means the only way that lldb could reconstruct the return value was if it stopped at the first instruction of the function, wrote down the address passed in, and then it would have it on stepping out.

We've toyed with ideas like: if you put a breakpoint anywhere in function A(), then we'll also put a breakpoint on the start of the function so we can transparently capture this value. And when we step into a function, we could stop at the first instruction and write down the address as well. This is probably doable, but no one has done it yet.

What I was mostly saying is if we have one test that tests both in register returns and in memory returns, then we'd have to disable the entire test for ABI's that don't restore the return address. But if we had one test for in register returns, and one for in memory, we could keep running the first test, and not lose coverage for something that should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think understand it.
A function can either return a value that fits in a register if it too big, it is placed in memory. hence register and memory returns.

Some ABIs you may reuse the register with the return value address.

The test needs to be split to handle register returns and memory returns.

The case of the memory returns does the new test in TestDAP_variables_children.py not cover that case ? as it returns a value that will not fit in one register.

@Da-Viper Da-Viper force-pushed the show_return_value_on_step_out branch from a5e336a to 42e3bd5 Compare February 13, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants