Storyyeller / Krakatau

Java decompiler, assembler, and disassembler
GNU General Public License v3.0
1.95k stars 220 forks source link

Optimization of new statement causes incorrect output #64

Closed samczsun closed 8 years ago

samczsun commented 8 years ago

Here is the hand written bytecode

.version 52 0 
.class public super [5] 
.super java/lang/Object 

.method public <init> : ()V 
    .code stack 1 locals 1 
L0:     aload_0 
L1:     invokespecial Method java/lang/Object <init> ()V 
L4:     return 
    .end code 
.end method 

.method public static main : ([Ljava/lang/String;)V 
    .code stack 10 locals 10 
L0:     invokestatic Method [5] test ()V 
L3:     return 
L4:     
    .end code 
.end method 

.method public static test : ()V 
    .code stack 10 locals 10 
L0:     new java/lang/System 
L3:     new java/lang/Integer 
L6:     iconst_0 
L7:     istore_0 

        .stack full 
            locals Integer 
            stack Uninitialized L0 Uninitialized L3 
        .end stack 
L8:     swap 
L9:     iinc 0 1 
L12:    iload_0 
L13:    ldc 2 
L15:    if_icmpeq L22 
L18:    swap 
L19:    goto L8 

        .stack full 
            locals Integer 
            stack Uninitialized L3 Uninitialized L0 
        .end stack 
L22:    ifnonnull L36 
L25:    pop 
L26:    ldc 'This will be called' 
L28:    getstatic Field java/lang/System out Ljava/io/PrintStream; 
L31:    swap 
L32:    invokevirtual Method java/io/PrintStream println (Ljava/lang/Object;)V 

        .stack full 
            locals Integer 
            stack 
        .end stack 
L35:    return 

        .stack full 
            locals Integer 
            stack Uninitialized L3 
        .end stack 
L36:    dup 
L37:    iconst_1 
L38:    invokespecial Method java/lang/Integer <init> (I)V 
L41:    pop 
L42:    ldc "This won't be called" 
L44:    getstatic Field java/lang/System out Ljava/io/PrintStream; 
L47:    swap 
L48:    invokevirtual Method java/io/PrintStream println (Ljava/lang/Object;)V 
L51:    return 
L52:    
    .end code 
.end method 
.innerclasses 
    [1] [5] Foo public 
.end innerclasses 
.const [1] = Class Confusion 
.const [5] = Class Confusion 
.end class 

(I've thrown in some things to break all the other decompilers so people testing are forced to use Krakatau)

The output by Krakatau is this:

public class Confusion {
    public Confusion()
    {
        super();
    }

    public static void main(String[] a)
    {
        Confusion.test();
    }

    public static void test()
    {
        int i = 0;
        while(true)
        {
            i = i + 1;
            if (i == 2)
            {
                System a = null;
                if (a == null)
                {
                    System.out.println((Object)"This will be called");
                    return;
                }
                Integer a0 = new Integer(1);
                System.out.println((Object)"This won't be called");
                return;
            }
        }
    }
}

In the decompiled code, it appears that "This will be called" should be printed. After all, a will always be null. However, in reality "This won't be called" is printed

Storyyeller commented 8 years ago

Thanks for the report. I'll look into it later.

Storyyeller commented 8 years ago

Should be fixed now.

Storyyeller commented 8 years ago

PS: You don't actually need a constructor

Storyyeller commented 8 years ago

Oops, looks like it's still not working

Storyyeller commented 8 years ago

Previously, that was true, but my last change should have fixed that. I'll have to look into it again to figure out why that didn't work.

2016-02-25 18:36 GMT-08:00 Sam Sun notifications@github.com:

I agree, but what I noticed was that Krakatau can distinguish that a new opcode will produce something nonnull. However, when you start swapping it around that's when Krakatau gets confused.

I don't know how Krakatau does it's optimizations but I'm assuming that this is just a case of misoptimization like the last issue.

— Reply to this email directly or view it on GitHub https://github.com/Storyyeller/Krakatau/issues/64#issuecomment-189084630 .

Storyyeller commented 8 years ago

Should be fixed for real now.

Turns out it was just a stupid mistake. My first change only covered one of the two cases for initial constraint creation.