RFR(S) 8077504: Unsafe load can loose control dependency and cause crash
roland.westrelin at oracle.com
Mon Apr 27 07:52:43 UTC 2015
Thanks for the review, Vladimir. See below.
> I agree that we have to pass parameter to GraphKit::make_load().
> I thought we can avoid it for LoadNode::make() but it has transform for compressed oops. AARGH!
> Add comment to code in library_call.cpp why we set flag to false.
> BTW, should we modify LoadNode::hash() to include _depends_only_on_test and prevent igvning?
If the graph already has a non-pinned LoadNode and we add a pinned LoadNode with the same inputs, it’s safe for GVN to replace the pinned LoadNode by the non-pinned LoadNode, otherwise it wouldn’t be safe to have a non pinned LoadNode with those inputs in the first place. If the graph already has a pinned LoadNode and we add a non pinned LoadNode with the same inputs, it’s safe for GVN to replace the non-pinned LoadNode by the pinned LoadNode. It’s also suboptimal but better than 2 LoadNodes?
I guess we could change LoadNode::hash() and then use LoadNode::Identity/Ideal to make sure the pinned LoadNode is always replaced by the non-pinned LoadNode in the scenarios above but that sounds like extra complexity for something that, as far as we know, never happens in practice.
> On 4/24/15 1:03 AM, Roland Westrelin wrote:
>>> Vladimir suggested privately to set _depends_only_on_test to true in the constructor and then use an explicit call to a new a method set_depends_only_on_test() to set it to false in the rare cases where it’s needed. That feels better indeed. What do you think?
>> Actually, using a set_depends_only_on_test() method doesn’t work well. In LibraryCallKit::inline_unsafe_access() the node returned by make_load() may have been transformed already and we could call set_depends_only_on_test() on a node that doesn’t need to be pinned. The call to set_depends_only_on_test() would have to be in LoadNode::make(). I went with default parameters instead to keep the change small:
More information about the hotspot-compiler-dev