Skip to content

Add the ability to break on call-site locations, improve inline stepping #112939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Oct 28, 2024
36 changes: 36 additions & 0 deletions lldb/include/lldb/Breakpoint/BreakpointLocation.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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;
Expand All @@ -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);

Expand Down Expand Up @@ -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;
Expand Down
5 changes: 5 additions & 0 deletions lldb/include/lldb/Breakpoint/BreakpointSite.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion lldb/include/lldb/Core/Declaration.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down
12 changes: 12 additions & 0 deletions lldb/include/lldb/Target/StopInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lldb/include/lldb/Target/ThreadPlanStepInRange.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 &
Expand Down
63 changes: 61 additions & 2 deletions lldb/source/Breakpoint/BreakpointLocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +516 to +519
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jimingham jimingham Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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();
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a (if depth > 1000) return {} condition to deal with completely broken debug info?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return {};
}

void BreakpointLocation::SwapLocation(BreakpointLocationSP swap_from) {
m_address = swap_from->m_address;
m_should_resolve_indirect_functions =
Expand Down
15 changes: 15 additions & 0 deletions lldb/source/Breakpoint/BreakpointResolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 17 additions & 0 deletions lldb/source/Breakpoint/BreakpointSite.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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]; }
Expand Down
5 changes: 3 additions & 2 deletions lldb/source/Core/Declaration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Symbol/Block.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
Loading