Two things

[samczsun]

Right now I'm working on making clickable code in Helios, and one issue I'm running into is the Java grammar doesn't allow semicolons after a method declaration if it's not abstract, so stuff like this is causing the entire class to fail:

    //Traceback (most recent call last):
    //  File "C:\Users\Sam\.helios\krakatau\a43b2e7e0a53bca9fe7c34d97b3b3738d662f8d5\Krakatau\java\javaclass.py", line 37, in _getMethod
    //    graph = cb(method) if method.code is not None else None
    //  File "decompile.py", line 46, in makeGraph
    //    s = Krakatau.ssa.ssaFromVerified(m.code, v)
    //  File "C:\Users\Sam\.helios\krakatau\a43b2e7e0a53bca9fe7c34d97b3b3738d662f8d5\Krakatau\ssa\graph.py", line 715, in ssaFromVerified
    //    data = blockmaker.BlockMaker(parent, iNodes, inputTypes, returnTypes, code.except_raw)
    //  File "C:\Users\Sam\.helios\krakatau\a43b2e7e0a53bca9fe7c34d97b3b3738d662f8d5\Krakatau\ssa\blockmaker.py", line 519, in __init__
    //    vals, outslot_norm = self._getInstrLine(node)
    //  File "C:\Users\Sam\.helios\krakatau\a43b2e7e0a53bca9fe7c34d97b3b3738d662f8d5\Krakatau\ssa\blockmaker.py", line 602, in _getInstrLine
    //    vals = _instructionHandlers[instr[0]](self, inslots, iNode)
    //KeyError: 'invokedynamic'
    public a(java.lang.Object arg);

Would it be possible to add an option to have the method body throw a RuntimeException or a subclass so that the code is still technically valid Java (as there are other methods which decompile successfully).

Speaking of invokedynamics, would it be possible to also add an option to 'decompile' invokedynamics as if it were an invokestatic of the bootstrap method and then throwing in a comment to show it's not a regular invokestatic? That way Krakatau won't bail on a method just because someone threw in a single invokedynamic instruction.

[Storyyeller]

It seems weird to me, since in general, the decompiled code won't be compileable anyway. But I guess for well behaved cases, this would be useful.

[Storyyeller]

What kind of tool are you trying to run on the decompiled code? It seems to me that if there is an error in decompilation, creating a fake method body would lead to a false sense of security, and incorrect results if people decompile and recompile (which they shouldn't, but lots of people do it anyway).

[samczsun]

It's a parser for Java. Basically, the decompiled code is still shown, but with the parser I can allow people to click on method invocations and jump directly to that invocation (or field calls, etc etc).

Here's the project I'm using to parse Java: https://github.com/javaparser/javaparser

[Storyyeller]

Is there a standard format for Java ASTs? It might be better to output Krakatau's AST directly instead of reparsing.

[samczsun]

I don't think so. Besides, the framework is already there for both of us - it just needs a few minor modifications.

[Storyyeller]

One advantage of using the AST directly is that you know which methods and fields are which, even when they have the same name.

[samczsun]

I can't argue with that.

Ultimately, it's up to you. If you want to add a new output for AST I'm fine with that.

[Storyyeller]

I went ahead and changed it to add a dummy method body. That should fix the issue.

[Storyyeller]

Krakatau will now print out a comment when invokedynamic appears instead of just crashing. Try it out and tell me what you think.

[samczsun]

It's a great improvement over the original handling. If you don't mind could you consider moving the comment somewhere else? Currently, the output is

this.string = /*invokedynamic(s0)*/;

Which is really just

this.string = ;

Something like this would be much appreciated (or however else you feel like outputting it) for the same reasons as above

this.string = /*invokedynamic*/the.bootstrap.method(s0);

[Storyyeller]

The problem is that there isn't really anything sensible to put there. I guess I could add in a dummy value.

Something like

this.string = /invokedynamic(s0)/null;

Would that be better? I'm not sure how picky your AST parser is.

2016-02-28 16:47 GMT-08:00 Sam Sun notifications@github.com:

It's a great improvement over the original handling. If you don't mind could you consider moving the comment somewhere else? Currently, the output is

this.string = /invokedynamic(s0)/;

Which is really just

this.string = ;

Something like this would be much appreciated (or however else you feel like outputting it) for the same reasons as above

this.string = /invokedynamic/the.bootstrap.method(s0);

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

[samczsun]

That should work too. I don't think the parser is aware of context so although you can't actually assign null to an integer it won't know that.

[Storyyeller]

Ok, I went ahead and added a dummy value to invokedynamic.

[samczsun]

Uh oh. I'm really sorry with my bugging you because of these grammatical issues, but it appears that with nested invokedynamic instructions this occurs

if (/*invokedynamic(/*invokedynamic(this.string)*/null)*/null)

The null in the if statement should be fine, but the nested comments are wreaking havoc

[Storyyeller]

I changed it to not print out the parameters in order to avoid nested comments. I hope you're happy. :P

[Storyyeller]

P.S. How did you get the null in an if statement? That doesn't seem right. It should be a boolean. What code triggered that?

[samczsun]

I copy/pasted a multi-line if statement and then when balancing brackets messed up the statement itself. Not a problem in Krakatau this time :P