-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[lldb] Add SB API to make a breakpoint a hardware breakpoint #146602
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
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis adds SBBreakpoint::SetIsHardware, allowing clients to mark an existing breakpoint as a hardware breakpoint purely through the API. This is safe to do after creation, as the hardware/software distinction doesn't affect how breakpoint locations are selected. In some cases (e.g. when writing a trap instruction would alter program behavior), it's important to use hardware breakpoints. Ideally, we’d extend the various rdar://153528045 Full diff: https://github.com/llvm/llvm-project/pull/146602.diff 6 Files Affected:
diff --git a/lldb/include/lldb/API/SBBreakpoint.h b/lldb/include/lldb/API/SBBreakpoint.h
index e08df3b6d5ab0..307c9e13d7e39 100644
--- a/lldb/include/lldb/API/SBBreakpoint.h
+++ b/lldb/include/lldb/API/SBBreakpoint.h
@@ -148,6 +148,8 @@ class LLDB_API SBBreakpoint {
bool IsHardware() const;
+ void SetIsHardware(bool is_hardware);
+
// Can only be called from a ScriptedBreakpointResolver...
SBError
AddLocation(SBAddress &address);
diff --git a/lldb/include/lldb/Breakpoint/Breakpoint.h b/lldb/include/lldb/Breakpoint/Breakpoint.h
index f623a2e0c295b..4821932d5e29f 100644
--- a/lldb/include/lldb/Breakpoint/Breakpoint.h
+++ b/lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -518,6 +518,7 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
lldb::break_id_t bp_loc_id);
bool IsHardware() const { return m_hardware; }
+ void SetIsHardware(bool is_hardware) { m_hardware = is_hardware; }
lldb::BreakpointResolverSP GetResolver() { return m_resolver_sp; }
diff --git a/lldb/source/API/SBBreakpoint.cpp b/lldb/source/API/SBBreakpoint.cpp
index 87fadbcec4f26..d790b3f4ca271 100644
--- a/lldb/source/API/SBBreakpoint.cpp
+++ b/lldb/source/API/SBBreakpoint.cpp
@@ -781,6 +781,17 @@ bool SBBreakpoint::IsHardware() const {
return false;
}
+void SBBreakpoint::SetIsHardware(bool is_hardware) {
+ LLDB_INSTRUMENT_VA(this, is_hardware);
+
+ BreakpointSP bkpt_sp = GetSP();
+ if (bkpt_sp) {
+ std::lock_guard<std::recursive_mutex> guard(
+ bkpt_sp->GetTarget().GetAPIMutex());
+ bkpt_sp->SetIsHardware(is_hardware);
+ }
+}
+
BreakpointSP SBBreakpoint::GetSP() const { return m_opaque_wp.lock(); }
// This is simple collection of breakpoint id's and their target.
diff --git a/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/Makefile b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/Makefile
new file mode 100644
index 0000000000000..304633c2dca1f
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/Makefile
@@ -0,0 +1,7 @@
+C_SOURCES := main.c
+
+ifeq ($(CC_TYPE), icc)
+ CFLAGS_EXTRAS := -debug inline-debug-info
+endif
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/TestSimpleHWBreakpoints.py b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/TestSimpleHWBreakpoints.py
new file mode 100644
index 0000000000000..b40bcf7b4ae9c
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/TestSimpleHWBreakpoints.py
@@ -0,0 +1,33 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+from functionalities.breakpoint.hardware_breakpoints.base import *
+
+
+class SimpleHWBreakpointTest(HardwareBreakpointTestBase):
+ def test(self):
+ """Test SBBreakpoint::SetIsHardware"""
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe)
+
+ breakpoint = target.BreakpointCreateByLocation("main.c", 1)
+ self.assertFalse(breakpoint.IsHardware())
+ breakpoint.SetIsHardware(True)
+ self.assertTrue(breakpoint.IsHardware())
+
+ self.runCmd("run", RUN_SUCCEEDED)
+
+ self.expect(
+ "thread list",
+ STOPPED_DUE_TO_BREAKPOINT,
+ substrs=["stopped", "stop reason = breakpoint"],
+ )
+
+ # Check the breakpoint list.
+ self.expect(
+ "breakpoint list -v",
+ substrs=["file = 'main.c'", "hardware = true"],
+ )
diff --git a/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/main.c b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/main.c
new file mode 100644
index 0000000000000..7d49a57d4c7b9
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/simple_hw_breakpoints/main.c
@@ -0,0 +1,9 @@
+int break_on_me() {
+ int i = 10;
+ i++;
+ return i;
+}
+
+int main() {
+ return break_on_me();
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
When I saw the request for this ability, I dreaded all the API surface impacted, not a bad way to do it. But changing the breakpoint to Hardware after it has been set (Z0 in gdb-remote) and will not be re-set as a hardware breakpoint (Z1). In fact, we'll continue execution with it as a software breakpoint, then try to remove it with a |
just a quick sniff test I tried
so maybe this is OK. |
ah sure enough,
there's no requirement to be correct about which packet you use to clear an existing breakpoint, with debugserver lldb-server passes whether a hardware or software breakpoint is being requested to be cleared,
and
good chance this won't work on linux. |
925f3fb
to
656beb2
Compare
I think SetIsHardware needs a return value. After all, you could have a software breakpoint set, and then make as many other hardware breakpoints as you have resources for, and THEN you go to change the software breakpoint to hardware. We need to tell the user that that didn't work. |
77cbaee
to
0098759
Compare
lldb/include/lldb/API/SBBreakpoint.h
Outdated
@@ -148,6 +148,8 @@ class LLDB_API SBBreakpoint { | |||
|
|||
bool IsHardware() const; | |||
|
|||
bool SetIsHardware(bool is_hardware); |
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.
Document the return value's meaning somewhere.
@@ -69,7 +69,7 @@ class BreakpointLocation | |||
// The next section deals with various breakpoint options. | |||
|
|||
/// If \a enabled is \b true, enable the breakpoint, if \b false disable it. | |||
void SetEnabled(bool enabled); | |||
bool SetEnabled(bool enabled); |
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.
Might be good here to document how this can fail. If you enable a HW breakpoint after you've used up all the resources with other breakpoints is the only case I can think of, but that's not entirely obvious. That's the only way this can fail that I can think of. If there are other ways this could fail, we should return an SBError to distinguish between them.
It would be good to test the response to enabling a HW breakpoint or trying to set it to HW when you've exhausted the number of HW breakpoint resources. For some reason we have SBProcess::GetNumSupportedHardwareWatchpoints but not SBProcess::GetNumSupportedHardwareBreakpoints. But you could just keep setting HW breakpoints till you get an error. |
There's one more tricky bit we need to handle here. What happens if I have a breakpoint that has more locations than we have hardware resources, and you do Maybe we need an actual error and not a bool to explain what went on there? |
e8e6afe
to
baad1c3
Compare
Please take another look. I improved the error handling and updated the test to test both the case where it fails because HW breakpoint are not available and when it succeeds and they are. |
Just noting that the values returned from these (partially imaginary) APIs would be hard to use correctly. The two resources might be inter-dependent. E.g. intel can have four hardware breakpoints OR four hardware watchpoints. However, it cannot have four HW breakpoints AND four HW watchpoints. And it might not be able to set even all four as the (linux) kernel might be using some of the registers for its own purposes (something to do with perf, I believe). |
} | ||
|
||
if (num_failures != 0) | ||
return llvm::createStringError( |
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.
The locations we didn't convert to hardware will be left disabled. That's a useful but not obvious bit of information, which it would be good to include in the error string.
Other than that this looks okay to me.
This adds SBBreakpoint::SetIsHardware, allowing clients to mark an existing breakpoint as a hardware breakpoint purely through the API. This is safe to do after creation, as the hardware/software distinction doesn't affect how breakpoint locations are selected. In some cases (e.g. when writing a trap instruction would alter program behavior), it's important to use hardware breakpoints. Ideally, we’d extend the various Create methods to support this, but given their number, this patch limits the scope to the post-creation API. As a workaround, users can also rely on target.require-hardware-breakpoint or use the breakpoint set command. rdar://153528045
baad1c3
to
6b5d907
Compare
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.
LGTM.
This RP changes some Breakpoint-related interfaces to return errors. On its own these improvements are small, but they encourage better error handling going forward. There are a bunch of other candidates, but these were the functions that I touched while working on llvm#146602.
This adds SBBreakpoint::SetIsHardware, allowing clients to mark an existing breakpoint as a hardware breakpoint purely through the API. This is safe to do after creation, as the hardware/software distinction doesn't affect how breakpoint locations are selected.
In some cases (e.g. when writing a trap instruction would alter program behavior), it's important to use hardware breakpoints. Ideally, we’d extend the various
Create
methods to support this, but given their number, this patch limits the scope to the post-creation API. As a workaround, users can also rely on target.require-hardware-breakpoint or use thebreakpoint set
command.rdar://153528045