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

[Roland Westrelin]

Hi Andrew,

Thanks for taking a look at this. See my answer below.

> 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);

I tried to use a code pattern that is used elsewhere in c2:

LoadLNode* LoadLNode::make_atomic(Node* ctl, Node* mem, Node* adr, const TypePtr* adr_type, const Type* rt, MemOrd mo) {
  bool require_atomic = true;
  return new LoadLNode(ctl, mem, adr, adr_type, rt->is_long(), mo, require_atomic);
}

I understand does_not_depend_only_on_test is confusing. I use that pattern because other options didn’t feel better and I can’t say I find it great.

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?

Roland.

> 
> 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)