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

[Doug Simon]

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