electronicarts / ea-async

EA Async implements async-await methods in the JVM.
https://go.ea.com/ea-async
Other
1.38k stars 129 forks source link

java.lang.VerifyError: on startup #6

Closed paulseawa closed 7 years ago

paulseawa commented 8 years ago

This is a strange one. Under very specific circumstances, our code is getting a verification error on startup when using runtime instrumentation. I've tried stripping it down to its bare essence, but it doesn't really make a lot of sense to me. I'm sure there's a larger pattern I'm not seeing.

Basically you need a local String initialized to null then an numeric variable initialized to anything, and a subsequent await statement.

I get this error, along with a lot of error output: java.lang.VerifyError: Bad local variable type

Here's and example I added to BasicTest.java

    @Test(timeout = 2_000)
    public void testNonInitilized() throws IllegalAccessException, InstantiationException
    {
        final SomethingWithNonInitialized a = new SomethingWithNonInitialized();
        CompletableFuture<String> blocker = new CompletableFuture<>();
        final CompletableFuture<Object> res = a.doSomething(blocker);
        blocker.complete("x");
        assertEquals(":x", res.join());
    }

    public static class SomethingWithNonInitialized
    {
        public CompletableFuture<Object> doSomething(CompletableFuture<String> blocker)
        {
            //these variables must occur in this order.  It doesn't matter if they are set later or not.  
            //Remove the initializers and then it works
            String var2 = null;         //this variable must be a string and initialized to null
            byte var3 = 0;              //this variable must be numeric and initialized to anything
            return CompletableFuture.completedFuture(":" + await(blocker));
        }
    }

Sorry for the ugly formatting. Code formatting isn't working. I've also included BasicTest.java

BasicTest.java.txt

JoeHegarty commented 8 years ago

Thanks for reporting this. Do you have any thoughts @DanielSperry? I'll dig in tomorrow if not.

JoeHegarty commented 8 years ago

Looking at this @DanielSperry. It looks like the fix you made here: https://github.com/electronicarts/ea-async/commit/fdc80980e424785242b33ec4afd3a72cc6aa2869 changes the behavior.

JoeHegarty commented 8 years ago

Repro test I am using:

public class NullThenBasicValueTest extends BaseTest
{
    @Test
    public void nullThenBasicValueTest() throws Exception
    {
        final Task<Void> res = NullThenBasicVal.doIt(getBlockedTask());
        completeFutures();
        assertNull(res.join());
    }

    public static class NullThenBasicVal
    {
        public static Task<Void> doIt(Task<Void> task)
        {
            String nullString = null; //this variable must be a string and initialized to null
            int basicInt = 0; //this variable must be numeric and initialized to anything

            await(task);
            return Task.done();
        }
    }
}
DanielSperry commented 8 years ago

it's probably the data flow for that local variable. the jvm demands that you account for the variable contents at all times.

Since ea-async modifies the code flow (adding new jumps) it needs to add extra stack info for all jumps it adds. The ea-async code flow analysis used to rely only on what asm provided. But that missed a few cases. I addressed most of those cases with that commit Joe mentioned.

Thanks for the example. I'll take a look at the issue later today.

DanielSperry commented 8 years ago

pasting the sample error message Joe sent me

java.lang.VerifyError: Bad local variable type
Exception Details:
  Location:
    com/ea/async/test/NullThenBasicValueTest$NullThenBasicVal.async$doIt(Lcom/ea/async/Task;ILcom/ea/async/Task;ILjava/lang/Object;)Ljava/util/concurrent/CompletableFuture; @89: iload_1
  Reason:
    Type null (current frame, locals[1]) is not assignable to integer
  Current Frame:
    bci: @89
    flags: { }
    locals: { 'com/ea/async/Task', null, 'com/ea/async/Task', integer, 'java/lang/Object' }
    stack: { 'com/ea/async/Task' }
  Bytecode:
    0x0000000: 1daa 0000 0000 005d 0000 0000 0000 0001
    0x0000010: 0000 0017 0000 0055 014c 033d 2a59 b900
    0x0000020: 4701 00b6 004b 9a00 234e 2db8 0051 b900
    0x0000030: 5502 002a 1c2d 1100 01ba 0062 0000 b900
    0x0000040: 6502 00b9 0047 0100 b0b9 0047 0100 b600
    0x0000050: 6b57 b800 6fb0 2c01 4c1b 3da7 ffee bb00
    0x0000060: 7759 b700 78bf                         
  Stackmap Table:
    full_frame(@24,{Object[#18],Integer,Object[#18],Integer,Object[#4]},{})
    full_frame(@73,{Object[#18],Null,Integer},{Object[#18]})
    full_frame(@86,{Object[#18],Integer,Object[#18],Integer,Object[#4]},{})
    full_frame(@94,{Object[#18],Integer,Object[#18],Integer,Object[#4]},{})
JoeHegarty commented 7 years ago

Any news on this one @DanielSperry ?

DanielSperry commented 7 years ago

I've added a PR with a fix for it. Sorry for the delay.

JoeHegarty commented 7 years ago

Awesome. Thanks Daniel.

JoeHegarty commented 7 years ago

Resolved in 1.0.5 https://github.com/electronicarts/ea-async/releases/tag/v1.0.5