-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Don't insert stackmap frames into class files with version < 1.6. #667
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marchof wow, didn't thought that this will be that simple as replacement of a single literal true
👏
While now we don't insert frames, ASM does 😆 - remember https://gitlab.ow2.org/asm/asm/issues/317800 ? Thus formally speaking the issue as it is stated in description is not fully resolved 😉 however AIOBE
from which all this started is gone.
Shall we stay with ASM 6.0
and apply workaround to really get rid of unwanted frames? Or update to ASM 6.1.1
? Or rephrase description and entry in changelog with focus on AIOBE
? Or something else? WDYT?
|
||
@Test | ||
public void needFrames_should_return_true_for_1_9() { | ||
assertTrue(InstrSupport.needsFrames(Opcodes.V9)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By analogy with #629 (comment) I think that compact form of single test is more appropriate here. And BTW BytecodeVersion.V10
is missing 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Java 10 (luckily ASM lets us write versions > 9).
As discussed before: I personally prefer to have separate tests to see separate results for separate cases. But if you prefer I can combine them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer to have separate tests to see separate results for separate cases.
I totally agree with this in general, however think that here just two cases and not ten - stackmap frames were introduced in 1.6 and required since then:
@Test
public void needFrames_should_return_false_for_versions_less_than_1_6() {
...
}
@Test
public void needFrames_should_return_true_for_versions_greater_than_or_equal_to_1_6() {
...
}
this makes clear accent on a functional aspect rather than on technical, which IMO should be prefereable in tests for understandability and hence maintainability.
Added Java 10
Strange - I don't see it.
(luckily ASM lets us write versions > 9).
As a side joke: yes it should - see question https://gitlab.ow2.org/asm/asm/merge_requests/92#note_22288 and answer https://gitlab.ow2.org/asm/asm/merge_requests/92#note_22291 , however this is/was not always possible until fix of corresponding bug in ASM 6.1 - https://gitlab.ow2.org/asm/asm/issues/317807 😉
And in any case we can always patch it up after creation.
* <pre> | ||
* public class Sample { | ||
* public Foo(boolean b){ | ||
* if(b){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange formatting. By analogy with
Lines 50 to 57 in ac702e7
* <code><pre> | |
* enum E { | |
* ; | |
* private E() { | |
* ... | |
* } | |
* } | |
* </pre></code> |
<code><pre>
will help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Also Foo
should read Sample
.
} | ||
|
||
private byte[] createClass(int version) { | ||
/** | ||
* Created a class of the following format. This class requires a frame to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about replacing Created
by Creates
, and rephrasing to be inserted
since created class already will have it in case of frames = true
, e.g. something like
Creates a class that requires a frame before the return statement. Also for this class instrumentation should insert another frame.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, fixed.
@@ -20,6 +20,12 @@ <h1>Change History</h1> | |||
|
|||
<h2>Snapshot Build @qualified.bundle.version@ (@build.date@)</h2> | |||
|
|||
<h3>Fixed Bugs</h3> | |||
<ul> | |||
<li>Don't insert stackmap frames into class files with version < 1.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have feeling that this might actually be a regression introduced in version 0.6.5
- see 8f81097#diff-c192e78bf00ddbf0e1659d9b0b30f4c3R63 So maybe confirm and rephrase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right about the regression. The old FrameTracker was not used at all for class files < 1.6: 8f81097#diff-604aa08a18997c858a24a3cab3c74a63L98
Something like this?
Don't insert stackmap frames into class files with version < 1.6. This is a regression which was introduced with release 0.6.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix itself is not a regression 😉 so I'd like to propose
Don't insert stackmap frames into class files with version < 1.6,
this fixes regression which was introduced in version 0.6.5
c2e39bb
to
80564b3
Compare
@Godin I see your point. Maybe for the next release we have this fix and the upgrade to 6.1.1 in a separate PR. Then the outcome is exactly what both change log entries claim (no frames & new ASM version). Even when strictly speaking they are related I think nobody will understand if we try to explain how they are related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for the next release we have this fix and the upgrade to 6.1.1 in a separate PR. Then the outcome is exactly what both change log entries claim (no frames & new ASM version). Even when strictly speaking they are related I think nobody will understand if we try to explain how they are related.
"nobody"... let's say that I egoistically was mainly thinking about targeting ourselves 😆 But you right, fine like that.
So just few minor comments to address and will be perfect 👍
|
||
@Test | ||
public void needFrames_should_return_true_for_1_9() { | ||
assertTrue(InstrSupport.needsFrames(Opcodes.V9)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer to have separate tests to see separate results for separate cases.
I totally agree with this in general, however think that here just two cases and not ten - stackmap frames were introduced in 1.6 and required since then:
@Test
public void needFrames_should_return_false_for_versions_less_than_1_6() {
...
}
@Test
public void needFrames_should_return_true_for_versions_greater_than_or_equal_to_1_6() {
...
}
this makes clear accent on a functional aspect rather than on technical, which IMO should be prefereable in tests for understandability and hence maintainability.
Added Java 10
Strange - I don't see it.
(luckily ASM lets us write versions > 9).
As a side joke: yes it should - see question https://gitlab.ow2.org/asm/asm/merge_requests/92#note_22288 and answer https://gitlab.ow2.org/asm/asm/merge_requests/92#note_22291 , however this is/was not always possible until fix of corresponding bug in ASM 6.1 - https://gitlab.ow2.org/asm/asm/issues/317807 😉
And in any case we can always patch it up after creation.
@@ -20,6 +20,12 @@ <h1>Change History</h1> | |||
|
|||
<h2>Snapshot Build @qualified.bundle.version@ (@build.date@)</h2> | |||
|
|||
<h3>Fixed Bugs</h3> | |||
<ul> | |||
<li>Don't insert stackmap frames into class files with version < 1.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix itself is not a regression 😉 so I'd like to propose
Don't insert stackmap frames into class files with version < 1.6,
this fixes regression which was introduced in version 0.6.5
For certain probes additional frames needs to be inserted, but only from class file version 1.6 on.
@Godin I tried to address all issues. Can you please check again? |
Steps to reproduce
JaCoCo version: 0.8.1
Operating system: any
Tool integration: any
Instrument a class with version 1.5 or below.
Expected behaviour
No stackmap frames are inserted.
Actual behaviour
When inserting probes for conditional jumps frames are actually inserted. By definition they are ignored by the java runtime but may cause problems with ASM (large methods) or other tools.
This issue was reported on our mailing list: https://groups.google.com/forum/?fromgroups=#!topic/jacoco/O84HGz6AWq4