[Lldb-commits] [lldb] Remove hardware index from watchpoints and breakpoints (PR #72012)
via lldb-commits
lldb-commits at lists.llvm.org
Fri Nov 10 18:51:49 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Jason Molenda (jasonmolenda)
<details>
<summary>Changes</summary>
The Watchpoint and Breakpoint objects try to track the hardware index that was used for them, if they are hardware wp/bp's. The majority of our debugging goes over the gdb remote serial protocol, and when we set the watchpoint/breakpoint, there is no (standard) way for the remote stub to communicate to lldb which hardware index was used. We have an lldb-extension packet to query the total number of watchpoint registers.
When a watchpoint is hit, there is an lldb extension to the stop reply packet (documented in lldb-gdb-remote.txt) to describe the watchpoint including its actual hardware index,
<addr within wp range> <wp hw index> <actual accessed address>
(the third field is specifically needed for MIPS). At this point, if the stub reported these three fields (the stub is only required to provide the first), we can know the actual hardware index for this watchpoint.
Breakpoints are worse; there's never any way for us to be notified about which hardware index was used. Breakpoints got this as a side effect of inherting from StoppointSite with Watchpoints.
We expose the watchpoint hardware index through "watchpoint list -v" and through SBWatchpoint::GetHardwareIndex.
With my large watchpoint support, there is no *single* hardware index that may be used for a watchpoint, it may need multiple resources. Also I don't see what a user is supposed to do with this information, or an IDE. Knowing the total number of watchpoint registers on the target, and knowing how many Watchpoint Resources are currently in use, is helpful. Knowing how many Watchpoint Resources
a single user-specified watchpoint needed to be implemented is useful. But knowing which registers were used is an implementation detail and not available until we hit the watchpoint when using gdb remote serial protocol.
So given all that, I'm removing watchpoint hardware index numbers. I'm changing the SB API to always return -1.
---
Full diff: https://github.com/llvm/llvm-project/pull/72012.diff
16 Files Affected:
- (modified) lldb/bindings/interface/SBTargetDocstrings.i (+1-1)
- (modified) lldb/bindings/interface/SBWatchpointDocstrings.i (+2-1)
- (modified) lldb/docs/use/tutorial.rst (+1-1)
- (modified) lldb/include/lldb/API/SBWatchpoint.h (+1-1)
- (modified) lldb/include/lldb/Breakpoint/StoppointSite.h (-7)
- (modified) lldb/source/API/SBWatchpoint.cpp (+5-10)
- (modified) lldb/source/Breakpoint/BreakpointLocation.cpp (+2-5)
- (modified) lldb/source/Breakpoint/BreakpointSite.cpp (+2-2)
- (modified) lldb/source/Breakpoint/StoppointSite.cpp (+5-6)
- (modified) lldb/source/Breakpoint/Watchpoint.cpp (+3-5)
- (modified) lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp (-18)
- (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp (-4)
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+6-7)
- (modified) lldb/source/Target/StopInfo.cpp (-3)
- (modified) lldb/test/API/python_api/default-constructor/sb_watchpoint.py (-1)
- (modified) lldb/test/API/python_api/watchpoint/TestWatchpointIter.py (-8)
``````````diff
diff --git a/lldb/bindings/interface/SBTargetDocstrings.i b/lldb/bindings/interface/SBTargetDocstrings.i
index f586552c99108c0..ce4992aade3a694 100644
--- a/lldb/bindings/interface/SBTargetDocstrings.i
+++ b/lldb/bindings/interface/SBTargetDocstrings.i
@@ -34,7 +34,7 @@ produces: ::
Watchpoint 1: addr = 0x1034ca048 size = 4 state = enabled type = rw
declare @ '/Volumes/data/lldb/svn/trunk/test/python_api/watchpoint/main.c:12'
- hw_index = 0 hit_count = 2 ignore_count = 0"
+ hit_count = 2 ignore_count = 0"
) lldb::SBTarget;
%feature("docstring", "
diff --git a/lldb/bindings/interface/SBWatchpointDocstrings.i b/lldb/bindings/interface/SBWatchpointDocstrings.i
index 4eb7b136282dc30..892a82e6d0519f2 100644
--- a/lldb/bindings/interface/SBWatchpointDocstrings.i
+++ b/lldb/bindings/interface/SBWatchpointDocstrings.i
@@ -9,7 +9,8 @@ watchpoints of the target."
) lldb::SBWatchpoint;
%feature("docstring", "
- With -1 representing an invalid hardware index."
+ Deprecated. Previously: Return the hardware index of the
+ watchpoint register. Now: -1 is always returned."
) lldb::SBWatchpoint::GetHardwareIndex;
%feature("docstring", "
diff --git a/lldb/docs/use/tutorial.rst b/lldb/docs/use/tutorial.rst
index 85dc173fa37cd33..ff956d750c29f23 100644
--- a/lldb/docs/use/tutorial.rst
+++ b/lldb/docs/use/tutorial.rst
@@ -431,7 +431,7 @@ a variable called 'global' for write operation, but only stop if the condition
Watchpoint 1: addr = 0x100001018 size = 4 state = enabled type = w
declare @ '/Volumes/data/lldb/svn/ToT/test/functionalities/watchpoint/watchpoint_commands/condition/main.cpp:12'
condition = '(global==5)'
- hw_index = 0 hit_count = 5 ignore_count = 0
+ hit_count = 5 ignore_count = 0
(lldb)
Starting or Attaching to Your Program
diff --git a/lldb/include/lldb/API/SBWatchpoint.h b/lldb/include/lldb/API/SBWatchpoint.h
index dc613a840beb299..131a963498ca979 100644
--- a/lldb/include/lldb/API/SBWatchpoint.h
+++ b/lldb/include/lldb/API/SBWatchpoint.h
@@ -45,7 +45,7 @@ class LLDB_API SBWatchpoint {
watch_id_t GetID();
- /// With -1 representing an invalid hardware index.
+ LLDB_DEPRECATED("Hardware index is not available, always returns -1")
int32_t GetHardwareIndex();
lldb::addr_t GetWatchAddress();
diff --git a/lldb/include/lldb/Breakpoint/StoppointSite.h b/lldb/include/lldb/Breakpoint/StoppointSite.h
index 7e5e33486345da4..bef19f37908c600 100644
--- a/lldb/include/lldb/Breakpoint/StoppointSite.h
+++ b/lldb/include/lldb/Breakpoint/StoppointSite.h
@@ -38,10 +38,6 @@ class StoppointSite {
virtual bool IsHardware() const = 0;
- uint32_t GetHardwareIndex() const { return m_hardware_index; }
-
- void SetHardwareIndex(uint32_t index) { m_hardware_index = index; }
-
virtual bool ShouldStop(StoppointCallbackContext* context) = 0;
virtual void Dump(Stream* stream) const = 0;
@@ -59,9 +55,6 @@ class StoppointSite {
/// the lack of resources).
bool m_is_hardware_required;
- /// The hardware resource index for this breakpoint/watchpoint.
- uint32_t m_hardware_index;
-
/// The size in bytes of stoppoint, e.g. the length of the trap opcode for
/// software breakpoints, or the optional length in bytes for hardware
/// breakpoints, or the length of the watchpoint.
diff --git a/lldb/source/API/SBWatchpoint.cpp b/lldb/source/API/SBWatchpoint.cpp
index 8b4e0ad3178b182..cf88d15a04f77a2 100644
--- a/lldb/source/API/SBWatchpoint.cpp
+++ b/lldb/source/API/SBWatchpoint.cpp
@@ -94,16 +94,11 @@ SBError SBWatchpoint::GetError() {
int32_t SBWatchpoint::GetHardwareIndex() {
LLDB_INSTRUMENT_VA(this);
- int32_t hw_index = -1;
-
- lldb::WatchpointSP watchpoint_sp(GetSP());
- if (watchpoint_sp) {
- std::lock_guard<std::recursive_mutex> guard(
- watchpoint_sp->GetTarget().GetAPIMutex());
- hw_index = watchpoint_sp->GetHardwareIndex();
- }
-
- return hw_index;
+ // For processes using gdb remote protocol,
+ // we cannot determine the hardware breakpoint
+ // index reliably; providing possibly correct
+ // guesses is not useful to anyone.
+ return UINT32_MAX;
}
addr_t SBWatchpoint::GetWatchAddress() {
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index 0fcefe5c63be749..27dc7458dc26f70 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -626,22 +626,19 @@ void BreakpointLocation::Dump(Stream *s) const {
bool is_resolved = IsResolved();
bool is_hardware = is_resolved && m_bp_site_sp->IsHardware();
- auto hardware_index = is_resolved ?
- m_bp_site_sp->GetHardwareIndex() : LLDB_INVALID_INDEX32;
lldb::tid_t tid = GetOptionsSpecifyingKind(BreakpointOptions::eThreadSpec)
.GetThreadSpecNoCreate()
->GetTID();
s->Printf("BreakpointLocation %u: tid = %4.4" PRIx64
" load addr = 0x%8.8" PRIx64 " state = %s type = %s breakpoint "
- "hw_index = %i hit_count = %-4u ignore_count = %-4u",
+ "hit_count = %-4u ignore_count = %-4u",
GetID(), tid,
(uint64_t)m_address.GetOpcodeLoadAddress(&m_owner.GetTarget()),
(m_options_up ? m_options_up->IsEnabled() : m_owner.IsEnabled())
? "enabled "
: "disabled",
- is_hardware ? "hardware" : "software", hardware_index,
- GetHitCount(),
+ is_hardware ? "hardware" : "software", GetHitCount(),
GetOptionsSpecifyingKind(BreakpointOptions::eIgnoreCount)
.GetIgnoreCount());
}
diff --git a/lldb/source/Breakpoint/BreakpointSite.cpp b/lldb/source/Breakpoint/BreakpointSite.cpp
index 72e1a0958907c10..5187bc560ba26ca 100644
--- a/lldb/source/Breakpoint/BreakpointSite.cpp
+++ b/lldb/source/Breakpoint/BreakpointSite.cpp
@@ -76,9 +76,9 @@ void BreakpointSite::Dump(Stream *s) const {
return;
s->Printf("BreakpointSite %u: addr = 0x%8.8" PRIx64
- " type = %s breakpoint hw_index = %i hit_count = %-4u",
+ " type = %s breakpoint hit_count = %-4u",
GetID(), (uint64_t)m_addr, IsHardware() ? "hardware" : "software",
- GetHardwareIndex(), GetHitCount());
+ GetHitCount());
}
void BreakpointSite::GetDescription(Stream *s, lldb::DescriptionLevel level) {
diff --git a/lldb/source/Breakpoint/StoppointSite.cpp b/lldb/source/Breakpoint/StoppointSite.cpp
index ba8c48326bdb65c..a28b6b927a1fa74 100644
--- a/lldb/source/Breakpoint/StoppointSite.cpp
+++ b/lldb/source/Breakpoint/StoppointSite.cpp
@@ -13,11 +13,10 @@ using namespace lldb;
using namespace lldb_private;
StoppointSite::StoppointSite(break_id_t id, addr_t addr, bool hardware)
- : m_id(id), m_addr(addr), m_is_hardware_required(hardware),
- m_hardware_index(LLDB_INVALID_INDEX32), m_byte_size(0), m_hit_counter() {}
+ : m_id(id), m_addr(addr), m_is_hardware_required(hardware), m_byte_size(0),
+ m_hit_counter() {}
-StoppointSite::StoppointSite(break_id_t id, addr_t addr,
- uint32_t byte_size, bool hardware)
+StoppointSite::StoppointSite(break_id_t id, addr_t addr, uint32_t byte_size,
+ bool hardware)
: m_id(id), m_addr(addr), m_is_hardware_required(hardware),
- m_hardware_index(LLDB_INVALID_INDEX32), m_byte_size(byte_size),
- m_hit_counter() {}
+ m_byte_size(byte_size), m_hit_counter() {}
diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp
index 14144214faea747..b58455f73030c9b 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -324,8 +324,8 @@ void Watchpoint::DumpWithLevel(Stream *s,
}
if (description_level >= lldb::eDescriptionLevelVerbose) {
- s->Printf("\n hw_index = %i hit_count = %-4u ignore_count = %-4u",
- GetHardwareIndex(), GetHitCount(), GetIgnoreCount());
+ s->Printf("\n hit_count = %-4u ignore_count = %-4u", GetHitCount(),
+ GetIgnoreCount());
}
}
@@ -350,9 +350,7 @@ bool Watchpoint::IsDisabledDuringEphemeralMode() {
void Watchpoint::SetEnabled(bool enabled, bool notify) {
if (!enabled) {
- if (!m_is_ephemeral)
- SetHardwareIndex(LLDB_INVALID_INDEX32);
- else
+ if (m_is_ephemeral)
++m_disabled_count;
// Don't clear the snapshots for now.
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index d60e6250c7c0aca..5c947bbc8d9cb95 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -495,10 +495,6 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target,
lldb::WatchpointSP wp_sp =
target->GetWatchpointList().FindByAddress((lldb::addr_t)exc_sub_code);
if (wp_sp && wp_sp->IsEnabled()) {
- // Debugserver may piggyback the hardware index of the fired watchpoint
- // in the exception data. Set the hardware index if that's the case.
- if (exc_data_count >= 3)
- wp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
return StopInfo::CreateStopReasonWithWatchpointID(thread, wp_sp->GetID());
}
}
@@ -511,10 +507,6 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread &thread, Target *target,
process_sp->GetBreakpointSiteList().FindByAddress(
(lldb::addr_t)exc_sub_code);
if (bp_sp && bp_sp->IsEnabled()) {
- // Debugserver may piggyback the hardware index of the fired breakpoint
- // in the exception data. Set the hardware index if that's the case.
- if (exc_data_count >= 3)
- bp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
return StopInfo::CreateStopReasonWithBreakpointSiteID(thread,
bp_sp->GetID());
}
@@ -681,11 +673,6 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
wp_sp = target->GetWatchpointList().FindByAddress(
(lldb::addr_t)exc_sub_code);
if (wp_sp && wp_sp->IsEnabled()) {
- // Debugserver may piggyback the hardware index of the fired
- // watchpoint in the exception data. Set the hardware index if
- // that's the case.
- if (exc_data_count >= 3)
- wp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
return StopInfo::CreateStopReasonWithWatchpointID(thread,
wp_sp->GetID());
} else {
@@ -762,11 +749,6 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
wp_sp = target->GetWatchpointList().FindByAddress(
(lldb::addr_t)exc_sub_code);
if (wp_sp && wp_sp->IsEnabled()) {
- // Debugserver may piggyback the hardware index of the fired
- // watchpoint in the exception data. Set the hardware index if
- // that's the case.
- if (exc_data_count >= 3)
- wp_sp->SetHardwareIndex((uint32_t)exc_sub_sub_code);
return StopInfo::CreateStopReasonWithWatchpointID(thread,
wp_sp->GetID());
}
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
index 6befec9f8654b9a..91b3216a57c34c1 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -405,10 +405,6 @@ void ProcessWindows::RefreshStateAfterStop() {
"{1:x} with watchpoint {2}",
m_session_data->m_debugger->GetProcess().GetProcessId(), pc, id);
- if (lldb::WatchpointSP wp_sp =
- GetTarget().GetWatchpointList().FindByID(id))
- wp_sp->SetHardwareIndex(slot_id);
-
stop_info = StopInfo::CreateStopReasonWithWatchpointID(
*stop_thread, id, m_watchpoints[id].address);
stop_thread->SetStopInfo(stop_info);
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index f90a561aae2e3b7..e653ef5d8ac54e4 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1789,8 +1789,12 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
// disable/step/re-enable it, so one of the valid watchpoint
// addresses should be provided as \a wp_addr.
StringExtractor desc_extractor(description.c_str());
+ // FIXME NativeThreadLinux::SetStoppedByWatchpoint sends this
+ // up as
+ // <address within wp range> <wp hw index> <actual accessed addr>
+ // but this is not reading the <wp hw index>. Seems like it
+ // wouldn't work on MIPS, where that third field is important.
addr_t wp_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
- uint32_t wp_index = desc_extractor.GetU32(LLDB_INVALID_INDEX32);
addr_t wp_hit_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
watch_id_t watch_id = LLDB_INVALID_WATCH_ID;
bool silently_continue = false;
@@ -1807,7 +1811,6 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
if (!wp_sp && wp_addr != LLDB_INVALID_ADDRESS)
wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_addr);
if (wp_sp) {
- wp_sp->SetHardwareIndex(wp_index);
watch_id = wp_sp->GetID();
}
if (watch_id == LLDB_INVALID_WATCH_ID) {
@@ -2238,17 +2241,13 @@ StateType ProcessGDBRemote::SetThreadStopInfo(StringExtractor &stop_packet) {
WatchpointSP wp_sp =
GetTarget().GetWatchpointList().FindByAddress(wp_addr);
- uint32_t wp_index = LLDB_INVALID_INDEX32;
-
- if (wp_sp)
- wp_index = wp_sp->GetHardwareIndex();
// Rewrite gdb standard watch/rwatch/awatch to
// "reason:watchpoint" + "description:ADDR",
// which is parsed in SetThreadStopInfo.
reason = "watchpoint";
StreamString ostr;
- ostr.Printf("%" PRIu64 " %" PRIu32, wp_addr, wp_index);
+ ostr.Printf("%" PRIu64, wp_addr);
description = std::string(ostr.GetString());
} else if (key.compare("library") == 0) {
auto error = LoadModules();
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index a189f4be1926a6f..0b858454782bc11 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -699,7 +699,6 @@ class StopInfoWatchpoint : public StopInfo {
eVoteNoOpinion),
m_stop_info_sp(stop_info_sp), m_watch_sp(watch_sp) {
assert(watch_sp);
- m_watch_index = watch_sp->GetHardwareIndex();
}
bool DoWillResume(lldb::StateType resume_state,
@@ -753,13 +752,11 @@ class StopInfoWatchpoint : public StopInfo {
return;
m_did_disable_wp = true;
GetThread().GetProcess()->EnableWatchpoint(m_watch_sp.get(), true);
- m_watch_sp->SetHardwareIndex(m_watch_index);
}
private:
StopInfoWatchpointSP m_stop_info_sp;
WatchpointSP m_watch_sp;
- uint32_t m_watch_index = LLDB_INVALID_INDEX32;
bool m_did_disable_wp = false;
};
diff --git a/lldb/test/API/python_api/default-constructor/sb_watchpoint.py b/lldb/test/API/python_api/default-constructor/sb_watchpoint.py
index 8aa38126ad4d61f..a43e946352f4d1e 100644
--- a/lldb/test/API/python_api/default-constructor/sb_watchpoint.py
+++ b/lldb/test/API/python_api/default-constructor/sb_watchpoint.py
@@ -8,7 +8,6 @@
def fuzz_obj(obj):
obj.GetID()
obj.IsValid()
- obj.GetHardwareIndex()
obj.GetWatchAddress()
obj.GetWatchSize()
obj.SetEnabled(True)
diff --git a/lldb/test/API/python_api/watchpoint/TestWatchpointIter.py b/lldb/test/API/python_api/watchpoint/TestWatchpointIter.py
index 25faaf16af4a7ac..5702b9934f329de 100644
--- a/lldb/test/API/python_api/watchpoint/TestWatchpointIter.py
+++ b/lldb/test/API/python_api/watchpoint/TestWatchpointIter.py
@@ -83,20 +83,12 @@ def test_watch_iter(self):
self.assertTrue(thread, "The thread stopped due to watchpoint")
self.DebugSBValue(value)
- # We currently only support hardware watchpoint. Verify that we have a
- # meaningful hardware index at this point. Exercise the printed repr of
- # SBWatchpointLocation.
- print(watchpoint)
- if not self.affected_by_radar_34564183():
- self.assertTrue(watchpoint.GetHardwareIndex() != -1)
-
# SBWatchpoint.GetDescription() takes a description level arg.
print(lldbutil.get_description(watchpoint, lldb.eDescriptionLevelFull))
# Now disable the 'rw' watchpoint. The program won't stop when it reads
# 'global' next.
watchpoint.SetEnabled(False)
- self.assertEqual(watchpoint.GetHardwareIndex(), -1)
self.assertFalse(watchpoint.IsEnabled())
# Continue. The program does not stop again when the variable is being
``````````
</details>
https://github.com/llvm/llvm-project/pull/72012
More information about the lldb-commits
mailing list