Skip to content

[lldb-dap] Support StackFrameFormat #137113

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 24, 2025
Merged

[lldb-dap] Support StackFrameFormat #137113

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2168161
[lldb-dap] Support StackFrameFormat
JDevlieghere Apr 24, 2025
5b1378f
Fix incorrect interpretation of includeAll
JDevlieghere Apr 24, 2025
3cf5b4f
Black & Darker disagree
JDevlieghere Apr 24, 2025
66f2253
Simplify dap.configuration.displayExtendedBacktrace
JDevlieghere Apr 24, 2025
0584669
Fix decorator
JDevlieghere Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ def request_modules(self):
return self.send_recv({"command": "modules", "type": "request"})

def request_stackTrace(
self, threadId=None, startFrame=None, levels=None, dump=False
self, threadId=None, startFrame=None, levels=None, format=None, dump=False
):
if threadId is None:
threadId = self.get_thread_id()
Expand All @@ -1055,6 +1055,8 @@ def request_stackTrace(
args_dict["startFrame"] = startFrame
if levels is not None:
args_dict["levels"] = levels
if format is not None:
args_dict["format"] = format
command_dict = {
"command": "stackTrace",
"type": "request",
Expand Down
18 changes: 14 additions & 4 deletions lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,14 @@ def get_dict_value(self, d, key_path):
return value

def get_stackFrames_and_totalFramesCount(
self, threadId=None, startFrame=None, levels=None, dump=False
self, threadId=None, startFrame=None, levels=None, format=None, dump=False
):
response = self.dap_server.request_stackTrace(
threadId=threadId, startFrame=startFrame, levels=levels, dump=dump
threadId=threadId,
startFrame=startFrame,
levels=levels,
format=format,
dump=dump,
)
if response:
stackFrames = self.get_dict_value(response, ["body", "stackFrames"])
Expand All @@ -177,9 +181,15 @@ def get_stackFrames_and_totalFramesCount(
return (stackFrames, totalFrames)
return (None, 0)

def get_stackFrames(self, threadId=None, startFrame=None, levels=None, dump=False):
def get_stackFrames(
self, threadId=None, startFrame=None, levels=None, format=None, dump=False
):
(stackFrames, totalFrames) = self.get_stackFrames_and_totalFramesCount(
threadId=threadId, startFrame=startFrame, levels=levels, dump=dump
threadId=threadId,
startFrame=startFrame,
levels=levels,
format=format,
dump=dump,
)
return stackFrames

Expand Down
30 changes: 30 additions & 0 deletions lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,3 +217,33 @@ def test_functionNameWithArgs(self):
self.continue_to_next_stop()
frame = self.get_stackFrames()[0]
self.assertEqual(frame["name"], "recurse(x=1)")

@skipIfWindows
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM
But I could not see why this test is skipped on windows
there is a #include <unistd.h> but it is not used any where

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. All the tests in this file are skipped on Windows for as far as the history goes back. I can try removing all of them in a followup and see what the Windows bot say.

def test_StackFrameFormat(self):
"""
Test the StackFrameFormat.
"""
program = self.getBuildArtifact("a.out")
self.build_and_launch(program)
source = "main.c"

self.set_source_breakpoints(source, [line_number(source, "recurse end")])

self.continue_to_next_stop()
frame = self.get_stackFrames(format={"includeAll": True})[0]
self.assertEqual(frame["name"], "a.out main.c:6:5 recurse(x=1)")

frame = self.get_stackFrames(format={"parameters": True})[0]
self.assertEqual(frame["name"], "recurse(x=1)")

frame = self.get_stackFrames(format={"parameterNames": True})[0]
self.assertEqual(frame["name"], "recurse(x=1)")

frame = self.get_stackFrames(format={"parameterValues": True})[0]
self.assertEqual(frame["name"], "recurse(x=1)")

frame = self.get_stackFrames(format={"parameters": False, "line": True})[0]
self.assertEqual(frame["name"], "main.c:6:5 recurse")

frame = self.get_stackFrames(format={"parameters": False, "module": True})[0]
self.assertEqual(frame["name"], "a.out recurse")
42 changes: 37 additions & 5 deletions lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,15 @@ static constexpr int StackPageSize = 20;
//
// s=3,l=3 = [th0->s3, label1, th1->s0]
static bool FillStackFrames(DAP &dap, lldb::SBThread &thread,
lldb::SBFormat &frame_format,
llvm::json::Array &stack_frames, int64_t &offset,
const int64_t start_frame, const int64_t levels) {
bool reached_end_of_stack = false;
for (int64_t i = start_frame;
static_cast<int64_t>(stack_frames.size()) < levels; i++) {
if (i == -1) {
stack_frames.emplace_back(
CreateExtendedStackFrameLabel(thread, dap.frame_format));
CreateExtendedStackFrameLabel(thread, frame_format));
continue;
}

Expand All @@ -67,7 +68,7 @@ static bool FillStackFrames(DAP &dap, lldb::SBThread &thread,
break;
}

stack_frames.emplace_back(CreateStackFrame(frame, dap.frame_format));
stack_frames.emplace_back(CreateStackFrame(frame, frame_format));
}

if (dap.configuration.displayExtendedBacktrace && reached_end_of_stack) {
Expand All @@ -80,7 +81,7 @@ static bool FillStackFrames(DAP &dap, lldb::SBThread &thread,
continue;

reached_end_of_stack = FillStackFrames(
dap, backtrace, stack_frames, offset,
dap, backtrace, frame_format, stack_frames, offset,
(start_frame - offset) > 0 ? start_frame - offset : -1, levels);
if (static_cast<int64_t>(stack_frames.size()) >= levels)
break;
Expand Down Expand Up @@ -178,14 +179,45 @@ void StackTraceRequestHandler::operator()(
llvm::json::Array stack_frames;
llvm::json::Object body;

lldb::SBFormat frame_format = dap.frame_format;

if (const auto *format = arguments->getObject("format")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing I notice is the StackFrameFormat inherits from ValueFormat and thus includes the hex?: boolean; field as well.

I'm not sure what that would mean in terms of a stack frame however, just thought I would note that for completeness.

const bool parameters = GetBoolean(format, "parameters").value_or(false);
const bool parameter_names =
GetBoolean(format, "parameterNames").value_or(false);
const bool parameter_values =
GetBoolean(format, "parameterValues").value_or(false);
const bool line = GetBoolean(format, "line").value_or(false);
const bool module = GetBoolean(format, "module").value_or(false);
const bool include_all = GetBoolean(format, "includeAll").value_or(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is maybe a different meaning than the spec meant for includeAll.

It says:

Includes all stack frames, including those the debug adapter might otherwise hide.

Which to me, sounds like its about hiding frames that we might not find interesting (e.g. frames from system libraries).

I think if this is false you could skip showing the extended backtraces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great observation, you're right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its worth noting that today I don't think we skip any frames, however we do use the 'presentationHint' to tweak the frames a little.


std::string format_str;
llvm::raw_string_ostream os(format_str);

if (include_all || module)
os << "{${module.file.basename} }";

if (include_all || line)
os << "{${line.file.basename}:${line.number}:${line.column} }";

if (include_all || parameters || parameter_names || parameter_values)
os << "{${function.name-with-args}}";
else
os << "{${function.name-without-args}}";

lldb::SBError error;
frame_format = lldb::SBFormat(format_str.c_str(), error);
assert(error.Success());
}

if (thread.IsValid()) {
const auto start_frame =
GetInteger<uint64_t>(arguments, "startFrame").value_or(0);
const auto levels = GetInteger<uint64_t>(arguments, "levels").value_or(0);
int64_t offset = 0;
bool reached_end_of_stack =
FillStackFrames(dap, thread, stack_frames, offset, start_frame,
levels == 0 ? INT64_MAX : levels);
FillStackFrames(dap, thread, frame_format, stack_frames, offset,
start_frame, levels == 0 ? INT64_MAX : levels);
body.try_emplace("totalFrames",
start_frame + stack_frames.size() +
(reached_end_of_stack ? 0 : StackPageSize));
Expand Down
Loading