RFR: 8261158: JVMState should not be shared between SafePointNodes

[Vladimir Ivanov]

On Mon, 10 May 2021 14:09:17 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> We often, for example with loop strip mining, clone `SafePointNodes` without cloning their `JVMState`, leading to the same state being shared by different nodes. With Valhalla, we then hit asserts when aggressively scalarizing inline types in safepoints during IGVN because `debug_end()` (`_endoff`) of the attached `JVMState` is larger than `SafePointNode::_max`. That happens because the same `JVMState` has been modified during scalarizing in another `SafePointNode`, the details are described in https://github.com/openjdk/valhalla/pull/322. I don't think `JVMStates` should be shared between safepoint nodes and added an assert to catch this.
> 
> The fix is to always shallow clone the `JVMState` when cloning a `SafepointNode`. Sometimes, a deep clone is required already by current code for `CallNodes` (see `CallNode::needs_clone_jvms`), I left that code as is.
> 
> Thanks,
> Tobias

Looks good.

src/hotspot/share/opto/callnode.hpp line 355:

> 353: 
> 354:   JVMState* jvms() const { return _jvms; }
> 355:   virtual bool needs_clone_jvms(Compile* C) { return false; }

Considering `clone_jvms()` always clones associated JVMS now, `needs_clone_jvms()` becomes confusing.
A variant that explicitly mentions deep copy is reqruired would be a better alternative.

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

Marked as reviewed by vlivanov (Reviewer).

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