[lldb] Fix source display for artificial locations #115876
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesWhen retrieving the location of the function declaration, we were dropping the file component on the floor, which resulted in an amusingly confusing situation were we displayed the file containing the implementation of the function, but used the line number of the declaration. This patch fixes that. It required a small refactor Function::GetStartLineSourceLineInfo to return a SupportFile (instead of just the file spec), which in turn necessitated changes in a couple of other places as well. Full diff: https://github.com/llvm/llvm-project/pull/115876.diff 10 Files Affected:
diff --git a/lldb/include/lldb/Symbol/Function.h b/lldb/include/lldb/Symbol/Function.h
index bbfc25fe74ea06..70f51a846f8d96 100644
--- a/lldb/include/lldb/Symbol/Function.h
+++ b/lldb/include/lldb/Symbol/Function.h
@@ -457,7 +457,8 @@ class Function : public UserID, public SymbolContextScope {
///
/// \param[out] line_no
/// The line number.
- void GetStartLineSourceInfo(FileSpec &source_file, uint32_t &line_no);
+ void GetStartLineSourceInfo(lldb::SupportFileSP &source_file_sp,
+ uint32_t &line_no);
/// Find the file and line number of the source location of the end of the
/// function.
diff --git a/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp b/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
index 8e7386df03e9bf..a94e9e23163d3e 100644
--- a/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -139,21 +139,23 @@ void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list) {
if (!sc.block)
continue;
- FileSpec file;
+ SupportFileSP file_sp;
uint32_t line;
const Block *inline_block = sc.block->GetContainingInlinedBlock();
if (inline_block) {
const Declaration &inline_declaration = inline_block->GetInlinedFunctionInfo()->GetDeclaration();
if (!inline_declaration.IsValid())
continue;
- file = inline_declaration.GetFile();
+ file_sp = std::make_shared<SupportFile>(inline_declaration.GetFile());
line = inline_declaration.GetLine();
} else if (sc.function)
- sc.function->GetStartLineSourceInfo(file, line);
+ sc.function->GetStartLineSourceInfo(file_sp, line);
else
continue;
- if (file != sc.line_entry.GetFile()) {
+ if (!file_sp ||
+ !file_sp->Equal(*sc.line_entry.file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet)) {
LLDB_LOG(log, "unexpected symbol context file {0}",
sc.line_entry.GetFile());
continue;
@@ -190,7 +192,8 @@ void BreakpointResolverFileLine::FilterContexts(SymbolContextList &sc_list) {
const int decl_line_is_too_late_fudge = 1;
if (line &&
m_location_spec.GetLine() < line - decl_line_is_too_late_fudge) {
- LLDB_LOG(log, "removing symbol context at {0}:{1}", file, line);
+ LLDB_LOG(log, "removing symbol context at {0}:{1}",
+ file_sp->GetSpecOnly(), line);
sc_list.RemoveContextAtIndex(i);
--i;
}
diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp
index 86c090f9f36c16..c8295fd10cf221 100644
--- a/lldb/source/Commands/CommandObjectSource.cpp
+++ b/lldb/source/Commands/CommandObjectSource.cpp
@@ -784,9 +784,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
if (sc.block == nullptr) {
// Not an inlined function
- FileSpec function_file_spec;
- sc.function->GetStartLineSourceInfo(function_file_spec, start_line);
- start_file = std::make_shared<SupportFile>(function_file_spec);
+ sc.function->GetStartLineSourceInfo(start_file, start_line);
if (start_line == 0) {
result.AppendErrorWithFormat("Could not find line information for "
"start of function: \"%s\".\n",
diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp
index 522a3ef2efc334..68e52144eb6ef8 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -205,16 +205,20 @@ Disassembler::GetFunctionDeclLineEntry(const SymbolContext &sc) {
return {};
LineEntry prologue_end_line = sc.line_entry;
- FileSpec func_decl_file;
+ SupportFileSP func_decl_file_sp;
uint32_t func_decl_line;
- sc.function->GetStartLineSourceInfo(func_decl_file, func_decl_line);
+ sc.function->GetStartLineSourceInfo(func_decl_file_sp, func_decl_line);
- if (func_decl_file != prologue_end_line.GetFile() &&
- func_decl_file != prologue_end_line.original_file_sp->GetSpecOnly())
+ if (!func_decl_file_sp)
+ return {};
+ if (!func_decl_file_sp->Equal(*prologue_end_line.file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet) &&
+ !func_decl_file_sp->Equal(*prologue_end_line.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet))
return {};
SourceLine decl_line;
- decl_line.file = func_decl_file;
+ decl_line.file = func_decl_file_sp->GetSpecOnly();
decl_line.line = func_decl_line;
// TODO: Do we care about column on these entries? If so, we need to plumb
// that through GetStartLineSourceInfo.
@@ -410,20 +414,24 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
LineEntry prologue_end_line = sc.line_entry;
if (!ElideMixedSourceAndDisassemblyLine(exe_ctx, sc,
prologue_end_line)) {
- FileSpec func_decl_file;
+ SupportFileSP func_decl_file_sp;
uint32_t func_decl_line;
- sc.function->GetStartLineSourceInfo(func_decl_file,
+ sc.function->GetStartLineSourceInfo(func_decl_file_sp,
func_decl_line);
- if (func_decl_file == prologue_end_line.GetFile() ||
- func_decl_file ==
- prologue_end_line.original_file_sp->GetSpecOnly()) {
+ if (func_decl_file_sp &&
+ (func_decl_file_sp->Equal(
+ *prologue_end_line.file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet) ||
+ func_decl_file_sp->Equal(
+ *prologue_end_line.original_file_sp,
+ SupportFile::eEqualFileSpecAndChecksumIfSet))) {
// Add all the lines between the function declaration and
// the first non-prologue source line to the list of lines
// to print.
for (uint32_t lineno = func_decl_line;
lineno <= prologue_end_line.line; lineno++) {
SourceLine this_line;
- this_line.file = func_decl_file;
+ this_line.file = func_decl_file_sp->GetSpecOnly();
this_line.line = lineno;
source_lines_to_display.lines.push_back(this_line);
}
diff --git a/lldb/source/Symbol/Function.cpp b/lldb/source/Symbol/Function.cpp
index f590cf4632bce0..0186d63e72879c 100644
--- a/lldb/source/Symbol/Function.cpp
+++ b/lldb/source/Symbol/Function.cpp
@@ -287,10 +287,10 @@ Function::Function(CompileUnit *comp_unit, lldb::user_id_t func_uid,
Function::~Function() = default;
-void Function::GetStartLineSourceInfo(FileSpec &source_file,
+void Function::GetStartLineSourceInfo(SupportFileSP &source_file_sp,
uint32_t &line_no) {
line_no = 0;
- source_file.Clear();
+ source_file_sp.reset();
if (m_comp_unit == nullptr)
return;
@@ -299,7 +299,8 @@ void Function::GetStartLineSourceInfo(FileSpec &source_file,
GetType();
if (m_type != nullptr && m_type->GetDeclaration().GetLine() != 0) {
- source_file = m_type->GetDeclaration().GetFile();
+ source_file_sp =
+ std::make_shared<SupportFile>(m_type->GetDeclaration().GetFile());
line_no = m_type->GetDeclaration().GetLine();
} else {
LineTable *line_table = m_comp_unit->GetLineTable();
@@ -310,7 +311,7 @@ void Function::GetStartLineSourceInfo(FileSpec &source_file,
if (line_table->FindLineEntryByAddress(GetAddressRange().GetBaseAddress(),
line_entry, nullptr)) {
line_no = line_entry.line;
- source_file = line_entry.GetFile();
+ source_file_sp = line_entry.file_sp;
}
}
}
diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
index ef71fc6acb1703..dbb31277b1e8ab 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -1918,15 +1918,15 @@ bool StackFrame::GetStatus(Stream &strm, bool show_frame_info, bool show_source,
if (m_sc.comp_unit && m_sc.line_entry.IsValid()) {
have_debuginfo = true;
if (source_lines_before > 0 || source_lines_after > 0) {
+ SupportFileSP source_file_sp = m_sc.line_entry.file_sp;
uint32_t start_line = m_sc.line_entry.line;
if (!start_line && m_sc.function) {
- FileSpec source_file;
- m_sc.function->GetStartLineSourceInfo(source_file, start_line);
+ m_sc.function->GetStartLineSourceInfo(source_file_sp, start_line);
}
size_t num_lines =
target->GetSourceManager().DisplaySourceLinesWithLineNumbers(
- m_sc.line_entry.file_sp, start_line, m_sc.line_entry.column,
+ source_file_sp, start_line, m_sc.line_entry.column,
source_lines_before, source_lines_after, "->", &strm);
if (num_lines != 0)
have_source = true;
diff --git a/lldb/test/API/source-manager/TestSourceManager.py b/lldb/test/API/source-manager/TestSourceManager.py
index ad7c85aac70eaf..5fd4020715992d 100644
--- a/lldb/test/API/source-manager/TestSourceManager.py
+++ b/lldb/test/API/source-manager/TestSourceManager.py
@@ -314,19 +314,28 @@ def test_set_breakpoint_with_absolute_path(self):
)
def test_artificial_source_location(self):
- src_file = "artificial_location.c"
- d = {"C_SOURCES": src_file}
+ src_file = "artificial_location.cpp"
+ d = {"C_SOURCES":"", "CXX_SOURCES": src_file}
self.build(dictionary=d)
- lldbutil.run_to_source_breakpoint(
- self, "main", lldb.SBFileSpec(src_file, False)
- )
+ target = lldbutil.run_to_breakpoint_make_target(self)
+
+ # Find the instruction with line=0 and put a breakpoint there.
+ sc_list = target.FindFunctions("A::foo")
+ self.assertEqual(len(sc_list), 1)
+ insns = sc_list[0].function.GetInstructions(target)
+ insn0 = next(filter(lambda insn: insn.addr.line_entry.line == 0, insns))
+ bkpt = target.BreakpointCreateBySBAddress(insn0.addr)
+ self.assertGreater(bkpt.GetNumLocations(), 0)
+
+ lldbutil.run_to_breakpoint_do_run(self, target, bkpt)
self.expect(
"process status",
substrs=[
"stop reason = breakpoint",
f"{src_file}:0",
+ "static int foo();",
"Note: this address is compiler-generated code in function",
"that has no source code associated with it.",
],
diff --git a/lldb/test/API/source-manager/artificial_location.c b/lldb/test/API/source-manager/artificial_location.c
deleted file mode 100644
index e1edd8d56b1aa6..00000000000000
--- a/lldb/test/API/source-manager/artificial_location.c
+++ /dev/null
@@ -1,6 +0,0 @@
-int foo() { return 42; }
-
-int main() {
-#line 0
- return foo();
-}
diff --git a/lldb/test/API/source-manager/artificial_location.cpp b/lldb/test/API/source-manager/artificial_location.cpp
new file mode 100644
index 00000000000000..e5f03b31393a57
--- /dev/null
+++ b/lldb/test/API/source-manager/artificial_location.cpp
@@ -0,0 +1,10 @@
+#include "artificial_location.h"
+
+int A::foo() {
+#line 0
+ return 42;
+}
+
+int main() {
+ return A::foo();
+}
diff --git a/lldb/test/API/source-manager/artificial_location.h b/lldb/test/API/source-manager/artificial_location.h
new file mode 100644
index 00000000000000..1e403593436576
--- /dev/null
+++ b/lldb/test/API/source-manager/artificial_location.h
@@ -0,0 +1,3 @@
+struct A {
+ static int foo();
+};
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
When retrieving the location of the function declaration, we were dropping the file component on the floor, which resulted in an amusingly confusing situation were we displayed the file containing the implementation of the function, but used the line number of the declaration. This patch fixes that. It required a small refactor Function::GetStartLineSourceLineInfo to return a SupportFile (instead of just the file spec), which in turn necessitated changes in a couple of other places as well.
JDevlieghere
approved these changes
Nov 12, 2024
| @@ -205,16 +205,20 @@ Disassembler::GetFunctionDeclLineEntry(const SymbolContext &sc) { | |||
| return {}; | |||
|
|
|||
| LineEntry prologue_end_line = sc.line_entry; | |||
| FileSpec func_decl_file; | |||
| SupportFileSP func_decl_file_sp; | |||
There was a problem hiding this comment.
As you've noticed, SupportFileSP is always valid in a few places (like in LineEntry). I don't think that's something that should generally hold (mostly because it's hard to enforce) so I would have done the same thing here. The alternative would be to initialize this to an empty SupportFile and have GetStartLineSourceInfo do the same instead of calling ::reset, though I think that's pretty inefficient. Anyway, just an observation, no need to change anything.
Michael137
reviewed
Nov 12, 2024
| #include "artificial_location.h" | ||
|
|
||
| int A::foo() { | ||
| #line 0 |
There was a problem hiding this comment.
FYI there's some discussion about this test going on in #107849, in case you had some thoughts on Jeremy's points
There was a problem hiding this comment.
Thanks for the pointer. Quite a coincidence that I end up touching the same test just as Jaremy ends up breaking it :P
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When retrieving the location of the function declaration, we were dropping the file component on the floor, which resulted in an amusingly confusing situation were we displayed the file containing the implementation of the function, but used the line number of the declaration. This patch fixes that.
It required a small refactor Function::GetStartLineSourceLineInfo to return a SupportFile (instead of just the file spec), which in turn necessitated changes in a couple of other places as well.