Skip to content

[lldb-dap] Prevent using an implicit step-in. #143644

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,12 @@ def stepOver(
granularity="statement",
timeout=DEFAULT_TIMEOUT,
):
self.dap_server.request_next(threadId=threadId, granularity=granularity)
response = self.dap_server.request_next(
threadId=threadId, granularity=granularity
)
self.assertTrue(
response["success"], f"next request failed: response {response}"
)
if waitForStop:
return self.dap_server.wait_for_stopped(timeout)
return None
Expand Down
57 changes: 57 additions & 0 deletions lldb/test/API/tools/lldb-dap/step/Number.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
#ifndef NUMBER_H
#define NUMBER_H

#include <cstddef>

template <std::size_t Size> class Numbers {
public:
using ValueType = int;
using PointerType = ValueType *;
using ReferenceType = ValueType &;

class Iterator {
// all the operators needs to be inlined so that there is no call
// instruction.
public:
__attribute__((always_inline)) explicit Iterator(PointerType ptr)
: m_ptr(ptr) {}

__attribute__((always_inline)) Iterator &operator++() noexcept {
++m_ptr;
return *this;
};

__attribute__((always_inline)) Iterator operator++(int) noexcept {
Iterator iter = *this;
m_ptr++;
return iter;
}

__attribute__((always_inline)) ReferenceType operator*() const noexcept {
return *m_ptr;
}
__attribute__((always_inline)) bool
operator==(const Iterator &iter) noexcept {
return m_ptr == iter.m_ptr;
}
__attribute__((always_inline)) bool
operator!=(const Iterator &iter) noexcept {
return !(*this == iter);
}

private:
PointerType m_ptr;
};

PointerType data() { return static_cast<PointerType>(m_buffer); }

Iterator begin() { return Iterator(data()); }
Iterator cbegin() { return Iterator(data()); }
Iterator end() { return Iterator(data() + Size); }
Iterator cend() { return Iterator(data() + Size); }

private:
int m_buffer[Size]{};
};

#endif // NUMBER_H
41 changes: 41 additions & 0 deletions lldb/test/API/tools/lldb-dap/step/TestDAP_step.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,44 @@ def test_step(self):

# only step one thread that is at the breakpoint and stop
break

def test_step_over(self):
"""
Test stepping over when the program counter is in another file.
"""
# when in a for range loop the program counter will in the iterator header.
# make sure the frame source file is correct.
program = self.getBuildArtifact("a.out")
self.build_and_launch(program)
source = "main.cpp"
breakpoint1_line = line_number(source, "// breakpoint 2")
step_over_pos = line_number(source, "// position_after_step_over")
lines = [breakpoint1_line]
breakpoint_ids = self.set_source_breakpoints(source, lines)
self.assertEqual(
len(breakpoint_ids), len(lines), "expect correct number of breakpoints."
)
self.continue_to_breakpoints(breakpoint_ids)

thread_id = self.dap_server.get_thread_id()
self.stepOver(thread_id)
levels = 1
frames = self.get_stackFrames(thread_id, 0, levels)
self.assertEqual(len(frames), levels, "expect current number of frame levels.")
top_frame = frames[0]
self.assertEqual(
top_frame["source"]["name"], source, "expect we are in the same file."
)
self.assertTrue(
top_frame["source"]["path"].endswith(source),
f"expect path ending with '{source}'.",
)
self.assertEqual(
top_frame["line"],
step_over_pos,
f"expect step_over on line {step_over_pos}",
)

# clear breakpoints
self.set_source_breakpoints(source, [])
self.continue_to_exit(exitCode=13)
17 changes: 16 additions & 1 deletion lldb/test/API/tools/lldb-dap/step/main.cpp
Original file line number Diff line number Diff line change
@@ -1,8 +1,23 @@
#include "Number.h"

int function(int x) {
if ((x % 2) == 0)
return function(x - 1) + x; // breakpoint 1
else
return x;
}

int main(int argc, char const *argv[]) { return function(2); }
int function2() {
Numbers<10> list;

int result = 0;
for (const int val : list) // position_after_step_over
result++; // breakpoint 2

return result;
}

