Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect instrumentation of code using wide locals. #1412

Closed
zerny opened this issue Mar 13, 2023 · 4 comments · Fixed by #893
Closed

Incorrect instrumentation of code using wide locals. #1412

zerny opened this issue Mar 13, 2023 · 4 comments · Fixed by #893
Labels
declined: duplicate ❌ This issue or pull request already exists
Milestone

Comments

@zerny
Copy link

zerny commented Mar 13, 2023

Steps to reproduce

  • JaCoCo version: 0.8.8

  • Operating system: any

  • Tool integration: Studio / AGP

  • Complete executable reproducer: (e.g. GitHub Repo)

  • Steps: (what exactly are you doing with the above reproducer?)

    Reproduction artifacts can be found in https://issuetracker.google.com/266109833

    Original classfile inputs prior to instrumentation:
    zcash-android-sdk-1.12.0-beta01-SNAPSHOT-runtime.jar

    Classfiles after instrumentation:
    instrumented_zcash-android-sdk-1.12.0-beta01-SNAPSHOT-runtime.jar

The instrumented classfiles contains invalid code (from the javap printing):

      56: invokestatic  #93                 // Method java/lang/System.currentTimeMillis:()J
      59: lstore        5
      61: aload         6

Here instruction 61 is inserted by jacoco and is reading the high-register of a wide value as if it was an object.

A note on what could be the cause of this: the original kotlin produced code has an object typed value in local 5 at entry to the method. However, the local 5 is later (in a completely valid and correct way) repurposed to hold a long (wide value) thus register 6 is now also occupied. It looks like jacoco fails to see the existing use of register 6 and incorrectly uses it for its injected instrumentation.

@zerny zerny added the type: bug 🐛 Something isn't working label Mar 13, 2023
@marchof
Copy link
Member

marchof commented Mar 13, 2023

@zerny Thanks for reporting this. We have seen this before in combination with the R8 minimizer. I completely agree that re-using parameters for local variables is a valid scenario but we didn't prioritize a fix #893 because it only happend in combination with the minimizer -- which will render the coverage reports useless anyways.

@zerny
Copy link
Author

zerny commented Mar 14, 2023

As far as I can tell the input code here is kotlinc generated code, so this is not just R8 produced code. Also, the argument that the R8 compiler is for creating obfuscated code and thus not needed for coverage is not accurate. R8 can be used without any loss of information, including line tables and debug info. Also, the D8 compiler used for desugaring shares the same internal compilation so the pattern can arise from that too. So while rare, this issue could pop up again in such contexts in the future.

Regarding the linked to fix #893 and previous issue #767, are those still unlanded / open?
If so, I suspect this can be marked as a duplicate of the existing bug.

@marchof
Copy link
Member

marchof commented Mar 14, 2023

@zerny Thanks for the additional information. I´ll close this issue as duplicate.

@marchof marchof closed this as completed Mar 14, 2023
@marchof marchof added declined: duplicate ❌ This issue or pull request already exists and removed type: bug 🐛 Something isn't working labels Mar 14, 2023
@Godin
Copy link
Member

Godin commented Mar 14, 2023

As far as I can tell the input code here is kotlinc generated code

And here is a reduced example:

for following Example.kt

package example

inline fun <R> example(priority: Int = 0, block: () -> R): R {
    val start = System.currentTimeMillis()
    val result = block()
    val elapsed = (System.currentTimeMillis() - start)
    return result
}

execution of

kotlin-compiler-1.8.10/bin/kotlinc Example.kt

java -jar jacoco-0.8.8/lib/jacococli.jar instrument example --dest instrumented

javap -v -p instrumented/ExampleKt.class | grep -C 1 "lstore_3"

produces

        40: invokestatic  #22                 // Method java/lang/System.currentTimeMillis:()J
        43: lstore_3
        44: aload         4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
declined: duplicate ❌ This issue or pull request already exists
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants