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

[Andrew Dinn]

Hi Roland,

On 16/04/15 12:57, Roland Westrelin wrote:
> http://cr.openjdk.java.net/~roland/8077504/webrev.00/
> 
> Because we consider that all loads that have their control set only
> depend on the test that immediately dominate them (and if we move the
> test, the load can follow the test), loads from unsafe intrinsics can
> be moved so they don’t depend on conditions that keep them valid
> anymore (see test cases). The fix I propose is to add a
> _depends_only_on_test flag to LoadNodes so when we construct a
> LoadNode for an unsafe load we can record that it shouldn’t be
> moved.

I must admit I was a tad confused by your use of variable
does_not_depend_only_on_test at library_call.cpp:2635. I understand that
at this point you are trying to emphasise that this call gets passed
false where other calls get passed true. So, in the call we see

  Node* p = make_load(control(), adr, value_type, type, adr_type, mo,
                      does_not_depend_only_on_test, is_volatile);

which is fine when you understand what is going on. However, the
preceding assignment:

  bool does_not_depend_only_on_test = false;

makes it look like you are suggesting that this case does depend only on
the test.

Would it not be clearer if you signalled what is happening by avoiding
the bool var declarations and instead tagging the calls with an
explanatory comment as follows:

  Node* p = make_load(control(), adr, value_type, type, adr_type, mo,
                      false, //does_not_depend_only_on_test
                      is_volatile);

vs

  Node* p = make_load(control(), adr, value_type, type, adr_type, mo,
                      true, //depends_only_on_test
                      is_volatile);

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in UK and Wales under Company Registration No. 3798903
Directors: Michael Cunningham (USA), Matt Parson (USA), Charlie Peters
(USA), Michael O'Neill (Ireland)