Fix a thinko in the CallSite handling code: #114896

jimingham · 2024-11-05T00:00:54Z

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).

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.

llvmbot · 2024-11-05T00:01:28Z

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

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).

Full diff: https://github.com/llvm/llvm-project/pull/114896.diff

5 Files Affected:

(modified) lldb/source/Symbol/CompileUnit.cpp (+6-4)
(added) lldb/test/API/functionalities/breakpoint/same_cu_name/Makefile (+19)
(added) lldb/test/API/functionalities/breakpoint/same_cu_name/TestFileBreakpoinsSameCUName.py (+29)
(added) lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp (+12)
(added) lldb/test/API/functionalities/breakpoint/same_cu_name/main.cpp (+25)

diff --git a/lldb/source/Symbol/CompileUnit.cpp b/lldb/source/Symbol/CompileUnit.cpp
index 73389b2e8479b3..d7df6ee1f221b3 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;
   }
 
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 00000000000000..4bfdb15e777d99
--- /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 00000000000000..74524685b55771
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/same_cu_name/TestFileBreakpoinsSameCUName.py
@@ -0,0 +1,29 @@
+"""
+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 00000000000000..a33a7f1d5b5c47
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp
@@ -0,0 +1,12 @@
+#define XSTR(x) STR(x)
+#define STR(x) #x
+
+
+namespace NAMESPACE {
+  static int g_value = 0;
+void DoSomeStuff() {
+  // A comment here
+  g_value++; // The line with code
+}
+
+}  // end 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 00000000000000..e4313db70a4935
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/same_cu_name/main.cpp
@@ -0,0 +1,25 @@
+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;
+}

github-actions · 2024-11-05T00:04:09Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-11-05T00:04:09Z

✅ With the latest revision this PR passed the C/C++ code formatter.

slackito

Thanks for the fix!

Looks good overall, just a couple of nits. I'm not familiar with this part of the code so feel free to wait for any of the other reviewers if you want someone to actually check your logic changes.

slackito · 2024-11-05T00:05:58Z

lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp

@@ -0,0 +1,12 @@
+#define XSTR(x) STR(x)
+#define STR(x) #x
+


These stringification macros from the original bug repro are no longer needed here.

slackito · 2024-11-05T00:06:18Z

lldb/test/API/functionalities/breakpoint/same_cu_name/common.cpp

+
+namespace NAMESPACE {
+  static int g_value = 0;
+void DoSomeStuff() {


Nit: inconsistent indentation.

jimingham · 2024-11-05T19:23:18Z

This patch seems to fail on x86_64 Linux, but I don't understand how this patch could cause the reported failure. The failure is in a lldb-dap test, here:

File "/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-9dkps-1/llvm-project/github-pull-requests/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py", line 104, in run_test_evaluate_expressions

Comparing the log to a successful one, all the breakpoints got set on the same lines. Then we ran and are stopped at the first breakpoint - which parallels a successful test. We also passed a bunch of self.assertEvaluate's to print various variables. Then we go to run the "var" command in "repl" mode:

        if context == "repl":
            # In the repl context expressions may be interpreted as lldb
            # commands since no variables have the same name as the command.
            self.assertEvaluate("var", r"\(lldb\) var\n.*")

and get no reply. Then we try to kill the DAP process, and that also seems to fail.

I can't reproduce this locally, and I can't see how this failure could result from this patch...

I'm going to apply this patch and keep an eye on the x86_64 bots to see if this was just a glitch or not.

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 803f957)

Added by #114896.

DavidSpickett · 2024-11-06T11:17:10Z

This is failing on Windows on Arm, I'm looking into it now.

DavidSpickett · 2024-11-06T11:41:18Z

Fixed by d8139ae.

jimingham · 2024-11-06T18:12:42Z

Thanks for figuring that out!

Fix a thinko in the CallSite handling code:

38c7625

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.

jimingham requested review from slackito and labath November 5, 2024 00:00

jimingham requested a review from JDevlieghere as a code owner November 5, 2024 00:00

llvmbot added the lldb label Nov 5, 2024

jimingham mentioned this pull request Nov 5, 2024

Fix call site breakpoint patch #114158

Merged

slackito approved these changes Nov 5, 2024

View reviewed changes

clang-format, remove unneeded defines

721d79d

labath approved these changes Nov 5, 2024

View reviewed changes

jimingham merged commit 803f957 into llvm:main Nov 5, 2024
5 of 7 checks passed

jimingham deleted the same-cu-name branch November 5, 2024 19:23

DavidSpickett added a commit that referenced this pull request Nov 6, 2024

[lldb][test] Correct typo in breakpoint test file name

9f8c3d3

Added by #114896.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix a thinko in the CallSite handling code: #114896

Fix a thinko in the CallSite handling code: #114896

Uh oh!

jimingham commented Nov 5, 2024

Uh oh!

llvmbot commented Nov 5, 2024

Uh oh!

github-actions bot commented Nov 5, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Nov 5, 2024 •

edited

Loading

Uh oh!

slackito left a comment

Uh oh!

slackito Nov 5, 2024

Uh oh!

slackito Nov 5, 2024

Uh oh!

jimingham commented Nov 5, 2024

Uh oh!

Uh oh!

DavidSpickett commented Nov 6, 2024

Uh oh!

DavidSpickett commented Nov 6, 2024

Uh oh!

jimingham commented Nov 6, 2024

Uh oh!

Uh oh!

Fix a thinko in the CallSite handling code: #114896

Fix a thinko in the CallSite handling code: #114896

Uh oh!

Conversation

jimingham commented Nov 5, 2024

Uh oh!

llvmbot commented Nov 5, 2024

Uh oh!

github-actions bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slackito left a comment

Choose a reason for hiding this comment

Uh oh!

slackito Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

slackito Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

jimingham commented Nov 5, 2024

Uh oh!

Uh oh!

DavidSpickett commented Nov 6, 2024

Uh oh!

DavidSpickett commented Nov 6, 2024

Uh oh!

jimingham commented Nov 6, 2024

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024 •

edited

Loading