Add a scripted way to re-present a stop location #158128
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
"Facade" locations instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations. rdar://152112327
jimingham
requested review from
medismailben
and removed request for
JDevlieghere
jimingham
requested review from
JDevlieghere,
felipepiovezan,
jasonmolenda and
labath
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesThis patch adds the notion of "Facade" locations which can be reported from a ScriptedResolver instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations. I apologize in advance for the size of the patch. Almost all of what's here was necessary to (a) make the feature testable and (b) not break any of the current behavior. The motivation for this feature is given in the "Providing Facade Locations" section that I added to the python-reference.rst so I won't repeat it here. rdar://152112327 Patch is 77.02 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/158128.diff 36 Files Affected:
diff --git a/lldb/bindings/python/python-swigsafecast.swig b/lldb/bindings/python/python-swigsafecast.swig
index 4721dfdc17e6a..3ea24f1a31414 100644
--- a/lldb/bindings/python/python-swigsafecast.swig
+++ b/lldb/bindings/python/python-swigsafecast.swig
@@ -142,5 +142,9 @@ PythonObject SWIGBridge::ToSWIGWrapper(
return ToSWIGHelper(module_spec_sb.release(), SWIGTYPE_p_lldb__SBModuleSpec);
}
+PythonObject SWIGBridge::ToSWIGWrapper(lldb::DescriptionLevel level) {
+ return PythonInteger((int64_t) level);
+}
+
} // namespace python
} // namespace lldb_private
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index 2c30d536a753d..64b7dc8381073 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -422,6 +422,30 @@ void *lldb_private::python::LLDBSWIGPython_CastPyObjectToSBBreakpoint(PyObject *
return sb_ptr;
}
+void *lldb_private::python::LLDBSWIGPython_CastPyObjectToSBFrame(PyObject * data) {
+ lldb::SBFrame *sb_ptr = nullptr;
+
+ int valid_cast =
+ SWIG_ConvertPtr(data, (void **)&sb_ptr, SWIGTYPE_p_lldb__SBFrame, 0);
+
+ if (valid_cast == -1)
+ return NULL;
+
+ return sb_ptr;
+}
+
+void *lldb_private::python::LLDBSWIGPython_CastPyObjectToSBBreakpointLocation(PyObject * data) {
+ lldb::SBBreakpointLocation *sb_ptr = nullptr;
+
+ int valid_cast =
+ SWIG_ConvertPtr(data, (void **)&sb_ptr, SWIGTYPE_p_lldb__SBBreakpointLocation, 0);
+
+ if (valid_cast == -1)
+ return NULL;
+
+ return sb_ptr;
+}
+
void *lldb_private::python::LLDBSWIGPython_CastPyObjectToSBAttachInfo(PyObject * data) {
lldb::SBAttachInfo *sb_ptr = nullptr;
diff --git a/lldb/docs/use/python-reference.rst b/lldb/docs/use/python-reference.rst
index 4292714c9c208..5f40526a7308e 100644
--- a/lldb/docs/use/python-reference.rst
+++ b/lldb/docs/use/python-reference.rst
@@ -420,6 +420,67 @@ of Modules and the list of CompileUnits that will make up the SearchFilter. If
you pass in empty lists, the breakpoint will use the default "search
everywhere,accept everything" filter.
+Providing Facade Locations:
+
+The breakpoint resolver interface also allows you to present a separate set
+of locations for the breakpoint than the ones that actually implement the
+breakpoint in the target.
+
+An example use case for this is if you are providing a debugging interface for a
+library that implements an interpreter for a language lldb can't debug. But
+while debugging that library at the level of the implementation language (e.g. C/C++, etc)
+you would like to offer the ability to "stop when a line in a source language
+file is executed".
+
+You can do this if you know where new lines of code are dispatched in the
+interpreter. You would set a breakpoint there, and then look at the state
+when that breakpoint is hit to see if it is dispatching the source file and
+line that were requested, and stop appropriately.
+
+Facade breakpoint locations are intended to make a more natural presentation
+of that sort of feature. The idea is that you would make a custom breakpoint
+resolver that sets actual locations in the places of interest in the interpreter.
+
+Then your resolver would add "facade locations" that represent the places in the
+interpreted code that you want the breakpoint to stop at, using SBBreakpoint::AddFacadeLocation.
+When lldb describes the breakpoint, it will only show the Facade locations.
+Since facade breakpoint location's description is customizable, you can make these
+locations more descriptive. And when the "real" location is hit, lldb will call the
+"was_hit" method of your resolver. That will return the facade location you
+consider to have been hit this time around, or if you return None, the breakpoint
+will be considered not to have been hit.
+
+Note, this feature is also useful if you don't intend to present facade
+locations since it essentially provides a scripted breakpoint condition. Every
+time one of the locations in your breakpoint is hit, you can run the code in
+your "was_hit" to determine whether to consider the breakpoint hit or not, and
+return the location you were passed in if you want it to be a hit, and None if not.
+
+The Facade location adds these optional affordances to the Resolver class:
+
++------------------------------+----------------------------------------+------------------------------------------------------------------------------------------------------------------+
+| Name | Arguments | Description |
++------------------------------+----------------------------------------+------------------------------------------------------------------------------------------------------------------+
+| ``was_hit`` | ``frame``:`lldb.SBFrame` | This will get called when one of the "real" locations set by your resolver is hit |
+| | ``bp_loc``:`lldb.SBBreakpointLocation` | |
+| | | |
+| | | ``frame`` is the stack frame that hit this location. |
+| | | |
+| | | |
+| | | ``bp_loc`` is the real location that was hit. |
+| | | |
+| | | Return either the facade location that you want to consider hit on this stop, or None if you don't consider |
+| | | any of your facade locations to have been hit. |
++------------------------------+----------------------------------------+------------------------------------------------------------------------------------------------------------------+
+| ``get_location_description`` | ``bp_loc``:`lldb.SBBreakpointLocation` | Use this to provide a helpful description of each facade location. |
+| | ``desc_level``:`lldb.DescriptionLevel` | |
+| | | ``bp_loc`` is the facade location to describe. |
+| | | |
+| | | |
+| | | ``desc_level`` is the level of description requested. The Brief description is printed when the location is |
+| | | hit. Full is printed for `break list` and Verbose for `break list -v`. |
++------------------------------+----------------------------------------+------------------------------------------------------------------------------------------------------------------+
+
Using the python API' to create custom stepping logic
-----------------------------------------------------
diff --git a/lldb/include/lldb/API/SBBreakpoint.h b/lldb/include/lldb/API/SBBreakpoint.h
index 18ed3e7226d3b..3bf87a1b7cc92 100644
--- a/lldb/include/lldb/API/SBBreakpoint.h
+++ b/lldb/include/lldb/API/SBBreakpoint.h
@@ -153,9 +153,16 @@ class LLDB_API SBBreakpoint {
/// fails, e.g. when there aren't enough hardware resources available.
lldb::SBError SetIsHardware(bool is_hardware);
- // Can only be called from a ScriptedBreakpointResolver...
+ /// Adds a location to the breakpoint at the address passed in.
+ /// Can only be called from a ScriptedBreakpointResolver...
SBError
AddLocation(SBAddress &address);
+ /// Add a "Facade location" to the breakpoint. This returns the Facade
+ /// Location that was added, which you can then use in
+ /// get_location_description and was_hit in your breakpoint resolver.
+ /// Can only be called from a ScriptedBreakpointResolver.
+ SBBreakpointLocation
+ AddFacadeLocation();
SBStructuredData SerializeToStructuredData();
diff --git a/lldb/include/lldb/API/SBBreakpointLocation.h b/lldb/include/lldb/API/SBBreakpointLocation.h
index fa823e2b518ac..deda4970cd0ed 100644
--- a/lldb/include/lldb/API/SBBreakpointLocation.h
+++ b/lldb/include/lldb/API/SBBreakpointLocation.h
@@ -24,6 +24,7 @@ class SWIGBridge;
namespace lldb {
class LLDB_API SBBreakpointLocation {
+ friend class lldb_private::ScriptInterpreter;
public:
SBBreakpointLocation();
diff --git a/lldb/include/lldb/API/SBFrame.h b/lldb/include/lldb/API/SBFrame.h
index e4bbcd5ddcd9c..4abb44b4bb0e5 100644
--- a/lldb/include/lldb/API/SBFrame.h
+++ b/lldb/include/lldb/API/SBFrame.h
@@ -225,7 +225,8 @@ class LLDB_API SBFrame {
friend class SBInstruction;
friend class SBThread;
friend class SBValue;
-
+
+ friend class lldb_private::ScriptInterpreter;
friend class lldb_private::python::SWIGBridge;
friend class lldb_private::lua::SWIGBridge;
diff --git a/lldb/include/lldb/Breakpoint/Breakpoint.h b/lldb/include/lldb/Breakpoint/Breakpoint.h
index 26a5e901a0d7e..2bef6a919d878 100644
--- a/lldb/include/lldb/Breakpoint/Breakpoint.h
+++ b/lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -248,6 +248,23 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
/// Returns a pointer to the new location.
lldb::BreakpointLocationSP AddLocation(const Address &addr,
bool *new_location = nullptr);
+ /// Add a `facade` location to the breakpoint's collection of facade locations.
+ /// This is only meant to be called by the breakpoint's resolver.
+ /// Facade locations are placeholders that a scripted breakpoint can use to
+ /// represent the stop locations provided by the breakpoint. The scripted
+ /// breakpoint should record the id of the facade location, and provide
+ /// the description of the location in the GetDescription method
+ /// To emulate hitting a facade location, the breakpoint's WasHit should
+ /// return the ID of the facade that was "hit".
+ ///
+ /// \param[out] new_location
+ /// Set to \b true if a new location was created, to \b false if there
+ /// already was a location at this Address.
+ /// \return
+ /// Returns a pointer to the new location.
+ lldb::BreakpointLocationSP AddFacadeLocation();
+
+ lldb::BreakpointLocationSP GetFacadeLocationByID(lldb::break_id_t);
/// Find a breakpoint location by Address.
///
@@ -268,27 +285,36 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
/// there is no breakpoint location at that address.
lldb::break_id_t FindLocationIDByAddress(const Address &addr);
- /// Find a breakpoint location for a given breakpoint location ID.
+ /// Find a breakpoint location for a given breakpoint location ID. If there
+ /// are Facade Locations in the breakpoint, the facade locations will be
+ /// searched instead of the "real" ones.
///
/// \param[in] bp_loc_id
/// The ID specifying the location.
+ ///
+ /// \param[in] use_facade
+ /// If \b true, then prefer facade locations over "real" ones if they exist.
+ ///
/// \return
/// Returns a shared pointer to the location with ID \a bp_loc_id. The
/// pointer
/// in the shared pointer will be nullptr if there is no location with that
/// ID.
- lldb::BreakpointLocationSP FindLocationByID(lldb::break_id_t bp_loc_id);
+ lldb::BreakpointLocationSP FindLocationByID(lldb::break_id_t bp_loc_id, bool use_facade = true);
/// Get breakpoint locations by index.
///
/// \param[in] index
/// The location index.
///
+ /// \param[in] use_facade
+ /// If \b true, then prefer facade locations over "real" ones if they exist.
+ ///
/// \return
/// Returns a shared pointer to the location with index \a
/// index. The shared pointer might contain nullptr if \a index is
/// greater than then number of actual locations.
- lldb::BreakpointLocationSP GetLocationAtIndex(size_t index);
+ lldb::BreakpointLocationSP GetLocationAtIndex(size_t index, bool use_facade = true);
/// Removes all invalid breakpoint locations.
///
@@ -409,9 +435,12 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
/// Return the number of breakpoint locations that have resolved to actual
/// breakpoint sites.
///
+ /// \param[in] use_facade
+ /// If \b true, then prefer facade locations over "real" ones if they exist.
+ ///
/// \return
/// The number locations resolved breakpoint sites.
- size_t GetNumResolvedLocations() const;
+ size_t GetNumResolvedLocations(bool use_facade = true) const;
/// Return whether this breakpoint has any resolved locations.
///
@@ -421,9 +450,12 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
/// Return the number of breakpoint locations.
///
+ /// \param[in] use_facade
+ /// If \b true, then prefer facade locations over "real" ones if they exist.
+ ///
/// \return
/// The number breakpoint locations.
- size_t GetNumLocations() const;
+ size_t GetNumLocations(bool use_facade = true) const;
/// Put a description of this breakpoint into the stream \a s.
///
@@ -529,6 +561,20 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
m_name_list.erase(name_to_remove);
}
+ enum TypeDisplay {
+ eDisplayFacade = 1,
+ eDisplayReal = 1 << 1,
+ eDisplayHeader = 1 << 2
+ };
+
+ void GetDescriptionForType(Stream *s, lldb::DescriptionLevel level,
+ uint8_t display_type, bool show_locations);
+
+ bool HasFacadeLocations() {
+ return m_facade_locations.GetSize() != 0;
+ }
+
+
public:
bool MatchesName(const char *name) {
return m_name_list.find(name) != m_name_list.end();
@@ -657,6 +703,8 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
BreakpointOptions m_options; // Settable breakpoint options
BreakpointLocationList
m_locations; // The list of locations currently found for this breakpoint.
+ BreakpointLocationCollection m_facade_locations;
+
std::string m_kind_description;
bool m_resolve_indirect_symbols;
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index ab2e5e170559d..c4fa9bec51aae 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -38,6 +38,12 @@ namespace lldb_private {
class BreakpointLocation
: public std::enable_shared_from_this<BreakpointLocation> {
+ friend class BreakpointSite;
+ friend class BreakpointLocationList;
+ friend class Breakpoint;
+ friend class Process;
+ friend class StopInfoBreakpoint;
+
public:
~BreakpointLocation();
@@ -55,16 +61,39 @@ class BreakpointLocation
Target &GetTarget();
+ /// This is a programmatic version of a breakpoint "condition". When a
+ /// breakpoint is hit, WasHit will get called before the synchronous ShouldStop
+ /// callback is run, and if it returns an empty BreakpointLocationSP, lldb will
+ /// act as if that breakpoint wasn't hit.
+ ///
+ /// \param[in] context
+ /// The context at the stop point
+ ///
+ /// \return
+ /// This will return the breakpoint location that was hit on this stop.
+ /// If there was no facade location this will be the original location.
+ /// If the shared pointer is empty, then we'll treat it as if the
+ /// breakpoint was not hit.
+ lldb::BreakpointLocationSP WasHit(StoppointCallbackContext *context);
+
/// Determines whether we should stop due to a hit at this breakpoint
/// location.
///
/// Side Effects: This may evaluate the breakpoint condition, and run the
/// callback. So this command may do a considerable amount of work.
///
+ /// \param[in] context
+ /// The context at the stop point
+ ///
+ /// \param[out] facade_loc_sp
+ /// If this stop should be attributed not to the location that was hit, but
+ /// to a facade location, it will be returned in this facade_loc_sp.
+ ///
/// \return
/// \b true if this breakpoint location thinks we should stop,
/// \b false otherwise.
- bool ShouldStop(StoppointCallbackContext *context);
+ bool ShouldStop(StoppointCallbackContext *context,
+ lldb::BreakpointLocationSP &facade_loc_sp);
// The next section deals with various breakpoint options.
@@ -292,11 +321,6 @@ class BreakpointLocation
}
protected:
- friend class BreakpointSite;
- friend class BreakpointLocationList;
- friend class Process;
- friend class StopInfoBreakpoint;
-
/// Set the breakpoint site for this location to \a bp_site_sp.
///
/// \param[in] bp_site_sp
@@ -346,9 +370,11 @@ class BreakpointLocation
// Constructors and Destructors
//
// Only the Breakpoint can make breakpoint locations, and it owns them.
-
/// Constructor.
///
+ /// \param[in] loc_id
+ /// The location id of the new location.
+ ///
/// \param[in] owner
/// A back pointer to the breakpoint that owns this location.
///
@@ -359,10 +385,25 @@ class BreakpointLocation
/// The thread for which this breakpoint location is valid, or
/// LLDB_INVALID_THREAD_ID if it is valid for all threads.
///
- BreakpointLocation(lldb::break_id_t bid, Breakpoint &owner,
+ BreakpointLocation(lldb::break_id_t loc_id, Breakpoint &owner,
const Address &addr, lldb::tid_t tid,
bool check_for_resolver = true);
+ /// This is the constructor for locations with no address. Currently this is
+ /// just used for Facade locations.
+ ///
+ /// \param[in] loc_id
+ /// The location id of the new location.
+ ///
+ /// \param[in] owner
+ /// A back pointer to the breakpoint that owns this location.
+ ///
+ ///
+public:
+ BreakpointLocation(lldb::break_id_t loc_id, Breakpoint &owner);
+ bool IsValid() const { return m_is_valid; }
+ bool IsFacade() const { return m_is_facade; }
+private:
// Data members:
bool m_should_resolve_indirect_functions;
bool m_is_reexported;
@@ -390,6 +431,17 @@ class BreakpointLocation
/// location was given somewhere in the virtual inlined call stack since the
/// Address always resolves to the lowest entry in the stack.
std::optional<LineEntry> m_preferred_line_entry;
+ bool m_is_valid = true; /// Because Facade locations don't have sites
+ /// we can't use the presence of the site to mean
+ /// this breakpoint is valid, but must manage
+ /// the state directly.
+ bool m_is_facade = false; /// Facade locations aren't directly triggered
+ /// and don't have a breakpoint site. They are
+ /// a useful ...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| m_name_list.erase(name_to_remove); | ||
| } | ||
|
|
||
| enum TypeDisplay { |
There was a problem hiding this comment.
Can we add a comment for this ?
There was a problem hiding this comment.
I also changed the enum name to DisplayType. I intended this to read as "Type of Display" but that's not so obvious.
Comment on lines
12
to
14
| self.sym_name = extra_args.GetValueForKey("symbol").GetStringValue(100) | ||
| self.num_locs = extra_args.GetValueForKey("num_locs").GetIntegerValue(5) | ||
| self.loc_to_miss = extra_args.GetValueForKey("loc_to_miss").GetIntegerValue(10000) |
There was a problem hiding this comment.
I believe you can do this with @kastiglione recent change.
Suggested change
| self.sym_name = extra_args.GetValueForKey("symbol").GetStringValue(100) | |
| self.num_locs = extra_args.GetValueForKey("num_locs").GetIntegerValue(5) | |
| self.loc_to_miss = extra_args.GetValueForKey("loc_to_miss").GetIntegerValue(10000) | |
| self.sym_name = str(extra_args.GetValueForKey("symbol")) | |
| self.num_locs = int(extra_args.GetValueForKey("num_locs")) | |
| self.loc_to_miss = int(extra_args.GetValueForKey("loc_to_miss")) |
| self.base_sym = None | ||
| self.facade_locs = [] | ||
| self.facade_locs_desc = [] | ||
| self.cur_facade_loc = 1 |
There was a problem hiding this comment.
Is that because the default location id is 1 and not 0 ?
There was a problem hiding this comment.
Locations are 1 based, not 0 based. As I say around here somewhere, going from 1 based locations to 0 based list indices was driving me nuts, so I made all the arrays 1 based by virtue of filling the first element with placeholders...
Comment on lines
35
to
36
| if self.cur_facade_loc == 5: | ||
| self.cur_facade_loc = 1 |
There was a problem hiding this comment.
I guess you're doing this because you expect at most 4 locations, correct ?
There was a problem hiding this comment.
Exactly. I actually pass the number of locations to make into the breakpoint init, so I could use that to symbolically represent the terminus instead. Let me do that, that will be cleaner.
| self.num_locs = extra_args.GetValueForKey("num_locs").GetIntegerValue(5) | ||
| self.loc_to_miss = extra_args.GetValueForKey("loc_to_miss").GetIntegerValue(10000) | ||
|
|
||
| def __callback__(self, sym_ctx): |
There was a problem hiding this comment.
I'm not sure when this __callback__ gets called. Also, this class implements __callback__ which is supposed to be a python "dunder" method but I think you meant to implement __call__. __callback__ doesn't exists in the python standard library, so we should either rename this to __call__ or remove the "dunder" if we're making our own callback function.
There was a problem hiding this comment.
__callback__ was what I called the "resolver do your work" function in ScriptedBreakpointResolvers. This part isn't new. It's documented in python-reference.rst and has been in place for a while now. I don't think we can change it.
Anyway, that's not germane to this patch in particular, and it's big enough already...
There was a problem hiding this comment.
I like how you give a conceptual explanation of the feature here but I think it would be great we could add a little tutorial to show what workflow this feature enables. I'd just take your test example and run the user through it.
I also think we have to add a ScriptedBreakpointResolver base class to the lldb python module and document it like we do for other Scripted Extensions, that way you could just link to the auto-generated extension page so it always stay up-to-date.
Lastly, I think we should break down the python-reference page into subpages targeting specific topics. I think that'd improve discoverability and readability. I can take of that part :)
There was a problem hiding this comment.
Adding a ScriptedBreakpointResolver Python base class is a general improvement to the ScriptedBreakpointResolver system, and while that seems like a fine improvement, it isn't germane to this patch, nor does this patch make it any easier or harder to add that python base class. That seems more appropriate for a follow-on patch.
There was a problem hiding this comment.
Let's do the base class as a follow-up
|
Huh, that Linux run failed building libcxx somewhere, which this patch doesn't touch. Maybe I got unlucky with the state of the tree when I made the PR. Let's see if updating helps... |
|
Oh, that was confusing, There was a build error in lldb some but the build went on to finish libcxx before stopping so the error was hard to see... Apparently the linux build has -Werror,-Wdangling-else: 2025-09-11T21:56:34.5604062Z /home/gha/actions-runner/_work/llvm-project/llvm-project/lldb/source/Breakpoint/Breakpoint.cpp:1049:7: error: add explicit braces to avoid dangling else [-Werror,-Wdangling-else] |
Also rename it to DisplayType, which makes more obvious sense.
|
Ismail helpfully moved the docs around so I have to re-introduce the docs I added in the new location. |
JDevlieghere
reviewed
Oct 7, 2025
Comment on lines
435
to
443
| bool m_is_valid = true; /// Because Facade locations don't have sites | ||
| /// we can't use the presence of the site to mean | ||
| /// this breakpoint is valid, but must manage | ||
| /// the state directly. | ||
| bool m_is_facade = false; /// Facade locations aren't directly triggered | ||
| /// and don't have a breakpoint site. They are | ||
| /// a useful fiction when you want to represent | ||
| /// the stop location as something lldb can't | ||
| /// naturally stop at. |
There was a problem hiding this comment.
Nit: please put these above the relevant lines. Trailing comments don't make sense with our column limit, plus the members above don't do that either.
lldb/source/API/SBBreakpoint.cpp
Outdated
Comment on lines
572
to
574
| if (!bkpt_sp) { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (!bkpt_sp) { | |
| return {}; | |
| } | |
| if (!bkpt_sp) | |
| return {}; |
Comment on lines
532
to
533
| else | ||
| return llvm::createStringError("no breakpoint site to clear"); |
|
|
||
| bool is_resolved = IsResolved(); | ||
| bool is_hardware = is_resolved && m_bp_site_sp->IsHardware(); | ||
| // Scripted breakpoint are currently always resolved. Does this seem right? |
| lldb::BreakpointLocationSP bp_loc_sp) { | ||
| if (m_interface_sp) | ||
| return m_interface_sp->WasHit(frame_sp, bp_loc_sp); | ||
|
|
There was a problem hiding this comment.
Nit: GetLocationDescription doesn't have a newline here. Let's pick one and stick with it.
Comment on lines
111
to
113
| if (frame.m_opaque_sp) { | ||
| return frame.m_opaque_sp->GetFrameSP(); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (frame.m_opaque_sp) { | |
| return frame.m_opaque_sp->GetFrameSP(); | |
| } | |
| if (frame.m_opaque_sp) | |
| return frame.m_opaque_sp->GetFrameSP(); |
medismailben
approved these changes
Oct 7, 2025
There was a problem hiding this comment.
We discussed offline and @jimingham said he'll implement a documented base class for this as a follow-up. LGTM!
svkeerthy
pushed a commit
that referenced
this pull request
Oct 9, 2025
This patch adds the notion of "Facade" locations which can be reported from a ScriptedResolver instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations. I apologize in advance for the size of the patch. Almost all of what's here was necessary to (a) make the feature testable and (b) not break any of the current behavior. The motivation for this feature is given in the "Providing Facade Locations" section that I added to the python-reference.rst so I won't repeat it here. rdar://152112327
DharuniRAcharya
pushed a commit
to DharuniRAcharya/llvm-project
that referenced
this pull request
Oct 13, 2025
This patch adds the notion of "Facade" locations which can be reported from a ScriptedResolver instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations. I apologize in advance for the size of the patch. Almost all of what's here was necessary to (a) make the feature testable and (b) not break any of the current behavior. The motivation for this feature is given in the "Providing Facade Locations" section that I added to the python-reference.rst so I won't repeat it here. rdar://152112327
akadutta
pushed a commit
to akadutta/llvm-project
that referenced
this pull request
Oct 14, 2025
This patch adds the notion of "Facade" locations which can be reported from a ScriptedResolver instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations. I apologize in advance for the size of the patch. Almost all of what's here was necessary to (a) make the feature testable and (b) not break any of the current behavior. The motivation for this feature is given in the "Providing Facade Locations" section that I added to the python-reference.rst so I won't repeat it here. rdar://152112327
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 14, 2025
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR llvm#158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop.
jimingham
added a commit
that referenced
this pull request
Oct 20, 2025
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR #158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop.
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 21, 2025
This patch adds the notion of "Facade" locations which can be reported from a ScriptedResolver instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations. I apologize in advance for the size of the patch. Almost all of what's here was necessary to (a) make the feature testable and (b) not break any of the current behavior. The motivation for this feature is given in the "Providing Facade Locations" section that I added to the python-reference.rst so I won't repeat it here. rdar://152112327 (cherry picked from commit 36bce68)
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 21, 2025
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR llvm#158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop. (cherry picked from commit c9124a1)
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 21, 2025
This patch adds the notion of "Facade" locations which can be reported from a ScriptedResolver instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations. I apologize in advance for the size of the patch. Almost all of what's here was necessary to (a) make the feature testable and (b) not break any of the current behavior. The motivation for this feature is given in the "Providing Facade Locations" section that I added to the python-reference.rst so I won't repeat it here. rdar://152112327 (cherry picked from commit 36bce68)
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 21, 2025
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR llvm#158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop. (cherry picked from commit c9124a1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds the notion of "Facade" locations which can be reported from a ScriptedResolver instead of the actual underlying breakpoint location for the breakpoint. Also add a "was_hit" method to the scripted resolver that allows the breakpoint to say which of these "Facade" locations was hit, and "get_location_description" to provide a description for the facade locations.
I apologize in advance for the size of the patch. Almost all of what's here was necessary to (a) make the feature testable and (b) not break any of the current behavior.
The motivation for this feature is given in the "Providing Facade Locations" section that I added to the python-reference.rst so I won't repeat it here.
rdar://152112327