Add the ability to break on call-site locations, improve inline stepping #112939
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
position in the virtual inlined call stack when they are hit, and step through the inlined stack from there.
jimingham
requested review from
kastiglione
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesPreviously lldb didn't support setting breakpoints on call site locations. This patch adds that ability. It would be very slow if we did this by searching all the debug information for every inlined subroutine record looking for a call-site match, so I added one restriction to the call-site support. This change will find all call sites for functions that also supply at least one line to the regular line table. That way we can use the fact that the line table search will move the location to that subsequent line (but only within the same function). When we find an actually moved source line match, we can search in the function that contained that line table entry for the call-site, and set the breakpoint location back to that. When I started writing tests for this new ability, it quickly became obvious that our support for virtual inline stepping was pretty buggy. We didn't print the right file & line number for the breakpoint, and we didn't set the position in the "virtual inlined stack" correctly when we hit the breakpoint. We also didn't step through the inlined frames correctly. There was code to try to detect the right inlined stack position, but it had been refactored a while back with the comment that it was super confusing and the refactor was supposed to make it clearer, but the refactor didn't work either. That code was made much clearer by abstracting the job of "handling the stack readjustment" to the various StopInfo's. Previously, there was a big (and buggy) switch over stop info's. Moving the responsibility to the stop info made this code much easier to reason about. We also had no tests for virtual inlined stepping (our inlined stepping test was actually written specifically to avoid the formation of a virtual inlined stack... So I also added tests for that along with the tests for setting the call-site breakpoints. Patch is 38.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112939.diff 16 Files Affected:
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index cca00335bc3c67..f9c258daf137f7 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"
@@ -281,6 +283,18 @@ 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.
+ void SetPreferredLineEntry(const LineEntry &line_entry) {
+ m_preferred_line_entry = line_entry;
+ }
+
+ const std::optional<LineEntry> GetPreferredLineEntry() {
+ return m_preferred_line_entry;
+ }
protected:
friend class BreakpointSite;
@@ -305,6 +319,16 @@ class BreakpointLocation
/// It also takes care of decrementing the ignore counters.
/// 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 +393,13 @@ 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.
+ std::optional<LineEntry> m_preferred_line_entry; // 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.
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..30cb5a80b908e0 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointSite.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointSite.h
@@ -169,6 +169,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.
///
diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h
index fae90364deaf0a..d997e0fd6fc550 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -77,6 +77,17 @@ 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..6bd79e1d287d35 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 35058a713aef86..61fe467987228b 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -508,8 +508,19 @@ 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 (GetPreferredLineEntry()) {
+ sc.line_entry = *GetPreferredLineEntry();
+ // 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 +548,10 @@ void BreakpointLocation::GetDescription(Stream *s,
if (sc.line_entry.line > 0) {
s->EOL();
s->Indent("location = ");
- sc.line_entry.DumpStopContext(s, true);
+ if (GetPreferredLineEntry())
+ GetPreferredLineEntry()->DumpStopContext(s, true);
+ else
+ sc.line_entry.DumpStopContext(s, true);
}
} else {
@@ -656,6 +670,49 @@ void BreakpointLocation::SendBreakpointLocationChangedEvent(
}
}
+std::optional<uint32_t> BreakpointLocation::GetSuggestedStackFrameIndex() {
+ if (!GetPreferredLineEntry())
+ return {};
+ LineEntry preferred = *GetPreferredLineEntry();
+ 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..465fc387aa43e5 100644
--- a/lldb/source/Breakpoint/BreakpointResolver.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolver.cpp
@@ -340,6 +340,18 @@ 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.
+ bp_loc_sp->SetPreferredLineEntry(sc.line_entry);
+ }
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..b283952d028305 100644
--- a/lldb/source/Breakpoint/BreakpointSite.cpp
+++ b/lldb/source/Breakpoint/BreakpointSite.cpp
@@ -87,6 +87,22 @@ 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> this_result = loc_sp->GetSuggestedStackFrameIndex();
+ if (this_result) {
+ if (!result)
+ result = this_result;
+ else
+ result = std::max(*this_result, *result);
+ }
+ }
+ 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..e7855f3e364376 100644
--- a/lldb/source/Core/Declaration.cpp
+++ b/lldb/source/Core/Declaration.cpp
@@ -71,7 +71,7 @@ int Declaration::Compare(const Declaration &a, const Declaration &b) {
}
bool Declaration::FileAndLineEqual(const Declaration &declaration) const {
- int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, true);
+ int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, false);
return file_compare == 0 && this->m_line == declaration.m_line;
}
diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp
index db8f8ce6bcbc92..f4c61d6ec1b196 100644
--- a/lldb/source/Symbol/CompileUnit.cpp
+++ b/lldb/source/Symbol/CompileUnit.cpp
@@ -251,7 +251,8 @@ 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
@@ -311,6 +312,107 @@ void CompileUnit::ResolveSymbolContext(
line_idx = line_table->FindLineEntryIndexByFileIndex(
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)
+ && (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->GetBlock(true);
+ examine_block(func_block);
+ }
+ // If we found entries here, we are done. We only get here because we
+ // didn't find an exact line entry for this line & column, but if we found
+ // an exact match from the call site info that's strictly better than
+ // continuing to look for matches further on in the file.
+ // FIXME: Should I also do this for "call site line exists between the
+ // given line number and the later line we found in the line table"? That's
+ // a closer approximation to our general sliding algorithm.
+ if (sc_list.GetSize())
+ return;
+ }
// If "exact == true", then "found_line" will be the same as "line". If
// "exact == false", the "found_line" will be the closest line entry
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index 3849ec5ed178d9..cda360baed6491 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -86,120 +86,31 @@ void StackFrameList::ResetCurrentInlinedDepth() {
std::lock_guard<std::recursive_mutex> guard(m_mutex);
- GetFramesUpTo(0, DoNotAllowInterruption);
- if (m_frames.empty())
- return;
- if (!m_frames[0]->IsInlined()) {
- m_current_inlined_depth = UINT32_MAX;
- m_current_inlined_pc = LLDB_INVALID_ADDRESS;
- Log *log = GetLog(LLDBLog::Step);
- if (log && log->GetVerbose())
- LLDB_LOGF(
- log,
- "ResetCurrentInlinedDepth: Invalidating current inlined depth.\n");
- return;
- }
+ m_current_inlined_pc = LLDB_INVALID_ADDRESS;
+ m_current_inlined_depth = UINT32_MAX;
- // We only need to do something special about inlined blocks when we are
- // at the beginning of an inlined function:
- // FIXME: We probably also have to do something special if the PC is at
- // the END of an inlined function, which coincides with the end of either
- // its containing function or another inlined function.
-
- Block *block_ptr = m_frames[0]->GetFrameBlock();
- if (!block_ptr)
- return;
-
- Address pc_as_address;
- lldb::addr_t curr_pc = m_thread.GetRegisterContext()->GetPC();
- pc_as_address.SetLoadAddress(curr_pc, &(m_thread.GetProcess()->GetTarget()));
- AddressRange containing_range;
- if (!block_ptr->GetR...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| // 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. |
There was a problem hiding this comment.
Nit: these should all be Doxygen comments using ///
| // 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. |
| @@ -369,6 +393,13 @@ 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. | |||
| std::optional<LineEntry> m_preferred_line_entry; // If this exists, use it to print the stop | |||
There was a problem hiding this comment.
IMO the end-of line comment style only makes sense if the comment fits on one line.
Suggested change
| std::optional<LineEntry> m_preferred_line_entry; // If this exists, use it to print the stop | |
| /// If this exists, use it to print the stop description rather than the LineEntry | |
| /// ... | |
| std::optional<LineEntry> m_preferred_line_entry; |
There was a problem hiding this comment.
And if you use the Doxygen comment syntax, IDEs will know to associate the comment with the following declaration.
| return depth + 1; | ||
| } | ||
| inlined_block = inlined_block->GetInlinedParent(); | ||
| depth++; |
There was a problem hiding this comment.
Should there be a (if depth > 1000) return {} condition to deal with completely broken debug info?
There was a problem hiding this comment.
That would mean there were cycles in the blocks? I don't think we guard against that anywhere else that we're traversing the block structure.
| // callsite. | ||
| const InlineFunctionInfo *inline_info = | ||
| sibling_block->GetInlinedFunctionInfo(); | ||
| if (inline_info) { |
There was a problem hiding this comment.
Suggested change
| if (inline_info) { | |
| if (!inline_info) | |
| continue; |
There was a problem hiding this comment.
I don't think that's right. We still need to do the bit after the if block to recurse the children and set the next sibling block. I'd have to have a "goto", you can't just continue here.
| """Test stepping through a virtual inlined call stack""" | ||
| self.build() | ||
| self.virtual_inline_stepping() | ||
|
|
||
| def setUp(self): |
There was a problem hiding this comment.
I believe this entire function is no longer needed.
There was a problem hiding this comment.
Not sure what you mean. This is the style we often write tests in where we have the to unittest significant test_whatever function be just a shim on the real test function. That makes it easy to see what all the tests are up front without having to wade through the details. That's how all the other tests in this file work as well.
There was a problem hiding this comment.
I was referring to the setUp() function that just calls the parent setUp function. If it's still needed ignore this,.
There was a problem hiding this comment.
Ah. The other tests still need the setUp. I don't need the work it does for the test I added, but it doesn't do very much, so IMO it's not worth making another test class just to avoid calling it.
|
|
||
| self.assertTrue(result, "Got a location description") | ||
| desc = strm.GetData() | ||
| print(f"Description:\n{desc}\n") |
There was a problem hiding this comment.
should this be if self.TraceOn():?
There was a problem hiding this comment.
Debugging detritus, I'll just remove it.
lldb/source/Symbol/CompileUnit.cpp
Outdated
| // it. | ||
| Declaration found_decl = inline_info->GetCallSite(); | ||
| uint32_t sought_column = sought_decl.GetColumn(); | ||
| if (found_decl.FileAndLineEqual(sought_decl) && |
There was a problem hiding this comment.
Suggested change
| if (found_decl.FileAndLineEqual(sought_decl) && | |
| if (!found_decl.FileAndLineEqual(sought_decl)) | |
| continue; | |
| if (sought_column != LLDB_INVALID_COLUMN_NUMBER && | |
| sought_column != found_decl.GetColumn()) | |
| continue; |
There was a problem hiding this comment.
These can't continue either, they'd need a goto to get the sibling iteration & child recursion at the end of the while.
| // 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( |
adrian-prantl
approved these changes
Oct 18, 2024
Comment on lines
514
to
515
| if (GetPreferredLineEntry()) { | ||
| sc.line_entry = *GetPreferredLineEntry(); |
There was a problem hiding this comment.
nit: avoid 2 function calls
Suggested change
| if (GetPreferredLineEntry()) { | |
| sc.line_entry = *GetPreferredLineEntry(); | |
| if (auto may_be_line_entry = GetPreferredLineEntry()) { | |
| sc.line_entry = *may_be_line_entry; |
clayborg
reviewed
Oct 21, 2024
Comment on lines
+516
to
+519
| // 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. |
There was a problem hiding this comment.
Would a solution that changed GetPreferredLineEntry() to be GetPreferredSymbolContext() where it would return a SymbolContext object where anything that is valid in the SymbolContext would end up overriding the actual symbol context in sc? We might want to add a new function like:
SymbolContext BreakpointLocation::CalculateSymbolContext()
That would calll m_address.CalculateSymbolContext(&sc); and then insert any preferred entries into that symbol context. That would allow anyone to get the right symbol context for a BreakpointLocation easily. Then this function would change or need to know anything about the preferred stuff.
There was a problem hiding this comment.
What would it mean to replace the Module, the CompileUnit or the Function? This is for a breakpoint location, so whatever we do has to result in this being the same address. I was hesitant to introduce a more general "breakpoint gets to modify its symbol context" if I didn't know what it meant or how to ensure we didn't end up with a SymbolContext that wasn't at the breakpoint location's address. I was originally planning to JUST store the "inlined call stack depth" to make it clear this couldn't change the PC value, but that ended up being more annoying than helpful.
There was a problem hiding this comment.
I added one more change here, since the intention is that you cannot change the PC of the breakpoint Location by setting a PreferredLineEntry, I changed the code to actually check that when you call SetPreferredLineEntry. I put in an assert in the setter, so we can catch any violations of this in the testsuite, and then if this happens IRL, I made the BreakpointLocation log the event and ignore the setting.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/9193 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/6021 Here is the relevant piece of the build log for the reference |
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 28, 2024
…ne stepping (llvm#112939)" This was breaking some gdb-remote packet counting tests on the bots. I can't see how this patch could cause that breakage, but I'm reverting to figure that out. This reverts commit f147437.
jimingham
added a commit
that referenced
this pull request
Oct 30, 2024
This fixes the two test suite failures that I missed in the PR: #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.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…ing (llvm#112939) Previously lldb didn't support setting breakpoints on call site locations. This patch adds that ability. It would be very slow if we did this by searching all the debug information for every inlined subroutine record looking for a call-site match, so I added one restriction to the call-site support. This change will find all call sites for functions that also supply at least one line to the regular line table. That way we can use the fact that the line table search will move the location to that subsequent line (but only within the same function). When we find an actually moved source line match, we can search in the function that contained that line table entry for the call-site, and set the breakpoint location back to that. When I started writing tests for this new ability, it quickly became obvious that our support for virtual inline stepping was pretty buggy. We didn't print the right file & line number for the breakpoint, and we didn't set the position in the "virtual inlined stack" correctly when we hit the breakpoint. We also didn't step through the inlined frames correctly. There was code to try to detect the right inlined stack position, but it had been refactored a while back with the comment that it was super confusing and the refactor was supposed to make it clearer, but the refactor didn't work either. That code was made much clearer by abstracting the job of "handling the stack readjustment" to the various StopInfo's. Previously, there was a big (and buggy) switch over stop info's. Moving the responsibility to the stop info made this code much easier to reason about. We also had no tests for virtual inlined stepping (our inlined stepping test was actually written specifically to avoid the formation of a virtual inlined stack... So I also added tests for that along with the tests for setting the call-site breakpoints.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
llvm#113947) …ne stepping (llvm#112939)" This was breaking some gdb-remote packet counting tests on the bots. I can't see how this patch could cause that breakage, but I'm reverting to figure that out. This reverts commit f147437.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
This fixes the two test suite failures that I missed in the PR: llvm#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.
jimingham
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Nov 7, 2024
This fixes the two test suite failures that I missed in the PR: llvm#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. (cherry picked from commit 7dbbd2b)
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.
Previously lldb didn't support setting breakpoints on call site locations. This patch adds that ability.
It would be very slow if we did this by searching all the debug information for every inlined subroutine record looking for a call-site match, so I added one restriction to the call-site support. This change will find all call sites for functions that also supply at least one line to the regular line table. That way we can use the fact that the line table search will move the location to that subsequent line (but only within the same function). When we find an actually moved source line match, we can search in the function that contained that line table entry for the call-site, and set the breakpoint location back to that.
When I started writing tests for this new ability, it quickly became obvious that our support for virtual inline stepping was pretty buggy. We didn't print the right file & line number for the breakpoint, and we didn't set the position in the "virtual inlined stack" correctly when we hit the breakpoint. We also didn't step through the inlined frames correctly. There was code to try to detect the right inlined stack position, but it had been refactored a while back with the comment that it was super confusing and the refactor was supposed to make it clearer, but the refactor didn't work either.
That code was made much clearer by abstracting the job of "handling the stack readjustment" to the various StopInfo's. Previously, there was a big (and buggy) switch over stop info's. Moving the responsibility to the stop info made this code much easier to reason about.
We also had no tests for virtual inlined stepping (our inlined stepping test was actually written specifically to avoid the formation of a virtual inlined stack... So I also added tests for that along with the tests for setting the call-site breakpoints.