RFR: 8271348: Add stronger sanity check of thread state when polling for safepoint/handshakes [v2]

[David Holmes]

On Fri, 30 Jul 2021 00:12:40 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Here's the code block:
>> 
>>   uint64_t safepoint_id = SafepointSynchronize::safepoint_counter();
>>   // Check that we have a valid thread_state at this point
>>   switch(state) {
>>     case _thread_in_vm_trans:
>>     case _thread_in_Java:        // From compiled code
>>     case _thread_in_native_trans:
>>     case _thread_blocked_trans:
>>     case _thread_new_trans:
>> 
>>       // We have no idea where the VMThread is, it might even be at next safepoint.
>>       // So we can miss this poll, but stop at next.
>> 
>>       // Load dependent store, it must not pass loading of safepoint_id.
>>       thread->safepoint_state()->set_safepoint_id(safepoint_id); // Release store
>
> So I think @robehn was trying to separate these two lines of code:
> 
> `uint64_t safepoint_id = SafepointSynchronize::safepoint_counter();`
> 
> and
> 
> `thread->safepoint_state()->set_safepoint_id(safepoint_id);`
> 
> I think the same situation applies in the updated code, but it is harder
> to see in this GitHub view. Especially now that I've added all these
> comments.

Thanks and apologies for the digression. I'm assuming the intended required order is:
- load safepoint counter into safepoint_id
- load thread safepoint state
- store safepoint_id into safepoint_state

But the store is a release_store, so it is effectively preceded by a LoadStore|SoreStore barrier. So both loads must come before the store. The loads themselves could be reordered AFAICS but with no affect on correctness. So I remain unclear about the "load dependent store" comment actually relates to.
Oh well, not a problem for this PR.

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

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