[lldb-dap] Prevent using an implicit step-in.
#143644
Conversation
When there is a function that is inlined at the current program counter. If you get the current `line_entry` using the program counter's address it will point to the location of the inline function that may be in another file. (this is in implicit step-in and should not happen what step over is called). Use the current frame to get the correct `line_entry`
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesWhen there is a function that is inlined at the current program counter. If you get the current Use the current frame to get the correct why_step_in.mp4I attached a video showing the issue. If there is a better way to test this, I could change the test. Full diff: https://github.com/llvm/llvm-project/pull/143644.diff 7 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index 3b54d598c3509..6299caf7631af 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -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
diff --git a/lldb/test/API/tools/lldb-dap/step/Number.h b/lldb/test/API/tools/lldb-dap/step/Number.h
new file mode 100644
index 0000000000000..e962a10e2a268
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/step/Number.h
@@ -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
\ No newline at end of file
diff --git a/lldb/test/API/tools/lldb-dap/step/TestDAP_step.py b/lldb/test/API/tools/lldb-dap/step/TestDAP_step.py
index 42a39e3c8c080..bb7be7fec7433 100644
--- a/lldb/test/API/tools/lldb-dap/step/TestDAP_step.py
+++ b/lldb/test/API/tools/lldb-dap/step/TestDAP_step.py
@@ -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)
diff --git a/lldb/test/API/tools/lldb-dap/step/main.cpp b/lldb/test/API/tools/lldb-dap/step/main.cpp
index 8905beb5e7eff..130724bfd243e 100644
--- a/lldb/test/API/tools/lldb-dap/step/main.cpp
+++ b/lldb/test/API/tools/lldb-dap/step/main.cpp
@@ -1,3 +1,5 @@
+#include "Number.h"
+
int function(int x) {
if ((x % 2) == 0)
return function(x - 1) + x; // breakpoint 1
@@ -5,4 +7,17 @@ int function(int x) {
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
+}
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 6cdde63e9796e..1beb98d38b8f0 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -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);
+ 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();
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();
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.cpp b/lldb/tools/lldb-dap/ProtocolUtils.cpp
index 6e0adf5bc8b59..28c04f14db632 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.cpp
+++ b/lldb/tools/lldb-dap/ProtocolUtils.cpp
@@ -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 {};
+
+ 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());
+}
+
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
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.h b/lldb/tools/lldb-dap/ProtocolUtils.h
index 2b2ac9e8e35fd..4cea966a769e0 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.h
+++ b/lldb/tools/lldb-dap/ProtocolUtils.h
@@ -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);
|
|
The test does look like it could be easily broken or invalidated by a change in how the compiler emits debug info. There probably is a better way to test this, but I'm not quite sure on what exactly is it that you're trying to do. Is it the following:
|
|
It is the combination of the first and third. we do an step-over, which lands us at an instruction, which happens to be the first instruction of an inlined function. This only happens in lldb-dap not lldb. |
| object.try_emplace("line", line_entry.GetLine()); | ||
| auto column = line_entry.GetColumn(); | ||
| uint32_t column = line_entry.GetColumn(); |
There was a problem hiding this comment.
are any of these changes related to this patch?
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
Would this patch become simpler if we called StackFrame::GetFrameCodeAddressForSymbolication instead of GetPCAddress here? I think that's how LLDB solves this problem
There was a problem hiding this comment.
We do not have access to the function because lldb-dap uses the SB-API
There was a problem hiding this comment.
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).
| @@ -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 {}; | |||
There was a problem hiding this comment.
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.
| if (ShouldDisplayAssemblySource(frame_pc, stop_disassembly_display)) | ||
| return CreateAssemblySource(target, frame_pc); | ||
|
|
||
| return CreateSource(frame.GetLineEntry().GetFileSpec()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
okay, so would something like this be sufficient to reproduce the issue (you can put the functions in different files as needed): Stop and the |
This should be reproducable but I am not sure if it is the same problem, because it now skips (lldb) target create "build/debug/main_c"
Current executable set to '/home/da-viper/Dev/projects/test_lldb/build/debug/main_c' (x86_64).
(lldb) b main
Breakpoint 1: where = main_c`main + 15 at main.c:5:3, address = 0x000000000040045f
(lldb) r
Process 757816 launched: '/home/da-viper/Dev/projects/test_lldb/build/debug/main_c' (x86_64)
Process 757816 stopped
* thread #1, name = 'main_c', stop reason = breakpoint 1.1
frame #0: 0x000000000040045f main_c`main at main.c:5:3
2 #include "fn2.h"
3
4 int main() {
-> 5 fn1();
6 fn2();
7 return 0;
8 }
(lldb) dis
main_c`main:
0x400450 <+0>: push rbp
0x400451 <+1>: mov rbp, rsp
0x400454 <+4>: sub rsp, 0x10
0x400458 <+8>: mov dword ptr [rbp - 0x4], 0x0
-> 0x40045f <+15>: mov al, 0x0
0x400461 <+17>: call 0x400480 ; fn1 at fn1.c:3
0x400466 <+22>: mov al, 0x0
0x400468 <+24>: call 0x400480 ; fn1 at fn1.c:3
0x40046d <+29>: xor eax, eax
0x40046f <+31>: add rsp, 0x10
0x400473 <+35>: pop rbp
0x400474 <+36>: ret
(lldb) n
Process 757816 stopped
* thread #1, name = 'main_c', stop reason = step over
frame #0: 0x000000000040046d main_c`main at main.c:7:3
4 int main() {
5 fn1();
6 fn2();
-> 7 return 0;
8 }
(lldb) dis
main_c`main:
0x400450 <+0>: push rbp
0x400451 <+1>: mov rbp, rsp
0x400454 <+4>: sub rsp, 0x10
0x400458 <+8>: mov dword ptr [rbp - 0x4], 0x0
0x40045f <+15>: mov al, 0x0
0x400461 <+17>: call 0x400480 ; fn1 at fn1.c:3
0x400466 <+22>: mov al, 0x0
0x400468 <+24>: call 0x400480 ; fn1 at fn1.c:3
-> 0x40046d <+29>: xor eax, eax
0x40046f <+31>: add rsp, 0x10
0x400473 <+35>: pop rbp
0x400474 <+36>: ret
(lldb) This happens in both lldb-dap and lldb |
|
Yeah, that sounds like a bug. A different one that what you are fixing, but I think it's kind of related, as now the stepping machinery who concludes it has stepped "into" fn2 (and then decides to step out of it). I've filed #144061 to track that. Would it work if we replace the call to |
When there is a function that is inlined at the current program counter. If you get the current
line_entryusing the program counter's address it will point to the location of the inline function that may be in another file. (this is in implicit step-in and should not happen what step over is called).Use the current frame to get the correct
line_entrywhy_step_in.mp4
I attached a video showing the issue.
I did not use the vector from the stdlib because
libc++iterator operators are not inlined so it does not trigger on my machine (x86_64-linux-gnu).If there is a better way to test this, I could change the test.