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

[Gilles Duboscq]

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.

 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
>