On Thu, 20 May 2021 05:14:43 GMT, David Holmes <[email protected]> wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Review fixes
>> - Merge branch 'master' into 8265753
>> - Fixes for Dan
>> - Merge branch 'master' into 8265753
>> - Removed manual transitions
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 236:
>
>> 234:
>> 235: template <typename PRE_PROC>
>> 236: class ThreadBlockInVMPreprocess : public ThreadStateTransition {
>
> Can we add a comment before this template definition please:
>
> // Perform a transition to _thread_blocked and take a call-back to be
> executed before
> // SafepointMechanism::process_if_requested when returning to the VM. This
> allows us
> // to perform an "undo" action if we might block processing a
> safepoint/handshake operation
> // (such as thread suspension).
Fixed
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 245:
>
>> 243: // Once we are blocked vm expects stack to be walkable
>> 244: thread->frame_anchor()->make_walkable(thread);
>> 245: thread->set_thread_state(_thread_blocked);
>
> This is a pre-existing issue. Everywhere we call make_walkable and then call
> plain set_thread_state (other than on PPC/Aarch64 which do a release_store)
> we are at risk of the thread_state update being reordered with stores related
> to making the stack walkable. Potentially allowing the VMThread (or other
> thread) to walk a stack that is not yet walkable! The original
> ObjectMonitor::enter code was aware of this:
> `current->frame_anchor()->make_walkable(current);`
> `// Thread must be walkable before it is blocked.`
> `// Read in reverse order.`
> `OrderAccess::storestore();`
> `for (;;) {`
> ` current->set_thread_state(_thread_blocked);`
Good catch, fixed.
> src/hotspot/share/runtime/objectMonitor.hpp line 309:
>
>> 307: protected:
>> 308: ObjectMonitor* _om;
>> 309: bool _exited;
>
> For consistency with raw monitor code this would be _om_exited
Fixed
-------------
PR: https://git.openjdk.java.net/jdk/pull/3875
