Probes inserted at locals that will be overridden

[mkroghj]

When injecting probes into functions jacoco assumes that the arguments are not overridden as per the following JVM bytecode for class Test:

public final class Test
  minor version: 0
  major version: 52
  flags: ACC_PUBLIC, ACC_FINAL, ACC_SUPER
Constant pool:
   #1 = Utf8               Test
   #2 = Class              #1             // Test
   #3 = Utf8               java/lang/Object
   #4 = Class              #3             // java/lang/Object
   #5 = Utf8               Test.java
   #6 = Utf8               <init>
   #7 = Utf8               ()V
   #8 = NameAndType        #6:#7          // "<init>":()V
   #9 = Methodref          #4.#8          // java/lang/Object."<init>":()V
  #10 = Utf8               this
  #11 = Utf8               LTest;
  #12 = Utf8               foo
  #13 = Utf8               (I)I
  #14 = Utf8               main
  #15 = Utf8               ([Ljava/lang/String;)V
  #16 = Utf8               java/lang/System
  #17 = Class              #16            // java/lang/System
  #18 = Utf8               out
  #19 = Utf8               Ljava/io/PrintStream;
  #20 = NameAndType        #18:#19        // out:Ljava/io/PrintStream;
  #21 = Fieldref           #17.#20        // java/lang/System.out:Ljava/io/PrintStream;
  #22 = NameAndType        #12:#13        // foo:(I)I
  #23 = Methodref          #2.#22         // Test.foo:(I)I
  #24 = Utf8               java/io/PrintStream
  #25 = Class              #24            // java/io/PrintStream
  #26 = Utf8               println
  #27 = Utf8               (I)V
  #28 = NameAndType        #26:#27        // println:(I)V
  #29 = Methodref          #25.#28        // java/io/PrintStream.println:(I)V
  #30 = Utf8               Code
  #31 = Utf8               LineNumberTable
  #32 = Utf8               LocalVariableTable
  #33 = Utf8               SourceFile
{
  static int foo(int);
    descriptor: (I)I
    flags: ACC_STATIC
    Code:
      stack=2, locals=2, args_size=1
         0: iload_0
         1: i2l
         2: lstore_0
         3: bipush        15
         5: ireturn

  public static void main(java.lang.String[]);
    descriptor: ([Ljava/lang/String;)V
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=2, locals=1, args_size=1
         0: getstatic     #21                 // Field java/lang/System.out:Ljava/io/PrintStream;
         3: bipush        42
         5: invokestatic  #23                 // Method foo:(I)I
         8: invokevirtual #29                 // Method java/io/PrintStream.println:(I)V
        11: return
      LineNumberTable:
        line 6: 0
        line 7: 11
}

After instrumentation, the foo function looks as follows, which is not valid (therefore, the format is a bit different from javap):

.method Test.foo(I)I
 0:   invokestatic Test.$jacocoInit()[Z
 1:   astore 1
 2:   iload 0
 3:   i2l
 4:   lstore 0
 5:   ldc 15
 6:   aload 1
 7:   ldc 1
 8:   ldc 1
 9:   bastore
10:   ireturn
.end method

Here, the probe is stored into local 1, however, local 1 is overriden by lstore 0 that takes up two locals.

Steps to reproduce

java -cp out.jar -javaagent:jacocoagent.jar=destfile=foo.txt,dumponexit=true,output=true Test

JaCoCo version: 0.8.3
Operating system: Linux
Tool integration: Command line (and gradle)

Expected behaviour

Running:
java -cp out.jar -javaagent:jacocoagent.jar=destfile=foo.txt,dumponexit=true,output=true Test
should ouput 15

Actual behaviour

java.lang.VerifyError: Bad local variable type
Exception Details:
  Location:
    Test.foo(I)I @34: aload_1
  Reason:
    Type long_2nd (current frame, locals[1]) is not assignable to reference type

[marchof]

@mkroghj Thanks for reporting this! In your example the first argument (slot 0) is overwritten. What compiler created such bytecode?

[marchof]

Due to JVM spec this seems to be allowed (even if not emitted by javac or other compilers we're testing).

[mkroghj]

This is a reduced program from the R8 compiler:
https://r8.googlesource.com/r8

R8 generates optimized code and will re-use locals when available.

[marchof]

Out of curiosity: What was the original source code of the method? What is the purpose of

0: iload_0
1: i2l
2: lstore_0

when the long value is not used at all? Or do you mean by reduced that you reduced the byte code?

[mkroghj]

The bytecode stems from something larger where local 0 is used for further computation. I just kept removing until the error would disappear.

[marchof]

Overwriting variables is not a problem as such. The issue seems to be specific to the situation where the last slot used by method parameters is overwritten with a two slot value (long or double).

Here is a JUnit reproducer for the issue: https://gist.github.com/marchof/6561c2344c4b284582dc4c778590ca72

As simple solution would be to keep a safety distance of one slot to the method parameters:

• 👍 Local and simple change in ProbeInserter
• 👎 For every stackframe we increase the local variables size by 2 instead of 1
• 👎 It becomes less likely that we can use the 1-byte opcodes aload_n for the probe array

@Godin WDYT?

[Godin]

@marchof While I agree that we should fix this to not cause failure. As far as I understood - R8 is a shrinker and minificator, not compiler, so usage of it while gathering coverage is questionable. Sounds like usage of obfuscators which should not be used together with coverage tools.

I'm wondering - can we detect when this distance is needed? to get rid of negative aspects of simple solution proposed by you.

[bvella]

I encountered this same scenario. My situation is as follows:

We have an annotation processor that that transforms code into continuations, providing async/await to java (https://github.com/ixaris/ixaris-oss/tree/master/commons-async/lib).

This is causing this same issue. In my case, however, I cannot opt to transform the code after coverage.

java.lang.VerifyError: Bad local variable type
Exception Details:
  Location:
    com/ixaris/connectors/plutus/impl/services/accounts/AccountsService.continuation$queuedAdjustAccountBalance(JJLcom/ixaris/connectors/plutus/impl/support/pojos/TransactionOperation;Lcom/ixaris/commons/financial/lib/CurrencyAmount;ILjava/lang/String;Lcom/ixaris/connectors/plutus/impl/services/events/data/EventEntity$Type;JLcom/ixaris/connectors/plutus/impl/services/accounts/data/AccountEntity;ILjava/util/concurrent/CompletionStage;)Ljava/util/concurrent/CompletionStage; @674: aload
  Reason:
    Type long_2nd (current frame, locals[15]) is not assignable to reference type
  Current Frame:
    bci: @674
    flags: { }
    locals: { 'com/ixaris/connectors/plutus/impl/services/accounts/AccountsService', long, long_2nd, long, long_2nd, 'com/ixaris/connectors/plutus/impl/support/pojos/TransactionOperation', 'com/ixaris/commons/financial/lib/CurrencyAmount', integer, 'java/lang/String', 'com/ixaris/connectors/plutus/impl/services/events/data/EventEntity$Type', long, long_2nd, 'com/ixaris/connectors/plutus/impl/services/accounts/data/AccountEntity', 'com/ixaris/connectors/plutus/impl/services/groups/velocitylimits/data/VelocityLimitGroupEntity', long, long_2nd }
    stack: { }
  Bytecode:
    0x0000000: b803 6f3a 0f15 0daa 0000 044b 0000 0000
    ...

In an attempt to fix this, I looked at #782 and modified some code suggested in that thread, to generate a hit method and mark hits by calling this method instead of embedding the code directly in each method. An additional benefit of this is that is it trivially easy to check if the hit was already recorded before updating the array, making better use of memory caching. This has resolved the issue without any slowdown.

I will submit a pull request with these changes for your consideration.

[Godin]

@bvella just my two cents

I looked at #782

IMO main thing discussed in #782 (cache line) is not much related to main thing discussed here (local variable).

This has resolved the issue without any slowdown.

While maybe there is no slowdown in your scenario, such significant changes require validation on variety of scenarios - IMO reading of a field is different from reading from local variable in terms of performance.

[Godin]

@bvella btw most interesting question - wondering why solution described here in #767 (comment) not suitable for you?

[Godin]

@marchof what are the other options? As far as I can see we already do two traversals (first being MethodSanitizer), so I can imagine

1. maybe we can track type of ASTOREs for last argument to determine when safety slot needed, but this is indeed not localized far more complex change
2. use slot maxLocals + 1, but this also requires complexity of passing maxLocals around and most likely will result in even bigger bytecode size increase than safety slot

but not sure that any of these is worth the efforts.

Therefore implemented your proposal in #893 and made following measurement: for rt.jar of JDK 8u131 size of offline instrumented classes using version 0.8.4 is 73964764 bytes, and 74378865 bytes after #893, 414101 bytes increase ~ 0.6%, what looks quite acceptable to me. Or maybe you have some other ideas?

[marchof]

@Godin My concern with my proposal also was runtime overhead (increased locals). Not sure though what the overhead with a typical code base would be.

[bvella]

Hi @Godin, apologies for not replying earlier. The asynchronous transformer aggressively tries to reuse locals to reduce the size of the continuation. It is difficult to explain in a couple of lines but in a nutshell, stackless continuations work by recreating the method's locals and operand stack, which means every local and operand on the stack would need to be passed as a parameter. For the sake of reducing the required parameters, locals are aggressively reused.

I see the following ways for jacoco to work with locals reuse:

1. As per @marchof's comment Probes inserted at locals that will be overridden #767 (comment) reserve 2 slots for the hits array and use the 2nd slot, leaving space for longs and doubles to spread over to the first reserved slot. This causes the method to use 2 extra locals instead of 1.

2. record coverage hits through a method call. This makes the least amount of changes to methods as the locals and operand stack remain intact (something like push index, invokestatic hitmethod), while also allowing the hit method to check before writing, which is more cache friendly than writing always. The downside is that the array reference has to be fetched to a local for every hit rather than once per method. The microbenchmark @Godin did in Remove Instrumentation #782 suggests that the overhead of both approaches seems pretty similar for single threaded performance. I ran a more complicated microbenchmark which also shows the 2 approaches to be on par

3. Use a slot after last used local for the hits array. This way, existing locals are not modified. This would involve scanning the method prior to rewriting it. It will also increase the size of all frame instructions because all the locals need to be declared as nulls to include the new last local (the hits array)

4. Use slot 1 for the array in instance methods (0 is always this) or slot 0 for static method and move all var references by 1. should be pretty easy to do as all variable references are +1 except 0 in case of instance methods, but requires moving parameters by 1 at the start of each method (load n, store n+1 from last to first parameter). Frames can also be easily modified by adding the hits array as the first local. This approach is similar to 2 in principle, i.e. do not insert a local in the middle of other locals.

IMHO, approach 2 is potentially the best because

• it introduces the least amount of new bytecode (push + invokestatic vs push, iconst, bastore)
• it does not change the methods' locals and operand stack, and because of this, does not change any of the method's own instructions
• it allows the hit method to check before writing, making it more efficient in multithreaded senarios while also not adding this bytecode everywhere (once per class instead of once per hit)

I have a patch for approach 2, extracted from @Godin's branch https://github.com/jacoco/jacoco/commits/issue-782, generalised a bit to avoid specific instanceof checks. Internally, we are using this patch.

If you think 3 and 4 are viable, I can have a go at them and run some benchmarks.

[bvella]

Hi @Godin and @marchof what do you think of the above approaches? I would like to stop using our internally patched version of jacoco once this issue is fixed.

[marchof]

@bvella We implemented approach 1) in #893. Can you please test this whether it works for your scenario? You either build the branch yourself of find the corresponding build here:

https://ci.appveyor.com/project/JaCoCo/jacoco/builds/25107476/artifacts

[marchof]

As described in #1412 it looks like only the Kotlin compiler may create such bytecode.