Incorrect number of parameters of lambda expressions about phosphor HOT 16 CLOSED

Hi,
This should be fixed now. Please keep using this issue if you find more INVOKEDYNAMIC issues - I'll leave it open. If you have some larger program that you are trying to run and want to share that (so I can try to run the whole thing too), I'm happy to take a look.

from phosphor.

Thank you for your help. There are still invokedynamic issues. The program behind those minimal test cases is actually ojluni (OpenJdk Lang, Util, Network, Io) from AOSP. The bugs I found are those preventing the instrumented library from being converted to Dalvik bytecode. Locating the bugs requires mapping the error message of d8 back to the portion of Java bytecode/source code causing the problem. I can send you an instruction on how to do the mapping if the extra effort is OK for you.

Thanks,
Qiang

from phosphor.

It's been > 5 years since I've looked at dx and co. Can you give an example of the error message that you get (before mapping it)?

from phosphor.

Please have a look at d8's output in #75 . The error message points out what the problem is, but not giving the location in the bytecode.

from phosphor.

Cool - this is probably enough for me to track them down from (as-is). Can you give me the details for instrumenting and d8'ing ojluni? I can try to take a look.

from phosphor.

I attached the original ojluni jar file (the extension is changed because GitHub does not support jar files) so that you don't need to obtain and build the whole AOSP to get it. The corresponding source code is over here in case you need it. The instrumentation process is as usual. Just run Phosphor against the jar file and the instrumented jar file will be the output. The remaining part is for compiling and using d8.

To compile d8

Before running the following, please ensure the default version of JDK is 1.8 and python is 2.x.

git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
# add the directory depot_tools to PATH
git clone https://r8.googlesource.com/r8
git checkout 1.4.22
cd r8
tools/gradle.py d8

You can find d8.jar at build/libs. The built jar file is so big that I cannot attach it here.

To run d8

java -jar d8.jar --min-api 28 instrumented.jar

The output will be classes*.dex in the current directory if no error occurs.

Thank you for your attention to this matter. Please let me know if you have any difficulties with that.

Thanks,
Qiang

core-oj.zip

from phosphor.

Great - I was able to reproduce. I got the same error that I assume you are getting now:

  Compilation failed with an internal error.
com.android.tools.r8.errors.Unreachable: Enforced and erased signatures are inconsistent in CallSite: { Name: accept, Proto: Proto LL java.util.function.IntConsumer java.util.function.Consumer, MethodHandle: {INVOKE_STATIC, java.lang.invoke.CallSite java.lang.invoke.LambdaMetafactory.metafactory(java.lang.invoke.MethodHandles$Lookup, java.lang.String, java.lang.invoke.MethodType, java.lang.invoke.MethodType, java.lang.invoke.MethodHandle, java.lang.invoke.MethodType)}, Args: Item 21 Proto VII void int int, Item 22 MethodHandle: {INVOKE_INTERFACE, void java.util.function.Consumer.accept(java.lang.Object)}, Item 21 Proto VI void int}
	at com.android.tools.r8.ir.desugar.LambdaDescriptor.infer(LambdaDescriptor.java:268)

I'll work through this and whatever else comes up to get this to pass the r8 verifier with lambdas. There was never a good test suite for the lambda support in Phosphor because of timing (Java 8 was released 1 week before the OOPSLA deadline that year :) ), so this is good.

FYI - here's the debugging strategy that I use for things like this:
Ideally we want to see the instrumented bytecode that is resulting in this error. From the error it's clear that there is some instance where we are incorrectly changing the instantiated or sam type (one is becoming void (int) and the other void (int int)). The exception doesn't tell you where it is though. So, re-run with a debugger attached and a breakpoint set on Unreachable exception, then unwind the stack trace and see the name of the class its processing. Then, a javap of our instrumented core-oj file shows the offending call site:

$ javap -private -verbose -bootclasspath oj-inst/core-oj.zip java.util.Spliterator\$OfInt
...
  1: #57 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
    Method arguments:
      #126 (II)V
      #59 invokeinterface java/util/function/Consumer.accept:(Ljava/lang/Object;)V
      #58 (I)V

from phosphor.

OK - it's now passing the transformation, but giving a lot of warnings about local variable debug info - I will take a look at those too, but wanted to let you know it seemed your blockers are now resolved.

from phosphor.

That's really good to know. Thank you for dedicating your time and energy into this.

