[Lldb-commits] [lldb] Fix call site breakpoint patch (PR #114158)

[via lldb-commits]

jimingham wrote:

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 <https://github.com/llvm/llvm-project/pull/114158#issuecomment-2454373743>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3EAWSSOK7BI5AW6ZDZ65FYTAVCNFSM6AAAAABQ27K63WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJUGM3TGNZUGM>.
>> You are receiving this because you were mentioned.
>> 
> 



https://github.com/llvm/llvm-project/pull/114158