[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
DavidSpickett wrote: I've reverted this due to failures on Linaro's Linux bots. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
dyung wrote: > Seems that it caused #73580 no? I believe so. @jasonmolenda I see you attempted to make a fix in a0a1ff3ab40e347589b4e27d8fd350c600526735, however I still have two buildbots that are failing to build. Can you either fix it or revert if you need time to investigate? https://lab.llvm.org/buildbot/#/builders/217/builds/31915 https://lab.llvm.org/buildbot/#/builders/243/builds/16118 https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
sylvestre wrote: Seems that it caused https://github.com/llvm/llvm-project/issues/73580 no? https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/jasonmolenda closed https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/JDevlieghere approved this pull request. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,37 @@
+//===-- StopPointSiteList.cpp
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/StopPointSiteList.h"
+#include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+
+#include "lldb/Utility/Stream.h"
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+// This method is only defined when we're specializing for
+// BreakpointSite / BreakpointLocation / Breakpoint.
+// Watchpoints don't have a similar structure, they are
+// WatchpointResource / Watchpoint
+
+template <>
+bool StopPointSiteList::StopPointSiteContainsBreakpoint(
+typename BreakpointSite::SiteID site_id, lldb::break_id_t bp_id) {
+ std::lock_guard guard(m_mutex);
+ typename collection::const_iterator pos = GetIDConstIterator(site_id);
JDevlieghere wrote:
This would be a good candidate for `auto`.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/bulbazord approved this pull request. LGTM as well. I like the name Constituents quite a bit as well! I think most feedback at this point can probably be handled in follow-ups, esp. since some of it pertains to existing interfaces. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
jasonmolenda wrote: > LGTM with a few nits. I like "constituents". > > This is a pretty big patch which makes reviewing it challenging. I know it's > a big change that touches a lot of things but I'm sure that this could've > been broken up into smaller patches if you keep that goal in mind from the > beginning. Something to look out for in the future. Thanks for the feedback. Yeah originally this patch was a bit smaller but it has Grown as I've addressed (correct, good) feedback from everyone and now it's a little bit of a monster. I'm surely going to have to rebase it before I can merge. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -1787,30 +1788,48 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
// addresses should be provided as \a wp_addr.
StringExtractor desc_extractor(description.c_str());
addr_t wp_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
- uint32_t wp_index = desc_extractor.GetU32(LLDB_INVALID_INDEX32);
+ wp_resource_id_t wp_hw_index =
+ desc_extractor.GetU32(LLDB_INVALID_WATCHPOINT_RESOURCE_ID);
addr_t wp_hit_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
watch_id_t watch_id = LLDB_INVALID_WATCH_ID;
bool silently_continue = false;
- WatchpointSP wp_sp;
+ WatchpointResourceSP wp_resource_sp;
+ if (wp_hw_index != LLDB_INVALID_WATCHPOINT_RESOURCE_ID) {
+wp_resource_sp = m_watchpoint_resource_list.FindByID(wp_hw_index);
+if (wp_resource_sp) {
+ // If we were given an access address, and the Resource we
+ // found by watchpoint register index does not contain that
+ // address, then the wp_resource_id's have not tracked the
+ // hardware watchpoint registers correctly, discard this
+ // Resource found by ID and look it up by access address.
+ if (wp_hit_addr != LLDB_INVALID_ADDRESS &&
+ !wp_resource_sp->Contains(wp_hit_addr)) {
+wp_resource_sp.reset();
+ }
+}
+ }
if (wp_hit_addr != LLDB_INVALID_ADDRESS) {
-wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_hit_addr);
+wp_resource_sp =
+m_watchpoint_resource_list.FindByAddress(wp_hit_addr);
// On MIPS, \a wp_hit_addr outside the range of a watched
// region means we should silently continue, it is a false hit.
ArchSpec::Core core = GetTarget().GetArchitecture().GetCore();
-if (!wp_sp && core >= ArchSpec::kCore_mips_first &&
+if (!wp_resource_sp && core >= ArchSpec::kCore_mips_first &&
core <= ArchSpec::kCore_mips_last)
silently_continue = true;
}
- 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) {
+ if (!wp_resource_sp && wp_addr != LLDB_INVALID_ADDRESS)
+wp_resource_sp = m_watchpoint_resource_list.FindByAddress(wp_addr);
+ if (!wp_resource_sp) {
Log *log(GetLog(GDBRLog::Watchpoints));
LLDB_LOGF(log, "failed to find watchpoint");
+abort(); // LWP_TODO FIXME don't continue executing this block if
jasonmolenda wrote:
My refactored code added a new nullptr deref, when I saw what I'd done I threw
in an abort and a FIXME comment to fix that. But I see how to fix it, let me
do that.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,235 @@
+//===-- StopPointSiteList.cpp
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/StopPointSiteList.h"
+#include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+
+#include "lldb/Utility/Stream.h"
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+// Add site to the list. However, if the element already exists in
+// the list, then we don't add it, and return InvalidSiteID.
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::Add(const StopPointSiteSP ) {
+ lldb::addr_t site_load_addr = site->GetLoadAddress();
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(site_load_addr);
+
+ if (iter == m_site_list.end()) {
+#if 0
+m_site_list.insert(iter, collection::value_type(site_load_addr, site));
+#endif
+m_site_list[site_load_addr] = site;
+return site->GetID();
+ } else {
+return UINT32_MAX;
+ }
+}
+
+template
+bool StopPointSiteList::ShouldStop(
+StoppointCallbackContext *context, typename StopPointSite::SiteID site_id)
{
+ StopPointSiteSP site_sp(FindByID(site_id));
+ if (site_sp) {
+// Let the site decide if it should stop here (could not have
+// reached it's target hit count yet, or it could have a callback that
+// decided it shouldn't stop (shared library loads/unloads).
+return site_sp->ShouldStop(context);
+ }
+ // We should stop here since this site isn't valid anymore or it
+ // doesn't exist.
+ return true;
+}
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::FindIDByAddress(lldb::addr_t addr) {
+ if (StopPointSiteSP site = FindByAddress(addr))
+return site->GetID();
+ return UINT32_MAX;
+}
+
+template
+bool StopPointSiteList::Remove(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator pos = GetIDIterator(site_id); // Predicate
+ if (pos != m_site_list.end()) {
+m_site_list.erase(pos);
+return true;
+ }
+ return false;
+}
+
+template
+bool StopPointSiteList::RemoveByAddress(lldb::addr_t address) {
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator pos = m_site_list.find(address);
+ if (pos != m_site_list.end()) {
+m_site_list.erase(pos);
+return true;
+ }
+ return false;
+}
+
+template
+typename StopPointSiteList::collection::iterator
+StopPointSiteList::GetIDIterator(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ auto id_matches = [site_id](const std::pair s) {
+return site_id == s.second->GetID();
+ };
+ return std::find_if(m_site_list.begin(),
+ m_site_list.end(), // Search full range
+ id_matches);
+}
+
+template
+typename StopPointSiteList::collection::const_iterator
+StopPointSiteList::GetIDConstIterator(
+typename StopPointSite::SiteID site_id) const {
+ std::lock_guard guard(m_mutex);
+ auto id_matches = [site_id](const std::pair s) {
+return site_id == s.second->GetID();
+ };
+ return std::find_if(m_site_list.begin(),
+ m_site_list.end(), // Search full range
+ id_matches);
+}
+
+template
+typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByID(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ StopPointSiteSP stop_sp;
+ typename collection::iterator pos = GetIDIterator(site_id);
+ if (pos != m_site_list.end())
+stop_sp = pos->second;
+
+ return stop_sp;
+}
+
+template
+const typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByID(
+typename StopPointSite::SiteID site_id) const {
+ std::lock_guard guard(m_mutex);
+ StopPointSiteSP stop_sp;
+ typename collection::const_iterator pos = GetIDConstIterator(site_id);
+ if (pos != m_site_list.end())
+stop_sp = pos->second;
+
+ return stop_sp;
+}
+
+template
+typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByAddress(lldb::addr_t addr) {
+ StopPointSiteSP found_sp;
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(addr);
+ if (iter != m_site_list.end())
+found_sp = iter->second;
+ return found_sp;
+}
+
+// This method is only defined when we're specializing for
+// BreakpointSite / BreakpointLocation / Breakpoint.
+// Watchpoints don't have a similar structure, they are
+// WatchpointResource / Watchpoint
+template <>
+bool StopPointSiteList::StopPointSiteContainsBreakpoint(
+typename BreakpointSite::SiteID site_id, break_id_t bp_id) {
+ std::lock_guard guard(m_mutex);
+ typename collection::const_iterator pos =
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,235 @@
+//===-- StopPointSiteList.cpp
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/StopPointSiteList.h"
+#include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+
+#include "lldb/Utility/Stream.h"
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+// Add site to the list. However, if the element already exists in
+// the list, then we don't add it, and return InvalidSiteID.
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::Add(const StopPointSiteSP ) {
+ lldb::addr_t site_load_addr = site->GetLoadAddress();
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(site_load_addr);
+
+ if (iter == m_site_list.end()) {
+#if 0
+m_site_list.insert(iter, collection::value_type(site_load_addr, site));
+#endif
+m_site_list[site_load_addr] = site;
+return site->GetID();
+ } else {
+return UINT32_MAX;
+ }
+}
+
+template
+bool StopPointSiteList::ShouldStop(
+StoppointCallbackContext *context, typename StopPointSite::SiteID site_id)
{
+ StopPointSiteSP site_sp(FindByID(site_id));
+ if (site_sp) {
+// Let the site decide if it should stop here (could not have
+// reached it's target hit count yet, or it could have a callback that
+// decided it shouldn't stop (shared library loads/unloads).
+return site_sp->ShouldStop(context);
+ }
+ // We should stop here since this site isn't valid anymore or it
+ // doesn't exist.
+ return true;
+}
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::FindIDByAddress(lldb::addr_t addr) {
+ if (StopPointSiteSP site = FindByAddress(addr))
+return site->GetID();
+ return UINT32_MAX;
+}
+
+template
+bool StopPointSiteList::Remove(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator pos = GetIDIterator(site_id); // Predicate
+ if (pos != m_site_list.end()) {
+m_site_list.erase(pos);
+return true;
+ }
+ return false;
+}
+
+template
+bool StopPointSiteList::RemoveByAddress(lldb::addr_t address) {
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator pos = m_site_list.find(address);
+ if (pos != m_site_list.end()) {
+m_site_list.erase(pos);
+return true;
+ }
+ return false;
+}
+
+template
+typename StopPointSiteList::collection::iterator
+StopPointSiteList::GetIDIterator(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ auto id_matches = [site_id](const std::pair s) {
+return site_id == s.second->GetID();
+ };
+ return std::find_if(m_site_list.begin(),
+ m_site_list.end(), // Search full range
+ id_matches);
+}
+
+template
+typename StopPointSiteList::collection::const_iterator
+StopPointSiteList::GetIDConstIterator(
+typename StopPointSite::SiteID site_id) const {
+ std::lock_guard guard(m_mutex);
+ auto id_matches = [site_id](const std::pair s) {
+return site_id == s.second->GetID();
+ };
+ return std::find_if(m_site_list.begin(),
+ m_site_list.end(), // Search full range
+ id_matches);
+}
+
+template
+typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByID(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ StopPointSiteSP stop_sp;
+ typename collection::iterator pos = GetIDIterator(site_id);
+ if (pos != m_site_list.end())
+stop_sp = pos->second;
+
+ return stop_sp;
+}
+
+template
+const typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByID(
+typename StopPointSite::SiteID site_id) const {
+ std::lock_guard guard(m_mutex);
+ StopPointSiteSP stop_sp;
+ typename collection::const_iterator pos = GetIDConstIterator(site_id);
+ if (pos != m_site_list.end())
+stop_sp = pos->second;
+
+ return stop_sp;
+}
+
+template
+typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByAddress(lldb::addr_t addr) {
+ StopPointSiteSP found_sp;
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(addr);
+ if (iter != m_site_list.end())
+found_sp = iter->second;
+ return found_sp;
+}
+
+// This method is only defined when we're specializing for
+// BreakpointSite / BreakpointLocation / Breakpoint.
+// Watchpoints don't have a similar structure, they are
+// WatchpointResource / Watchpoint
+template <>
+bool StopPointSiteList::StopPointSiteContainsBreakpoint(
+typename BreakpointSite::SiteID site_id, break_id_t bp_id) {
+ std::lock_guard guard(m_mutex);
+ typename collection::const_iterator pos =
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,235 @@
+//===-- StopPointSiteList.cpp
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/StopPointSiteList.h"
+#include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+
+#include "lldb/Utility/Stream.h"
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+// Add site to the list. However, if the element already exists in
+// the list, then we don't add it, and return InvalidSiteID.
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::Add(const StopPointSiteSP ) {
+ lldb::addr_t site_load_addr = site->GetLoadAddress();
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(site_load_addr);
+
+ if (iter == m_site_list.end()) {
+#if 0
+m_site_list.insert(iter, collection::value_type(site_load_addr, site));
+#endif
JDevlieghere wrote:
Remove.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -1787,30 +1788,48 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
// addresses should be provided as \a wp_addr.
StringExtractor desc_extractor(description.c_str());
addr_t wp_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
- uint32_t wp_index = desc_extractor.GetU32(LLDB_INVALID_INDEX32);
+ wp_resource_id_t wp_hw_index =
+ desc_extractor.GetU32(LLDB_INVALID_WATCHPOINT_RESOURCE_ID);
addr_t wp_hit_addr = desc_extractor.GetU64(LLDB_INVALID_ADDRESS);
watch_id_t watch_id = LLDB_INVALID_WATCH_ID;
bool silently_continue = false;
- WatchpointSP wp_sp;
+ WatchpointResourceSP wp_resource_sp;
+ if (wp_hw_index != LLDB_INVALID_WATCHPOINT_RESOURCE_ID) {
+wp_resource_sp = m_watchpoint_resource_list.FindByID(wp_hw_index);
+if (wp_resource_sp) {
+ // If we were given an access address, and the Resource we
+ // found by watchpoint register index does not contain that
+ // address, then the wp_resource_id's have not tracked the
+ // hardware watchpoint registers correctly, discard this
+ // Resource found by ID and look it up by access address.
+ if (wp_hit_addr != LLDB_INVALID_ADDRESS &&
+ !wp_resource_sp->Contains(wp_hit_addr)) {
+wp_resource_sp.reset();
+ }
+}
+ }
if (wp_hit_addr != LLDB_INVALID_ADDRESS) {
-wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_hit_addr);
+wp_resource_sp =
+m_watchpoint_resource_list.FindByAddress(wp_hit_addr);
// On MIPS, \a wp_hit_addr outside the range of a watched
// region means we should silently continue, it is a false hit.
ArchSpec::Core core = GetTarget().GetArchitecture().GetCore();
-if (!wp_sp && core >= ArchSpec::kCore_mips_first &&
+if (!wp_resource_sp && core >= ArchSpec::kCore_mips_first &&
core <= ArchSpec::kCore_mips_last)
silently_continue = true;
}
- 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) {
+ if (!wp_resource_sp && wp_addr != LLDB_INVALID_ADDRESS)
+wp_resource_sp = m_watchpoint_resource_list.FindByAddress(wp_addr);
+ if (!wp_resource_sp) {
Log *log(GetLog(GDBRLog::Watchpoints));
LLDB_LOGF(log, "failed to find watchpoint");
+abort(); // LWP_TODO FIXME don't continue executing this block if
JDevlieghere wrote:
??
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,235 @@
+//===-- StopPointSiteList.cpp
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/StopPointSiteList.h"
+#include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+
+#include "lldb/Utility/Stream.h"
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+// Add site to the list. However, if the element already exists in
+// the list, then we don't add it, and return InvalidSiteID.
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::Add(const StopPointSiteSP ) {
+ lldb::addr_t site_load_addr = site->GetLoadAddress();
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(site_load_addr);
+
+ if (iter == m_site_list.end()) {
+#if 0
+m_site_list.insert(iter, collection::value_type(site_load_addr, site));
+#endif
+m_site_list[site_load_addr] = site;
+return site->GetID();
+ } else {
+return UINT32_MAX;
+ }
+}
+
+template
+bool StopPointSiteList::ShouldStop(
+StoppointCallbackContext *context, typename StopPointSite::SiteID site_id)
{
+ StopPointSiteSP site_sp(FindByID(site_id));
+ if (site_sp) {
JDevlieghere wrote:
```if(StopPointSiteSP site_sp = FindByID(site_id))```
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,235 @@
+//===-- StopPointSiteList.cpp
-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/StopPointSiteList.h"
+#include "lldb/Breakpoint/BreakpointSite.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+
+#include "lldb/Utility/Stream.h"
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+// Add site to the list. However, if the element already exists in
+// the list, then we don't add it, and return InvalidSiteID.
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::Add(const StopPointSiteSP ) {
+ lldb::addr_t site_load_addr = site->GetLoadAddress();
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(site_load_addr);
+
+ if (iter == m_site_list.end()) {
+#if 0
+m_site_list.insert(iter, collection::value_type(site_load_addr, site));
+#endif
+m_site_list[site_load_addr] = site;
+return site->GetID();
+ } else {
+return UINT32_MAX;
+ }
+}
+
+template
+bool StopPointSiteList::ShouldStop(
+StoppointCallbackContext *context, typename StopPointSite::SiteID site_id)
{
+ StopPointSiteSP site_sp(FindByID(site_id));
+ if (site_sp) {
+// Let the site decide if it should stop here (could not have
+// reached it's target hit count yet, or it could have a callback that
+// decided it shouldn't stop (shared library loads/unloads).
+return site_sp->ShouldStop(context);
+ }
+ // We should stop here since this site isn't valid anymore or it
+ // doesn't exist.
+ return true;
+}
+
+template
+typename StopPointSite::SiteID
+StopPointSiteList::FindIDByAddress(lldb::addr_t addr) {
+ if (StopPointSiteSP site = FindByAddress(addr))
+return site->GetID();
+ return UINT32_MAX;
+}
+
+template
+bool StopPointSiteList::Remove(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator pos = GetIDIterator(site_id); // Predicate
+ if (pos != m_site_list.end()) {
+m_site_list.erase(pos);
+return true;
+ }
+ return false;
+}
+
+template
+bool StopPointSiteList::RemoveByAddress(lldb::addr_t address) {
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator pos = m_site_list.find(address);
+ if (pos != m_site_list.end()) {
+m_site_list.erase(pos);
+return true;
+ }
+ return false;
+}
+
+template
+typename StopPointSiteList::collection::iterator
+StopPointSiteList::GetIDIterator(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ auto id_matches = [site_id](const std::pair s) {
+return site_id == s.second->GetID();
+ };
+ return std::find_if(m_site_list.begin(),
+ m_site_list.end(), // Search full range
+ id_matches);
+}
+
+template
+typename StopPointSiteList::collection::const_iterator
+StopPointSiteList::GetIDConstIterator(
+typename StopPointSite::SiteID site_id) const {
+ std::lock_guard guard(m_mutex);
+ auto id_matches = [site_id](const std::pair s) {
+return site_id == s.second->GetID();
+ };
+ return std::find_if(m_site_list.begin(),
+ m_site_list.end(), // Search full range
+ id_matches);
+}
+
+template
+typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByID(
+typename StopPointSite::SiteID site_id) {
+ std::lock_guard guard(m_mutex);
+ StopPointSiteSP stop_sp;
+ typename collection::iterator pos = GetIDIterator(site_id);
+ if (pos != m_site_list.end())
+stop_sp = pos->second;
+
+ return stop_sp;
+}
+
+template
+const typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByID(
+typename StopPointSite::SiteID site_id) const {
+ std::lock_guard guard(m_mutex);
+ StopPointSiteSP stop_sp;
+ typename collection::const_iterator pos = GetIDConstIterator(site_id);
+ if (pos != m_site_list.end())
+stop_sp = pos->second;
+
+ return stop_sp;
+}
+
+template
+typename StopPointSiteList::StopPointSiteSP
+StopPointSiteList::FindByAddress(lldb::addr_t addr) {
+ StopPointSiteSP found_sp;
+ std::lock_guard guard(m_mutex);
+ typename collection::iterator iter = m_site_list.find(addr);
+ if (iter != m_site_list.end())
+found_sp = iter->second;
+ return found_sp;
+}
+
+// This method is only defined when we're specializing for
+// BreakpointSite / BreakpointLocation / Breakpoint.
+// Watchpoints don't have a similar structure, they are
+// WatchpointResource / Watchpoint
+template <>
+bool StopPointSiteList::StopPointSiteContainsBreakpoint(
+typename BreakpointSite::SiteID site_id, break_id_t bp_id) {
+ std::lock_guard guard(m_mutex);
+ typename collection::const_iterator pos =
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/JDevlieghere approved this pull request. LGTM with a few nits. I like "constituents". This is a pretty big patch which makes reviewing it challenging. I know it's a big change that touches a lot of things but I'm sure that this could've been broken up into smaller patches if you keep that goal in mind from the beginning. Something to look out for in the future. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/JDevlieghere edited https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,146 @@ +//===-- WatchpointResourceList.h *- C++ -*-===// jasonmolenda wrote: I think it's the same except for the obvious BreakpointSite/WatchpointResource and BreakpointLocation/Watchpoint. It's a little tricky reading through BreakpointSiteList.h to tell if all of the arguments refer to BreakpointLocations (they do it by id values) or if they might refer to Breakpoints - they seem to all use the same `break_id_t`? (and `break_id_t` is also used for the BreakpointSite numbers); sometimes the arguments are called `bp_id` and sometimes they're called `BreakID` but I THINK they're always referring to BreakpointLocations. It'd be nice if BreakpointSite and WatchpointResource could inherit from the same base class and have the StopPointList work in terms of the base class methods, but I think there's enough difference between these two that this might not work very cleanly. Maybe a templated StopPointList could work though. I'll need to look at that. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
jasonmolenda wrote:
The convention for these methods seems to be to return a stream without a final
end-of-line character. In StopInfo.cpp we call this method and check if it
printed anything, and if so, print an EOL. In Watchpoint::DumpWithLevel we
ignore the return type from DumpSnapshots, but that's only because every line
it adds is assumed to end without an EOL. I don't really need a variable in
this method, I could return false in the two places it is false and true in the
one place it is true.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,155 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ bool WatchpointResourceRead() const;
+
+ bool WatchpointResourceWrite() const;
+
+ void SetType(bool read, bool write);
+
+ typedef std::vector WatchpointCollection;
+ typedef LockingAdaptedIterable
+ WatchpointIterable;
+
+ /// Iterate over the watchpoint owners for this resource
+ ///
+ /// \return
+ /// An Iterable object which can be used to loop over the watchpoints
+ /// that are owners of this resource.
+ WatchpointIterable Owners() {
+return WatchpointIterable(m_owners, m_owners_mutex);
+ }
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
+
+ lldb::wp_resource_id_t GetID() const;
+
+ bool Contains(lldb::addr_t addr);
+
+protected:
+ // The StopInfoWatchpoint knows when it is processing a hit for a thread for
+ // a site, so let it be the one to manage setting the location hit count once
+ // and only once.
+ friend class StopInfoWatchpoint;
+
+ void BumpHitCounts();
+
+private:
+ lldb::wp_resource_id_t m_id;
+
+ // start address & size aligned & expanded to be a valid watchpoint
+ // memory granule on this target.
+ lldb::addr_t m_addr;
+ size_t m_size;
+
+ bool m_watch_read; // true if we stop when the watched data is read from
+ bool m_watch_write; // true if we stop when the watched data is written to
+
+ // The watchpoints using this WatchpointResource.
+ WatchpointCollection m_owners;
+
+ std::recursive_mutex
+ m_owners_mutex; ///< This mutex protects the owners collection.
jasonmolenda wrote:
No, I was just copying BreakpointSite when I used that.
https://github.com/llvm/llvm-project/pull/68845
___
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
jasonmolenda wrote:
Just getting back to the review comments. I remember writing something about
this last week but we have some API to select specific BreakpointLocations that
own the BreakpointSite and the code to do that is most naturally exposed as
"GetAtIndex" style access. I'm trying to create the WatchpointResource class
similar to BreakpointSite so I can follow the same coding style for these
Watchpoints that own the WatchpointResources. I may end up being able to drop
the GetAtIndex method later if it's natural go access it via an Owners method,
but I'm starting with the assumption that this will be more natural.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
+
+ lldb::wp_resource_id_t GetID() const;
+
+ bool Contains(lldb::addr_t addr);
+
+protected:
+ // The StopInfoWatchpoint knows when it is processing a hit for a thread for
+ // a site, so let it be the one to manage setting the location hit count once
+ // and only once.
+ friend class StopInfoWatchpoint;
+
+ void BumpHitCounts();
+
+private:
+ lldb::wp_resource_id_t m_id;
+
+ // start address & size aligned & expanded to be a valid watchpoint
+ // memory granule on this target.
+ lldb::addr_t m_addr;
+ size_t m_size;
+
+ bool m_watch_read; // true if we stop when the watched data is read from
+ bool m_watch_write; // true if we stop when the watched data is written to
JDevlieghere wrote:
+1. Otherwise they should go above the line, with `///` and make it a
capitalized sentence with a period.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,146 @@
+//===-- WatchpointResourceList.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCELIST_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCELIST_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-private.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResourceList {
+
+public:
+ // Constructors and Destructors
JDevlieghere wrote:
Remove
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
JDevlieghere wrote:
Doesn't look like you actually need this. You change the value exactly once
below. I would just return false on line 276 and 337 and true on line 334. I
think the return value of a `Dump` function is more self explanatory than
having to scroll up and see if `printed_anything` happened to be modified in
the 50 lines between 275 and 325.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,97 @@
+//===-- WatchpointResource.cpp
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/WatchpointResource.h"
+#include "lldb/Utility/LLDBAssert.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+WatchpointResource::WatchpointResource(lldb::addr_t addr, size_t size,
+ bool read, bool write)
+: m_id(LLDB_INVALID_WATCHPOINT_RESOURCE_ID), m_addr(addr), m_size(size),
+ m_watch_read(read), m_watch_write(write) {}
+
+WatchpointResource::~WatchpointResource() {
+ std::lock_guard guard(m_owners_mutex);
+ m_owners.clear();
+}
+
+addr_t WatchpointResource::GetAddress() const { return m_addr; }
+
+size_t WatchpointResource::GetByteSize() const { return m_size; }
+
+bool WatchpointResource::WatchpointResourceRead() const { return m_watch_read;
}
+
+bool WatchpointResource::WatchpointResourceWrite() const {
+ return m_watch_write;
+}
+
+void WatchpointResource::SetType(bool read, bool write) {
+ m_watch_read = read;
+ m_watch_write = write;
+}
+
+wp_resource_id_t WatchpointResource::GetID() const { return m_id; }
+
+void WatchpointResource::SetID(wp_resource_id_t id) { m_id = id; }
+
+bool WatchpointResource::Contains(addr_t addr) {
+ if (addr >= m_addr && addr < m_addr + m_size)
+return true;
+ return false;
+}
+
+void WatchpointResource::AddOwner(const WatchpointSP _sp) {
+ std::lock_guard guard(m_owners_mutex);
+ m_owners.push_back(wp_sp);
+}
+
+void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
+ std::lock_guard guard(m_owners_mutex);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
+}
+
+size_t WatchpointResource::GetNumberOfOwners() {
+ std::lock_guard guard(m_owners_mutex);
+ return m_owners.size();
+}
+
+bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
+ std::lock_guard guard(m_owners_mutex);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ return it != m_owners.end();
+}
+
+bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
+ std::lock_guard guard(m_owners_mutex);
+ WatchpointCollection::const_iterator match =
+ std::find_if(m_owners.begin(), m_owners.end(),
+ [](const WatchpointSP ) { return x.get() == wp; });
+ return match != m_owners.end();
+}
+
+WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
+ std::lock_guard guard(m_owners_mutex);
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
+return {};
+
+ return m_owners[idx];
+}
+
+size_t
+WatchpointResource::CopyOwnersList(WatchpointCollection _collection) {
+ std::lock_guard guard(m_owners_mutex);
+ for (auto wp_sp : m_owners) {
+out_collection.push_back(wp_sp);
+ }
JDevlieghere wrote:
No braces
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -331,6 +373,16 @@ void Watchpoint::DumpWithLevel(Stream *s,
bool Watchpoint::IsEnabled() const { return m_enabled; }
+uint32_t Watchpoint::GetHardwareIndex() const {
+ if (IsEnabled())
+return m_target.GetProcessSP()
+->GetWatchpointResourceList()
+.FindByWatchpoint(this)
+->GetID();
+ else
+return UINT32_MAX;
JDevlieghere wrote:
+1
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,154 @@
+//===-- WatchpointResourceList.cpp
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/WatchpointResourceList.h"
+#include "lldb/Breakpoint/WatchpointResource.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+WatchpointResourceList::WatchpointResourceList() : m_resources(), m_mutex() {}
+
+WatchpointResourceList::~WatchpointResourceList() { Clear(); }
+
+wp_resource_id_t
+WatchpointResourceList::Add(const WatchpointResourceSP _res_sp) {
+ Log *log = GetLog(LLDBLog::Watchpoints);
+ std::lock_guard guard(m_mutex);
+
+ LLDB_LOGF(log, "WatchpointResourceList::Add(addr 0x%" PRIx64 " size %zu)",
+wp_res_sp->GetAddress(), wp_res_sp->GetByteSize());
+
+ // The goal is to have the wp_resource_id_t match the actual hardware
+ // watchpoint register number. If we assume that the remote stub is
+ // setting them in the register context in the same order that they
+ // are sent from lldb, and if a watchpoint is removed and then a new
+ // one is added and gets the same register number, then we can
+ // iterate over all used IDs looking for the first unused number.
+
+ // If the Process was able to find the actual hardware watchpoint register
+ // number that was used, it can set the ID for the WatchpointResource
+ // before we get here.
+
+ if (wp_res_sp->GetID() == LLDB_INVALID_WATCHPOINT_RESOURCE_ID) {
+std::set used_ids;
+size_t size = m_resources.size();
+for (size_t i = 0; i < size; ++i)
+ used_ids.insert(m_resources[i]->GetID());
+
+wp_resource_id_t best = 0;
+for (wp_resource_id_t id : used_ids)
+ if (id == best)
+best++;
+ else
+break;
+
+LLDB_LOGF(log,
+ "WatchpointResourceList::Add assigning next "
+ "available WatchpointResource ID, %u",
+ best);
+wp_res_sp->SetID(best);
+ }
+
+ m_resources.push_back(wp_res_sp);
+ return wp_res_sp->GetID();
+}
+
+bool WatchpointResourceList::Remove(wp_resource_id_t id) {
+ std::lock_guard guard(m_mutex);
+ Log *log = GetLog(LLDBLog::Watchpoints);
+ for (collection::iterator pos = m_resources.begin(); pos !=
m_resources.end();
+ ++pos) {
+if ((*pos)->GetID() == id) {
+ LLDB_LOGF(log,
+"WatchpointResourceList::Remove(addr 0x%" PRIx64 " size %zu)",
+(*pos)->GetAddress(), (*pos)->GetByteSize());
+ m_resources.erase(pos);
+ return true;
+}
+ }
+ return false;
+}
+
+bool WatchpointResourceList::RemoveByAddress(addr_t addr) {
+ std::lock_guard guard(m_mutex);
+ Log *log = GetLog(LLDBLog::Watchpoints);
+ for (collection::iterator pos = m_resources.begin(); pos !=
m_resources.end();
+ ++pos) {
+if ((*pos)->Contains(addr)) {
+ LLDB_LOGF(log,
+"WatchpointResourceList::RemoveByAddress(addr 0x%" PRIx64
+" size %zu)",
+(*pos)->GetAddress(), (*pos)->GetByteSize());
+ m_resources.erase(pos);
+ return true;
+}
+ }
+ return false;
+}
+
+WatchpointResourceSP WatchpointResourceList::FindByAddress(addr_t addr) {
+ std::lock_guard guard(m_mutex);
+ for (collection::iterator pos = m_resources.begin(); pos !=
m_resources.end();
+ ++pos)
+if ((*pos)->Contains(addr))
+ return *pos;
JDevlieghere wrote:
Can you use a range-based for loop here and below?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
JDevlieghere wrote:
Starting with C++17, copy elision is guaranteed.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,155 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ bool WatchpointResourceRead() const;
+
+ bool WatchpointResourceWrite() const;
+
+ void SetType(bool read, bool write);
+
+ typedef std::vector WatchpointCollection;
+ typedef LockingAdaptedIterable
+ WatchpointIterable;
+
+ /// Iterate over the watchpoint owners for this resource
+ ///
+ /// \return
+ /// An Iterable object which can be used to loop over the watchpoints
+ /// that are owners of this resource.
+ WatchpointIterable Owners() {
+return WatchpointIterable(m_owners, m_owners_mutex);
+ }
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
+
+ lldb::wp_resource_id_t GetID() const;
+
+ bool Contains(lldb::addr_t addr);
+
+protected:
+ // The StopInfoWatchpoint knows when it is processing a hit for a thread for
+ // a site, so let it be the one to manage setting the location hit count once
+ // and only once.
+ friend class StopInfoWatchpoint;
+
+ void BumpHitCounts();
+
+private:
+ lldb::wp_resource_id_t m_id;
+
+ // start address & size aligned & expanded to be a valid watchpoint
+ // memory granule on this target.
+ lldb::addr_t m_addr;
+ size_t m_size;
+
+ bool m_watch_read; // true if we stop when the watched data is read from
+ bool m_watch_write; // true if we stop when the watched data is written to
+
+ // The watchpoints using this WatchpointResource.
+ WatchpointCollection m_owners;
+
+ std::recursive_mutex
+ m_owners_mutex; ///< This mutex protects the owners collection.
JDevlieghere wrote:
Move the comment to the line above. Does this have to be a recursive mutex?
https://github.com/llvm/llvm-project/pull/68845
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,155 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ bool WatchpointResourceRead() const;
+
+ bool WatchpointResourceWrite() const;
+
+ void SetType(bool read, bool write);
+
+ typedef std::vector WatchpointCollection;
+ typedef LockingAdaptedIterable
+ WatchpointIterable;
+
+ /// Iterate over the watchpoint owners for this resource
+ ///
+ /// \return
+ /// An Iterable object which can be used to loop over the watchpoints
+ /// that are owners of this resource.
+ WatchpointIterable Owners() {
+return WatchpointIterable(m_owners, m_owners_mutex);
+ }
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
JDevlieghere wrote:
Does this method care about the shared pointer or the underlying watchpoint? If
it's the former, the docs should make that explicit.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,155 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ bool WatchpointResourceRead() const;
+
+ bool WatchpointResourceWrite() const;
+
+ void SetType(bool read, bool write);
+
+ typedef std::vector WatchpointCollection;
+ typedef LockingAdaptedIterable
+ WatchpointIterable;
+
+ /// Iterate over the watchpoint owners for this resource
+ ///
+ /// \return
+ /// An Iterable object which can be used to loop over the watchpoints
+ /// that are owners of this resource.
+ WatchpointIterable Owners() {
+return WatchpointIterable(m_owners, m_owners_mutex);
+ }
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
+
+ lldb::wp_resource_id_t GetID() const;
+
+ bool Contains(lldb::addr_t addr);
+
+protected:
+ // The StopInfoWatchpoint knows when it is processing a hit for a thread for
+ // a site, so let it be the one to manage setting the location hit count once
+ // and only once.
+ friend class StopInfoWatchpoint;
+
+ void BumpHitCounts();
+
+private:
+ lldb::wp_resource_id_t m_id;
+
+ // start address & size aligned & expanded to be a valid watchpoint
+ // memory granule on this target.
+ lldb::addr_t m_addr;
+ size_t m_size;
+
+ bool m_watch_read; // true if we stop when the watched data is read from
+ bool m_watch_write; // true if we stop when the watched data is written to
+
+ // The watchpoints using this WatchpointResource.
JDevlieghere wrote:
`///`
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,146 @@ +//===-- WatchpointResourceList.h *- C++ -*-===// JDevlieghere wrote: Do we really need this class? It looks like a pretty trivial wrapper around a vector and a mutex. How much overlap is there with the `BreakpointSiteList.h`? Can we template this into a `StopPointList`? https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,155 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ bool WatchpointResourceRead() const;
+
+ bool WatchpointResourceWrite() const;
+
+ void SetType(bool read, bool write);
+
+ typedef std::vector WatchpointCollection;
+ typedef LockingAdaptedIterable
+ WatchpointIterable;
+
+ /// Iterate over the watchpoint owners for this resource
+ ///
+ /// \return
+ /// An Iterable object which can be used to loop over the watchpoints
+ /// that are owners of this resource.
+ WatchpointIterable Owners() {
+return WatchpointIterable(m_owners, m_owners_mutex);
+ }
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
JDevlieghere wrote:
Can this be a const reference?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,155 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
JDevlieghere wrote:
Nit: these comments are useless. We've been gradually removing them. Please
remove this.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,155 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Utility/Iterable.h"
+#include "lldb/lldb-public.h"
+
+#include
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ bool WatchpointResourceRead() const;
+
+ bool WatchpointResourceWrite() const;
+
+ void SetType(bool read, bool write);
+
+ typedef std::vector WatchpointCollection;
+ typedef LockingAdaptedIterable
+ WatchpointIterable;
+
+ /// Iterate over the watchpoint owners for this resource
+ ///
+ /// \return
+ /// An Iterable object which can be used to loop over the watchpoints
+ /// that are owners of this resource.
+ WatchpointIterable Owners() {
+return WatchpointIterable(m_owners, m_owners_mutex);
+ }
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
+
+ lldb::wp_resource_id_t GetID() const;
+
+ bool Contains(lldb::addr_t addr);
+
+protected:
+ // The StopInfoWatchpoint knows when it is processing a hit for a thread for
+ // a site, so let it be the one to manage setting the location hit count once
+ // and only once.
+ friend class StopInfoWatchpoint;
+
+ void BumpHitCounts();
+
+private:
+ lldb::wp_resource_id_t m_id;
+
+ // start address & size aligned & expanded to be a valid watchpoint
JDevlieghere wrote:
Nit: Capitalize
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,97 @@
+//===-- WatchpointResource.cpp
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/WatchpointResource.h"
+#include "lldb/Utility/LLDBAssert.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+WatchpointResource::WatchpointResource(lldb::addr_t addr, size_t size,
+ bool read, bool write)
+: m_id(LLDB_INVALID_WATCHPOINT_RESOURCE_ID), m_addr(addr), m_size(size),
+ m_watch_read(read), m_watch_write(write) {}
+
+WatchpointResource::~WatchpointResource() {
+ std::lock_guard guard(m_owners_mutex);
+ m_owners.clear();
+}
+
+addr_t WatchpointResource::GetAddress() const { return m_addr; }
+
+size_t WatchpointResource::GetByteSize() const { return m_size; }
+
+bool WatchpointResource::WatchpointResourceRead() const { return m_watch_read;
}
+
+bool WatchpointResource::WatchpointResourceWrite() const {
+ return m_watch_write;
+}
+
+void WatchpointResource::SetType(bool read, bool write) {
+ m_watch_read = read;
+ m_watch_write = write;
+}
+
+wp_resource_id_t WatchpointResource::GetID() const { return m_id; }
+
+void WatchpointResource::SetID(wp_resource_id_t id) { m_id = id; }
+
+bool WatchpointResource::Contains(addr_t addr) {
+ if (addr >= m_addr && addr < m_addr + m_size)
+return true;
+ return false;
+}
+
+void WatchpointResource::AddOwner(const WatchpointSP _sp) {
+ std::lock_guard guard(m_owners_mutex);
+ m_owners.push_back(wp_sp);
+}
+
+void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
+ std::lock_guard guard(m_owners_mutex);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
+}
+
+size_t WatchpointResource::GetNumberOfOwners() {
+ std::lock_guard guard(m_owners_mutex);
+ return m_owners.size();
+}
+
+bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
+ std::lock_guard guard(m_owners_mutex);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ return it != m_owners.end();
+}
+
+bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
+ std::lock_guard guard(m_owners_mutex);
+ WatchpointCollection::const_iterator match =
+ std::find_if(m_owners.begin(), m_owners.end(),
+ [](const WatchpointSP ) { return x.get() == wp; });
+ return match != m_owners.end();
+}
+
+WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
+ std::lock_guard guard(m_owners_mutex);
+ lldbassert(idx < m_owners.size());
JDevlieghere wrote:
I'd just make this a normal assert.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
}
WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
std::lock_guard guard(m_owners_mutex);
- assert(idx < m_owners.GetSize());
- if (idx >= m_owners.GetSize())
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
return {};
- return m_owners.GetByIndex(idx);
+ return m_owners[idx];
jasonmolenda wrote:
This is a good point. I am mostly trying to copy the API of BreakpointSite
which has methods like this, and that's how its Owners are accessed today in
different parts of lldb. I haven't gotten to the point where I use the Owners
anywhere yet, so I'm not locked in to the same API that BreakpointSite exposes.
We have things like `SBThread::GetStopReasonDataAtIndex()` which for
Breakpoints exposes all of the Breakpoints that were owning the BreakpointSite
that was hit. We have other places like
`StackFrameList::ResetCurrentInlinedDepth` which is iterating over a
BreakpointSite's owners to see if any of them are internal; the
IterableWatchpoint returned by `WatchpointResources::Owners` would be a cleaner
approach in this case. We have some places in the code that need to know the
address of the breakpoint that was hit and are grabbing the first
BreakpointLocation associated with the BreakpointSite, e.g.
`PlatformDarwin::GetSoftwareBreakpointTrapOpcode` (for some arm/thumb reason).
In `ThreadPlanCallFunction::DoPlanExplainsStop` we're iterating over the Owners
to log about them and detect if any are Internal breakpoint locations, which
could be done with an Iterable.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -67,18 +67,15 @@ size_t WatchpointResource::GetNumberOfOwners() {
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
- if (it != m_owners.end())
-return true;
- return false;
+ return it != m_owners.end();
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- for (WatchpointCollection::const_iterator it = m_owners.begin();
- it != m_owners.end(); ++it)
-if ((*it).get() == wp)
- return true;
- return false;
+ WatchpointCollection::const_iterator match =
bulbazord wrote:
My bad... 臘
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/jasonmolenda edited https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -67,18 +67,15 @@ size_t WatchpointResource::GetNumberOfOwners() {
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
- if (it != m_owners.end())
-return true;
- return false;
+ return it != m_owners.end();
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- for (WatchpointCollection::const_iterator it = m_owners.begin();
- it != m_owners.end(); ++it)
-if ((*it).get() == wp)
- return true;
- return false;
+ WatchpointCollection::const_iterator match =
jasonmolenda wrote:
It's a typedef to std::vector.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -67,18 +67,15 @@ size_t WatchpointResource::GetNumberOfOwners() {
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
- if (it != m_owners.end())
-return true;
- return false;
+ return it != m_owners.end();
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- for (WatchpointCollection::const_iterator it = m_owners.begin();
- it != m_owners.end(); ++it)
-if ((*it).get() == wp)
- return true;
- return false;
+ WatchpointCollection::const_iterator match =
bulbazord wrote:
Looks fine, but the type should be different right? I think you removed
`WatchpointCollection` in the 3rd commit
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
}
WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
std::lock_guard guard(m_owners_mutex);
- assert(idx < m_owners.GetSize());
- if (idx >= m_owners.GetSize())
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
return {};
- return m_owners.GetByIndex(idx);
+ return m_owners[idx];
bulbazord wrote:
Well, I'm not entirely sure how we're going to want to access the Owners list,
so I'd like to understand more how you plan on using it. If you want to do
something like `wp_resource_sp->Owners()[2]` then this method makes sense to
keep around, but I'd also be curious to know why you would want to access the
3rd owner directly. What I would like to avoid is writing a loop like this:
```
for (int i = 0; i < wp_resource_sp.GetNumberOfOwners(); i++) {
WatchpointSP wp_sp = wp_resource_sp->GetOwnerAtIndex(i);
// Use wp_sp
}
```
The two main reasons I am not a fan of this pattern:
1. As you pointed out, one would need to grab the `wp_resource_sp` owner mutex
to do this safely since another thread may call `RemoveOwner` in the middle of
this loop. I can't say I'm a fan of exposing the vector primarily because it
would mean we'd have to expose the mutex too.
2. Off-by-one errors are fairly easy to put in by mistake. Doubly so if we ever
iterate in reverse (i >0 vs i >=0 for the termination condition).
If we don't need to be able to access any of the owners at an arbitrary index,
I would suggest removing it so nobody can write that kind of loop. WDYT?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
}
WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
std::lock_guard guard(m_owners_mutex);
- assert(idx < m_owners.GetSize());
- if (idx >= m_owners.GetSize())
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
return {};
- return m_owners.GetByIndex(idx);
+ return m_owners[idx];
jasonmolenda wrote:
I'm not completely clear what having WatchpointResource::Owners() returning a
WatchpointIterable is going to do beyond allowing someone to write `for
(WatchpointSP _sp : wp_resource_sp->Owners())` - it's not exposing the
underlying std::vector so I can't call `wp_resource_sp->Owners().size()` or
`wp_resource_sp->Owners()[2]` is it? Right now the WatchpointResource has
methods like `AddOwner`, `RemoveOwner`, `GetNumberOfOwners`, `GetOwnerAtIndex`,
and some methods to check if the list of owners contains a watchpoint; should
callers be able to lock the Resource's mutex and get a reference to the
std::vector and query/manipulate it directly?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
}
WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
std::lock_guard guard(m_owners_mutex);
- assert(idx < m_owners.GetSize());
- if (idx >= m_owners.GetSize())
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
return {};
- return m_owners.GetByIndex(idx);
+ return m_owners[idx];
jasonmolenda wrote:
Right now nothing is using the WatchpointResource owners - I need to add that
in a follow on patch when a watchpoint has been triggered. BreakpointSite uses
this method in many places, but I agree that the Owners() method could be used.
We lose the range check/possible-assert in GetOwnerAtIndex() but hopefully
that shouldn't actually happen...
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
bulbazord wrote:
It should take advantage of copy elision. Specifically we should be able to
take advantage of NRVO.
https://en.cppreference.com/w/cpp/language/copy_elision
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
bulbazord wrote:
`WatchpointCollection` got removed right? I think you'll need to rewrite it...
I'd suggest using `find_if`
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
}
WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
std::lock_guard guard(m_owners_mutex);
- assert(idx < m_owners.GetSize());
- if (idx >= m_owners.GetSize())
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
return {};
- return m_owners.GetByIndex(idx);
+ return m_owners[idx];
bulbazord wrote:
Looking over this again, is there a use case where you would want to reference
a specific owner by index instead of using `Owners()`?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
bulbazord wrote:
`return it != m_owners.end();`
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
medismailben wrote:
Thanks for the explanation, that makes more sense to me now.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -839,11 +841,11 @@ std::optional
ProcessWindows::GetWatchpointSlotCount() {
return RegisterContextWindows::GetNumHardwareBreakpointSlots();
}
-Status ProcessWindows::EnableWatchpoint(Watchpoint *wp, bool notify) {
+Status ProcessWindows::EnableWatchpoint(WatchpointSP wp_sp, bool notify) {
Status error;
- if (wp->IsEnabled()) {
-wp->SetEnabled(true, notify);
+ if (wp_sp->IsEnabled()) {
jasonmolenda wrote:
The original code didn't check it, but more importantly on this topic, I have
real concern about touching any of ProcessWindows, NativeProcessWindows because
I can't build or test it directly myself. I have a feeling I'll keep most/all
of the existing interfaces for creating a StopInfo etc so the Windows code can
continue to work unmodified.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -491,14 +491,13 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread
, Target *target,
uint64_t exc_sub_sub_code) {
// Try hardware watchpoint.
if (target) {
+// LWP_TODO: We need to find the WatchpointResource that matches
jasonmolenda wrote:
Yeah good point my idea was that these are temporary placeholders as I roll
this out, so they wouldn't be in the source for very long. (this feature is not
really useful with all the parts landed so it's unlikely I'll be distracted by
some other issue )
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
jasonmolenda wrote:
Hm, I do not have a lot of confidence about avoiding an extra copy of the
vector if I return by value.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
jasonmolenda wrote:
none of that is done yet though.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
jasonmolenda wrote:
tbh there could be an argument for locking the whole object because a
WatchpointResource may be expanded to service multiple watchpoints (one
watchpoint watches bytes 0-1, a second watchpoint watches bytes 2-3) and then
reduced again when one of those watchpoints is deleted/disabled.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
bulbazord wrote:
Oh wait, the mutex is to protect the Owners. Nevermind!
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
bulbazord wrote:
Ah I could have sworn the mutex was marked as `mutable`. How are things like
`GetType` and `GetByteSize` marked `const` then? Do they not lock?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
jasonmolenda wrote:
idk about this one, `GetType(bool , bool )` makes the order easy to
see, and matches the `SetType (bool read, bool write)` whereas the pair would
be less explicitly named. I should follow the Watchpoint class method and have
`bool WatchpointResourceRead()`, `bool WatchpointResourceWrite()`.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
jasonmolenda wrote:
I lock the object's mutex in all of these methods.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
jasonmolenda wrote:
I originally wrote the WatchpointResource::GetMemoryRange that returned both
via out parameters, then at a later point I added a GetAddress & GetByteSize
methods meaning to remove GetMemoryRange which I wasn't happy with... but
forgot.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
+
+ // For read watchpoints, don't display any before/after value changes.
+ if (m_watch_read && !m_watch_modify && !m_watch_write)
+return printed_anything;
+
+ s->Printf("\n");
+ s->Printf("Watchpoint %u hit:\n", GetID());
+
+ StreamString values_ss;
+ if (prefix)
+values_ss.Indent(prefix);
if (m_old_value_sp) {
const char *old_value_cstr = m_old_value_sp->GetValueAsCString();
-if (old_value_cstr && old_value_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_value_cstr);
-else {
+if (old_value_cstr) {
+ values_ss.Printf("old value: %s", old_value_cstr);
+} else {
const char *old_summary_cstr = m_old_value_sp->GetSummaryAsCString();
- if (old_summary_cstr && old_summary_cstr[0])
-s->Printf("\n%sold value: %s", prefix, old_summary_cstr);
+ if (old_summary_cstr)
+values_ss.Printf("old value: %s", old_summary_cstr);
+ else {
+StreamString strm;
+DumpValueObjectOptions options;
+options.SetUseDynamicType(eNoDynamicValues)
+.SetHideRootType(true)
+.SetHideRootName(true)
+.SetHideName(true);
+m_old_value_sp->Dump(strm, options);
+if (strm.GetData())
+ values_ss.Printf("old value: %s", strm.GetData());
+ }
}
}
if (m_new_value_sp) {
+if (values_ss.GetSize())
+ values_ss.Printf("\n");
+
const char *new_value_cstr = m_new_value_sp->GetValueAsCString();
-if (new_value_cstr && new_value_cstr[0])
- s->Printf("\n%snew value: %s", prefix, new_value_cstr);
+if (new_value_cstr)
+ values_ss.Printf("new value: %s", new_value_cstr);
bulbazord wrote:
Same here
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -491,14 +491,13 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread
, Target *target,
uint64_t exc_sub_sub_code) {
// Try hardware watchpoint.
if (target) {
+// LWP_TODO: We need to find the WatchpointResource that matches
bulbazord wrote:
`LWP_TODO` doesn't mean much to a lot of contributors, maybe you can change
this to something like `TODO(Large Watchpoint Support)`? Mostly to avoid
acronyms that aren't written down anywhere else.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -3105,102 +3122,184 @@ Status
ProcessGDBRemote::DisableBreakpointSite(BreakpointSite *bp_site) {
}
// Pre-requisite: wp != NULL.
-static GDBStoppointType GetGDBStoppointType(Watchpoint *wp) {
- assert(wp);
- bool watch_read = wp->WatchpointRead();
- bool watch_write = wp->WatchpointWrite();
- bool watch_modify = wp->WatchpointModify();
-
- // watch_read, watch_write, watch_modify cannot all be false.
- assert((watch_read || watch_write || watch_modify) &&
- "watch_read, watch_write, watch_modify cannot all be false.");
- if (watch_read && (watch_write || watch_modify))
+static GDBStoppointType
+GetGDBStoppointType(const WatchpointResourceSP _res_sp) {
+ assert(wp_res_sp);
+ bool read, write;
+ wp_res_sp->GetType(read, write);
+
+ assert((read || write) && "read and write cannot both be false.");
+ if (read && write)
return eWatchpointReadWrite;
- else if (watch_read)
+ else if (read)
return eWatchpointRead;
- else // Must be watch_write or watch_modify, then.
+ else
return eWatchpointWrite;
}
-Status ProcessGDBRemote::EnableWatchpoint(Watchpoint *wp, bool notify) {
+Status ProcessGDBRemote::EnableWatchpoint(WatchpointSP wp_sp, bool notify) {
Status error;
- if (wp) {
-user_id_t watchID = wp->GetID();
-addr_t addr = wp->GetLoadAddress();
+ if (wp_sp) {
bulbazord wrote:
Since you're already refactoring this part, maybe you could flip the condition
and do the error handling up front. It's small, but it saves a level of
indentation in an already large code block.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
+
+ // For read watchpoints, don't display any before/after value changes.
+ if (m_watch_read && !m_watch_modify && !m_watch_write)
+return printed_anything;
+
+ s->Printf("\n");
+ s->Printf("Watchpoint %u hit:\n", GetID());
+
+ StreamString values_ss;
+ if (prefix)
+values_ss.Indent(prefix);
if (m_old_value_sp) {
const char *old_value_cstr = m_old_value_sp->GetValueAsCString();
-if (old_value_cstr && old_value_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_value_cstr);
-else {
+if (old_value_cstr) {
+ values_ss.Printf("old value: %s", old_value_cstr);
+} else {
const char *old_summary_cstr = m_old_value_sp->GetSummaryAsCString();
- if (old_summary_cstr && old_summary_cstr[0])
-s->Printf("\n%sold value: %s", prefix, old_summary_cstr);
+ if (old_summary_cstr)
+values_ss.Printf("old value: %s", old_summary_cstr);
+ else {
+StreamString strm;
+DumpValueObjectOptions options;
+options.SetUseDynamicType(eNoDynamicValues)
+.SetHideRootType(true)
+.SetHideRootName(true)
+.SetHideName(true);
+m_old_value_sp->Dump(strm, options);
+if (strm.GetData())
+ values_ss.Printf("old value: %s", strm.GetData());
+ }
}
}
if (m_new_value_sp) {
+if (values_ss.GetSize())
+ values_ss.Printf("\n");
+
const char *new_value_cstr = m_new_value_sp->GetValueAsCString();
-if (new_value_cstr && new_value_cstr[0])
- s->Printf("\n%snew value: %s", prefix, new_value_cstr);
+if (new_value_cstr)
+ values_ss.Printf("new value: %s", new_value_cstr);
else {
const char *new_summary_cstr = m_new_value_sp->GetSummaryAsCString();
- if (new_summary_cstr && new_summary_cstr[0])
-s->Printf("\n%snew value: %s", prefix, new_summary_cstr);
+ if (new_summary_cstr)
+values_ss.Printf("new value: %s", new_summary_cstr);
bulbazord wrote:
Same here
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,90 @@
+//===-- WatchpointCollection.cpp
--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/Breakpoint/Watchpoint.h"
+#include "lldb/Core/ModuleList.h"
+#include "lldb/Target/Thread.h"
+#include "lldb/Target/ThreadSpec.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+// WatchpointCollection constructor
+WatchpointCollection::WatchpointCollection() = default;
+
+// Destructor
+WatchpointCollection::~WatchpointCollection() = default;
+
+void WatchpointCollection::Add(const WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ m_collection.push_back(wp);
+}
+
+bool WatchpointCollection::Remove(WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ for (collection::iterator pos = m_collection.begin();
+ pos != m_collection.end(); ++pos) {
+if (*pos == wp) {
+ m_collection.erase(pos);
+ return true;
+}
+ }
+ return false;
+}
+
+WatchpointSP WatchpointCollection::GetByIndex(size_t i) {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+const WatchpointSP WatchpointCollection::GetByIndex(size_t i) const {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+WatchpointCollection &
+WatchpointCollection::operator=(const WatchpointCollection ) {
+ if (this != ) {
+std::lock(m_collection_mutex, rhs.m_collection_mutex);
+std::lock_guard lhs_guard(m_collection_mutex, std::adopt_lock);
+std::lock_guard rhs_guard(rhs.m_collection_mutex,
+ std::adopt_lock);
bulbazord wrote:
You can use `std::scoped_lock` for this now that we're using C++17!
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -3105,102 +3122,184 @@ Status
ProcessGDBRemote::DisableBreakpointSite(BreakpointSite *bp_site) {
}
// Pre-requisite: wp != NULL.
-static GDBStoppointType GetGDBStoppointType(Watchpoint *wp) {
- assert(wp);
- bool watch_read = wp->WatchpointRead();
- bool watch_write = wp->WatchpointWrite();
- bool watch_modify = wp->WatchpointModify();
-
- // watch_read, watch_write, watch_modify cannot all be false.
- assert((watch_read || watch_write || watch_modify) &&
- "watch_read, watch_write, watch_modify cannot all be false.");
- if (watch_read && (watch_write || watch_modify))
+static GDBStoppointType
+GetGDBStoppointType(const WatchpointResourceSP _res_sp) {
+ assert(wp_res_sp);
+ bool read, write;
+ wp_res_sp->GetType(read, write);
+
+ assert((read || write) && "read and write cannot both be false.");
bulbazord wrote:
The assertion message here could be fleshed out a bit more. Something like
`"WatchpointResource type is neither read nor write"`.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
bulbazord wrote:
Instead of exposing this primitive, maybe it would make more sense to return an
iterator or an iterator range?
Something like:
```
IteratorRange Owners();
```
IMO this would make it easier to write bug-free code since we're not going to
have to manually set up index iteration logic.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
bulbazord wrote:
Why return the size instead of returning just a new `WatchpointCollection`
object? That way callers don't have to create a new one and pass it in, they
can just grab it from this method directly.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/bulbazord commented: A lot of the `WatchpointResourceList::FindBy` methods can probably be rewritten with `std::find_if` (or `llvm::find_if` which is a little easier to use IMO). Overall, looks like a pretty nice mostly-NFC refactor! https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,90 @@
+//===-- WatchpointCollection.cpp
--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/Breakpoint/Watchpoint.h"
+#include "lldb/Core/ModuleList.h"
+#include "lldb/Target/Thread.h"
+#include "lldb/Target/ThreadSpec.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+// WatchpointCollection constructor
+WatchpointCollection::WatchpointCollection() = default;
+
+// Destructor
+WatchpointCollection::~WatchpointCollection() = default;
+
+void WatchpointCollection::Add(const WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ m_collection.push_back(wp);
+}
+
+bool WatchpointCollection::Remove(WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ for (collection::iterator pos = m_collection.begin();
+ pos != m_collection.end(); ++pos) {
+if (*pos == wp) {
+ m_collection.erase(pos);
+ return true;
+}
+ }
+ return false;
+}
+
+WatchpointSP WatchpointCollection::GetByIndex(size_t i) {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+const WatchpointSP WatchpointCollection::GetByIndex(size_t i) const {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+WatchpointCollection &
+WatchpointCollection::operator=(const WatchpointCollection ) {
+ if (this != ) {
+std::lock(m_collection_mutex, rhs.m_collection_mutex);
+std::lock_guard lhs_guard(m_collection_mutex, std::adopt_lock);
+std::lock_guard rhs_guard(rhs.m_collection_mutex,
+ std::adopt_lock);
+m_collection = rhs.m_collection;
+ }
+ return *this;
+}
+
+void WatchpointCollection::Clear() {
+ std::lock_guard guard(m_collection_mutex);
+ m_collection.clear();
+}
+
+bool WatchpointCollection::Contains(const WatchpointSP _sp) {
+ std::lock_guard guard(m_collection_mutex);
+ const size_t size = m_collection.size();
+ for (size_t i = 0; i < size; ++i) {
+if (m_collection[i] == wp_sp)
+ return true;
+ }
bulbazord wrote:
Why not use iterators here? You can also use `llvm::is_contained` too.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
+
+ lldb::wp_resource_id_t GetID() const;
+
+ bool Contains(lldb::addr_t addr);
+
+protected:
+ // The StopInfoWatchpoint knows when it is processing a hit for a thread for
+ // a site, so let it be the one to manage setting the location hit count once
+ // and only once.
+ friend class StopInfoWatchpoint;
+
+ void BumpHitCounts();
+
+private:
+ lldb::wp_resource_id_t m_id;
+
+ // start address & size aligned & expanded to be a valid watchpoint
+ // memory granule on this target.
+ lldb::addr_t m_addr;
+ size_t m_size;
+
+ bool m_watch_read; // true if we stop when the watched data is read from
+ bool m_watch_write; // true if we stop when the watched data is written to
bulbazord wrote:
IMO these comments are unnecessary, the names of the variables already describe
the intended semantics quite well I think.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
+
+ // For read watchpoints, don't display any before/after value changes.
+ if (m_watch_read && !m_watch_modify && !m_watch_write)
+return printed_anything;
+
+ s->Printf("\n");
+ s->Printf("Watchpoint %u hit:\n", GetID());
+
+ StreamString values_ss;
+ if (prefix)
+values_ss.Indent(prefix);
if (m_old_value_sp) {
const char *old_value_cstr = m_old_value_sp->GetValueAsCString();
-if (old_value_cstr && old_value_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_value_cstr);
-else {
+if (old_value_cstr) {
+ values_ss.Printf("old value: %s", old_value_cstr);
+} else {
const char *old_summary_cstr = m_old_value_sp->GetSummaryAsCString();
- if (old_summary_cstr && old_summary_cstr[0])
-s->Printf("\n%sold value: %s", prefix, old_summary_cstr);
+ if (old_summary_cstr)
bulbazord wrote:
Same here
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
bulbazord wrote:
Suggestion: Return `std::pair` instead of passing out parameters.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
bulbazord wrote:
This can be `const`-qualified right?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
bulbazord wrote:
Instead of out parameters, you could do `std::pair`.
But why have this and `GetAddress()` + `GetByteSize()`?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
jasonmolenda wrote:
To be honest, I'm VERY close to just removing the hardware index numbers
altogether and having SBWatchpoint::GetHardwareIndex return UINT32_MAX. It
might be the better idea than these convoluted things I'm doing to try to
heuristically guess the hw index.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
jasonmolenda wrote:
When the Process method is given a watchpoint request from the user, which must
be broken into one or more WatchpointResources (or an existing
WatchpointResource reused), we can only get the hardware register number by
looking at the IDs of currently allocated WatchpointResources in the
WatchpointResourceList, which start at 0. I assume that the remote stub will
allocate its watchpoints in the hardware registers starting at index 0, and
when removing/disabling them, will leave that slot unused until another request
comes along, at which point the stub will start from 0 and look for the first
unused hardware register.
In the specific case of gdb remote serial protocol (ProcessGDBRemote), it has
none of this. The Z2/Z3/Z4 packets don't tell you what hardware register was
used.
Today, lldb sets the hardware watchpoint index based on the watchpoint access
stop. SOME ways of passing the watchpoint trap back to lldb can communicate
the hardware watchpoint index. The standard gdb remote serial protocol does
not with its `watch/rwatch/awatch:address` key-value in the stop packet.
lldb-server has a different way of reporting watchpoints, `reason:watchpoint`
and `description:addr[ index]` where the address is sent in the description:
and it may be followed by a space and then the index number of the hardware
watchpoint that was triggered. For macOS systems, the mach exception data is
sent at
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.h *- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
medismailben wrote:
IIUC, this ID shouldn't change during the `Resource`'s lifetime so why not set
it once in the constructor, and get rid of this setter ?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
jasonmolenda wrote: @bulbazord @JDevlieghere OK this is the second set of refinement where it's closer to the structure of BreakpointSites. It's still not actually doing anything -- each one WatchpointResource corresponds to one Watchpoint, a single Watchpoint's request is not broken into multiple WatchpointResources, and when a watchpoint access is hit, I'm not iterating all of the Watchpoints (lol, all 1 of them) to bump their hit count, run conditions, etc. These remaining pieces of work will be smaller scale, more complicated I think, and it will be easier to consider those parts if the patches are small. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/DavidSpickett edited https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -135,5 +135,5 @@ def test_watch_address_with_invalid_watch_size(self): self.expect( error.GetCString(), exe=False, -substrs=["watch size of %d is not supported" % 365], +substrs=["Setting one of the watchpoint resources failed"], DavidSpickett wrote: Yeah I don't think it would need to be super specific, that is what logs are for. Drawing the distinction between failed because the stub didn't have resources, and failed because the stub exploded, that's more what I'd like to see. And here the original error message is actually still pretty vague. It tells me that size N isn't support, but not what is supported (which is a whole other thing). https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -2,8 +2,8 @@
#include
int main() {
- uint8_t x1 = 0;
- uint16_t x2 = 0;
+ long x1 = 0;
+ long x2 = 0;
jasonmolenda wrote:
You're right. I thought this test was *unintentionally* testing the case where
we have two hardware watchpoint register watching the same doubleword of
memory. But that was actually the point of the test (v.
https://reviews.llvm.org/D77173 ). I'm not sure how thrilled I am about this
test, it seems to be encoding the behavior of the cpu with multiple watchpoint
registers on the same doubleword and I don't know how universal that is - if a
CPU might only evaluate the first watchpoint on a doubleword and ignore the
second.
This is very much in the wheelhouse of the work I need to do on large
watchpoints where a single WatchpointResource should be used for both
watchpoints, expanded to include the byte range of both, and then reduced back
down again when one is disabled or deleted. I'll be adding tests like this one
as part of that work. I'll revert my change here.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -1,3 +1,10 @@ # REQUIRES: system-darwin +# TODO: This test is breaking with my output +# reformatting done for Large Watchpoint support, +# but the lines being output by lldb are identical, +# by visual inspection. +# FileCheck is seeing some difference between them, +# which I need to get to the bottom of. jasonmolenda wrote: Good idea, it must surely be something like that. The test wasn't adding anything super useful and I spent so much time trying to rearrange things to make it pass I finally punted on it for now. I'm going to need to add a bunch of new tests once I get the large watchpoint work finished. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -135,5 +135,5 @@ def test_watch_address_with_invalid_watch_size(self): self.expect( error.GetCString(), exe=False, -substrs=["watch size of %d is not supported" % 365], +substrs=["Setting one of the watchpoint resources failed"], jasonmolenda wrote: This test is trying to watch a 365 byte region, it was intended to detect the fact that you can only watch 1/2/4/8 byte blocks of memory (the --size was really an enum and it had to be one of those). With this patch currently, lldb will accept the 365 byte watch request and pass it to the remote stub, which will probably return an error. (debugserver will actually break it into aligned watchable regions and do the right thing). When I finish the large watchpoint work, lldb will break this 365 byte request into 46 8-byte regions (for linux with BAS only watchpoints) and when we try to set the 5th one (or however many the core actually supports), that watch request will fail. Maybe the error message should say something about how your watchpoint was broken into watchpoint requests but only of them could be set. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -2,8 +2,8 @@
#include
int main() {
- uint8_t x1 = 0;
- uint16_t x2 = 0;
+ long x1 = 0;
+ long x2 = 0;
DavidSpickett wrote:
We need the type change here because?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,61 @@
+//===-- WatchpointResourceList.cpp
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/WatchpointResourceList.h"
+#include "lldb/Target/WatchpointResource.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+WatchpointResourceList::WatchpointResourceList() : m_resources(), m_mutex() {}
+
+WatchpointResourceList::~WatchpointResourceList() { Clear(); }
+
+uint32_t WatchpointResourceList::GetSize() {
+ std::lock_guard guard(m_mutex);
+ return m_resources.size();
+}
+
+lldb::WatchpointResourceSP
+WatchpointResourceList::GetResourceAtIndex(uint32_t idx) {
+ std::lock_guard guard(m_mutex);
+ if (idx < m_resources.size()) {
+return m_resources[idx];
+ } else {
+return {};
DavidSpickett wrote:
Single line ifs.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,61 @@
+//===-- WatchpointResourceList.cpp
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/WatchpointResourceList.h"
+#include "lldb/Target/WatchpointResource.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+WatchpointResourceList::WatchpointResourceList() : m_resources(), m_mutex() {}
+
+WatchpointResourceList::~WatchpointResourceList() { Clear(); }
+
+uint32_t WatchpointResourceList::GetSize() {
+ std::lock_guard guard(m_mutex);
+ return m_resources.size();
+}
+
+lldb::WatchpointResourceSP
+WatchpointResourceList::GetResourceAtIndex(uint32_t idx) {
+ std::lock_guard guard(m_mutex);
+ if (idx < m_resources.size()) {
+return m_resources[idx];
+ } else {
+return {};
DavidSpickett wrote:
Also return in an else (as in, no need for the else)
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -135,5 +135,5 @@ def test_watch_address_with_invalid_watch_size(self): self.expect( error.GetCString(), exe=False, -substrs=["watch size of %d is not supported" % 365], +substrs=["Setting one of the watchpoint resources failed"], DavidSpickett wrote: Is the original error still relevant? Should it be one of the watchpoint resources failed because... https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -1,3 +1,10 @@
# REQUIRES: system-darwin
+# TODO: This test is breaking with my output
+# reformatting done for Large Watchpoint support,
+# but the lines being output by lldb are identical,
+# by visual inspection.
+# FileCheck is seeing some difference between them,
+# which I need to get to the bottom of.
DavidSpickett wrote:
A trick I used to do for python doctests was `str.replace(" ", "?")`. Then you
could see the trailing spaces. Might work here if you have that or extra
newlines.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
jasonmolenda wrote: I started on the next step of updating ProcessGDBRemote to identify the WatchpointResource that was hit by a watchpoint stop, and evaluate all of its Watchpoints, when I looked at BreakpointSite and BreakpointSiteList and realize I should have started by coping their design/functionality. I'm rewriting WatchpointResource / WatchpointResourceList right now. https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 1379a7286e156af2d96cb0f3d8aa8e5c7f56bccd
791fd06fe642f1163c39d79c57c7a6daae2c8ea6 --
lldb/include/lldb/Target/WatchpointResource.h
lldb/include/lldb/Target/WatchpointResourceList.h
lldb/source/Target/WatchpointResource.cpp
lldb/source/Target/WatchpointResourceList.cpp
lldb/include/lldb/Breakpoint/Watchpoint.h
lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h
lldb/include/lldb/Target/Process.h lldb/include/lldb/lldb-forward.h
lldb/source/API/SBWatchpoint.cpp lldb/source/Breakpoint/Watchpoint.cpp
lldb/source/Commands/CommandObjectWatchpoint.cpp
lldb/source/Interpreter/OptionGroupWatchpoint.cpp
lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h
lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
lldb/source/Target/Process.cpp lldb/source/Target/StopInfo.cpp
lldb/source/Target/Target.cpp
lldb/test/API/commands/watchpoints/watchpoint_count/main.c
lldb/test/Shell/Watchpoint/Inputs/val.c
``
View the diff from clang-format here.
``diff
diff --git a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
index d1ae916cd74b..48cbc9ec0e75 100644
--- a/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
+++ b/lldb/source/Interpreter/OptionGroupWatchpoint.cpp
@@ -43,8 +43,15 @@ static constexpr OptionDefinition g_option_table[] = {
{LLDB_OPT_SET_1, false, "watch", 'w', OptionParser::eRequiredArgument,
nullptr, OptionEnumValues(g_watch_type), 0, eArgTypeWatchType,
"Specify the type of watching to perform."},
-{LLDB_OPT_SET_1, false, "size", 's', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeByteSize,
+{LLDB_OPT_SET_1,
+ false,
+ "size",
+ 's',
+ OptionParser::eRequiredArgument,
+ nullptr,
+ {},
+ 0,
+ eArgTypeByteSize,
"Number of bytes to use to watch a region."},
{LLDB_OPT_SET_2,
false,
``
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
llvmbot wrote:
@llvm/pr-subscribers-lldb
Author: Jason Molenda (jasonmolenda)
Changes
This patch is rearranging code a bit to add WatchpointResources to Process. A
WatchpointResource is meant to represent a hardware watchpoint register in the
inferior process. It has an address, a size, a type, and a list of Watchpoints
that are using this WatchpointResource.
This current patch doesn't add any of the features of WatchpointResources that
make them interesting -- a user asking to watch a 24 byte object could watch
this with three 8 byte WatchpointResources. Or a Watchpoint on 1 byte at
0x1002 and a second watchpoint on 1 byte at 0x1003, these must both be served
by a single WatchpointResource on that doubleword at 0x1000 on a 64-bit target,
if two hardware watchpoint registers were used to track these separately, one
of them may not be hit. Or if you have one Watchpoint on a variable with a
condition set, and another Watchpoint on that same variable with a command
defined or different condition, or ignorecount, both of those Watchpoints need
to evaluate their criteria/commands when their WatchpointResource has been hit.
There's a bit of code movement to rearrange things in the direction I'll need
for implementing this feature, so I want to start with reviewing landing
this mostly NFC patch and we can focus on the algorithmic choices about how
WatchpointResources are shared and handled as they're triggeed, separately.
This patch also stops printing "Watchpoint n hit: old value: x,
new vlaue: y" for Read watchpoints. I could make an argument for print
"Watchpoint n hit: current value x" but the current output
doesn't make any sense, and the user can print the value if they are
particularly interested. Read watchpoints are used primarily to understand
what code is reading a variable.
This patch adds more fallbacks for how to print the objects being watched if we
have types, instead of assuming they are all integral values, so a struct will
print its elements. As large watchpoints are added, we'll be doing a lot more
of those.
To track the WatchpointSP in the WatchpointResources, I changed the internal
API which took a WatchpointSP and devolved it to a Watchpoint*, which meant
touching several different Process files. I removed the watchpoint code in
ProcessKDP which only reported that watchpoints aren't supported, the base
class does that already.
I haven't yet changed how we receive a watchpoint to identify the
WatchpointResource responsible for the trigger, and identify all Watchpoints
that are using this Resource to evaluate their conditions etc. This is the
same work that a BreakpointSite needs to do when it has been tiggered, where
multiple Breakpoints may be at the same address.
There is not yet any printing of the Resources that a Watchpoint is implemented
in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).
"watchpoint set var" and "watchpoint set expression" take a size argument which
was previously 1, 2, 4, or 8 (an enum). I've changed this to an unsigned int.
Most hardware implementations can only watch 1, 2, 4, 8 byte ranges, but with
Resources we'll allow a user to ask for different sized watchpoints and set
them in hardware-expressble terms soon.
I've annotated areas where I know there is work still needed with LWP_TODO that
I'll be working on once this is landed.
I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116
---
Patch is 54.63 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/68845.diff
28 Files Affected:
- (modified) lldb/include/lldb/Breakpoint/Watchpoint.h (+1-1)
- (modified) lldb/include/lldb/Interpreter/OptionGroupWatchpoint.h (+2-3)
- (modified) lldb/include/lldb/Target/Process.h (+10-2)
- (added) lldb/include/lldb/Target/WatchpointResource.h (+57)
- (added) lldb/include/lldb/Target/WatchpointResourceList.h (+85)
- (modified) lldb/include/lldb/lldb-forward.h (+2)
- (modified) lldb/source/API/SBWatchpoint.cpp (+2-2)
- (modified) lldb/source/Breakpoint/Watchpoint.cpp (+56-15)
- (modified) lldb/source/Commands/CommandObjectWatchpoint.cpp (+4-4)
- (modified) lldb/source/Interpreter/OptionGroupWatchpoint.cpp (+6-37)
- (modified) lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp (-14)
- (modified) lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h (-7)
- (modified) lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp (+9)
- (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp
(+21-19)
- (modified) lldb/source/Plugins/Process/Windows/Common/ProcessWindows.h (+4-2)
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
(+143-61)
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+4-2)
- (modified) lldb/source/Target/CMakeLists.txt (+2)
- (modified) lldb/source/Target/Process.cpp
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/jasonmolenda created
https://github.com/llvm/llvm-project/pull/68845
This patch is rearranging code a bit to add WatchpointResources to Process. A
WatchpointResource is meant to represent a hardware watchpoint register in the
inferior process. It has an address, a size, a type, and a list of Watchpoints
that are using this WatchpointResource.
This current patch doesn't add any of the features of WatchpointResources that
make them interesting -- a user asking to watch a 24 byte object could watch
this with three 8 byte WatchpointResources. Or a Watchpoint on 1 byte at
0x1002 and a second watchpoint on 1 byte at 0x1003, these must both be served
by a single WatchpointResource on that doubleword at 0x1000 on a 64-bit target,
if two hardware watchpoint registers were used to track these separately, one
of them may not be hit. Or if you have one Watchpoint on a variable with a
condition set, and another Watchpoint on that same variable with a command
defined or different condition, or ignorecount, both of those Watchpoints need
to evaluate their criteria/commands when their WatchpointResource has been hit.
There's a bit of code movement to rearrange things in the direction I'll need
for implementing this feature, so I want to start with reviewing & landing this
mostly NFC patch and we can focus on the algorithmic choices about how
WatchpointResources are shared and handled as they're triggeed, separately.
This patch also stops printing "Watchpoint hit: old value: , new vlaue:
" for Read watchpoints. I could make an argument for print "Watchpoint
hit: current value " but the current output doesn't make any sense, and the
user can print the value if they are particularly interested. Read watchpoints
are used primarily to understand what code is reading a variable.
This patch adds more fallbacks for how to print the objects being watched if we
have types, instead of assuming they are all integral values, so a struct will
print its elements. As large watchpoints are added, we'll be doing a lot more
of those.
To track the WatchpointSP in the WatchpointResources, I changed the internal
API which took a WatchpointSP and devolved it to a Watchpoint*, which meant
touching several different Process files. I removed the watchpoint code in
ProcessKDP which only reported that watchpoints aren't supported, the base
class does that already.
I haven't yet changed how we receive a watchpoint to identify the
WatchpointResource responsible for the trigger, and identify all Watchpoints
that are using this Resource to evaluate their conditions etc. This is the
same work that a BreakpointSite needs to do when it has been tiggered, where
multiple Breakpoints may be at the same address.
There is not yet any printing of the Resources that a Watchpoint is implemented
in terms of ("watchpoint list", or
SBWatchpoint::GetDescription).
"watchpoint set var" and "watchpoint set expression" take a size argument which
was previously 1, 2, 4, or 8 (an enum). I've changed this to an unsigned int.
Most hardware implementations can only watch 1, 2, 4, 8 byte ranges, but with
Resources we'll allow a user to ask for different sized watchpoints and set
them in hardware-expressble terms soon.
I've annotated areas where I know there is work still needed with LWP_TODO that
I'll be working on once this is landed.
I've tested this on aarch64 macOS, aarch64 Linux, and Intel macOS.
https://discourse.llvm.org/t/rfc-large-watchpoint-support-in-lldb/72116
>From 791fd06fe642f1163c39d79c57c7a6daae2c8ea6 Mon Sep 17 00:00:00 2001
From: Jason Molenda
Date: Wed, 11 Oct 2023 20:05:58 -0700
Subject: [PATCH] [lldb] [mostly NFC] Large WP foundation: WatchpointResources
This patch is rearranging code a bit to add WatchpointResources to
Process. A WatchpointResource is meant to represent a hardware
watchpoint register in the inferior process. It has an address, a
size, a type, and a list of Watchpoints that are using this
WatchpointResource.
This current patch doesn't add any of the features of WatchpointResources
that make them interesting -- a user asking to watch a 24 byte
object could watch this with three 8 byte WatchpointResources. Or
a Watchpoint on 1 byte at 0x1002 and a second watchpoint on 1 byte
at 0x1003, these must both be served by a single WatchpointResource
on that doubleword at 0x1000 on a 64-bit target, if two hardware
watchpoint registers were used to track these separately, one of
them may not be hit. Or if you have one Watchpoint on a variable
with a condition set, and another Watchpoint on that same variable
with a command defined or different condition, or ignorecount, both
of those Watchpoints need to evaluate their criteria/commands when
their WatchpointResource has been hit.
There's a bit of code movement to rearrange things in the direction
I'll need for implementing this feature, so I want to start with
reviewing & landing this mostly NFC patch and we can
