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

Probe inserted at the beginning of a try-catch block should be included in the block. #265

Closed
allenhair opened this Issue Dec 16, 2014 · 7 comments

Comments

Projects
None yet
4 participants
@allenhair
Contributor

allenhair commented Dec 16, 2014

We insert a probe before a label if it is a successor and also a target. If the label is also used to define the start of an try-catch block, the inserted probe will not be included in the block.

This causes some issues when dealing with synchronized blocks. Normally, the compiler adds a catch-all entry to the exception table which covers the synchronized block in order to call monitorexit if any exceptions are thrown. If we need to insert a probe before the label which is used to define the start of the try-catch block, the catch-all entry will no longer cover all code between the monitorenter and monitorexit calls. While the probe is unlikely to cause an exception, this approach is not strictly correct.

We should probably split labels if they define a jump target and the start of a try-catch block. That way, the probe can be inserted after the start of the try-catch block, but before the jump target.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 26, 2014

Member

@allenhair Thanks for this insight! I'm always stunned when people dive into this level of detail in the JaCoCo core...

Just curious: How did you discover this? Did the potentially missed monitorexit calls cause any issues for your?

Member

marchof commented Dec 26, 2014

@allenhair Thanks for this insight! I'm always stunned when people dive into this level of detail in the JaCoCo core...

Just curious: How did you discover this? Did the potentially missed monitorexit calls cause any issues for your?

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 26, 2014

Member

If we pull this change in we need to increment the exec file version number as this will change probe placement in a incompatible way.

Member

marchof commented Dec 26, 2014

If we pull this change in we need to increment the exec file version number as this will change probe placement in a incompatible way.

@diggybub

This comment has been minimized.

Show comment
Hide comment
@diggybub

diggybub Dec 30, 2014

Just curious, is this issue linked to the following Android issue?

https://code.google.com/p/android/issues/detail?id=80961

I've run into similar problems using jacoco on a project using a device with the Android ART Runtime. I keep getting a java.lang.VerifyError exception on a class using try/catch blocks.

diggybub commented Dec 30, 2014

Just curious, is this issue linked to the following Android issue?

https://code.google.com/p/android/issues/detail?id=80961

I've run into similar problems using jacoco on a project using a device with the Android ART Runtime. I keep getting a java.lang.VerifyError exception on a class using try/catch blocks.

@allenhair

This comment has been minimized.

Show comment
Hide comment
@allenhair

allenhair Dec 31, 2014

Contributor

@marchof I'm using JaCoCo on Android and ran into this issue when using the ART runtime. ART performs bytecode verification and enforces that monitorexit must be called from all code-paths after a monitorenter. Because the probe is not covered by the catch-all block, bytecode verification will fail.

@diggybub Yes, this change will fix that issue.

Contributor

allenhair commented Dec 31, 2014

@marchof I'm using JaCoCo on Android and ran into this issue when using the ART runtime. ART performs bytecode verification and enforces that monitorexit must be called from all code-paths after a monitorenter. Because the probe is not covered by the catch-all block, bytecode verification will fail.

@diggybub Yes, this change will fix that issue.

@diggybub

This comment has been minimized.

Show comment
Hide comment
@diggybub

diggybub commented Dec 31, 2014

Thanks @allenhair!

marchof added a commit that referenced this issue Dec 31, 2014

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Dec 31, 2014

Member

Added a reproducer to the validation test suite which shows the problem of unprotected instructions inserted by JaCoCo. 5f625c7

Member

marchof commented Dec 31, 2014

Added a reproducer to the validation test suite which shows the problem of unprotected instructions inserted by JaCoCo. 5f625c7

@marchof marchof closed this in #266 Jan 4, 2015

marchof added a commit that referenced this issue Jan 4, 2015

marchof added a commit that referenced this issue Jan 4, 2015

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Jan 4, 2015

Member

Fixed on master.

Member

marchof commented Jan 4, 2015

Fixed on master.

@Godin Godin added this to the 0.7.3 milestone Jan 4, 2015

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.