Fix call site breakpoint patch #114158
Merged
Fix call site breakpoint patch #114158
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ove inli… (llvm#113947)" This reverts commit b54bc10.
fixed that in a previous patch, but missed one place where it was checking against 0 incorrectly.
gdb-server was complete, lldb MUST have fetched the complete register state for the stopped thread. There's no reason why it has to do that (especially since we get the PC from the stop message). In a previous patch, I made it not unnecessarily pull the full register contect, causing this test to fail. This commit changes the test to only require the 'g' packet AFTER we had done 'read_register'.
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesThis fixes the two test suite failures that I missed in the PR: 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 The other was a place where some code had used 0 when it meant LLDB_INVALID_LINE_NUMBER, which I had fixed but missed one place where it was still compared to 0. Patch is 43.37 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114158.diff 19 Files Affected:
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index cca00335bc3c67..3592291bb2d06e 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -11,10 +11,12 @@
#include <memory>
#include <mutex>
+#include <optional>
#include "lldb/Breakpoint/BreakpointOptions.h"
#include "lldb/Breakpoint/StoppointHitCounter.h"
#include "lldb/Core/Address.h"
+#include "lldb/Symbol/LineEntry.h"
#include "lldb/Utility/UserID.h"
#include "lldb/lldb-private.h"
@@ -282,6 +284,25 @@ class BreakpointLocation
/// Returns the breakpoint location ID.
lldb::break_id_t GetID() const { return m_loc_id; }
+ /// Set the line entry that should be shown to users for this location.
+ /// It is up to the caller to verify that this is a valid entry to show.
+ /// The current use of this is to distinguish among line entries from a
+ /// virtual inlined call stack that all share the same address.
+ /// The line entry must have the same start address as the address for this
+ /// location.
+ bool SetPreferredLineEntry(const LineEntry &line_entry) {
+ if (m_address == line_entry.range.GetBaseAddress()) {
+ m_preferred_line_entry = line_entry;
+ return true;
+ }
+ assert(0 && "Tried to set a preferred line entry with a different address");
+ return false;
+ }
+
+ const std::optional<LineEntry> GetPreferredLineEntry() {
+ return m_preferred_line_entry;
+ }
+
protected:
friend class BreakpointSite;
friend class BreakpointLocationList;
@@ -306,6 +327,16 @@ class BreakpointLocation
/// If it returns false we should continue, otherwise stop.
bool IgnoreCountShouldStop();
+ /// If this location knows that the virtual stack frame it represents is
+ /// not frame 0, return the suggested stack frame instead. This will happen
+ /// when the location's address contains a "virtual inlined call stack" and
+ /// the breakpoint was set on a file & line that are not at the bottom of that
+ /// stack. For now we key off the "preferred line entry" - looking for that
+ /// in the blocks that start with the stop PC.
+ /// This version of the API doesn't take an "inlined" parameter because it
+ /// only changes frames in the inline stack.
+ std::optional<uint32_t> GetSuggestedStackFrameIndex();
+
private:
void SwapLocation(lldb::BreakpointLocationSP swap_from);
@@ -369,6 +400,11 @@ class BreakpointLocation
lldb::break_id_t m_loc_id; ///< Breakpoint location ID.
StoppointHitCounter m_hit_counter; ///< Number of times this breakpoint
/// location has been hit.
+ /// If this exists, use it to print the stop description rather than the
+ /// LineEntry m_address resolves to directly. Use this for instance when the
+ /// location was given somewhere in the virtual inlined call stack since the
+ /// Address always resolves to the lowest entry in the stack.
+ std::optional<LineEntry> m_preferred_line_entry;
void SetShouldResolveIndirectFunctions(bool do_resolve) {
m_should_resolve_indirect_functions = do_resolve;
diff --git a/lldb/include/lldb/Breakpoint/BreakpointSite.h b/lldb/include/lldb/Breakpoint/BreakpointSite.h
index 17b76d51c1ae53..7b3f7be23639f2 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointSite.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointSite.h
@@ -170,6 +170,11 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>,
/// \see lldb::DescriptionLevel
void GetDescription(Stream *s, lldb::DescriptionLevel level);
+ // This runs through all the breakpoint locations owning this site and returns
+ // the greatest of their suggested stack frame indexes. This only handles
+ // inlined stack changes.
+ std::optional<uint32_t> GetSuggestedStackFrameIndex();
+
/// Tell whether a breakpoint has a location at this site.
///
/// \param[in] bp_id
diff --git a/lldb/include/lldb/Core/Declaration.h b/lldb/include/lldb/Core/Declaration.h
index 4a0e9047b54695..c864b88c6b32a3 100644
--- a/lldb/include/lldb/Core/Declaration.h
+++ b/lldb/include/lldb/Core/Declaration.h
@@ -84,10 +84,14 @@ class Declaration {
/// \param[in] declaration
/// The const Declaration object to compare with.
///
+ /// \param[in] full
+ /// Same meaning as Full in FileSpec::Equal. True means an empty
+ /// directory is not equal to a specified one, false means it is equal.
+ ///
/// \return
/// Returns \b true if \b declaration is at the same file and
/// line, \b false otherwise.
- bool FileAndLineEqual(const Declaration &declaration) const;
+ bool FileAndLineEqual(const Declaration &declaration, bool full) const;
/// Dump a description of this object to a Stream.
///
diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h
index fae90364deaf0a..45beac129e86f7 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -77,6 +77,18 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
m_description.clear();
}
+ /// This gives the StopInfo a chance to suggest a stack frame to select.
+ /// Passing true for inlined_stack will request changes to the inlined
+ /// call stack. Passing false will request changes to the real stack
+ /// frame. The inlined stack gets adjusted before we call into the thread
+ /// plans so they can reason based on the correct values. The real stack
+ /// adjustment is handled after the frame recognizers get a chance to adjust
+ /// the frame.
+ virtual std::optional<uint32_t>
+ GetSuggestedStackFrameIndex(bool inlined_stack) {
+ return {};
+ }
+
virtual bool IsValidForOperatingSystemThread(Thread &thread) { return true; }
/// A Continue operation can result in a false stop event
diff --git a/lldb/include/lldb/Target/ThreadPlanStepInRange.h b/lldb/include/lldb/Target/ThreadPlanStepInRange.h
index f9ef87942a7c03..9da8370ef1c925 100644
--- a/lldb/include/lldb/Target/ThreadPlanStepInRange.h
+++ b/lldb/include/lldb/Target/ThreadPlanStepInRange.h
@@ -80,8 +80,8 @@ class ThreadPlanStepInRange : public ThreadPlanStepRange,
bool m_step_past_prologue; // FIXME: For now hard-coded to true, we could put
// a switch in for this if there's
// demand for that.
- bool m_virtual_step; // true if we've just done a "virtual step", i.e. just
- // moved the inline stack depth.
+ LazyBool m_virtual_step; // true if we've just done a "virtual step", i.e.
+ // just moved the inline stack depth.
ConstString m_step_into_target;
ThreadPlanStepInRange(const ThreadPlanStepInRange &) = delete;
const ThreadPlanStepInRange &
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index ad9057c8141e99..c7ea50407ae1c7 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -508,8 +508,20 @@ void BreakpointLocation::GetDescription(Stream *s,
s->PutCString("re-exported target = ");
else
s->PutCString("where = ");
+
+ // If there's a preferred line entry for printing, use that.
+ bool show_function_info = true;
+ if (auto preferred = GetPreferredLineEntry()) {
+ sc.line_entry = *preferred;
+ // FIXME: We're going to get the function name wrong when the preferred
+ // line entry is not the lowest one. For now, just leave the function
+ // out in this case, but we really should also figure out how to easily
+ // fake the function name here.
+ show_function_info = false;
+ }
sc.DumpStopContext(s, m_owner.GetTarget().GetProcessSP().get(), m_address,
- false, true, false, true, true, true);
+ false, true, false, show_function_info,
+ show_function_info, show_function_info);
} else {
if (sc.module_sp) {
s->EOL();
@@ -537,7 +549,10 @@ void BreakpointLocation::GetDescription(Stream *s,
if (sc.line_entry.line > 0) {
s->EOL();
s->Indent("location = ");
- sc.line_entry.DumpStopContext(s, true);
+ if (auto preferred = GetPreferredLineEntry())
+ preferred->DumpStopContext(s, true);
+ else
+ sc.line_entry.DumpStopContext(s, true);
}
} else {
@@ -656,6 +671,50 @@ void BreakpointLocation::SendBreakpointLocationChangedEvent(
}
}
+std::optional<uint32_t> BreakpointLocation::GetSuggestedStackFrameIndex() {
+ auto preferred_opt = GetPreferredLineEntry();
+ if (!preferred_opt)
+ return {};
+ LineEntry preferred = *preferred_opt;
+ SymbolContext sc;
+ if (!m_address.CalculateSymbolContext(&sc))
+ return {};
+ // Don't return anything special if frame 0 is the preferred line entry.
+ // We not really telling the stack frame list to do anything special in that
+ // case.
+ if (!LineEntry::Compare(sc.line_entry, preferred))
+ return {};
+
+ if (!sc.block)
+ return {};
+
+ // Blocks have their line info in Declaration form, so make one here:
+ Declaration preferred_decl(preferred.GetFile(), preferred.line,
+ preferred.column);
+
+ uint32_t depth = 0;
+ Block *inlined_block = sc.block->GetContainingInlinedBlock();
+ while (inlined_block) {
+ // If we've moved to a block that this isn't the start of, that's not
+ // our inlining info or call site, so we can stop here.
+ Address start_address;
+ if (!inlined_block->GetStartAddress(start_address) ||
+ start_address != m_address)
+ return {};
+
+ const InlineFunctionInfo *info = inlined_block->GetInlinedFunctionInfo();
+ if (info) {
+ if (preferred_decl == info->GetDeclaration())
+ return depth;
+ if (preferred_decl == info->GetCallSite())
+ return depth + 1;
+ }
+ inlined_block = inlined_block->GetInlinedParent();
+ depth++;
+ }
+ return {};
+}
+
void BreakpointLocation::SwapLocation(BreakpointLocationSP swap_from) {
m_address = swap_from->m_address;
m_should_resolve_indirect_functions =
diff --git a/lldb/source/Breakpoint/BreakpointResolver.cpp b/lldb/source/Breakpoint/BreakpointResolver.cpp
index 8307689c7640cf..9643602d78c751 100644
--- a/lldb/source/Breakpoint/BreakpointResolver.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolver.cpp
@@ -340,6 +340,21 @@ void BreakpointResolver::AddLocation(SearchFilter &filter,
}
BreakpointLocationSP bp_loc_sp(AddLocation(line_start));
+ // If the address that we resolved the location to returns a different
+ // LineEntry from the one in the incoming SC, we're probably dealing with an
+ // inlined call site, so set that as the preferred LineEntry:
+ LineEntry resolved_entry;
+ if (!skipped_prologue && bp_loc_sp &&
+ line_start.CalculateSymbolContextLineEntry(resolved_entry) &&
+ LineEntry::Compare(resolved_entry, sc.line_entry)) {
+ // FIXME: The function name will also be wrong here. Do we need to record
+ // that as well, or can we figure that out again when we report this
+ // breakpoint location.
+ if (!bp_loc_sp->SetPreferredLineEntry(sc.line_entry)) {
+ LLDB_LOG(log, "Tried to add a preferred line entry that didn't have the "
+ "same address as this location's address.");
+ }
+ }
if (log && bp_loc_sp && !GetBreakpoint()->IsInternal()) {
StreamString s;
bp_loc_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);
diff --git a/lldb/source/Breakpoint/BreakpointSite.cpp b/lldb/source/Breakpoint/BreakpointSite.cpp
index 3ca93f908e30b8..9700a57d3346e0 100644
--- a/lldb/source/Breakpoint/BreakpointSite.cpp
+++ b/lldb/source/Breakpoint/BreakpointSite.cpp
@@ -87,6 +87,23 @@ void BreakpointSite::GetDescription(Stream *s, lldb::DescriptionLevel level) {
m_constituents.GetDescription(s, level);
}
+std::optional<uint32_t> BreakpointSite::GetSuggestedStackFrameIndex() {
+
+ std::optional<uint32_t> result;
+ std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex);
+ for (BreakpointLocationSP loc_sp : m_constituents.BreakpointLocations()) {
+ std::optional<uint32_t> loc_frame_index =
+ loc_sp->GetSuggestedStackFrameIndex();
+ if (loc_frame_index) {
+ if (result)
+ result = std::max(*loc_frame_index, *result);
+ else
+ result = loc_frame_index;
+ }
+ }
+ return result;
+}
+
bool BreakpointSite::IsInternal() const { return m_constituents.IsInternal(); }
uint8_t *BreakpointSite::GetTrapOpcodeBytes() { return &m_trap_opcode[0]; }
diff --git a/lldb/source/Core/Declaration.cpp b/lldb/source/Core/Declaration.cpp
index 579a3999d14ea0..a485c4b9ba48a7 100644
--- a/lldb/source/Core/Declaration.cpp
+++ b/lldb/source/Core/Declaration.cpp
@@ -70,8 +70,9 @@ int Declaration::Compare(const Declaration &a, const Declaration &b) {
return 0;
}
-bool Declaration::FileAndLineEqual(const Declaration &declaration) const {
- int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, true);
+bool Declaration::FileAndLineEqual(const Declaration &declaration,
+ bool full) const {
+ int file_compare = FileSpec::Compare(this->m_file, declaration.m_file, full);
return file_compare == 0 && this->m_line == declaration.m_line;
}
diff --git a/lldb/source/Symbol/Block.cpp b/lldb/source/Symbol/Block.cpp
index f7d9c0d2d33065..5c7772a6db780d 100644
--- a/lldb/source/Symbol/Block.cpp
+++ b/lldb/source/Symbol/Block.cpp
@@ -230,7 +230,7 @@ Block *Block::GetContainingInlinedBlockWithCallSite(
const auto *function_info = inlined_block->GetInlinedFunctionInfo();
if (function_info &&
- function_info->GetCallSite().FileAndLineEqual(find_call_site))
+ function_info->GetCallSite().FileAndLineEqual(find_call_site, true))
return inlined_block;
inlined_block = inlined_block->GetInlinedParent();
}
diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp
index db8f8ce6bcbc92..73389b2e8479b3 100644
--- a/lldb/source/Symbol/CompileUnit.cpp
+++ b/lldb/source/Symbol/CompileUnit.cpp
@@ -251,7 +251,10 @@ void CompileUnit::ResolveSymbolContext(
SymbolContextItem resolve_scope, SymbolContextList &sc_list,
RealpathPrefixes *realpath_prefixes) {
const FileSpec file_spec = src_location_spec.GetFileSpec();
- const uint32_t line = src_location_spec.GetLine().value_or(0);
+ const uint32_t line =
+ src_location_spec.GetLine().value_or(LLDB_INVALID_LINE_NUMBER);
+ const uint32_t column_num =
+ src_location_spec.GetColumn().value_or(LLDB_INVALID_COLUMN_NUMBER);
const bool check_inlines = src_location_spec.GetCheckInlines();
// First find all of the file indexes that match our "file_spec". If
@@ -268,7 +271,7 @@ void CompileUnit::ResolveSymbolContext(
SymbolContext sc(GetModule());
sc.comp_unit = this;
- if (line == 0) {
+ if (line == LLDB_INVALID_LINE_NUMBER) {
if (file_spec_matches_cu_file_spec && !check_inlines) {
// only append the context if we aren't looking for inline call sites by
// file and line and if the file spec matches that of the compile unit
@@ -312,6 +315,112 @@ void CompileUnit::ResolveSymbolContext(
0, file_indexes, src_location_spec, &line_entry);
}
+ // If we didn't manage to find a breakpoint that matched the line number
+ // requested, that might be because it is only an inline call site, and
+ // doesn't have a line entry in the line table. Scan for that here.
+ //
+ // We are making the assumption that if there was an inlined function it will
+ // contribute at least 1 non-call-site entry to the line table. That's handy
+ // because we don't move line breakpoints over function boundaries, so if we
+ // found a hit, and there were also a call site entry, it would have to be in
+ // the function containing the PC of the line table match. That way we can
+ // limit the call site search to that function.
+ // We will miss functions that ONLY exist as a call site entry.
+
+ if (line_entry.IsValid() &&
+ (line_entry.line != line || line_entry.column != column_num) &&
+ resolve_scope & eSymbolContextLineEntry && check_inlines) {
+ // We don't move lines over function boundaries, so the address in the
+ // line entry will be the in function that contained the line that might
+ // be a CallSite, and we can just iterate over that function to find any
+ // inline records, and dig up their call sites.
+ Address start_addr = line_entry.range.GetBaseAddress();
+ Function *function = start_addr.CalculateSymbolContextFunction();
+
+ Declaration sought_decl(file_spec, line, column_num);
+ // We use this recursive function to descend the block structure looking
+ // for a block that has this Declaration as in it's CallSite info.
+ // This function recursively scans the sibling blocks of the incoming
+ // block parameter.
+ std::function<void(Block &)> examine_block =
+ [&sought_decl, &sc_list, &src_location_spec, resolve_scope,
+ &examine_block](Block &block) -> void {
+ // Iterate over the sibling child blocks of the incoming block.
+ Block *sibling_block = block.GetFirstChild();
+ while (sibling_block) {
+ // We only have to descend through the regular blocks, looking for
+ // immediate inlines, since those are the only ones that will have this
+ // callsite.
+ const InlineFunctionInfo *inline_info =
+ sibling_block->GetInlinedFunctionInfo();
+ if (inline_info) {
+ // If this is the call-site we are looking for, record that:
+ // We need to be careful because the call site from the debug info
+ // will generally have a column, but the user might not have specified
+ // it.
+ Declaration found_decl = inline_info->GetCallSite();
+ uint32_t sought_column = sought_decl.GetColumn();
+ if (found_decl.FileAndLineEqual(sought_decl, false) &&
+ (sought_column == LLDB_INVALID_COLUMN_NUMBER ||
+ sought_column == found_decl.GetColumn())) {
+ // If we found a call site, it belongs not in this inlined block,
+ // but in the parent block that inlined it.
+ Address parent_start_addr;
+ if (sibling_block->GetParent()->GetStartAddress(
+ parent_start_addr)) {
+ SymbolContext sc;
+ parent_start_addr.CalculateSymbolContext(&sc, resolve_scope);
+ // Now swap out the line entry for the one we found.
+ LineEntry call_site_line = sc.line_entry;
+ call_site_line.line = found_decl.GetLine();
+ call_site_line.column = found_decl.GetColumn();
+ bool matches_spec = true;
+ // If the user asked for an exact match, we need to make sure the
+ // call site we found actually matches the location.
+ if (src_location_spec.GetExactMatch()) {
+ matches_spec = false;
+ if ((src_location_spec.GetFileSpec() ==
+ sc.line_entry.GetFile()) &&
+ (src_location_spec.GetLine() &&
+ *src_location_spec.GetLine() == call_site_line.line) &&
+ (src_location_spec.GetColumn() &&
+ *src_location_spec.GetColumn() == call_site_line.column))
+ matches_spec = true;
+ }
+ if (matches_spec &&
+ sibling_block->GetRangeAtIndex(0, call_site_line.range)) {
+ SymbolContext call_site_sc(sc.target_sp, sc.module_sp,
+ sc.comp_unit, sc.function, sc.block,
+ &call_site_line, sc.symbol);
+ sc_list.Append(call_site_sc);
+ }
+ }
+ }
+ }
+
+ // Descend into the child blocks:
+ examine_block(*sibling_block);
+ // Now go to the next sibling:
+ sibling_block = sibling_block->GetSibling();
+ }
+ };
+
+ if (function) {
+ // We don't need to examine the function block, it can't be inlined.
+ Block &func_block = function->GetBlo...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/8023 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/7689 Here is the relevant piece of the build log for the reference |
|
@jimingham your change is causing two cross-project-test failures, can you take a look or revert if you need time to investigate? |
|
That's fixed in:
#114337
Fix stepping away from the bottom-most frame of a virtual inlined call stack. by jimingham · Pull Request #114337 · llvm/llvm-project
github.com
which is waiting on review. Would have fixed this sooner but dexter is a pretty opaque test harness...
Jim
… On Oct 30, 2024, at 4:07 PM, dyung ***@***.***> wrote:
@jimingham <https://github.com/jimingham> your change is causing two cross-project-test failures, can you take a look or revert if you need time to investigate?
—
Reply to this email directly, view it on GitHub <#114158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZKLVM7OELXQQLPSVLZ6FRCXAVCNFSM6AAAAABQ27K63WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYGYZTOMBVGQ>.
You are receiving this because you were mentioned.
|
|
Hi, I'm having some problems with one of our tests, that also reproduce after #114337. I don't have a repro yet but I'll describe what I know of the problem so far in case it rings a bell. The test in question has a common source file with the following structure: This file gets compiled multiple times with Before this commit, running I'll do my best to share a repro case later today. |
|
Repro was pretty straightforward from the description above. common.cc: #include <cstdio>
#define XSTR(x) STR(x)
#define STR(x) #x
namespace NAMESPACE {
void DoSomeStuff() {
printf("%s::DoSomeStuff()\n", XSTR(NAMESPACE));
}
} // end NAMESPACEmain.cc: namespace ns1 {
extern void DoSomeStuff();
} // namespace ns1
namespace ns2 {
extern void DoSomeStuff();
} // namespace ns2
namespace ns3 {
extern void DoSomeStuff();
} // namespace ns3
namespace ns4 {
extern void DoSomeStuff();
} // namespace ns4
int main(int argc, char* argv[]) {
ns1::DoSomeStuff();
ns2::DoSomeStuff();
ns3::DoSomeStuff();
ns4::DoSomeStuff();
return 0;
}build.sh #!/bin/bash
g++ -c -g -DNAMESPACE=ns1 -o ns1.o common.cc
g++ -c -g -DNAMESPACE=ns2 -o ns2.o common.cc
g++ -c -g -DNAMESPACE=ns3 -o ns3.o common.cc
g++ -c -g -DNAMESPACE=ns4 -o ns4.o common.cc
g++ -g -o main ns1.o ns2.o ns3.o ns4.o main.ccwith an lldb built at the revision prior to this commit: at this revision (and also at HEAD as of 5 minutes ago): |
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
This fixes the two test suite failures that I missed in the PR: llvm#112939 One was a poorly written test case - it assumed that on connect to a gdb-remote with a running process, lldb MUST have fetched all the frame 0 registers. In fact, there's no need for it to do so (as the CallSite patch showed...) and if we don't need to we shouldn't. So I fixed the test to only expect a `g` packet AFTER calling read_registers. The other was a place where some code had used 0 when it meant LLDB_INVALID_LINE_NUMBER, which I had fixed but missed one place where it was still compared to 0.
|
So it sounds like the problem is that lldb no longer looks for all compile units with the given name when setting a breakpoint. Changing that doesn't seem like it was the intention of this patch. Jim, is there an easy fix for this or should we revert the patch for now? |
|
Give me a bit to look at this. The intention of this patch was just to add more locations, it shouldn't be reducing the number of breakpoints. There's likely some simple goof here.
Jim
… On Nov 4, 2024, at 2:44 AM, Pavel Labath ***@***.***> wrote:
So it sounds like the problem is that lldb no longer looks for all compile units with the given name when setting a breakpoint. Changing that doesn't seem like it was the intention of this patch. Jim, is there an easy fix for this or should we revert the patch for now?
—
Reply to this email directly, view it on GitHub <#114158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3EAWSSOK7BI5AW6ZDZ65FYTAVCNFSM6AAAAABQ27K63WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJUGM3TGNZUGM>.
You are receiving this because you were mentioned.
|
|
There were two issues here, one very minor, and then one that mattered... This was a little thinko, I forgot that we build into the same SymbolContextList over all the CU iterations, so I needed to check "did I add to the SC list" not "is there anything in the SC list" to see if looking for a call site found anything:
diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp
index 73389b2e8479..d7df6ee1f221 100644
--- a/lldb/source/Symbol/CompileUnit.cpp
+++ b/lldb/source/Symbol/CompileUnit.cpp
@@ -326,16 +326,18 @@ void CompileUnit::ResolveSymbolContext(
// 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) {
+ (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
@@ -417,7 +419,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;
}
But I'm kind of surprised that nothing in our test suite was sensitive to this. I'll work up this example into a test, and then submit this.
Thanks for the example!
Jim
… On Nov 4, 2024, at 9:36 AM, Jim Ingham ***@***.***> wrote:
Give me a bit to look at this. The intention of this patch was just to add more locations, it shouldn't be reducing the number of breakpoints. There's likely some simple goof here.
Jim
> On Nov 4, 2024, at 2:44 AM, Pavel Labath ***@***.***> wrote:
>
>
> So it sounds like the problem is that lldb no longer looks for all compile units with the given name when setting a breakpoint. Changing that doesn't seem like it was the intention of this patch. Jim, is there an easy fix for this or should we revert the patch for now?
>
> —
> Reply to this email directly, view it on GitHub <#114158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3EAWSSOK7BI5AW6ZDZ65FYTAVCNFSM6AAAAABQ27K63WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJUGM3TGNZUGM>.
> You are receiving this because you were mentioned.
>
|
|
Thanks for the quick response, Jim! |
|
#114896
Fix a thinko in the CallSite handling code: by jimingham · Pull Request #114896 · llvm/llvm-project
github.com
Jim
… On Nov 4, 2024, at 11:03 AM, Jorge Gorbe Moya ***@***.***> wrote:
Thanks for the quick response, Jim!
—
Reply to this email directly, view it on GitHub <#114158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW5PYD25ZZPH2OFDBFLZ66ZHZAVCNFSM6AAAAABQ27K63WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJVGQ4DINBUGM>.
You are receiving this because you were mentioned.
|
jimingham
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Nov 7, 2024
This fixes the two test suite failures that I missed in the PR: llvm#112939 One was a poorly written test case - it assumed that on connect to a gdb-remote with a running process, lldb MUST have fetched all the frame 0 registers. In fact, there's no need for it to do so (as the CallSite patch showed...) and if we don't need to we shouldn't. So I fixed the test to only expect a `g` packet AFTER calling read_registers. The other was a place where some code had used 0 when it meant LLDB_INVALID_LINE_NUMBER, which I had fixed but missed one place where it was still compared to 0. (cherry picked from commit 7dbbd2b)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the two test suite failures that I missed in the PR:
#112939
One was a poorly written test case - it assumed that on connect to a gdb-remote with a running process, lldb MUST have fetched all the frame 0 registers. In fact, there's no need for it to do so (as the CallSite patch showed...) and if we don't need to we shouldn't. So I fixed the test to only expect a
gpacket 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.