Provide ability to disallow watchpoints inside certain containers

[kpgriesser]

#1517 was required since some container types like std::queue invalidate references causing the debugger to crash or simply report stale values. A 'refresh' function was also provided as a way to improve performance by only refreshing the object from the current pwd down.

Besides the likely performance issues, refreshing all or part of the object map to support watchpoints on elements in these containers has the additional requirement of updated the references stored in the watchpoint objects. Until this is resolved, we should disallow elements in these containers from being watched.

To achieve this I added an 'isWatchable' function to the ObjectMap. The function normally returns true but returns false for types defined in serialize_adapter.h. Namely, std::stack, std::queue, std::priority_queue. Users can be advised to simple provide data members that capture data accesses and/or control for these types.

When traveling down the object map, if a non-watchable type is detected, that object is saved. The user then cannot set a watchpoint on any element below this level of hierarchy. When traversing up the hierarchy, this object pointer is cleared and watchpoints can then be set.

With this PR in place, we can start looking at options for handling watchpoints efficiently for these cases. Additionally, if testing uncovers other cases, we have a method to exclude them from watchpoint support.

[github-actions]

CLANG-FORMAT TEST - FAILED (on last commit):
Run > ./scripts/clang-format-test.sh using clang-format v20 to check formatting

[github-actions]

CLANG-FORMAT TEST - FAILED (on last commit):
Run > ./scripts/clang-format-test.sh using clang-format v20 to check formatting

[github-actions]

CLANG-FORMAT TEST - PASSED

[sst-autotester]

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

[github-actions]

CLANG-FORMAT TEST - PASSED

[feldergast]

Probably need to add std::vector, std::unordered_map and std::unordered_set to the unwatchable list. All of these can reallocate on inserts and/or deletes. There may be others, but I'll need to look to see if the data addresses are stable for things like deques.

[leekillough]

The adapter classes std::stack, std::queue and std::priority_queue are not unique in their behavior of invalidating iterators/references to their elements, even though std::queue was the first test failure found. Just because std::queue was the first test failure found, should not cause undue focus on serialize_adapter.h.

All containers in serialize_insertable.h potentially have the same problem. Some, like std::map and std::multimap, are guaranteed not to invalidate iterators/references except to deleted elements. Others like std::vector may invalidate all iterators/references.

I would attempt to fix the issue which #1517 started addressing, rather than adding blanket rules against watchpoints in certain containers.

Please reference this page for iterator/reference invalidation rules.

[leekillough]

A test failure having to do with std::queue has caused undue focus on std::queue and related adapter types.

As was made clear in #1517, it is a more general issue of C++ container modification invalidating iterators/references to their elements. It has nothing to do with std::queue or serialize_adapter.h in particular, which is a red herring.

So I do not agree with this PR, which singles out certain classes which have failed tests in the past, rather than addressing the real underlying issue.

[kpgriesser]

Thanks for the feedback. I should clarify that this isn't intended, by any stretch of the imagination, to be a long term solution for watchpoints. Instead, it is a temporary patch to help us avoid ( and focus on ) a few known problematic cases.

[leekillough]

I think that only std::list, std::forward_list, std::map and std::multimap can meet your expectations.

[kpgriesser]

Converting this back to draft

[leekillough]

