RFR(S) 8077504: Unsafe load can loose control dependency and cause crash

[Roland Westrelin]

My change is broken.
GraphKit::make_load() takes 2 boolean arguments: bool depends_only_on_test = true, bool require_atomic_access = false)
and Parse::do_get_xxx() passes the value needs_atomic_access as argument depends_only_on_test instead of require_atomic_access. That goes unnoticed because of the default values. It’s the exact reason I didn’t use default arguments in my first webrev. It’s too easy to make a mistake and not notice it.
If we keep default arguments, one way to have help from the compiler would be to use a 2 value enum:

  enum DependsOnlyOnTest {
    DoesDependOnlyOnTest,
    DoesNotDependOnlyOnTest
  };

and use it to pass parameters around. The _depends_only_on_test LoadNode field would still be a boolean. Is this acceptable?

Roland.


> On May 18, 2015, at 10:04 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> 
>>>> 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?
>> 
>> Okay. Add comment to _depends_only_on_test field why we don't use it in hash().
> 
> I added the comment. Thanks for the review.
> I had the change go through a performance run to be safe and I found no regression so I’m pushing this.
> 
> Roland.
> 
>> 
>> Thanks,
>> Vladimir
>> 
>>> 
>>> 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.
>>> 
>>> Roland.
>>> 
>>> 
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> 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:
>>>>> 
>>>>> http://cr.openjdk.java.net/~roland/8077504/webrev.01/
>>>>> 
>>>>> Roland.
>>>>> 
>>> 
>