Skip to content

Fix a bug in the breakpoint ID verifier in CommandObjectBreakpoint. #145994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 27, 2025

Conversation

jimingham
Copy link
Collaborator

It was assuming that for any location M.N, N was always less than the number of breakpoint locations. But if you rebuild the target and rerun multiple times, when the section backing one of the locations is no longer valid, we remove the location, but we don't reuse the ID. So you can have a breakpoint that only has location 1.3. The num_locations check would say that was an invalid location.

It was assuming that for any location M.N, N was always less than
the number of breakpoint locations.  But if you rebuild the target
and rerun multiple times, when the section backing one of the
locations is no longer valid, we remove the location, but we don't
reuse the ID.  So you can have a breakpoint that only has location 1.3.
The num_locations check would say that was an invalid location.
@jimingham jimingham requested a review from JDevlieghere as a code owner June 26, 2025 23:43
@llvmbot llvmbot added the lldb label Jun 26, 2025
@jimingham jimingham requested a review from adrian-prantl June 26, 2025 23:43
@llvmbot
Copy link
Member

llvmbot commented Jun 26, 2025

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

It was assuming that for any location M.N, N was always less than the number of breakpoint locations. But if you rebuild the target and rerun multiple times, when the section backing one of the locations is no longer valid, we remove the location, but we don't reuse the ID. So you can have a breakpoint that only has location 1.3. The num_locations check would say that was an invalid location.


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

