llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: None (jimingham) <details> <summary>Changes</summary> …ne stepping (#<!-- -->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 f14743794587db102c6d1b20f9c87a1ac20decfd. --- Patch is 40.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/113947.diff 18 Files Affected: - (modified) lldb/include/lldb/Breakpoint/BreakpointLocation.h (-36) - (modified) lldb/include/lldb/Breakpoint/BreakpointSite.h (-5) - (modified) lldb/include/lldb/Core/Declaration.h (+1-5) - (modified) lldb/include/lldb/Target/StopInfo.h (-12) - (modified) lldb/include/lldb/Target/ThreadPlanStepInRange.h (+2-2) - (modified) lldb/source/Breakpoint/BreakpointLocation.cpp (+2-61) - (modified) lldb/source/Breakpoint/BreakpointResolver.cpp (-15) - (modified) lldb/source/Breakpoint/BreakpointSite.cpp (-17) - (modified) lldb/source/Core/Declaration.cpp (+2-3) - (modified) lldb/source/Symbol/Block.cpp (+1-1) - (modified) lldb/source/Symbol/CompileUnit.cpp (+1-110) - (modified) lldb/source/Target/StackFrameList.cpp (+115-56) - (modified) lldb/source/Target/StopInfo.cpp (-55) - (modified) lldb/source/Target/Thread.cpp (-8) - (modified) lldb/source/Target/ThreadPlanStepInRange.cpp (+6-18) - (modified) lldb/source/Target/ThreadPlanStepOverRange.cpp (+1-1) - (modified) lldb/test/API/functionalities/inline-stepping/TestInlineStepping.py (-63) - (modified) lldb/test/API/functionalities/inline-stepping/calling.cpp (-25) ``````````diff diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h index 3592291bb2d06e..cca00335bc3c67 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h +++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h @@ -11,12 +11,10 @@ #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" @@ -284,25 +282,6 @@ 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; @@ -327,16 +306,6 @@ 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); @@ -400,11 +369,6 @@ class BreakpointLocation lldb::break_id_t m_loc_id; ///< Breakpoint location ID. StoppointHitCounter m_hit_counter; ///< Number of times this breakpoint /// location has been hit. - /// If this exists, use it to print the stop description rather than the - /// LineEntry m_address resolves to directly. Use this for instance when the - /// location was given somewhere in the virtual inlined call stack since the - /// Address always resolves to the lowest entry in the stack. - std::optional<LineEntry> m_preferred_line_entry; void SetShouldResolveIndirectFunctions(bool do_resolve) { m_should_resolve_indirect_functions = do_resolve; diff --git a/lldb/include/lldb/Breakpoint/BreakpointSite.h b/lldb/include/lldb/Breakpoint/BreakpointSite.h index 7b3f7be23639f2..17b76d51c1ae53 100644 --- a/lldb/include/lldb/Breakpoint/BreakpointSite.h +++ b/lldb/include/lldb/Breakpoint/BreakpointSite.h @@ -170,11 +170,6 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>, /// \see lldb::DescriptionLevel void GetDescription(Stream *s, lldb::DescriptionLevel level); - // This runs through all the breakpoint locations owning this site and returns - // the greatest of their suggested stack frame indexes. This only handles - // inlined stack changes. - std::optional<uint32_t> GetSuggestedStackFrameIndex(); - /// Tell whether a breakpoint has a location at this site. /// /// \param[in] bp_id diff --git a/lldb/include/lldb/Core/Declaration.h b/lldb/include/lldb/Core/Declaration.h index c864b88c6b32a3..4a0e9047b54695 100644 --- a/lldb/include/lldb/Core/Declaration.h +++ b/lldb/include/lldb/Core/Declaration.h @@ -84,14 +84,10 @@ 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, bool full) const; + bool FileAndLineEqual(const Declaration &declaration) 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 45beac129e86f7..fae90364deaf0a 100644 --- a/lldb/include/lldb/Target/StopInfo.h +++ b/lldb/include/lldb/Target/StopInfo.h @@ -77,18 +77,6 @@ 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 9da8370ef1c925..f9ef87942a7c03 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. - LazyBool m_virtual_step; // true if we've just done a "virtual step", i.e. - // just moved the inline stack depth. + bool 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 c7ea50407ae1c7..ad9057c8141e99 100644 --- a/lldb/source/Breakpoint/BreakpointLocation.cpp +++ b/lldb/source/Breakpoint/BreakpointLocation.cpp @@ -508,20 +508,8 @@ 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, show_function_info, - show_function_info, show_function_info); + false, true, false, true, true, true); } else { if (sc.module_sp) { s->EOL(); @@ -549,10 +537,7 @@ void BreakpointLocation::GetDescription(Stream *s, if (sc.line_entry.line > 0) { s->EOL(); s->Indent("location = "); - if (auto preferred = GetPreferredLineEntry()) - preferred->DumpStopContext(s, true); - else - sc.line_entry.DumpStopContext(s, true); + sc.line_entry.DumpStopContext(s, true); } } else { @@ -671,50 +656,6 @@ void BreakpointLocation::SendBreakpointLocationChangedEvent( } } -std::optional<uint32_t> BreakpointLocation::GetSuggestedStackFrameIndex() { - auto preferred_opt = GetPreferredLineEntry(); - if (!preferred_opt) - return {}; - LineEntry preferred = *preferred_opt; - SymbolContext sc; - if (!m_address.CalculateSymbolContext(&sc)) - return {}; - // Don't return anything special if frame 0 is the preferred line entry. - // We not really telling the stack frame list to do anything special in that - // case. - if (!LineEntry::Compare(sc.line_entry, preferred)) - return {}; - - if (!sc.block) - return {}; - - // Blocks have their line info in Declaration form, so make one here: - Declaration preferred_decl(preferred.GetFile(), preferred.line, - preferred.column); - - uint32_t depth = 0; - Block *inlined_block = sc.block->GetContainingInlinedBlock(); - while (inlined_block) { - // If we've moved to a block that this isn't the start of, that's not - // our inlining info or call site, so we can stop here. - Address start_address; - if (!inlined_block->GetStartAddress(start_address) || - start_address != m_address) - return {}; - - const InlineFunctionInfo *info = inlined_block->GetInlinedFunctionInfo(); - if (info) { - if (preferred_decl == info->GetDeclaration()) - return depth; - if (preferred_decl == info->GetCallSite()) - return depth + 1; - } - inlined_block = inlined_block->GetInlinedParent(); - depth++; - } - return {}; -} - void BreakpointLocation::SwapLocation(BreakpointLocationSP swap_from) { m_address = swap_from->m_address; m_should_resolve_indirect_functions = diff --git a/lldb/source/Breakpoint/BreakpointResolver.cpp b/lldb/source/Breakpoint/BreakpointResolver.cpp index 9643602d78c751..8307689c7640cf 100644 --- a/lldb/source/Breakpoint/BreakpointResolver.cpp +++ b/lldb/source/Breakpoint/BreakpointResolver.cpp @@ -340,21 +340,6 @@ 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 9700a57d3346e0..3ca93f908e30b8 100644 --- a/lldb/source/Breakpoint/BreakpointSite.cpp +++ b/lldb/source/Breakpoint/BreakpointSite.cpp @@ -87,23 +87,6 @@ void BreakpointSite::GetDescription(Stream *s, lldb::DescriptionLevel level) { m_constituents.GetDescription(s, level); } -std::optional<uint32_t> BreakpointSite::GetSuggestedStackFrameIndex() { - - std::optional<uint32_t> result; - std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex); - for (BreakpointLocationSP loc_sp : m_constituents.BreakpointLocations()) { - std::optional<uint32_t> loc_frame_index = - loc_sp->GetSuggestedStackFrameIndex(); - if (loc_frame_index) { - if (result) - result = std::max(*loc_frame_index, *result); - else - result = loc_frame_index; - } - } - return result; -} - bool BreakpointSite::IsInternal() const { return m_constituents.IsInternal(); } uint8_t *BreakpointSite::GetTrapOpcodeBytes() { return &m_trap_opcode[0]; } diff --git a/lldb/source/Core/Declaration.cpp b/lldb/source/Core/Declaration.cpp index a485c4b9ba48a7..579a3999d14ea0 100644 --- a/lldb/source/Core/Declaration.cpp +++ b/lldb/source/Core/Declaration.cpp @@ -70,9 +70,8 @@ int Declaration::Compare(const Declaration &a, const Declaration &b) { return 0; } -bool Declaration::FileAndLineEqual(const Declaration &declaration, - bool full) const { - int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, full); +bool Declaration::FileAndLineEqual(const Declaration &declaration) const { + int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, true); 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 5c7772a6db780d..f7d9c0d2d33065 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, true)) + function_info->GetCallSite().FileAndLineEqual(find_call_site)) return inlined_block; inlined_block = inlined_block->GetInlinedParent(); } diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp index f0f7e40ae70d83..db8f8ce6bcbc92 100644 --- a/lldb/source/Symbol/CompileUnit.cpp +++ b/lldb/source/Symbol/CompileUnit.cpp @@ -251,10 +251,7 @@ 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(LLDB_INVALID_LINE_NUMBER); - const uint32_t column_num = - src_location_spec.GetColumn().value_or(LLDB_INVALID_COLUMN_NUMBER); + const uint32_t line = src_location_spec.GetLine().value_or(0); const bool check_inlines = src_location_spec.GetCheckInlines(); // First find all of the file indexes that match our "file_spec". If @@ -315,112 +312,6 @@ void CompileUnit::ResolveSymbolContext( 0, file_indexes, src_location_spec, &line_entry); } - // If we didn't manage to find a breakpoint that matched the line number - // requested, that might be because it is only an inline call site, and - // doesn't have a line entry in the line table. Scan for that here. - // - // We are making the assumption that if there was an inlined function it will - // contribute at least 1 non-call-site entry to the line table. That's handy - // because we don't move line breakpoints over function boundaries, so if we - // found a hit, and there were also a call site entry, it would have to be in - // the function containing the PC of the line table match. That way we can - // limit the call site search to that function. - // We will miss functions that ONLY exist as a call site entry. - - if (line_entry.IsValid() && - (line_entry.line != line || line_entry.column != column_num) && - resolve_scope & eSymbolContextLineEntry && check_inlines) { - // We don't move lines over function boundaries, so the address in the - // line entry will be the in function that contained the line that might - // be a CallSite, and we can just iterate over that function to find any - // inline records, and dig up their call sites. - Address start_addr = line_entry.range.GetBaseAddress(); - Function *function = start_addr.CalculateSymbolContextFunction(); - - Declaration sought_decl(file_spec, line, column_num); - // We use this recursive function to descend the block structure looking - // for a block that has this Declaration as in it's CallSite info. - // This function recursively scans the sibling blocks of the incoming - // block parameter. - std::function<void(Block &)> examine_block = - [&sought_decl, &sc_list, &src_location_spec, resolve_scope, - &examine_block](Block &block) -> void { - // Iterate over the sibling child blocks of the incoming block. - Block *sibling_block = block.GetFirstChild(); - while (sibling_block) { - // We only have to descend through the regular blocks, looking for - // immediate inlines, since those are the only ones that will have this - // callsite. - const InlineFunctionInfo *inline_info = - sibling_block->GetInlinedFunctionInfo(); - if (inline_info) { - // If this is the call-site we are looking for, record that: - // We need to be careful because the call site from the debug info - // will generally have a column, but the user might not have specified - // it. - Declaration found_decl = inline_info->GetCallSite(); - uint32_t sought_column = sought_decl.GetColumn(); - if (found_decl.FileAndLineEqual(sought_decl, false) && - (sought_column == LLDB_INVALID_COLUMN_NUMBER || - sought_column == found_decl.GetColumn())) { - // If we found a call site, it belongs not in this inlined block, - // but in the parent block that inlined it. - Address parent_start_addr; - if (sibling_block->GetParent()->GetStartAddress( - parent_start_addr)) { - SymbolContext sc; - parent_start_addr.CalculateSymbolContext(&sc, resolve_scope); - // Now swap out the line entry for the one we found. - LineEntry call_site_line = sc.line_entry; - call_site_line.line = found_decl.GetLine(); - call_site_line.column = found_decl.GetColumn(); - bool matches_spec = true; - // If the user asked for an exact match, we need to make sure the - // call site we found actually matches the location. - if (src_location_spec.GetExactMatch()) { - matches_spec = false; - if ((src_location_spec.GetFileSpec() == - sc.line_entry.GetFile()) && - (src_location_spec.GetLine() && - *src_location_spec.GetLine() == call_site_line.line) && - (src_location_spec.GetColumn() && - *src_location_spec.GetColumn() == call_site_line.column)) - matches_spec = true; - } - if (matches_spec && - sibling_block->GetRangeAtIndex(0, call_site_line.range)) { - SymbolContext call_site_sc(sc.target_sp, sc.module_sp, - sc.comp_unit, sc.function, sc.block, - &call_site_line, sc.symbol); - sc_list.Append(call_site_sc); - } - } - } - } - - // Descend into the child blocks: - examine_block(*sibling_block); - // Now go to the next sibling: - sibling_block = sibling_block->GetSibling(); - } - }; - - if (function) { - // We don't need to examine the function block, it can't be inlined. - Block &func_block = function->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 ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/113947 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
