[14] RFR(M): 8229496: SIGFPE (division by zero) in C2 OSR compiled method

[Doerr, Martin]

Hi Tobias,

>> I thought we were using implicit div by zero checks on x86 (SIGFPE gets
>> caught in signal handler on AMD64). So was the
>> continuation_for_implicit_div0_exception not found in
>> ImplicitExceptionTable? Or why did the VM crash?
> 
> Yes, same as for implicit null checks, that's decided here:
> http://hg.openjdk.java.net/jdk/jdk/file/2172fd713350/src/hotspot/share/op
> to/graphKit.cpp#l1312
> 
> I.e., we do an implicit null check if it doesn't happen frequently. However, in
> the failing case we
> always throw and therefore an explicit zero check is used.

Makes sense. Thanks.


>> I wonder if it's fine to use "if (lo1 >= 0) lo = 0" in the CastIINode /
>> CastLLNode. Shouldn't lo > 0 stay valid?
>> (Also in the existing CastIINode, but your usage is new.)
> 
> The type is intentionally worsened here. See:
> http://hg.openjdk.java.net/jdk/jdk/file/2172fd713350/src/hotspot/share/op
> to/convertnode.cpp#l277

Ok, it's a trade-off. Worse type information, but potentially better GCM opts.


> I'll refactor that code with https://bugs.openjdk.java.net/browse/JDK-
> 8230382

I appreciate that.


>> This issue sounds a little bit similar to
>> https://bugs.openjdk.java.net/browse/JDK-8220376
>> but I guess there's more work to be done for that.
> 
> In general, the problem is that C2's type system is not able to propagate the
> information that a
> value is "not zero". That's why I had to use TypeInt::INT for the CastII instead
> of something like
> TypeInt::NOT_NULL.
> 
> Without having looked at your test for 8220376, I would assume that with x !=
> 0, C2 merges the two
> checks (the one that is in the Java code and the one added for the div) and
> therefore the div is
> removed. That does not happen with x > 0.

You mean the cmp + uncommon trap is removed. Yes, that could be.


Thanks for your explanations. Your change looks good to me.

Best regards,
Martin