[lldb]Make list command work with headers when possible.
#139002
Conversation
|
@llvm/pr-subscribers-lldb Author: Vy Nguyen (oontvoo) ChangesFull diff: https://github.com/llvm/llvm-project/pull/139002.diff 3 Files Affected:
diff --git a/lldb/source/Commands/CommandObjectSource.cpp b/lldb/source/Commands/CommandObjectSource.cpp
index c205813565d52..475317021255c 100644
--- a/lldb/source/Commands/CommandObjectSource.cpp
+++ b/lldb/source/Commands/CommandObjectSource.cpp
@@ -1104,6 +1104,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
bool check_inlines = false;
SymbolContextList sc_list;
size_t num_matches = 0;
+ uint32_t start_line = m_options.start_line;
if (!m_options.modules.empty()) {
ModuleList matching_modules;
@@ -1114,7 +1115,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
matching_modules.Clear();
target.GetImages().FindModules(module_spec, matching_modules);
num_matches += matching_modules.ResolveSymbolContextForFilePath(
- filename, 0, check_inlines,
+ filename, start_line, check_inlines,
SymbolContextItem(eSymbolContextModule |
eSymbolContextCompUnit),
sc_list);
@@ -1122,7 +1123,7 @@ class CommandObjectSourceList : public CommandObjectParsed {
}
} else {
num_matches = target.GetImages().ResolveSymbolContextForFilePath(
- filename, 0, check_inlines,
+ filename, start_line, check_inlines,
eSymbolContextModule | eSymbolContextCompUnit, sc_list);
}
@@ -1170,8 +1171,37 @@ class CommandObjectSourceList : public CommandObjectParsed {
if (m_options.num_lines == 0)
m_options.num_lines = 10;
const uint32_t column = 0;
+
+ // Headers aren't always in the DWARF but if they have
+ // executable code (eg., inlined-functions) then the callsite's file(s)
+ // will be found.
+ // So if a header was requested and we got a primary file, then look
+ // thru its support file(s) for the header.
+ lldb::SupportFileSP actual_file_sp =
+ sc.comp_unit->GetPrimarySupportFile();
+ if (llvm::StringRef(m_options.file_name).ends_with(".h")) {
+ int support_matches_count = 0;
+ for (auto &file : sc.comp_unit->GetSupportFiles()) {
+ if (llvm::StringRef(file->GetSpecOnly().GetPath()).ends_with(filename)) {
+ actual_file_sp = file;
+ ++support_matches_count;
+ }
+ }
+ if (support_matches_count == 0) {
+ result.AppendErrorWithFormat(
+ "No file found for requested header: \"%s.\"\n",
+ m_options.file_name.c_str());
+ return;
+ } else if (support_matches_count > 1) {
+ result.AppendErrorWithFormat(
+ "Multiple files found for requested header: \"%s.\"\n",
+ m_options.file_name.c_str());
+ return;
+ }
+ }
+
target.GetSourceManager().DisplaySourceLinesWithLineNumbers(
- sc.comp_unit->GetPrimarySupportFile(),
+ actual_file_sp,
m_options.start_line, column, 0, m_options.num_lines, "",
&result.GetOutputStream(), GetBreakpointLocations());
diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index d5ddf6e846112..90c6a62727734 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -714,6 +714,8 @@ uint32_t ModuleList::ResolveSymbolContextsForFileSpec(
const FileSpec &file_spec, uint32_t line, bool check_inlines,
SymbolContextItem resolve_scope, SymbolContextList &sc_list) const {
std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
+ // If we're looking for a header (not source), then need to check inline.
+ check_inlines = check_inlines || !file_spec.IsSourceImplementationFile();
for (const ModuleSP &module_sp : m_modules) {
module_sp->ResolveSymbolContextsForFileSpec(file_spec, line, check_inlines,
resolve_scope, sc_list);
diff --git a/lldb/test/Shell/Commands/list-header.test b/lldb/test/Shell/Commands/list-header.test
new file mode 100644
index 0000000000000..ca700cd2b2fb1
--- /dev/null
+++ b/lldb/test/Shell/Commands/list-header.test
@@ -0,0 +1,59 @@
+## Test that `list header.h:<line>` works correctly when header is available.
+##
+# REQUIRES: x86
+# RUN: split-file %s %t
+
+# RUN: %clang_host -g %t/main_with_inlined.cc %t/foo.cc -o %t/main_with_inlined.out
+# RUN: %clang_host -g %t/main_no_inlined.cc %t/foo.cc -o %t/main_no_inlined.out
+
+# RUN: %lldb %t/main_with_inlined.out -o "list foo.h:2" -o "exit" 2>&1 \
+# RUN: | FileCheck %s --check-prefix=CHECK-INLINED
+
+## Would be nice if this listed the header too - but probably not something
+## we want to support right now.
+# RUN: echo quit | %lldb %t/main_no_inlined.out -o "list foo.h:2" -o "exit" 2>&1 \
+# RUN: | FileCheck %s --check-prefix=CHECK-NO-INLINED
+
+# CHECK-INLINED: 2 extern int* ptr;
+# CHECK-INLINED: 3 void f(int x);
+# CHECK-INLINED: 4
+# CHECK-INLINED: 5 inline void g(int x) {
+# CHECK-INLINED: 6 *ptr = x; // should raise a SIGILL
+# CHECK-INLINED: 7 }
+
+# CHECK-NO-INLINED: error: Could not find source file "foo.h".
+
+#--- foo.h
+// foo.h
+extern int* ptr;
+void f(int x);
+
+inline void g(int x) {
+ *ptr = x; // should raise a SIGILL
+}
+
+#--- foo.cc
+#include "foo.h"
+
+int* ptr;
+
+void f(int x) {
+ *ptr = x;
+}
+
+#--- main_with_inlined.cc
+#include "foo.h"
+
+int main() {
+ f(234);
+ g(123); // Call the inlined function
+ return 0;
+}
+
+#--- main_no_inlined.cc
+#include "foo.h"
+
+int main() {
+ f(234);
+ return 0;
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| // thru its support file(s) for the header. | ||
| lldb::SupportFileSP actual_file_sp = | ||
| sc.comp_unit->GetPrimarySupportFile(); | ||
| if (llvm::StringRef(m_options.file_name).ends_with(".h")) { |
There was a problem hiding this comment.
There are many other "header file" extensions: .H, .hpp, .hh, etc. Also .inc is used sometimes.
The standard headers don't have any extension at all: <algorithm> etc.
Is there a way to fix this without hard-coding one particular extension?
There was a problem hiding this comment.
How about we change to check to if the file that was found has different name from the requested?
(That should cover most cases?)
| } | ||
| if (support_matches_count == 0) { | ||
| result.AppendErrorWithFormat( | ||
| "No file found for requested header: \"%s.\"\n", filename); |
There was a problem hiding this comment.
The error message implies that you're looking for a header, but maybe somebody's not looking for a header per se. You could do something like #include "foo.inc" and maybe a developer wouldn't conceptualize that as including a header.
Maybe a more generic message like Failed to find requested file?
| @@ -0,0 +1,58 @@ | |||
| ## Test that `list header.h:<line>` works correctly when header is available. | |||
| ## | |||
| # REQUIRES: x86 | |||
There was a problem hiding this comment.
Why does this test require x86?
There was a problem hiding this comment.
It seems rather wasteful (and error-prone) to repeat the search through the support files of a compile unit, given that ResolveSymbolContextForFilePath has already searched through them and told us that the CU really does contain a file with the given name. The problem is that the return value is just the CU itself, so all the information we get is "the file is somewhere in this CU".
However, this function is also (and primarily) used for setting file+line breakpoints, and you definitely can set breakpoints in the header files. So, how does that work?
When setting a breakpoint, we pass eSymbolContextLineEntry to the function so that it returns a line entry matching the query. In this case, we're not really interested in the entire line entry, but I think it should be possible the line entry as a carrier for the file name: we ask for the function to fill it out, and then fetch the file from there. Can you see if that works?
I also think that changing 0 to start_line was not right change. Since we're not actually line entry (just the file name, we'll extract the lines ourselves), we want to maximize our chances of finding a matching line entry. If it cannot find an exact file+line match, ResolveSymbolContextForFilePath will return a line entry for any line that comes after it. Using zero ensures we don't skip any line entries.
Yes - that worked! Thanks!
Leaving the line-number as |
Line zero means "I don't know", so it's possible that it's treated specially (you could step through the code to see if it bails out somewhere). Maybe we should use line 1 than as that's the first real line number? |
done! thanks! |
Given this simple test case:
When you run
main.outunder lldb, it'd stop insidevoid g(int)because of the crash.The stack trace would show that it had crashed in
foo.h, but if you dolist foo.h:2, lldb would complain that it could not find the source file, which is confusing.This patch make
listwork with headers.