6 Files Affected:

  • (modified) lldb/source/Commands/CommandObjectBreakpoint.cpp (+3-2)
  • (added) lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/Makefile (+1)
  • (added) lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/TestLocationsAfterRebuild.py (+54)
  • (added) lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/main.c (+4)
  • (added) lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/second_main.c (+5)
  • (added) lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/third_main.c (+5)
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index a0c39cf670d46..4631c97bf50ae 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -2485,8 +2485,9 @@ void CommandObjectMultiwordBreakpoint::VerifyIDs(
     Breakpoint *breakpoint =
         target.GetBreakpointByID(cur_bp_id.GetBreakpointID()).get();
     if (breakpoint != nullptr) {
-      const size_t num_locations = breakpoint->GetNumLocations();
-      if (static_cast<size_t>(cur_bp_id.GetLocationID()) > num_locations) {
+      lldb::break_id_t cur_loc_id = cur_bp_id.GetLocationID();
+      // GetLocationID returns 0 when the location isn't specified.
+      if (cur_loc_id != 0 && !breakpoint->FindLocationByID(cur_loc_id)) {
         StreamString id_str;
         BreakpointID::GetCanonicalReference(
             &id_str, cur_bp_id.GetBreakpointID(), cur_bp_id.GetLocationID());
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/Makefile b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/Makefile
new file mode 100644
index 0000000000000..22f1051530f87
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/Makefile
@@ -0,0 +1 @@
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/TestLocationsAfterRebuild.py b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/TestLocationsAfterRebuild.py
new file mode 100644
index 0000000000000..146e5c7c8b95b
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/TestLocationsAfterRebuild.py
@@ -0,0 +1,54 @@
+"""
+When a rebuild causes a location to be removed, make sure
+we still handle the remaining locations correctly.
+"""
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+import os
+
+class TestLocationsAfterRebuild(TestBase):
+    # If your test case doesn't stress debug info, then
+    # set this to true.  That way it won't be run once for
+    # each debug info format.
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_remaining_location_spec(self):
+        """If we rebuild a couple of times some of the old locations
+           get removed.  Make sure the command-line breakpoint id
+           validator still works correctly."""
+        self.build(dictionary={"C_SOURCES" : "main.c", "EXE" : "a.out"})
+
+        path_to_exe = self.getBuildArtifact()
+        
+        (target, process, thread, bkpt) = lldbutil.run_to_name_breakpoint(
+            self, "main"
+        )
+
+        # Let the process continue to exit:
+        process.Continue()
+        self.assertEqual(process.GetState(), lldb.eStateExited, "Ran to completion")
+        os.remove(path_to_exe)
+
+        # We have to rebuild twice with changed sources to get
+        # us to remove the first set of locations:
+        self.build(dictionary={"C_SOURCES" : "second_main.c", "EXE" : "a.out"})
+
+        (target, process, thread, bkpt) = lldbutil.run_to_breakpoint_do_run(self, target, bkpt)
+        
+        # Let the process continue to exit:
+        process.Continue()
+        self.assertEqual(process.GetState(), lldb.eStateExited, "Ran to completion")
+
+        os.remove(path_to_exe)
+
+        self.build(dictionary={"C_SOURCES" : "third_main.c", "EXE" : "a.out"})
+
+        (target, process, thread, bkpt) = lldbutil.run_to_breakpoint_do_run(self, target, bkpt)
+
+        bkpt_id = bkpt.GetID()
+        loc_string = f"{bkpt_id}.3"
+        self.runCmd(f"break disable {loc_string}")
+        
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/main.c b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/main.c
new file mode 100644
index 0000000000000..1060823a1fb59
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/main.c
@@ -0,0 +1,4 @@
+int
+main() {
+  return 1111;
+}
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/second_main.c b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/second_main.c
new file mode 100644
index 0000000000000..8cbcb2f2d7add
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/second_main.c
@@ -0,0 +1,5 @@
+int
+main()
+{
+  return 22222;
+}
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/third_main.c b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/third_main.c
new file mode 100644
index 0000000000000..1227194d36694
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/after_rebuild/third_main.c
@@ -0,0 +1,5 @@
+int
+main()
+{
+  return 33333;
+}

Copy link

github-actions bot commented Jun 26, 2025

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

Copy link

github-actions bot commented Jun 26, 2025

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

@zwuis
Copy link
Contributor

zwuis commented Jun 26, 2025

Please add a tag to the beginning of the PR title in square brackets like other PRs. E.g. [lldb].

See https://llvm.org/docs/DeveloperPolicy.html#commit-messages.

Copy link
Contributor

@kastiglione kastiglione left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing this

@jimingham jimingham merged commit ec48d15 into llvm:main Jun 27, 2025
7 checks passed
DavidSpickett added a commit that referenced this pull request Jun 27, 2025
We can't remove the program file while lldb has it open.

Test added in #145994.
@slydiman
Copy link
Contributor

The buildbot lldb-x86_64-win is broken after this patch.
Please fix it ASAP. We are planning to switch this buildbot to the production mode soon.

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jun 27, 2025
We can't remove the program file while lldb has it open.

Test added in llvm/llvm-project#145994.
@DavidSpickett
Copy link
Collaborator

Already done by 3f00cff.

@jimingham
Copy link
Collaborator Author

Why did this fail? I built a binary in the build directory, then went to remove it with os.remove and apparently don't have access to remove the file I just built???

@jimingham
Copy link
Collaborator Author

There weren't even any processes still using that file at the point where I went to delete it - not that that should be a problem. I was careful to run to completion before trying to delete the file.

@DavidSpickett
Copy link
Collaborator

We can't remove the program file while lldb has it open.

        path_to_exe = self.getBuildArtifact()

        (target, process, thread, bkpt) = lldbutil.run_to_name_breakpoint(self, "main")

        # Let the process continue to exit:
        process.Continue()
        self.assertEqual(process.GetState(), lldb.eStateExited, "Ran to completion")
        os.remove(path_to_exe)

The interactive equivalent is:

target file
b main
run
continue
(program exits)

You cannot delete the file here because something is still holding onto it. If you exit LLDB completely, then you can delete the file.

Over zealous code in the Windows process plugin? Probably, but I wasn't about to go digging for it.

rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
…lvm#145994)

It was assuming that for any location M.N, N was always less than the
number of breakpoint locations. But if you rebuild the target and rerun
multiple times, when the section backing one of the locations is no
longer valid, we remove the location, but we don't reuse the ID. So you
can have a breakpoint that only has location 1.3. The num_locations
check would say that was an invalid location.
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
We can't remove the program file while lldb has it open.

Test added in llvm#145994.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants