llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) <details> <summary>Changes</summary> Fixes #<!-- -->205120 In which due to delayed breakpoints, a breakpoint that would become an external breakpoint later (meaning managed by the debug server) was temporarily stored as a software breakpoint (which is managed by lldb) with a zero size breakpoint site. That zero site site tripped an assertion when you tried to write over the site. To fix this, I've explicitly ignored zero size sites in FindInRange by defining them as never overlapping. FindInRange is only used for patching reads and writes, so I think this is safe to do. I have documented this in the docstring. I considered adding a breakpoint type "uncommitted", but software breakpoints are actually handled in the most conservative manner (reads and writes are always patched). So I think as a default it's fine (also I don't want to go and audit all the places that use that enum and end up with a way larger fix for this issue). Though I have a few debugserver fixes in flight, since this is a fix in lldb, I think the added API test will work for lldb-server and debugserver. --- Full diff: https://github.com/llvm/llvm-project/pull/217919.diff 2 Files Affected: - (modified) lldb/include/lldb/Breakpoint/StopPointSiteList.h (+9-2) - (modified) lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py (+88) ``````````diff diff --git a/lldb/include/lldb/Breakpoint/StopPointSiteList.h b/lldb/include/lldb/Breakpoint/StopPointSiteList.h index 101eccda4616b..088c2b3c1ee1d 100644 --- a/lldb/include/lldb/Breakpoint/StopPointSiteList.h +++ b/lldb/include/lldb/Breakpoint/StopPointSiteList.h @@ -182,6 +182,9 @@ template <typename StopPointSite> class StopPointSiteList { return false; } + /// Find breakpoint sites that in any way overlap the range starting at + /// \a lower_bound and ending at \a upper_bound (but not including it). + /// Zero sized sites are treated as never overlapping. bool FindInRange(lldb::addr_t lower_bound, lldb::addr_t upper_bound, StopPointSiteList &bp_site_list) const { if (lower_bound > upper_bound) @@ -200,14 +203,18 @@ template <typename StopPointSite> class StopPointSiteList { typename collection::const_iterator prev_pos = lower; prev_pos--; const StopPointSiteSP &prev_site = (*prev_pos).second; - if (prev_site->GetLoadAddress() + prev_site->GetByteSize() > lower_bound) + if (prev_site->GetByteSize() != 0 && + (prev_site->GetLoadAddress() + prev_site->GetByteSize() > + lower_bound)) bp_site_list.Add(prev_site); } upper = m_site_list.upper_bound(upper_bound); for (pos = lower; pos != upper; pos++) - bp_site_list.Add((*pos).second); + if (pos->second->GetByteSize() != 0) + bp_site_list.Add(pos->second); + return true; } diff --git a/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py index 4d33382fe3bd9..6b9f599ef2bc2 100644 --- a/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py +++ b/lldb/test/API/functionalities/breakpoint/write_over_software_breakpoint/TestWriteOverSoftwareBreakpoint.py @@ -159,3 +159,91 @@ def test_write_over_breakpoint(self): self.assertState(process.GetState(), lldb.eStateStopped) self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonBreakpoint) self.assertEqual(loop_start_breakpoint_addr, thread.selected_frame.GetPC()) + + def test_write_over_uncommitted_breakpoint(self): + TestBase.setUp(self) + self.line = line_number("main.c", "// break here") + self.build() + exe = self.getBuildArtifact("a.out") + self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET) + + self.runCmd("settings set target.process.use-delayed-breakpoints true") + + lldbutil.run_break_set_by_file_and_line( + self, "main.c", self.line, num_expected_locations=1, loc_exact=True + ) + self.runCmd("run", RUN_SUCCEEDED) + self.expect( + "thread list", + STOPPED_DUE_TO_BREAKPOINT, + substrs=["stopped", "stop reason = breakpoint"], + ) + + target = self.dbg.GetSelectedTarget() + process = target.GetProcess() + + loop_start_breakpoint_addr = ( + target.breakpoints[0].GetLocationAtIndex(0).GetLoadAddress() + ) + + bkpt = target.BreakpointCreateByName("foo") + self.assertTrue(bkpt.IsValid()) + self.assertEqual(bkpt.GetNumLocations(), 1) + self.assertFalse(bkpt.IsHardware()) + + # At this point only lldb knows about the breakpoint, it has not been + # sent to the debug server yet. It is treated as software with a 0 size + # breakpoint site. + + bkpt_address = bkpt.GetLocationAtIndex(0).GetLoadAddress() + # The largest breakpoint instruction we know of is 4 bytes. + read_size = 4 + err = lldb.SBError() + original_data = bytearray(process.ReadMemory(bkpt_address, read_size, err)) + self.assertSuccess(err) + self.assertEqual(len(original_data), read_size) + + # The smallest break instruction we know about is 1 byte, so we will + # write only 1 byte. The value is ideally != byte 1 of the break and + # != to the value currently in memory. 0xcd is different to x86's 0xcc, + # and not used by any other platform we support. 0xdc is the fallback + # and if the original value is also 0xcd. + write_data = bytearray([0xCD if original_data[0] != 0xCD else 0xDC]) + + # Write something over the breakpoint. Use a single byte since x86's + # break is a single byte and we do not want to corrupt other code. + # LLDB used to crash at this point. + err = lldb.SBError() + wrote = process.WriteMemory(bkpt_address, write_data, err) + self.assertSuccess(err) + self.assertEqual(wrote, 1) + + def check_memory(): + err = lldb.SBError() + got = bytearray(process.ReadMemory(bkpt_address, read_size, err)) + self.assertSuccess(err) + self.assertEqual(len(got), read_size) + read_expected = write_data + original_data[1:] + self.assertEqual(got, read_expected) + + # We should see the original data with our new single byte at the start. + check_memory() + + # The instruction in memory should still be intact so we can continue + # to the breakpoint. + process.Continue() + + thread = process.thread[0] + self.assertState(process.GetState(), lldb.eStateStopped) + self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonBreakpoint) + # Should be stopped at the breakpoint we placed in foo. This proves that + # the breakpoint instruction was intact. + self.assertEqual( + bkpt_address, + thread.selected_frame.GetPC(), + ) + + # We should see the new first byte still. At this point the debug server + # should be managing the breakpoint, but this checks that the handover + # was done correctly. + check_memory() `````````` </details> https://github.com/llvm/llvm-project/pull/217919 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