from phosphor.

Unfortunately, it seems the invokedynamic issue is still not completely fixed. Could you please have a look at the attached jar file? Thank you for your help.

android.hidl.base-V1.0-java.zip

from phosphor.

K - fixed that one too. If there are more and you are able to just share all of the files that are having errors, I can go through them all at once - it would probably be more efficient.

FYI - the local variable warnings are due to restrictions that d8 puts on JVM bytecode - these are stricter restrictions than the JVM spec, so I'm going to leave it as-is.

from phosphor.

Thank you for the information. I am writing a script to extract all jar files need to be instrumented so that we can look at them at once instead of getting blocked by one file.

from phosphor.

Hi,

I have run phosphor and d8 against all the jar files need to be instrumented. Apart from the invokedynamic one, there are several other problems. I have uploaded the data, script, output, and a summary here, with instructions and explainations in README.md. Please have a look at them when available. Thank you for your help.

Thanks,
Qiang

from phosphor.

Sorry for the delay... as you can probably tell from the merge, there were a lot of fixes that needed to be made (and our semester started last week). Proguard generates byte code that is valid, but unlike any generated by javac so we had never gotten around to testing all of these special cases.

I've also added two options though that are particular to generating stuff that d8 will process. Using these two flags, the entire process seems to work now.

First: -skipLocals will avoid generating local variable debugging information - d8 only tolerates a subset of the full JVM spec for this (and it's stripped away anyway).

Second: -alwaysCheckForFrames will always scan the entire body of every method in each class file to ensure that it has frames before processing it. Normally, Java 8+ class files are required to have frames, so we don't bother checking for them. However, ProGuard removes them, because d8 ignores them. This check adds a bit of a performance hit so we don't do it by default (but you can with this flag).

Let me know how this goes.

from phosphor.

Sorry for the delay getting back to you as I struggled with ART for a while. The two options work perfectly for me. They really make d8 satisfied. Thank you a lot for doing this.

Currently, I am still trying to make the dex2oat part of ART work. There are several problems I've encountered up to now which I would like to share with you.

First, similar to that described in the paper, I have to change several constants to make the hard-coded assumptions about class size and instance size consistent with the instrumented classes.

Second, Phosphor sometimes will produce code that invokespecial a public method. While JVM should allow this, it won't pass ART's class verifier. The current workaround is to remove this check in the hope the runtime won't complain about this.

The third problem is hard to decipher, as it involves both Phosphor and d8.

For the case where a method returns a primitive, the instrumented code will have two methods: one with suffix $$PHOSPHORTAGGED doing the real stuff and one wrapper method invoking the $$PHOSPHORTAGGED one. Of course, the original method could be a synthesized one from a lambda expression (let's call the method LM). In this case, the $$PHOSPHORTAGGED one is the new LM.

However, d8 treats lambda expressions in a different way. It will synthesis a real class implementing the functional interface (desugar). And in that class, a wrapper method will call the LM. To make this work, sometimes they must change the signature of LM, e.g. from private to public static and add a parameter for the instance invoked upon. However, d8 didn't search for other call sites of the LM, as it assumes the LM will only be used at the definition of the lambda expression (invokedynamic). Thus, the call site inside the wrapper method generated by Phosphor is not modified accordingly.

The easiest fix for me would be let Phosphor not to generate the wrapper method for an LM. It won't hurt anyway as no one will call the wrapper. As it is not the problem of Phosphor, I will try to do this. It would be better to merge it into your codebase if you find it appropriate.

I am closing this issue as all problems generic to java are all solved. Again, thank you for your help along the way.

Thanks,
Qiang

from phosphor.

Hooray!

Second, Phosphor sometimes will produce code that invokespecial a public method. While JVM should allow this, it won't pass ART's class verifier. The current workaround is to remove this check in the hope the runtime won't complain about this.

I think that this is probably laziness on my part, and that Phosphor could decide to emit invokespecial or invokevirtual as appropriate.

For the case where a method returns a primitive, the instrumented code will have two methods: one with suffix $$PHOSPHORTAGGED doing the real stuff and one wrapper method invoking the $$PHOSPHORTAGGED one. Of course, the original method could be a synthesized one from a lambda expression (let's call the method LM). In this case, the $$PHOSPHORTAGGED one is the new LM.

Maybe it makes sense to add a flag for instrumentation that will ignore generating these?

from phosphor.