I would suggest that type traits be defined and used to query if a type should be excluded, because type traits can be evaluated at compile-time (better optimization as well as the ability to give compile-time errors if desired if a condition is true), and because their trait "specialization" definitions can be placed in the serialize/impl/*.h files which directly pertain to the types being excluded, so that it does not have to be handled in a central place like ObjectMap.

In serialize/impl/*.h there are many of these "traits" defined. Many of them have names starting with is_ to test if a condition is true for a type (or even a template without supplying its arguments).

Refer to https://en.cppreference.com/w/cpp/header/type_traits.html for the standard type traits, and define yours like I defined them in serialize/impl/*.h. By convention, traits are template classes which have either a type or value member, indicating a type derived from the trait's arguments, or a value like bool or size_t derived from the arguments, and shortened names of these trait members are indicated by _t or _v after the trait class name so that typename trait<...>::type and trait<...>::value do not need to be used and trait_t<...> and trait_v<...> can be used instead.

Depending on the circumstances, it may make more sense to define the trait class (with the type or value member) by using templates and their specializations, and then define the shortened name with _t or _v suffix in terms of the traits class, or it might make more sense to define a _t or _v shortened name as a templated name first, and then make a wrapper of it to create the traits class (such as using std::bool_constant or std::integral_constant for _v).

Traits can be specialized for subsets of their arguments. For adapter classes std::stack, std::queue, std::priority_queue, you can define a specialization which makes the trait true if and only if either the adapter class's element type or container type is forbidden, such as:

// The shortened name boolean trait template -- this should be declared somewhere which is compiled first, like serialize.h
template<typename T>
constexpr bool is_watchpoint_disallowed_v = false;

// The trait name without _v at the end, defined in terms of the shortened _v trait name
template<typename T>
using is_watchpoint_disallowed = std::bool_constant<is_watchpoint_disallowed_v<T>>;

// Specialization for std::queue<T, C> defined as the OR of whether T or C is disallowed
template<typename T, typename C>
constexpr bool is_watchpoint_disallowed_v<std::queue<T, C>> = std::disjunction_v<is_watchpoint_disallowed<T>, is_watchpoint_disallowed<C>>;

Here there is not a blanked rule against disallowing watchpoints on all std::queue, but disallowing it only if it is disallowed for the element type or container type.

serialize_adapter.h has been abstracted out so that the same lines of code apply to all three of std::stack, std::queue, and std::priority_queue. There is an is_adapter_v trait defined, and then the rest of the code is enabled if and only if that trait is true. It would also be possible to write a is_watchpoint_disallowed_v in terms of it, but you must add an extra argument to use SFINAE techniques, like this:

template<typename T, typename = void>
constexpr bool is_watchpoint_disallowed_v = false;

template<typename T, typename C, template<typename, typename> class A>
constexpr bool is_watchpoint_disallowed_v<A<T,C>, std::enable_if_t<is_adapter_v<A<T,C>>>> = std::disjunction_v<is_watchpoint_disallowed<T>, is_watchpoint_disallowed<C>>;

The last line (or something like it) would be the only thing which needs to be added to serialize_adapter.h to specialize the is_watchpoint_disallowed_v trait to cover all adapters, no what their element types and container types, and it would be done locally to the same code which serializes the adapters, so you don't need to put it in ObjectMap or the debugger. serialize_adapter.h is expertly aware of adapter classes, and is where you want to specialize traits or other things which may depend on those adapters.

---

My preferred solution would be to not ban watchpoints outright, but to try to find a solution to the problem of refreshing the ObjectMap when the iterators/references might have been invalidated.

If you're concerned about performance, then since references to some elements of all containers' can be invalidated by at least one operation on the container (such as deletion of the element), then a warning about possibly long debugging time should be printed if a watchpoint is made on a container or its elements:

template<typename T, template<typename...> class... CONTAINERS>
constexpr bool matches_some_template_v = std::disjunction_v<is_same_type_template<T, CONTAINERS>...>;

template<typename T>
constexpr bool is_container_v = matches_some_template_v<T,  std::vector, std::map, std::multimap, std::deque, std::unordered_map, std::unordered_multimap, std::set, std::unordered_set, std::multiset, std::unordered_multiset, std::list, std::forward_list, std::stack, std::queue, std::priority_queue>;

This simple definition pair compares a type T to see whether it is an instance of one of many containers or adapter classes, by using the already-defined is_same_type_template trait (defined in serialize_utility.h). An extra template is required so that a list of candidate templates can be compared against in a compact notation. The std::disjunction_v is short-circuiting.

It is a simple compile-time test which can tell whether a type is an instance of one those containers/adapters. If you want to make it a compile-time, error, you can use static_assert( !is_container_v<T>, "containers not allowed in watchpoints"); Or you can make it a run-time decision, and have the debugger print an error message.

[leekillough]

Also note that all of this may be redundant.

SST already has an ObjectMap::isContainer() method, which may be sufficient. Even ObjectMapArray, which has a variable runtime size, can invalidate references to its elements if its runtime size variable is decreased, so the same issue remains with it.

Note that std::array<T,N> and T[N] are fixed in size, and never invalidate their elements. I seem to recall that I moved them to ObjectMapFundamental instead of ObjectMapContainer for that very reason.

So obj->isContainer() is probably all you need, and it already exists!!! There's no need for new methods or exotic template traits!!!

[kpgriesser]

Thanks Lee! Really appreciate the time and energy you've put into this-
Ken

[feldergast]

I agree with Lee that any container with variable size could have an issue with invalidating elements, but the issue of "watchable" goes so much deeper than containers. Really, any pointer, whether stored in a container or not, can be "invalidated" by being deleted somewhere along the way, and any WatchPoint that tries to access it after being deleted risks a segfault. I think we'll need to rethink the ObjectMaps for pointer types. We may need to keep the address to where the pointer is stored, add a note that anything that gets deleted should have it's pointer to set to nullptr, and then check that the value of the pointer is nullptr before dereferencing it. This, of course, won't catch all cases, but it could catch a fair number of them and avoid segfaults in those cases. We need to have a deep discussion about watchability and the potential pitfalls. I'm sure there are still cases we haven't thought about yet.