From 45ac0ba9071d2ecb67aca2f777a677a9243a7a3a Mon Sep 17 00:00:00 2001 From: jimingham Date: Wed, 30 Oct 2024 09:28:38 -0700 Subject: [PATCH 1/6] Fix call site breakpoint patch (#114158) 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. (cherry picked from commit 7dbbd2b251412b7b0809aabe672f3f57f0805dbb) --- .../lldb/Breakpoint/BreakpointLocation.h | 36 ++++ lldb/include/lldb/Breakpoint/BreakpointSite.h | 5 + lldb/include/lldb/Core/Declaration.h | 6 +- lldb/include/lldb/Target/StopInfo.h | 12 ++ .../lldb/Target/ThreadPlanStepInRange.h | 4 +- lldb/source/Breakpoint/BreakpointLocation.cpp | 63 ++++++- lldb/source/Breakpoint/BreakpointResolver.cpp | 15 ++ lldb/source/Breakpoint/BreakpointSite.cpp | 17 ++ lldb/source/Core/Declaration.cpp | 5 +- lldb/source/Symbol/Block.cpp | 2 +- lldb/source/Symbol/CompileUnit.cpp | 113 +++++++++++- lldb/source/Target/StackFrameList.cpp | 171 ++++++------------ lldb/source/Target/StopInfo.cpp | 55 ++++++ lldb/source/Target/Thread.cpp | 8 + lldb/source/Target/ThreadPlanStepInRange.cpp | 24 ++- .../source/Target/ThreadPlanStepOverRange.cpp | 2 +- .../gdb_remote_client/TestGDBRemoteClient.py | 35 +++- .../inline-stepping/TestInlineStepping.py | 63 +++++++ .../inline-stepping/calling.cpp | 25 +++ 19 files changed, 525 insertions(+), 136 deletions(-) diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h index cca00335bc3c6..3592291bb2d06 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h +++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h @@ -11,10 +11,12 @@ #include #include +#include #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 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 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 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 17b76d51c1ae5..7b3f7be23639f 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, /// \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 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 4a0e9047b5469..c864b88c6b32a 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 f520ea88803e5..3f7e03dc316ee 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 { 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 + 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 e5a63c9f8f7d5..5b8b4d066076a 100644 --- a/lldb/include/lldb/Target/ThreadPlanStepInRange.h +++ b/lldb/include/lldb/Target/ThreadPlanStepInRange.h @@ -100,8 +100,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; std::vector m_step_in_deep_bps; // Places where we might // want to stop when we do a diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp index 35058a713aef8..48ee3b9f96a53 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 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 8307689c7640c..9643602d78c75 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 3ca93f908e30b..9700a57d3346e 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 BreakpointSite::GetSuggestedStackFrameIndex() { + + std::optional result; + std::lock_guard guard(m_constituents_mutex); + for (BreakpointLocationSP loc_sp : m_constituents.BreakpointLocations()) { + std::optional 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 579a3999d14ea..a485c4b9ba48a 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 f7d9c0d2d3306..5c7772a6db780 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 ddeacf18e855e..91a67e2770a5d 100644 --- a/lldb/source/Symbol/CompileUnit.cpp +++ b/lldb/source/Symbol/CompileUnit.cpp @@ -249,7 +249,10 @@ void CompileUnit::ResolveSymbolContext( const SourceLocationSpec &src_location_spec, SymbolContextItem resolve_scope, SymbolContextList &sc_list) { 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 @@ -266,7 +269,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 @@ -310,6 +313,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 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->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 // with a line number greater than "line" and we will use this for our diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp index 397582c0e0286..8ab179a45db32 100644 --- a/lldb/source/Target/StackFrameList.cpp +++ b/lldb/source/Target/StackFrameList.cpp @@ -85,121 +85,32 @@ void StackFrameList::ResetCurrentInlinedDepth() { return; std::lock_guard 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; - } - // 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; + m_current_inlined_pc = LLDB_INVALID_ADDRESS; + m_current_inlined_depth = UINT32_MAX; - 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->GetRangeContainingAddress(pc_as_address, containing_range) || - pc_as_address != containing_range.GetBaseAddress()) - return; - - // If we got here because of a breakpoint hit, then set the inlined depth - // depending on where the breakpoint was set. If we got here because of a - // crash, then set the inlined depth to the deepest most block. Otherwise, - // we stopped here naturally as the result of a step, so set ourselves in the - // containing frame of the whole set of nested inlines, so the user can then - // "virtually" step into the frames one by one, or next over the whole mess. - // Note: We don't have to handle being somewhere in the middle of the stack - // here, since ResetCurrentInlinedDepth doesn't get called if there is a - // valid inlined depth set. StopInfoSP stop_info_sp = m_thread.GetStopInfo(); if (!stop_info_sp) return; - switch (stop_info_sp->GetStopReason()) { - case eStopReasonWatchpoint: - case eStopReasonException: - case eStopReasonExec: - case eStopReasonFork: - case eStopReasonVFork: - case eStopReasonVForkDone: - case eStopReasonSignal: - // In all these cases we want to stop in the deepest frame. - m_current_inlined_pc = curr_pc; - m_current_inlined_depth = 0; - break; - case eStopReasonBreakpoint: { - // FIXME: Figure out what this break point is doing, and set the inline - // depth appropriately. Be careful to take into account breakpoints that - // implement step over prologue, since that should do the default - // calculation. For now, if the breakpoints corresponding to this hit are - // all internal, I set the stop location to the top of the inlined stack, - // since that will make things like stepping over prologues work right. - // But if there are any non-internal breakpoints I do to the bottom of the - // stack, since that was the old behavior. - uint32_t bp_site_id = stop_info_sp->GetValue(); - BreakpointSiteSP bp_site_sp( - m_thread.GetProcess()->GetBreakpointSiteList().FindByID(bp_site_id)); - bool all_internal = true; - if (bp_site_sp) { - uint32_t num_owners = bp_site_sp->GetNumberOfConstituents(); - for (uint32_t i = 0; i < num_owners; i++) { - Breakpoint &bp_ref = - bp_site_sp->GetConstituentAtIndex(i)->GetBreakpoint(); - if (!bp_ref.IsInternal()) { - all_internal = false; - } - } - } - if (!all_internal) { - m_current_inlined_pc = curr_pc; - m_current_inlined_depth = 0; - break; - } - } - [[fallthrough]]; - default: { - // Otherwise, we should set ourselves at the container of the inlining, so - // that the user can descend into them. So first we check whether we have - // more than one inlined block sharing this PC: - int num_inlined_functions = 0; - - for (Block *container_ptr = block_ptr->GetInlinedParent(); - container_ptr != nullptr; - container_ptr = container_ptr->GetInlinedParent()) { - if (!container_ptr->GetRangeContainingAddress(pc_as_address, - containing_range)) - break; - if (pc_as_address != containing_range.GetBaseAddress()) - break; - num_inlined_functions++; - } - m_current_inlined_pc = curr_pc; - m_current_inlined_depth = num_inlined_functions + 1; - Log *log = GetLog(LLDBLog::Step); + bool inlined = true; + auto inline_depth = stop_info_sp->GetSuggestedStackFrameIndex(inlined); + // We're only adjusting the inlined stack here. + Log *log = GetLog(LLDBLog::Step); + if (inline_depth) { + m_current_inlined_depth = *inline_depth; + m_current_inlined_pc = m_thread.GetRegisterContext()->GetPC(); + if (log && log->GetVerbose()) LLDB_LOGF(log, "ResetCurrentInlinedDepth: setting inlined " "depth: %d 0x%" PRIx64 ".\n", - m_current_inlined_depth, curr_pc); - - break; - } + m_current_inlined_depth, m_current_inlined_pc); + } else { + if (log && log->GetVerbose()) + LLDB_LOGF( + log, + "ResetCurrentInlinedDepth: Invalidating current inlined depth.\n"); } } @@ -811,19 +722,48 @@ void StackFrameList::SelectMostRelevantFrame() { RecognizedStackFrameSP recognized_frame_sp = frame_sp->GetRecognizedFrame(); - if (!recognized_frame_sp) { - LLDB_LOG(log, "Frame #0 not recognized"); - return; + if (recognized_frame_sp) { + if (StackFrameSP most_relevant_frame_sp = + recognized_frame_sp->GetMostRelevantFrame()) { + LLDB_LOG(log, "Found most relevant frame at index {0}", + most_relevant_frame_sp->GetFrameIndex()); + SetSelectedFrame(most_relevant_frame_sp.get()); + return; + } } + LLDB_LOG(log, "Frame #0 not recognized"); - if (StackFrameSP most_relevant_frame_sp = - recognized_frame_sp->GetMostRelevantFrame()) { - LLDB_LOG(log, "Found most relevant frame at index {0}", - most_relevant_frame_sp->GetFrameIndex()); - SetSelectedFrame(most_relevant_frame_sp.get()); - } else { - LLDB_LOG(log, "No relevant frame!"); + // If this thread has a non-trivial StopInof, then let it suggest + // a most relevant frame: + StopInfoSP stop_info_sp = m_thread.GetStopInfo(); + uint32_t stack_idx = 0; + bool found_relevant = false; + if (stop_info_sp) { + // Here we're only asking the stop info if it wants to adjust the real stack + // index. We have to ask about the m_inlined_stack_depth in + // Thread::ShouldStop since the plans need to reason with that info. + bool inlined = false; + std::optional stack_opt = + stop_info_sp->GetSuggestedStackFrameIndex(inlined); + if (stack_opt) { + stack_idx = *stack_opt; + found_relevant = true; + } } + + frame_sp = GetFrameAtIndex(stack_idx); + if (!frame_sp) + LLDB_LOG(log, "Stop info suggested relevant frame {0} but it didn't exist", + stack_idx); + else if (found_relevant) + LLDB_LOG(log, "Setting selected frame from stop info to {0}", stack_idx); + // Note, we don't have to worry about "inlined" frames here, because we've + // already calculated the inlined frame in Thread::ShouldStop, and + // SetSelectedFrame will take care of that adjustment for us. + SetSelectedFrame(frame_sp.get()); + + if (!found_relevant) + LLDB_LOG(log, "No relevant frame!"); } uint32_t StackFrameList::GetSelectedFrameIndex( @@ -836,6 +776,7 @@ uint32_t StackFrameList::GetSelectedFrameIndex( // isn't set, then don't force a selection here, just return 0. if (!select_most_relevant) return 0; + // If the inlined stack frame is set, then use that: m_selected_frame_idx = 0; } return *m_selected_frame_idx; diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp index 6367b02dae82c..e54dc9fdb65d6 100644 --- a/lldb/source/Target/StopInfo.cpp +++ b/lldb/source/Target/StopInfo.cpp @@ -16,6 +16,7 @@ #include "lldb/Core/Debugger.h" #include "lldb/Core/ValueObject.h" #include "lldb/Expression/UserExpression.h" +#include "lldb/Symbol/Block.h" #include "lldb/Target/Process.h" #include "lldb/Target/StopInfo.h" #include "lldb/Target/Target.h" @@ -246,6 +247,22 @@ class StopInfoBreakpoint : public StopInfo { return m_description.c_str(); } + std::optional + GetSuggestedStackFrameIndex(bool inlined_stack) override { + if (!inlined_stack) + return {}; + + ThreadSP thread_sp(m_thread_wp.lock()); + if (!thread_sp) + return {}; + BreakpointSiteSP bp_site_sp( + thread_sp->GetProcess()->GetBreakpointSiteList().FindByID(m_value)); + if (!bp_site_sp) + return {}; + + return bp_site_sp->GetSuggestedStackFrameIndex(); + } + protected: bool ShouldStop(Event *event_ptr) override { // This just reports the work done by PerformAction or the synchronous @@ -1141,6 +1158,44 @@ class StopInfoTrace : public StopInfo { else return m_description.c_str(); } + + std::optional + GetSuggestedStackFrameIndex(bool inlined_stack) override { + // Trace only knows how to adjust inlined stacks: + if (!inlined_stack) + return {}; + + ThreadSP thread_sp = GetThread(); + StackFrameSP frame_0_sp = thread_sp->GetStackFrameAtIndex(0); + if (!frame_0_sp) + return {}; + if (!frame_0_sp->IsInlined()) + return {}; + Block *block_ptr = frame_0_sp->GetFrameBlock(); + if (!block_ptr) + return {}; + Address pc_address = frame_0_sp->GetFrameCodeAddress(); + AddressRange containing_range; + if (!block_ptr->GetRangeContainingAddress(pc_address, containing_range) || + pc_address != containing_range.GetBaseAddress()) + return {}; + + int num_inlined_functions = 0; + + for (Block *container_ptr = block_ptr->GetInlinedParent(); + container_ptr != nullptr; + container_ptr = container_ptr->GetInlinedParent()) { + if (!container_ptr->GetRangeContainingAddress(pc_address, + containing_range)) + break; + if (pc_address != containing_range.GetBaseAddress()) + break; + + num_inlined_functions++; + } + inlined_stack = true; + return num_inlined_functions + 1; + } }; // StopInfoException diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp index bdab46b048f24..2abf92cc49a33 100644 --- a/lldb/source/Target/Thread.cpp +++ b/lldb/source/Target/Thread.cpp @@ -643,6 +643,14 @@ void Thread::WillStop() { void Thread::SetupForResume() { if (GetResumeState() != eStateSuspended) { + // First check whether this thread is going to "actually" resume at all. + // For instance, if we're stepping from one level to the next of an + // virtual inlined call stack, we just change the inlined call stack index + // without actually running this thread. In that case, for this thread we + // shouldn't push a step over breakpoint plan or do that work. + if (GetCurrentPlan()->IsVirtualStep()) + return; + // If we're at a breakpoint push the step-over breakpoint plan. Do this // before telling the current plan it will resume, since we might change // what the current plan is. diff --git a/lldb/source/Target/ThreadPlanStepInRange.cpp b/lldb/source/Target/ThreadPlanStepInRange.cpp index 90bbdcf1df69b..3e6203c23f4bd 100644 --- a/lldb/source/Target/ThreadPlanStepInRange.cpp +++ b/lldb/source/Target/ThreadPlanStepInRange.cpp @@ -43,7 +43,7 @@ ThreadPlanStepInRange::ThreadPlanStepInRange( "Step Range stepping in", thread, range, addr_context, stop_others), ThreadPlanShouldStopHere(this), m_step_past_prologue(true), - m_virtual_step(false), m_step_into_target(step_into_target) { + m_virtual_step(eLazyBoolCalculate), m_step_into_target(step_into_target) { SetCallbacks(); SetFlagsToDefault(); SetupAvoidNoDebug(step_in_avoids_code_without_debug_info, @@ -152,7 +152,7 @@ bool ThreadPlanStepInRange::ShouldStop(Event *event_ptr) { m_sub_plan_sp.reset(); } - if (m_virtual_step) { + if (m_virtual_step == eLazyBoolYes) { // If we've just completed a virtual step, all we need to do is check for a // ShouldStopHere plan, and otherwise we're done. // FIXME - This can be both a step in and a step out. Probably should @@ -564,7 +564,7 @@ bool ThreadPlanStepInRange::DoPlanExplainsStop(Event *event_ptr) { bool return_value = false; - if (m_virtual_step) { + if (m_virtual_step == eLazyBoolYes) { return_value = true; } else { StopInfoSP stop_info_sp = GetPrivateStopInfo(); @@ -599,10 +599,13 @@ bool ThreadPlanStepInRange::DoPlanExplainsStop(Event *event_ptr) { bool ThreadPlanStepInRange::DoWillResume(lldb::StateType resume_state, bool current_plan) { - m_virtual_step = false; + m_virtual_step = eLazyBoolCalculate; if (resume_state == eStateStepping && current_plan) { Thread &thread = GetThread(); // See if we are about to step over a virtual inlined call. + // But if we already know we're virtual stepping, don't decrement the + // inlined depth again... + bool step_without_resume = thread.DecrementCurrentInlinedDepth(); if (step_without_resume) { Log *log = GetLog(LLDBLog::Step); @@ -615,11 +618,20 @@ bool ThreadPlanStepInRange::DoWillResume(lldb::StateType resume_state, // FIXME: Maybe it would be better to create a InlineStep stop reason, but // then // the whole rest of the world would have to handle that stop reason. - m_virtual_step = true; + m_virtual_step = eLazyBoolYes; } return !step_without_resume; } return true; } -bool ThreadPlanStepInRange::IsVirtualStep() { return m_virtual_step; } +bool ThreadPlanStepInRange::IsVirtualStep() { + if (m_virtual_step == eLazyBoolCalculate) { + Thread &thread = GetThread(); + if (thread.GetCurrentInlinedDepth() == UINT32_MAX) + m_virtual_step = eLazyBoolNo; + else + m_virtual_step = eLazyBoolYes; + } + return m_virtual_step == eLazyBoolYes; +} diff --git a/lldb/source/Target/ThreadPlanStepOverRange.cpp b/lldb/source/Target/ThreadPlanStepOverRange.cpp index 4ea8d69035306..cdc0fddd78d83 100644 --- a/lldb/source/Target/ThreadPlanStepOverRange.cpp +++ b/lldb/source/Target/ThreadPlanStepOverRange.cpp @@ -355,7 +355,7 @@ bool ThreadPlanStepOverRange::DoWillResume(lldb::StateType resume_state, if (in_inlined_stack) { Log *log = GetLog(LLDBLog::Step); LLDB_LOGF(log, - "ThreadPlanStepInRange::DoWillResume: adjusting range to " + "ThreadPlanStepOverRange::DoWillResume: adjusting range to " "the frame at inlined depth %d.", thread.GetCurrentInlinedDepth()); StackFrameSP stack_sp = thread.GetStackFrameAtIndex(0); diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py index 5eb3fc3cada92..08ac9290ee85a 100644 --- a/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py +++ b/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py @@ -132,12 +132,39 @@ def test_read_registers_using_g_packets(self): target = self.createTarget("a.yaml") process = self.connect(target) - self.assertEqual(1, self.server.responder.packetLog.count("g")) - self.server.responder.packetLog = [] + # We want to make sure that the process is using the g packet, but it's + # not required the "connect" should read all registers. However, it might + # have... So we need to wait till we explicitly 'read_registers' to do + # test. + # Also, even with the use-g-packet-for-reading lldb will sometimes send p0 + # early on to see if the packet is supported. So we can't say that there + # will be NO p packets. + # But there certainly should be no p packets after the g packet. + self.read_registers(process) - # Reading registers should not cause any 'p' packets to be exchanged. + print(f"\nPACKET LOG:\n{self.server.responder.packetLog}\n") + g_pos = 0 + try: + g_pos = self.server.responder.packetLog.index("g") + except err: + self.fail("'g' packet not found after fetching registers") + + try: + second_g = self.server.responder.packetLog.index("g", g_pos) + self.fail("Found more than one 'g' packet") + except: + pass + + # Make sure there aren't any `p` packets after the `g` packet: self.assertEqual( - 0, len([p for p in self.server.responder.packetLog if p.startswith("p")]) + 0, + len( + [ + p + for p in self.server.responder.packetLog[g_pos:] + if p.startswith("p") + ] + ), ) def test_read_registers_using_p_packets(self): diff --git a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py index 752c3a9cbd286..f52e0f0fd5bcf 100644 --- a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py +++ b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py @@ -32,6 +32,12 @@ def test_step_in_template_with_python_api(self): self.build() self.step_in_template() + @add_test_categories(["pyapi"]) + def test_virtual_inline_stepping(self): + """Test stepping through a virtual inlined call stack""" + self.build() + self.virtual_inline_stepping() + def setUp(self): # Call super's setUp(). TestBase.setUp(self) @@ -357,3 +363,60 @@ def step_in_template(self): step_sequence = [["// In max_value specialized", "into"]] self.run_step_sequence(step_sequence) + + def run_to_call_site_and_step(self, source_regex, func_name, start_pos): + main_spec = lldb.SBFileSpec("calling.cpp") + # Set the breakpoint by file and line, not sourced regex because + # we want to make sure we can set breakpoints on call sites: + call_site_line_num = line_number(self.main_source, source_regex) + target, process, thread, bkpt = lldbutil.run_to_line_breakpoint( + self, main_spec, call_site_line_num + ) + + # Make sure that the location is at the call site (run_to_line_breakpoint already asserted + # that there's one location.): + bkpt_loc = bkpt.location[0] + strm = lldb.SBStream() + result = bkpt_loc.GetDescription(strm, lldb.eDescriptionLevelFull) + + self.assertTrue(result, "Got a location description") + desc = strm.GetData() + self.assertIn(f"calling.cpp:{call_site_line_num}", desc, "Right line listed") + # We don't get the function name right yet - so we omit it in printing. + # Turn on this test when that is working. + # self.assertIn(func_name, desc, "Right function listed") + + pc = thread.frame[0].pc + for i in range(start_pos, 3): + thread.StepInto() + frame_0 = thread.frame[0] + + trivial_line_num = line_number( + self.main_source, f"In caller_trivial_inline_{i}." + ) + self.assertEqual( + frame_0.line_entry.line, + trivial_line_num, + f"Stepped into the caller_trivial_inline_{i}", + ) + if pc != frame_0.pc: + # If we get here, we stepped to the expected line number, but + # the compiler on this system has decided to insert an instruction + # between the call site of an inlined function with no arguments, + # returning void, and its immediate call to another void inlined function + # with no arguments. We aren't going to be testing virtual inline + # stepping for this function... + break + + process.Kill() + target.Clear() + + def virtual_inline_stepping(self): + """Use the Python API's to step through a virtual inlined stack""" + self.run_to_call_site_and_step("At caller_trivial_inline_1", "main", 1) + self.run_to_call_site_and_step( + "In caller_trivial_inline_1", "caller_trivial_inline_1", 2 + ) + self.run_to_call_site_and_step( + "In caller_trivial_inline_2", "caller_trivial_inline_2", 3 + ) diff --git a/lldb/test/API/functionalities/inline-stepping/calling.cpp b/lldb/test/API/functionalities/inline-stepping/calling.cpp index 49179ce7c9788..d7ee56b3c0790 100644 --- a/lldb/test/API/functionalities/inline-stepping/calling.cpp +++ b/lldb/test/API/functionalities/inline-stepping/calling.cpp @@ -13,6 +13,12 @@ int called_by_inline_ref (int &value); inline void inline_trivial_1 () __attribute__((always_inline)); inline void inline_trivial_2 () __attribute__((always_inline)); +// These three should share the same initial pc so we can test +// virtual inline stepping. +inline void caller_trivial_inline_1() __attribute__((always_inline)); +inline void caller_trivial_inline_2() __attribute__((always_inline)); +inline void caller_trivial_inline_3() __attribute__((always_inline)); + void caller_trivial_1 (); void caller_trivial_2 (); @@ -79,6 +85,23 @@ caller_trivial_2 () inline_value += 1; // At increment in caller_trivial_2. } +// When you call caller_trivial_inline_1, the inlined call-site +// should share a PC with all three of the following inlined +// functions, so we can exercise "virtual inline stepping". +void caller_trivial_inline_1() { + caller_trivial_inline_2(); // In caller_trivial_inline_1. + inline_value += 1; +} + +void caller_trivial_inline_2() { + caller_trivial_inline_3(); // In caller_trivial_inline_2. + inline_value += 1; +} + +void caller_trivial_inline_3() { + inline_value += 1; // In caller_trivial_inline_3. +} + void called_by_inline_trivial () { @@ -132,5 +155,7 @@ main (int argc, char **argv) max_value(123, 456); // Call max_value template max_value(std::string("abc"), std::string("0022")); // Call max_value specialized + caller_trivial_inline_1(); // At caller_trivial_inline_1. + return 0; // About to return from main. } From 8238e94d53417c791e3e50ce9022dfde6dfe664d Mon Sep 17 00:00:00 2001 From: jimingham Date: Wed, 30 Oct 2024 18:26:38 -0700 Subject: [PATCH 2/6] Fix stepping away from the bottom-most frame of a virtual inlined call stack. (#114337) The computation of 'Thread::IsVirtualStep" was wrong - it called being at the bottom of a virtual call stack a "virtual step" but that is actually when you've gotten to concrete code and need to step for real. I also added a test for this. (cherry picked from commit 3243e3d8872585091d65ea7ff0639155b4c1dd7a) --- lldb/source/Target/ThreadPlanStepInRange.cpp | 3 ++- .../inline-stepping/TestInlineStepping.py | 18 +++++++++++++++++- .../inline-stepping/calling.cpp | 2 +- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/lldb/source/Target/ThreadPlanStepInRange.cpp b/lldb/source/Target/ThreadPlanStepInRange.cpp index 3e6203c23f4bd..9f6d77b269cf8 100644 --- a/lldb/source/Target/ThreadPlanStepInRange.cpp +++ b/lldb/source/Target/ThreadPlanStepInRange.cpp @@ -628,7 +628,8 @@ bool ThreadPlanStepInRange::DoWillResume(lldb::StateType resume_state, bool ThreadPlanStepInRange::IsVirtualStep() { if (m_virtual_step == eLazyBoolCalculate) { Thread &thread = GetThread(); - if (thread.GetCurrentInlinedDepth() == UINT32_MAX) + uint32_t cur_inline_depth = thread.GetCurrentInlinedDepth(); + if (cur_inline_depth == UINT32_MAX || cur_inline_depth == 0) m_virtual_step = eLazyBoolNo; else m_virtual_step = eLazyBoolYes; diff --git a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py index f52e0f0fd5bcf..3283918f85274 100644 --- a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py +++ b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py @@ -364,7 +364,9 @@ def step_in_template(self): step_sequence = [["// In max_value specialized", "into"]] self.run_step_sequence(step_sequence) - def run_to_call_site_and_step(self, source_regex, func_name, start_pos): + def run_to_call_site_and_step( + self, source_regex, func_name, start_pos, one_more_step_loc=None + ): main_spec = lldb.SBFileSpec("calling.cpp") # Set the breakpoint by file and line, not sourced regex because # we want to make sure we can set breakpoints on call sites: @@ -408,6 +410,14 @@ def run_to_call_site_and_step(self, source_regex, func_name, start_pos): # stepping for this function... break + if one_more_step_loc: + thread.StepInto() + frame_0 = thread.frame[0] + self.assertEqual( + frame_0.line_entry.line, + line_number(self.main_source, one_more_step_loc), + "Was able to step one more time", + ) process.Kill() target.Clear() @@ -420,3 +430,9 @@ def virtual_inline_stepping(self): self.run_to_call_site_and_step( "In caller_trivial_inline_2", "caller_trivial_inline_2", 3 ) + self.run_to_call_site_and_step( + "In caller_trivial_inline_3", + "caller_trivial_inline_3", + 4, + "After caller_trivial_inline_3", + ) diff --git a/lldb/test/API/functionalities/inline-stepping/calling.cpp b/lldb/test/API/functionalities/inline-stepping/calling.cpp index d7ee56b3c0790..ba71c25a3c648 100644 --- a/lldb/test/API/functionalities/inline-stepping/calling.cpp +++ b/lldb/test/API/functionalities/inline-stepping/calling.cpp @@ -95,7 +95,7 @@ void caller_trivial_inline_1() { void caller_trivial_inline_2() { caller_trivial_inline_3(); // In caller_trivial_inline_2. - inline_value += 1; + inline_value += 1; // After caller_trivial_inline_3 } void caller_trivial_inline_3() { From ba101accce9eaa650fe889a8861d8322ab9155a5 Mon Sep 17 00:00:00 2001 From: jimingham Date: Thu, 31 Oct 2024 02:06:42 -0700 Subject: [PATCH 3/6] [lldb][test] Skip one inline stepping test for arm-ubuntu. (#114295) The test is currently passing everywhere but this 32-bit arm ubuntu bot. I don't have an easy way to debug this, so I'm skipping the test on that platform till we get a chance to figure this out. (cherry picked from commit a218f0f354e9df2ce689686be503f3d85fea44f9) --- .../API/functionalities/inline-stepping/TestInlineStepping.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py index 3283918f85274..1118758cc88fb 100644 --- a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py +++ b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py @@ -14,6 +14,8 @@ class TestInlineStepping(TestBase): compiler="icc", bugnumber="# Not really a bug. ICC combines two inlined functions.", ) + + @skipIf(oslist=["linux"], archs=["arm"]) # Fails for 32 bit arm def test_with_python_api(self): """Test stepping over and into inlined functions.""" self.build() From f0ba8aa257cad6326b48d8cd1faac37302a51f57 Mon Sep 17 00:00:00 2001 From: jimingham Date: Tue, 5 Nov 2024 10:33:24 -0800 Subject: [PATCH 4/6] More refinement of call site handling in stepping. (#114628) When you set a "next branch breakpoint" and run to it while stepping, you have to claim the stop at that breakpoint to be the top of the inlined call stack, or you will seem to "step in" and then plans might try to step back out again. This records the PrefferedLineEntry for next branch breakpoints and adds a test to make sure this works. (cherry picked from commit 23a01a413d29f2d5b1f6204d0237e3884ae0231e) --- lldb/source/Target/ThreadPlanStepRange.cpp | 50 +++++++++++++++++-- .../inline-stepping/TestInlineStepping.py | 21 +++++++- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/lldb/source/Target/ThreadPlanStepRange.cpp b/lldb/source/Target/ThreadPlanStepRange.cpp index b514ab03d671d..134bf6d956b54 100644 --- a/lldb/source/Target/ThreadPlanStepRange.cpp +++ b/lldb/source/Target/ThreadPlanStepRange.cpp @@ -358,10 +358,10 @@ bool ThreadPlanStepRange::SetNextBranchBreakpoint() { !m_next_branch_bp_sp->HasResolvedLocations()) m_could_not_resolve_hw_bp = true; + BreakpointLocationSP bp_loc = + m_next_branch_bp_sp->GetLocationAtIndex(0); if (log) { lldb::break_id_t bp_site_id = LLDB_INVALID_BREAK_ID; - BreakpointLocationSP bp_loc = - m_next_branch_bp_sp->GetLocationAtIndex(0); if (bp_loc) { BreakpointSiteSP bp_site = bp_loc->GetBreakpointSite(); if (bp_site) { @@ -374,7 +374,51 @@ bool ThreadPlanStepRange::SetNextBranchBreakpoint() { m_next_branch_bp_sp->GetID(), bp_site_id, run_to_address.GetLoadAddress(&m_process.GetTarget())); } - + // The "next branch breakpoint might land on a virtual inlined call + // stack. If that's true, we should always stop at the top of the + // inlined call stack. Only virtual steps should walk deeper into the + // inlined call stack. + Block *block = run_to_address.CalculateSymbolContextBlock(); + if (bp_loc && block) { + LineEntry top_most_line_entry; + lldb::addr_t run_to_addr = run_to_address.GetFileAddress(); + for (Block *inlined_parent = block->GetContainingInlinedBlock(); + inlined_parent; + inlined_parent = inlined_parent->GetInlinedParent()) { + AddressRange range; + if (!inlined_parent->GetRangeContainingAddress(run_to_address, + range)) + break; + Address range_start_address = range.GetBaseAddress(); + // Only compare addresses here, we may have different symbol + // contexts (for virtual inlined stacks), but we just want to know + // that they are all at the same address. + if (range_start_address.GetFileAddress() != run_to_addr) + break; + const InlineFunctionInfo *inline_info = + inlined_parent->GetInlinedFunctionInfo(); + if (!inline_info) + break; + const Declaration &call_site = inline_info->GetCallSite(); + top_most_line_entry.line = call_site.GetLine(); + top_most_line_entry.column = call_site.GetColumn(); + FileSpec call_site_file_spec = call_site.GetFile(); + top_most_line_entry.original_file_sp.reset( + new SupportFile(call_site_file_spec)); + top_most_line_entry.range = range; + top_most_line_entry.file_sp.reset(); + top_most_line_entry.ApplyFileMappings( + GetThread().CalculateTarget()); + if (!top_most_line_entry.file_sp) + top_most_line_entry.file_sp = + top_most_line_entry.original_file_sp; + } + if (top_most_line_entry.IsValid()) { + LLDB_LOG(log, "Setting preferred line entry: {0}:{1}", + top_most_line_entry.GetFile(), top_most_line_entry.line); + bp_loc->SetPreferredLineEntry(top_most_line_entry); + } + } m_next_branch_bp_sp->SetThreadID(m_tid); m_next_branch_bp_sp->SetBreakpointKind("next-branch-location"); diff --git a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py index 1118758cc88fb..1501ad36519e2 100644 --- a/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py +++ b/lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py @@ -14,7 +14,6 @@ class TestInlineStepping(TestBase): compiler="icc", bugnumber="# Not really a bug. ICC combines two inlined functions.", ) - @skipIf(oslist=["linux"], archs=["arm"]) # Fails for 32 bit arm def test_with_python_api(self): """Test stepping over and into inlined functions.""" @@ -293,7 +292,7 @@ def inline_stepping_step_over(self): break_1_in_main = target.BreakpointCreateBySourceRegex( "// At second call of caller_ref_1 in main.", self.main_source_spec ) - self.assertTrue(break_1_in_main, VALID_BREAKPOINT) + self.assertGreater(break_1_in_main.GetNumLocations(), 0, VALID_BREAKPOINT) # Now launch the process, and do not stop at entry point. self.process = target.LaunchSimple( @@ -319,6 +318,24 @@ def inline_stepping_step_over(self): ] self.run_step_sequence(step_sequence) + # Now make sure that next to a virtual inlined call stack + # gets the call stack depth correct. + break_2_in_main = target.BreakpointCreateBySourceRegex( + "// Call max_value specialized", self.main_source_spec + ) + self.assertGreater(break_2_in_main.GetNumLocations(), 0, VALID_BREAKPOINT) + threads = lldbutil.continue_to_breakpoint(self.process, break_2_in_main) + self.assertEqual(len(threads), 1, "Hit our second breakpoint") + self.assertEqual(threads[0].id, self.thread.id, "Stopped at right thread") + self.thread.StepOver() + frame_0 = self.thread.frames[0] + line_entry = frame_0.line_entry + self.assertEqual( + line_entry.file.basename, self.main_source_spec.basename, "File matches" + ) + target_line = line_number("calling.cpp", "// At caller_trivial_inline_1") + self.assertEqual(line_entry.line, target_line, "Lines match as well.") + def step_in_template(self): """Use Python APIs to test stepping in to templated functions.""" exe = self.getBuildArtifact("a.out") From 71ce5228681a662054592679b2719e8e28705026 Mon Sep 17 00:00:00 2001 From: jimingham Date: Tue, 5 Nov 2024 11:23:23 -0800 Subject: [PATCH 5/6] Fix a thinko in the CallSite handling code: (#114896) I have to check for the sc list size being changed by the call-site search, not just that it had more than one element. Added a test for multiple CU's with the same name in a given module, which would have caught this mistake. We were also doing all the work to find call sites when the found decl and specified decl's only difference was a column, but the incoming specification hadn't specified a column (column number == 0). (cherry picked from commit 803f957e87e4083f6d61c8991171eeeaf0e6bd61) --- lldb/source/Symbol/CompileUnit.cpp | 9 ++++-- .../breakpoint/same_cu_name/Makefile | 19 +++++++++++ .../TestFileBreakpoinsSameCUName.py | 32 +++++++++++++++++++ .../breakpoint/same_cu_name/common.cpp | 8 +++++ .../breakpoint/same_cu_name/main.cpp | 24 ++++++++++++++ 5 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 lldb/test/API/functionalities/breakpoint/same_cu_name/Makefile create mode 100644 lldb/test/API/functionalities/breakpoint/same_cu_name/TestFileBreakpoinsSameCUName.py create mode 100644 lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp create mode 100644 lldb/test/API/functionalities/breakpoint/same_cu_name/main.cpp diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp index 91a67e2770a5d..e2592feec74cf 100644 --- a/lldb/source/Symbol/CompileUnit.cpp +++ b/lldb/source/Symbol/CompileUnit.cpp @@ -326,14 +326,17 @@ void CompileUnit::ResolveSymbolContext( // 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) { + (line_entry.line != line || + (column_num != 0 && 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(); + // Record the size of the list to see if we added to it: + size_t old_sc_list_size = sc_list.GetSize(); Declaration sought_decl(file_spec, line, column_num); // We use this recursive function to descend the block structure looking @@ -415,7 +418,7 @@ void CompileUnit::ResolveSymbolContext( // 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()) + if (sc_list.GetSize() > old_sc_list_size) return; } diff --git a/lldb/test/API/functionalities/breakpoint/same_cu_name/Makefile b/lldb/test/API/functionalities/breakpoint/same_cu_name/Makefile new file mode 100644 index 0000000000000..4bfdb15e777d9 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/same_cu_name/Makefile @@ -0,0 +1,19 @@ +CXX_SOURCES := main.cpp +LD_EXTRAS := ns1.o ns2.o ns3.o ns4.o + +a.out: main.o ns1.o ns2.o ns3.o ns4.o + +ns1.o: common.cpp + $(CC) -g -c -DNAMESPACE=ns1 -o $@ $< + +ns2.o: common.cpp + $(CC) -g -c -DNAMESPACE=ns2 -o $@ $< + +ns3.o: common.cpp + $(CC) -g -c -DNAMESPACE=ns3 -o $@ $< + +ns4.o: common.cpp + $(CC) -g -c -DNAMESPACE=ns4 -o $@ $< + + +include Makefile.rules diff --git a/lldb/test/API/functionalities/breakpoint/same_cu_name/TestFileBreakpoinsSameCUName.py b/lldb/test/API/functionalities/breakpoint/same_cu_name/TestFileBreakpoinsSameCUName.py new file mode 100644 index 0000000000000..dc10d407d7230 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/same_cu_name/TestFileBreakpoinsSameCUName.py @@ -0,0 +1,32 @@ +""" +Test setting a breakpoint by file and line when many instances of the +same file name exist in the CU list. +""" + + +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + + +class TestBreakpointSameCU(TestBase): + def test_breakpoint_same_cu(self): + self.build() + target = self.createTestTarget() + + # Break both on the line before the code: + comment_line = line_number("common.cpp", "// A comment here") + self.assertNotEqual(comment_line, 0, "line_number worked") + bkpt = target.BreakpointCreateByLocation("common.cpp", comment_line) + self.assertEqual( + bkpt.GetNumLocations(), 4, "Got the right number of breakpoints" + ) + + # And break on the code, both should work: + code_line = line_number("common.cpp", "// The line with code") + self.assertNotEqual(comment_line, 0, "line_number worked again") + bkpt = target.BreakpointCreateByLocation("common.cpp", code_line) + self.assertEqual( + bkpt.GetNumLocations(), 4, "Got the right number of breakpoints" + ) diff --git a/lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp b/lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp new file mode 100644 index 0000000000000..ed9a43f27b173 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp @@ -0,0 +1,8 @@ +namespace NAMESPACE { +static int g_value = 0; +void DoSomeStuff() { + // A comment here + g_value++; // The line with code +} + +} // namespace NAMESPACE diff --git a/lldb/test/API/functionalities/breakpoint/same_cu_name/main.cpp b/lldb/test/API/functionalities/breakpoint/same_cu_name/main.cpp new file mode 100644 index 0000000000000..43d9e3271ece2 --- /dev/null +++ b/lldb/test/API/functionalities/breakpoint/same_cu_name/main.cpp @@ -0,0 +1,24 @@ +namespace ns1 { +extern void DoSomeStuff(); +} + +namespace ns2 { +extern void DoSomeStuff(); +} + +namespace ns3 { +extern void DoSomeStuff(); +} + +namespace ns4 { +extern void DoSomeStuff(); +} + +int main(int argc, char *argv[]) { + ns1::DoSomeStuff(); + ns2::DoSomeStuff(); + ns3::DoSomeStuff(); + ns4::DoSomeStuff(); + + return 0; +} From d21e17feb3bdd40aa446567fff9a184a45f58fff Mon Sep 17 00:00:00 2001 From: Jim Ingham Date: Tue, 5 Nov 2024 17:05:03 -0800 Subject: [PATCH 6/6] Add a test for swift call site breakpoint and stepping. --- lldb/test/API/lang/swift/macro/Macro.swift | 2 + .../test/API/lang/swift/macro/MacroImpl.swift | 13 +++++ .../API/lang/swift/macro/TestSwiftMacro.py | 49 +++++++++++++++++-- lldb/test/API/lang/swift/macro/main.swift | 7 ++- 4 files changed, 64 insertions(+), 7 deletions(-) diff --git a/lldb/test/API/lang/swift/macro/Macro.swift b/lldb/test/API/lang/swift/macro/Macro.swift index 92190f7e38281..6b0de79ec9426 100644 --- a/lldb/test/API/lang/swift/macro/Macro.swift +++ b/lldb/test/API/lang/swift/macro/Macro.swift @@ -1 +1,3 @@ @freestanding(expression) public macro stringify(_ value: T) -> (T, String) = #externalMacro(module: "MacroImpl", type: "StringifyMacro") + +@freestanding(expression) public macro no_return(_ value: T) = #externalMacro(module: "MacroImpl", type: "NoReturnMacro") diff --git a/lldb/test/API/lang/swift/macro/MacroImpl.swift b/lldb/test/API/lang/swift/macro/MacroImpl.swift index 20cd14c6aad14..dc864407f87c1 100644 --- a/lldb/test/API/lang/swift/macro/MacroImpl.swift +++ b/lldb/test/API/lang/swift/macro/MacroImpl.swift @@ -14,3 +14,16 @@ public struct StringifyMacro: ExpressionMacro { return "(\(argument), \(StringLiteralExprSyntax(content: argument.description)))" } } + +public struct NoReturnMacro: ExpressionMacro { + public static func expansion( + of macro: some FreestandingMacroExpansionSyntax, + in context: some MacroExpansionContext + ) -> ExprSyntax { + guard let argument = macro.argumentList.first?.expression else { + fatalError("boom") + } + + return "print(\(argument), \(StringLiteralExprSyntax(content: argument.description)))" + } +} diff --git a/lldb/test/API/lang/swift/macro/TestSwiftMacro.py b/lldb/test/API/lang/swift/macro/TestSwiftMacro.py index df8089b7efad6..d0f87141aa380 100644 --- a/lldb/test/API/lang/swift/macro/TestSwiftMacro.py +++ b/lldb/test/API/lang/swift/macro/TestSwiftMacro.py @@ -34,19 +34,58 @@ def testDebugging(self): """Test Swift macros""" self.build(dictionary={'SWIFT_SOURCES': 'main.swift'}) self.setupPluginServerForTesting() + main_spec = lldb.SBFileSpec('main.swift') target, process, thread, bkpt = lldbutil.run_to_source_breakpoint( - self, 'break here', lldb.SBFileSpec('main.swift')) + self, 'break here', main_spec + ) + + # We're testing line breakpoint setting here: + call_site_line = lldbtest.line_number("main.swift", "#no_return(a / b)") + call_site_bp = target.BreakpointCreateByLocation(main_spec, call_site_line) thread.StepOver() thread.StepInto() + # This is the expanded macro source, we should be able to step into it. - self.expect('reg read pc', substrs=[ - '[inlined] freestanding macro expansion #1 of stringify in module a file main.swift line 5 column 11', - 'stringify' - ]) + # Don't check the actual line number so we are line independent + self.assertIn( + 'freestanding macro expansion #1 of stringify in module a file main.swift line 5 column 11', + thread.frames[0].name, + "Stopped in stringify macro" + ) self.expect('expression -- #stringify(1)', substrs=['0 = 1', '1 = "1"']) + # Step out should get us out of stringify, then in to the next macro: + thread.StepOut() + self.assertIn("a.testStringify", thread.frames[0].name, "Step out back to origin") + thread.StepInto() + self.assertIn( + "freestanding macro expansion #1 of no_return in module a file main.swift line 6 column 3", + thread.frames[0].name, + "Step out and in gets to no_return" + ) + + # We've set a breakpoint on the call site for another instance - run to that: + threads = lldbutil.continue_to_breakpoint(process, call_site_bp) + self.assertEqual(len(threads), 1, "Stopped at one thread") + thread = threads[0] + frame_0 = thread.frames[0] + line_entry_0 = frame_0.line_entry + self.assertEqual(line_entry_0.line, call_site_line, "Got the right line attribution") + self.assertEqual(line_entry_0.file, main_spec, "Got the right file attribution") + + # Now test stepping in and back out again: + thread.StepInto() + self.assertIn( + "freestanding macro expansion #3 of no_return in module a file main.swift line 8 column 3", + thread.frames[0].name, + "Step out and in gets to no_return" + ) + + thread.StepOut() + self.assertIn("a.testStringify", thread.frames[0].name, "Step out from no_return") + # Make sure we can set a symbolic breakpoint on a macro. b = target.BreakpointCreateByName("stringify") self.assertGreaterEqual(b.GetNumLocations(), 1) diff --git a/lldb/test/API/lang/swift/macro/main.swift b/lldb/test/API/lang/swift/macro/main.swift index f94cd87c4401b..56643acfd9944 100644 --- a/lldb/test/API/lang/swift/macro/main.swift +++ b/lldb/test/API/lang/swift/macro/main.swift @@ -1,9 +1,12 @@ import Macro - +// This test depends on the layout of the lines in this file. func testStringify(a: Int, b: Int) { print("break here") let s = #stringify(a / b) + #no_return(a) + #no_return(b) + #no_return(a / b) print(s.1) } -testStringify(a: 23, b: 0) +testStringify(a: 23, b: 1)