int main(int argc, char const *argv[]) {
int func_result = function2();
return function(2) + func_result; // returns 13
}
10 changes: 5 additions & 5 deletions lldb/tools/lldb-dap/JSONUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -581,19 +581,19 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,

EmplaceSafeString(object, "name", frame_name);

auto target = frame.GetThread().GetProcess().GetTarget();
auto source = CreateSource(frame.GetPCAddress(), target);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this patch become simpler if we called StackFrame::GetFrameCodeAddressForSymbolication instead of GetPCAddress here? I think that's how LLDB solves this problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not have access to the function because lldb-dap uses the SB-API

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not have access to the function because lldb-dap uses the SB-API

I don't know if it makes sense here, but I want to point out that we can definitely consider adding new SB functions if they are useful for some users (like lldb-dap).

lldb::SBTarget target = frame.GetThread().GetProcess().GetTarget();
protocol::Source source = CreateSource(frame);

if (!IsAssemblySource(source)) {
// This is a normal source with a valid line entry.
auto line_entry = frame.GetLineEntry();
lldb::SBLineEntry line_entry = frame.GetLineEntry();
object.try_emplace("line", line_entry.GetLine());
auto column = line_entry.GetColumn();
uint32_t column = line_entry.GetColumn();
Copy link
Contributor

Choose a reason for hiding this comment

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

are any of these changes related to this patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see that the source one is different.
In general, please move NFC changes to their own separate patches, to avoid throwing reviewers off.

object.try_emplace("column", column);
} else if (frame.GetSymbol().IsValid()) {
// This is a source where the disassembly is used, but there is a valid
// symbol. Calculate the line of the current PC from the start of the
// current symbol.
lldb::SBTarget target = frame.GetThread().GetProcess().GetTarget();
lldb::SBInstructionList inst_list = target.ReadInstructions(
frame.GetSymbol().GetStartAddress(), frame.GetPCAddress(), nullptr);
size_t inst_line = inst_list.GetSize();
Expand Down
15 changes: 15 additions & 0 deletions lldb/tools/lldb-dap/ProtocolUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,21 @@ protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target) {
return CreateSource(line_entry.GetFileSpec());
}

protocol::Source CreateSource(lldb::SBFrame frame) {
if (!frame.IsValid())
return {};
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 we should make this function return an std::optional<protocol::Source> as I think this should only be set if the source is valid. The source should have at least path and/or sourceReference set.


const lldb::SBTarget target = frame.GetThread().GetProcess().GetTarget();
lldb::SBDebugger debugger = target.GetDebugger();
lldb::StopDisassemblyType stop_disassembly_display =
GetStopDisassemblyDisplay(debugger);
const lldb::SBAddress frame_pc = frame.GetPCAddress();
if (ShouldDisplayAssemblySource(frame_pc, stop_disassembly_display))
return CreateAssemblySource(target, frame_pc);

return CreateSource(frame.GetLineEntry().GetFileSpec());
Copy link
Contributor

Choose a reason for hiding this comment

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

As with the comment on line 109, I think we should update CreateSource on line 84 also return an std::optional<protocol::Source>, if the file spec is invalid.

Copy link
Contributor Author

@da-viper da-viper Jun 13, 2025

Choose a reason for hiding this comment

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

I am thinking maybe I move this refactor to a different PR so it is easier to review the main issue ?, Since the CreateSource function is used in other files.

}

bool IsAssemblySource(const protocol::Source &source) {
// According to the specification, a source must have either `path` or
// `sourceReference` specified. We use `path` for sources with known source
Expand Down
11 changes: 11 additions & 0 deletions lldb/tools/lldb-dap/ProtocolUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,17 @@ protocol::Source CreateSource(const lldb::SBFileSpec &file);
/// definition outlined by Microsoft.
protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target);

/// Create a `protocol::Source` object as described in the debug adapter
/// definition.
///
/// \param[in] frame
/// The frame to use when populating the "Source" object.
///
/// \return
/// A `protcol::Source` object that follows the formal JSON
/// definition outlined by Microsoft.
protocol::Source CreateSource(lldb::SBFrame frame);

/// Checks if the given source is for assembly code.
bool IsAssemblySource(const protocol::Source &source);

Expand Down
Loading