[Lldb-commits] [lldb] Fix call site breakpoint patch (PR #114158)
via lldb-commits
lldb-commits at lists.llvm.org
Tue Oct 29 17:10:55 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: None (jimingham)
<details>
<summary>Changes</summary>
This fixes the two test suite failures that I missed in the PR:
https://github.com/llvm/llvm-project/pull/112939
One was a poorly written test case - it assumed that on connect to a gdb-remote with a running process, lldb MUST have fetched all the frame 0 registers. In fact, there's no need for it to do so (as the CallSite patch showed...) and if we don't need to we shouldn't. So I fixed the test to only expect a `g` packet AFTER calling read_registers.
The other was a place where some code had used 0 when it meant LLDB_INVALID_LINE_NUMBER, which I had fixed but missed one place where it was still compared to 0.
---
Patch is 43.37 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114158.diff
19 Files Affected:
- (modified) lldb/include/lldb/Breakpoint/BreakpointLocation.h (+36)
- (modified) lldb/include/lldb/Breakpoint/BreakpointSite.h (+5)
- (modified) lldb/include/lldb/Core/Declaration.h (+5-1)
- (modified) lldb/include/lldb/Target/StopInfo.h (+12)
- (modified) lldb/include/lldb/Target/ThreadPlanStepInRange.h (+2-2)
- (modified) lldb/source/Breakpoint/BreakpointLocation.cpp (+61-2)
- (modified) lldb/source/Breakpoint/BreakpointResolver.cpp (+15)
- (modified) lldb/source/Breakpoint/BreakpointSite.cpp (+17)
- (modified) lldb/source/Core/Declaration.cpp (+3-2)
- (modified) lldb/source/Symbol/Block.cpp (+1-1)
- (modified) lldb/source/Symbol/CompileUnit.cpp (+111-2)
- (modified) lldb/source/Target/StackFrameList.cpp (+56-115)
- (modified) lldb/source/Target/StopInfo.cpp (+55)
- (modified) lldb/source/Target/Thread.cpp (+8)
- (modified) lldb/source/Target/ThreadPlanStepInRange.cpp (+18-6)
- (modified) lldb/source/Target/ThreadPlanStepOverRange.cpp (+1-1)
- (modified) lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py (+24-4)
- (modified) lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py (+63)
- (modified) lldb/test/API/functionalities/inline-stepping/calling.cpp (+25)
``````````diff
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index cca00335bc3c67..3592291bb2d06e 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -11,10 +11,12 @@
#include <memory>
#include <mutex>
+#include <optional>
#include "lldb/Breakpoint/BreakpointOptions.h"
#include "lldb/Breakpoint/StoppointHitCounter.h"
#include "lldb/Core/Address.h"
+#include "lldb/Symbol/LineEntry.h"
#include "lldb/Utility/UserID.h"
#include "lldb/lldb-private.h"
@@ -282,6 +284,25 @@ class BreakpointLocation
/// Returns the breakpoint location ID.
lldb::break_id_t GetID() const { return m_loc_id; }
+ /// Set the line entry that should be shown to users for this location.
+ /// It is up to the caller to verify that this is a valid entry to show.
+ /// The current use of this is to distinguish among line entries from a
+ /// virtual inlined call stack that all share the same address.
+ /// The line entry must have the same start address as the address for this
+ /// location.
+ bool SetPreferredLineEntry(const LineEntry &line_entry) {
+ if (m_address == line_entry.range.GetBaseAddress()) {
+ m_preferred_line_entry = line_entry;
+ return true;
+ }
+ assert(0 && "Tried to set a preferred line entry with a different address");
+ return false;
+ }
+
+ const std::optional<LineEntry> GetPreferredLineEntry() {
+ return m_preferred_line_entry;
+ }
+
protected:
friend class BreakpointSite;
friend class BreakpointLocationList;
@@ -306,6 +327,16 @@ class BreakpointLocation
/// If it returns false we should continue, otherwise stop.
bool IgnoreCountShouldStop();
+ /// If this location knows that the virtual stack frame it represents is
+ /// not frame 0, return the suggested stack frame instead. This will happen
+ /// when the location's address contains a "virtual inlined call stack" and
+ /// the breakpoint was set on a file & line that are not at the bottom of that
+ /// stack. For now we key off the "preferred line entry" - looking for that
+ /// in the blocks that start with the stop PC.
+ /// This version of the API doesn't take an "inlined" parameter because it
+ /// only changes frames in the inline stack.
+ std::optional<uint32_t> GetSuggestedStackFrameIndex();
+
private:
void SwapLocation(lldb::BreakpointLocationSP swap_from);
@@ -369,6 +400,11 @@ class BreakpointLocation
lldb::break_id_t m_loc_id; ///< Breakpoint location ID.
StoppointHitCounter m_hit_counter; ///< Number of times this breakpoint
/// location has been hit.
+ /// If this exists, use it to print the stop description rather than the
+ /// LineEntry m_address resolves to directly. Use this for instance when the
+ /// location was given somewhere in the virtual inlined call stack since the
+ /// Address always resolves to the lowest entry in the stack.
+ std::optional<LineEntry> m_preferred_line_entry;
void SetShouldResolveIndirectFunctions(bool do_resolve) {
m_should_resolve_indirect_functions = do_resolve;
diff --git a/lldb/include/lldb/Breakpoint/BreakpointSite.h b/lldb/include/lldb/Breakpoint/BreakpointSite.h
index 17b76d51c1ae53..7b3f7be23639f2 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointSite.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointSite.h
@@ -170,6 +170,11 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>,
/// \see lldb::DescriptionLevel
void GetDescription(Stream *s, lldb::DescriptionLevel level);
+ // This runs through all the breakpoint locations owning this site and returns
+ // the greatest of their suggested stack frame indexes. This only handles
+ // inlined stack changes.
+ std::optional<uint32_t> GetSuggestedStackFrameIndex();
+
/// Tell whether a breakpoint has a location at this site.
///
/// \param[in] bp_id
diff --git a/lldb/include/lldb/Core/Declaration.h b/lldb/include/lldb/Core/Declaration.h
index 4a0e9047b54695..c864b88c6b32a3 100644
--- a/lldb/include/lldb/Core/Declaration.h
+++ b/lldb/include/lldb/Core/Declaration.h
@@ -84,10 +84,14 @@ class Declaration {
/// \param[in] declaration
/// The const Declaration object to compare with.
///
+ /// \param[in] full
+ /// Same meaning as Full in FileSpec::Equal. True means an empty
+ /// directory is not equal to a specified one, false means it is equal.
+ ///
/// \return
/// Returns \b true if \b declaration is at the same file and
/// line, \b false otherwise.
- bool FileAndLineEqual(const Declaration &declaration) const;
+ bool FileAndLineEqual(const Declaration &declaration, bool full) const;
/// Dump a description of this object to a Stream.
///
diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h
index fae90364deaf0a..45beac129e86f7 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -77,6 +77,18 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
m_description.clear();
}
+ /// This gives the StopInfo a chance to suggest a stack frame to select.
+ /// Passing true for inlined_stack will request changes to the inlined
+ /// call stack. Passing false will request changes to the real stack
+ /// frame. The inlined stack gets adjusted before we call into the thread
+ /// plans so they can reason based on the correct values. The real stack
+ /// adjustment is handled after the frame recognizers get a chance to adjust
+ /// the frame.
+ virtual std::optional<uint32_t>
+ GetSuggestedStackFrameIndex(bool inlined_stack) {
+ return {};
+ }
+
virtual bool IsValidForOperatingSystemThread(Thread &thread) { return true; }
/// A Continue operation can result in a false stop event
diff --git a/lldb/include/lldb/Target/ThreadPlanStepInRange.h b/lldb/include/lldb/Target/ThreadPlanStepInRange.h
index f9ef87942a7c03..9da8370ef1c925 100644
--- a/lldb/include/lldb/Target/ThreadPlanStepInRange.h
+++ b/lldb/include/lldb/Target/ThreadPlanStepInRange.h
@@ -80,8 +80,8 @@ class ThreadPlanStepInRange : public ThreadPlanStepRange,
bool m_step_past_prologue; // FIXME: For now hard-coded to true, we could put
// a switch in for this if there's
// demand for that.
- bool m_virtual_step; // true if we've just done a "virtual step", i.e. just
- // moved the inline stack depth.
+ LazyBool m_virtual_step; // true if we've just done a "virtual step", i.e.
+ // just moved the inline stack depth.
ConstString m_step_into_target;
ThreadPlanStepInRange(const ThreadPlanStepInRange &) = delete;
const ThreadPlanStepInRange &
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index ad9057c8141e99..c7ea50407ae1c7 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -508,8 +508,20 @@ void BreakpointLocation::GetDescription(Stream *s,
s->PutCString("re-exported target = ");
else
s->PutCString("where = ");
+
+ // If there's a preferred line entry for printing, use that.
+ bool show_function_info = true;
+ if (auto preferred = GetPreferredLineEntry()) {
+ sc.line_entry = *preferred;
+ // FIXME: We're going to get the function name wrong when the preferred
+ // line entry is not the lowest one. For now, just leave the function
+ // out in this case, but we really should also figure out how to easily
+ // fake the function name here.
+ show_function_info = false;
+ }
sc.DumpStopContext(s, m_owner.GetTarget().GetProcessSP().get(), m_address,
- false, true, false, true, true, true);
+ false, true, false, show_function_info,
+ show_function_info, show_function_info);
} else {
if (sc.module_sp) {
s->EOL();
@@ -537,7 +549,10 @@ void BreakpointLocation::GetDescription(Stream *s,
if (sc.line_entry.line > 0) {
s->EOL();
s->Indent("location = ");
- sc.line_entry.DumpStopContext(s, true);
+ if (auto preferred = GetPreferredLineEntry())
+ preferred->DumpStopContext(s, true);
+ else
+ sc.line_entry.DumpStopContext(s, true);
}
} else {
@@ -656,6 +671,50 @@ void BreakpointLocation::SendBreakpointLocationChangedEvent(
}
}
+std::optional<uint32_t> BreakpointLocation::GetSuggestedStackFrameIndex() {
+ auto preferred_opt = GetPreferredLineEntry();
+ if (!preferred_opt)
+ return {};
+ LineEntry preferred = *preferred_opt;
+ SymbolContext sc;
+ if (!m_address.CalculateSymbolContext(&sc))
+ return {};
+ // Don't return anything special if frame 0 is the preferred line entry.
+ // We not really telling the stack frame list to do anything special in that
+ // case.
+ if (!LineEntry::Compare(sc.line_entry, preferred))
+ return {};
+
+ if (!sc.block)
+ return {};
+
+ // Blocks have their line info in Declaration form, so make one here:
+ Declaration preferred_decl(preferred.GetFile(), preferred.line,
+ preferred.column);
+
+ uint32_t depth = 0;
+ Block *inlined_block = sc.block->GetContainingInlinedBlock();
+ while (inlined_block) {
+ // If we've moved to a block that this isn't the start of, that's not
+ // our inlining info or call site, so we can stop here.
+ Address start_address;
+ if (!inlined_block->GetStartAddress(start_address) ||
+ start_address != m_address)
+ return {};
+
+ const InlineFunctionInfo *info = inlined_block->GetInlinedFunctionInfo();
+ if (info) {
+ if (preferred_decl == info->GetDeclaration())
+ return depth;
+ if (preferred_decl == info->GetCallSite())
+ return depth + 1;
+ }
+ inlined_block = inlined_block->GetInlinedParent();
+ depth++;
+ }
+ return {};
+}
+
void BreakpointLocation::SwapLocation(BreakpointLocationSP swap_from) {
m_address = swap_from->m_address;
m_should_resolve_indirect_functions =
diff --git a/lldb/source/Breakpoint/BreakpointResolver.cpp b/lldb/source/Breakpoint/BreakpointResolver.cpp
index 8307689c7640cf..9643602d78c751 100644
--- a/lldb/source/Breakpoint/BreakpointResolver.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolver.cpp
@@ -340,6 +340,21 @@ void BreakpointResolver::AddLocation(SearchFilter &filter,
}
BreakpointLocationSP bp_loc_sp(AddLocation(line_start));
+ // If the address that we resolved the location to returns a different
+ // LineEntry from the one in the incoming SC, we're probably dealing with an
+ // inlined call site, so set that as the preferred LineEntry:
+ LineEntry resolved_entry;
+ if (!skipped_prologue && bp_loc_sp &&
+ line_start.CalculateSymbolContextLineEntry(resolved_entry) &&
+ LineEntry::Compare(resolved_entry, sc.line_entry)) {
+ // FIXME: The function name will also be wrong here. Do we need to record
+ // that as well, or can we figure that out again when we report this
+ // breakpoint location.
+ if (!bp_loc_sp->SetPreferredLineEntry(sc.line_entry)) {
+ LLDB_LOG(log, "Tried to add a preferred line entry that didn't have the "
+ "same address as this location's address.");
+ }
+ }
if (log && bp_loc_sp && !GetBreakpoint()->IsInternal()) {
StreamString s;
bp_loc_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);
diff --git a/lldb/source/Breakpoint/BreakpointSite.cpp b/lldb/source/Breakpoint/BreakpointSite.cpp
index 3ca93f908e30b8..9700a57d3346e0 100644
--- a/lldb/source/Breakpoint/BreakpointSite.cpp
+++ b/lldb/source/Breakpoint/BreakpointSite.cpp
@@ -87,6 +87,23 @@ void BreakpointSite::GetDescription(Stream *s, lldb::DescriptionLevel level) {
m_constituents.GetDescription(s, level);
}
+std::optional<uint32_t> BreakpointSite::GetSuggestedStackFrameIndex() {
+
+ std::optional<uint32_t> result;
+ std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex);
+ for (BreakpointLocationSP loc_sp : m_constituents.BreakpointLocations()) {
+ std::optional<uint32_t> loc_frame_index =
+ loc_sp->GetSuggestedStackFrameIndex();
+ if (loc_frame_index) {
+ if (result)
+ result = std::max(*loc_frame_index, *result);
+ else
+ result = loc_frame_index;
+ }
+ }
+ return result;
+}
+
bool BreakpointSite::IsInternal() const { return m_constituents.IsInternal(); }
uint8_t *BreakpointSite::GetTrapOpcodeBytes() { return &m_trap_opcode[0]; }
diff --git a/lldb/source/Core/Declaration.cpp b/lldb/source/Core/Declaration.cpp
index 579a3999d14ea0..a485c4b9ba48a7 100644
--- a/lldb/source/Core/Declaration.cpp
+++ b/lldb/source/Core/Declaration.cpp
@@ -70,8 +70,9 @@ int Declaration::Compare(const Declaration &a, const Declaration &b) {
return 0;
}
-bool Declaration::FileAndLineEqual(const Declaration &declaration) const {
- int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, true);
+bool Declaration::FileAndLineEqual(const Declaration &declaration,
+ bool full) const {
+ int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, full);
return file_compare == 0 && this->m_line == declaration.m_line;
}
diff --git a/lldb/source/Symbol/Block.cpp b/lldb/source/Symbol/Block.cpp
index f7d9c0d2d33065..5c7772a6db780d 100644
--- a/lldb/source/Symbol/Block.cpp
+++ b/lldb/source/Symbol/Block.cpp
@@ -230,7 +230,7 @@ Block *Block::GetContainingInlinedBlockWithCallSite(
const auto *function_info = inlined_block->GetInlinedFunctionInfo();
if (function_info &&
- function_info->GetCallSite().FileAndLineEqual(find_call_site))
+ function_info->GetCallSite().FileAndLineEqual(find_call_site, true))
return inlined_block;
inlined_block = inlined_block->GetInlinedParent();
}
diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp
index db8f8ce6bcbc92..73389b2e8479b3 100644
--- a/lldb/source/Symbol/CompileUnit.cpp
+++ b/lldb/source/Symbol/CompileUnit.cpp
@@ -251,7 +251,10 @@ void CompileUnit::ResolveSymbolContext(
SymbolContextItem resolve_scope, SymbolContextList &sc_list,
RealpathPrefixes *realpath_prefixes) {
const FileSpec file_spec = src_location_spec.GetFileSpec();
- const uint32_t line = src_location_spec.GetLine().value_or(0);
+ const uint32_t line =
+ src_location_spec.GetLine().value_or(LLDB_INVALID_LINE_NUMBER);
+ const uint32_t column_num =
+ src_location_spec.GetColumn().value_or(LLDB_INVALID_COLUMN_NUMBER);
const bool check_inlines = src_location_spec.GetCheckInlines();
// First find all of the file indexes that match our "file_spec". If
@@ -268,7 +271,7 @@ void CompileUnit::ResolveSymbolContext(
SymbolContext sc(GetModule());
sc.comp_unit = this;
- if (line == 0) {
+ if (line == LLDB_INVALID_LINE_NUMBER) {
if (file_spec_matches_cu_file_spec && !check_inlines) {
// only append the context if we aren't looking for inline call sites by
// file and line and if the file spec matches that of the compile unit
@@ -312,6 +315,112 @@ void CompileUnit::ResolveSymbolContext(
0, file_indexes, src_location_spec, &line_entry);
}
+ // If we didn't manage to find a breakpoint that matched the line number
+ // requested, that might be because it is only an inline call site, and
+ // doesn't have a line entry in the line table. Scan for that here.
+ //
+ // We are making the assumption that if there was an inlined function it will
+ // contribute at least 1 non-call-site entry to the line table. That's handy
+ // because we don't move line breakpoints over function boundaries, so if we
+ // found a hit, and there were also a call site entry, it would have to be in
+ // the function containing the PC of the line table match. That way we can
+ // limit the call site search to that function.
+ // We will miss functions that ONLY exist as a call site entry.
+
+ if (line_entry.IsValid() &&
+ (line_entry.line != line || line_entry.column != column_num) &&
+ resolve_scope & eSymbolContextLineEntry && check_inlines) {
+ // We don't move lines over function boundaries, so the address in the
+ // line entry will be the in function that contained the line that might
+ // be a CallSite, and we can just iterate over that function to find any
+ // inline records, and dig up their call sites.
+ Address start_addr = line_entry.range.GetBaseAddress();
+ Function *function = start_addr.CalculateSymbolContextFunction();
+
+ Declaration sought_decl(file_spec, line, column_num);
+ // We use this recursive function to descend the block structure looking
+ // for a block that has this Declaration as in it's CallSite info.
+ // This function recursively scans the sibling blocks of the incoming
+ // block parameter.
+ std::function<void(Block &)> examine_block =
+ [&sought_decl, &sc_list, &src_location_spec, resolve_scope,
+ &examine_block](Block &block) -> void {
+ // Iterate over the sibling child blocks of the incoming block.
+ Block *sibling_block = block.GetFirstChild();
+ while (sibling_block) {
+ // We only have to descend through the regular blocks, looking for
+ // immediate inlines, since those are the only ones that will have this
+ // callsite.
+ const InlineFunctionInfo *inline_info =
+ sibling_block->GetInlinedFunctionInfo();
+ if (inline_info) {
+ // If this is the call-site we are looking for, record that:
+ // We need to be careful because the call site from the debug info
+ // will generally have a column, but the user might not have specified
+ // it.
+ Declaration found_decl = inline_info->GetCallSite();
+ uint32_t sought_column = sought_decl.GetColumn();
+ if (found_decl.FileAndLineEqual(sought_decl, false) &&
+ (sought_column == LLDB_INVALID_COLUMN_NUMBER ||
+ sought_column == found_decl.GetColumn())) {
+ // If we found a call site, it belongs not in this inlined block,
+ // but in the parent block that inlined it.
+ Address parent_start_addr;
+ if (sibling_block->GetParent()->GetStartAddress(
+ parent_start_addr)) {
+ SymbolContext sc;
+ parent_start_addr.CalculateSymbolContext(&sc, resolve_scope);
+ // Now swap out the line entry for the one we found.
+ LineEntry call_site_line = sc.line_entry;
+ call_site_line.line = found_decl.GetLine();
+ call_site_line.column = found_decl.GetColumn();
+ bool matches_spec = true;
+ // If the user asked for an exact match, we need to make sure the
+ // call site we found actually matches the location.
+ if (src_location_spec.GetExactMatch()) {
+ matches_spec = false;
+ if ((src_location_spec.GetFileSpec() ==
+ sc.line_entry.GetFile()) &&
+ (src_location_spec.GetLine() &&
+ *src_location_spec.GetLine() == call_site_line.line) &&
+ (src_location_spec.GetColumn() &&
+ *src_location_spec.GetColumn() == call_site_line.column))
+ matches_spec = true;
+ }
+ if (matches_spec &&
+ sibling_block->GetRangeAtIndex(0, call_site_line.range)) {
+ SymbolContext call_site_sc(sc.target_sp, sc.module_sp,
+ sc.comp_unit, sc.function, sc.block,
+ &call_site_line, sc.symbol);
+ sc_list.Append(call_site_sc);
+ }
+ }
+ }
+ }
+
+ // Descend into the child blocks:
+ examine_block(*sibling_block);
+ // Now go to the next sibling:
+ sibling_block = sibling_block->GetSibling();
+ }
+ };
+
+ if (function) {
+ // We don't need to examine the function block, it can't be inlined.
+ Block &func_block = function->GetBlo...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/114158
More information about the lldb-commits
mailing list