-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Conversation
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.
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesIt 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:
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;
+}
|
✅ With the latest revision this PR passed the Python code formatter. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
Please add a tag to the beginning of the PR title in square brackets like other PRs. E.g. See https://llvm.org/docs/DeveloperPolicy.html#commit-messages. |
There was a problem hiding this 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
We can't remove the program file while lldb has it open. Test added in #145994.
The buildbot lldb-x86_64-win is broken after this patch. |
We can't remove the program file while lldb has it open. Test added in llvm/llvm-project#145994.
Already done by 3f00cff. |
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??? |
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. |
The interactive equivalent is:
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. |
…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.
We can't remove the program file while lldb has it open. Test added in llvm#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.