RFR: 8355960: JvmtiAgentList::Iterator dtor double free with -fno-elide-constructors

[Alex Menkov]

On Wed, 2 Jul 2025 07:16:57 GMT, David Holmes <dholmes at openjdk.org> wrote:

> I'm not very familiar with these aspects of C++ but this seems to be a very complex solution to what sounds in JBS to be a pretty straight-forward problem.

Straight-forward solution would be to add copy ctor for Iterator class which clones GrowableArray (allocates new instance and copies all elements).
But I'm not quite happy with the current implementation of AgentList and its iterator, so prefer to update it to keep entries in the original order (and iterator becomes much simpler).
Changes in the callers are required as they used `const JvmtiAgentList::Iterator` and const `next` method is removed.

> src/hotspot/share/prims/jvmtiAgentList.cpp line 35:
> 
>> 33: 
>> 34: JvmtiAgent* JvmtiAgentList::_head = nullptr;
>> 35: JvmtiAgent** JvmtiAgentList::_tail = nullptr;
> 
> Why is the tail a pointer to a pointer? Sorry I'm not understanding how your list is being managed here.
> 
> I'm trying to work out where you need acquire/release because I'm pretty sure it is missing where needed, but again I can't understand how you are actually constructing and using the list.

pointer to pointer because it contains not the pointer to the last agent in the list, but address of the `JvmtiAgent::_next` field.
`_tail` is for faster adding at the end of the list. Performance is not important here and number of agents is always low, so I think `_tail` can be removed to make the logic simpler. Will update the fix after testing completes

> src/hotspot/share/prims/jvmtiAgentList.cpp line 105:
> 
>> 103:   while (true) {
>> 104:     // set _tail to address of agent->_next
>> 105:     JvmtiAgent** tail = Atomic::load_acquire(&_tail);
> 
> You don't need acquire semantics here as you are not doing anything with the `_tail` pointer value other than use it in relation to cmpxchg which provides fully memory barriers.

ok, thanks

-------------

PR Comment: https://git.openjdk.org/jdk/pull/26083#issuecomment-3029654109
PR Review Comment: https://git.openjdk.org/jdk/pull/26083#discussion_r2181149878
PR Review Comment: https://git.openjdk.org/jdk/pull/26083#discussion_r2181149860