RFR: 8264123: add ThreadsList.is_valid() support

[David Holmes]

On Mon, 29 Mar 2021 21:48:23 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

> ThreadsLists need an is_valid() function and checks in various
> places to help catch bugs where a ThreadsList is dangling.
> 
> Other minor changes:
> - change raw `_threads_hazard_ptr` access to used `get_threads_hazard_ptr()`.
> - `get_threads_hazard_ptr()` should be `const`.
> - fix a couple of old typos.

Hi Dan,

I'm not quite seeing how this single-field checks constitutes a useful validity check - sorry.

Any why is this all product code rather than debug?

Thanks,
David

src/hotspot/share/runtime/threadSMR.cpp line 285:

> 283:     }
> 284: 
> 285:     guarantee(ThreadsList::is_valid(current_list), "current_list="

Why a guarantee and not an assert? What is the cost of doing this check?

src/hotspot/share/runtime/threadSMR.cpp line 319:

> 317:       // We only validate hazard_ptrs that are not tagged since a tagged
> 318:       // hazard ptr can be deleted at any time.
> 319:       guarantee(ThreadsList::is_valid(hazard_ptr), "hazard_ptr=" INTPTR_FORMAT

Same query about guarantee versus assert

src/hotspot/share/runtime/threadSMR.cpp line 383:

> 381:     if (hazard_ptr == NULL) return;
> 382:     // If the hazard ptr is unverified, then ignore it since it could
> 383:     // be deleted at any time now.

Why the difference in comment compared to line 317/318 when the subsequent checks are identical?

src/hotspot/share/runtime/threadSMR.hpp line 209:

> 207:   bool includes(const JavaThread * const p) const;
> 208: 
> 209:   static bool is_valid(ThreadsList* list) { return list->_magic == THREADS_LIST_MAGIC; }

This seems like a necessary, but not sufficient, check for validity. Unless we zap memory when these things are freed I don't see how this ensures validity.

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

PR: https://git.openjdk.java.net/jdk/pull/3255