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

[Roland Schatz]

On 12/01/2017 01:04 PM, Gilles Duboscq wrote:
> I agree that it's a problem to add those Div nodes after FrameState assignement in `CountedLoopTest`.
>
> We would need to change the tests in `checkMidTierGraph` to be inserted in the `MidTier` before  `FrameStateAssignmentPhase`.
> It can be done by overriding `createSuites` and inserting a phase manually.
Or we just use `checkHighTierGraph`. All the loop optimizations that 
could potentially break this test are in the high tier, so I think it's 
sufficient to do the check at the end of high tier.

- Roland
>
>   Gilles
>
> On 01/12/17 09:04, Jackson Davis 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
>>