Fwd: Missing deopt for MIN_VALUE / -1 case causes crash from unhandled SIGFPE

[Doug Simon]

HotSpot devs,

Is there some reason HotSpot doesn't (or shouldn't) handle a SIGFPE caused by Integer.MIN_VALUE/-1 explicitly?

-Doug


> Begin forwarded message:
> 
> From: Doug Simon <doug.simon at oracle.com>
> Subject: Re: Missing deopt for MIN_VALUE / -1 case causes crash from unhandled SIGFPE
> Date: 1 December 2017 at 12:09:55 CET
> To: Jackson Davis <jackson at jcdav.is>
> Cc: graal developers <graal-dev at openjdk.java.net>
> 
> Hi Jackson,
> 
> Awesome catch - thanks!
> 
> The reason we don't check for -1 is probably due to the fact Graal came from Maxine VM which handles a SIGFPE caused by Integer.MIN_VALUE/-1 explicitly:
> 
> https://github.com/dougxc/maxine/blob/master/com.oracle.max.vm.native/substrate/trap.c#L232
> 
> I'm not sure why HotSpot doesn't do this.
> 
> In any case, we obviously need to handle this properly in Graal and will take your PR as the starting point.
> 
> -Doug
> 
>> On 1 Dec 2017, at 09:17, Jackson Davis <jackson at jcdav.is> wrote:
>> 
>> I should add that this "fix" is based on the idea that letting the idiv
>> throw the SIGFPE is, in fact, the desired / expected behavior here. C2
>> handles this case by adding guards.
>> 
>> -Jackson
>> 
>> On Fri, Dec 1, 2017 at 12:04 AM, Jackson Davis <jackson at jcdav.is> wrote:
>> 
>>> One afternoon over Thanksgiving break I hacked together a very basic
>>> fuzzer for JVM JITs (https://github.com/jcdavis/kabur/) which much to my
>>> surprise actually managed to find an issue in Graal :)
>>> This minimal test case reliably crashes the JVM from an unhandled SIGFPE:
>>> 
>>> public class Crasher {
>>> public static int crash(int n, int d) {
>>>   return n/(d|1);
>>> }
>>> public static void main(String[] args) {
>>>   for(int i = 0; i < 100000; i++) {
>>>     crash(i, i+1);
>>>   }
>>>   crash(Integer.MIN_VALUE, -1);
>>> }
>>> }
>>> Ran with -XX:+UseJVMCICompiler -XX:-TieredCompilation
>>> -XX:-BackgroundCompilation (Tested with local jdk9 and graal master)
>>> 
>>> The source of this issue appears to be that IntegerDivRemNode.canDeoptimize
>>> only checks whether or not the denominator can be 0, not whether it can
>>> also be -1. Since d|1 is never 0, it doesn't generate the appropriate deopt
>>> info, so when MIN_VALUE/-1 causes a SIGFPE, the signal handler doesn't know
>>> what to do and dies. Adding the -1 check seems to make this to work as
>>> intended (AFAICT). I have a PR for a basic fix here:
>>> https://github.com/graalvm/graal/pull/266
>>> 
>>> Unfortunately this check breaks CountedLoopTest in a non-obvious way,
>>> which is where I am out of my depth. It seems the test's CheckGraphPhase
>>> introduces a SignedDivNode, however since this runs after
>>> FrameStateAssignmentPhase, there is no frame data, so an assert fails in
>>> the lowering. This seems to be fundamentally broken regardless of my
>>> change, but only started failing here since the denominator has as stamp of
>>> (-65536,-1)
>>> 
>>> -Jackson
>>> 
>>> 
>>> 
>