End try-catch blocks before inserted probes. #627
Conversation
| @@ -28,7 +28,7 @@ | |||
|
|
|||
| static { | |||
| // Runtime initialize to ensure javac does not inline the value. | |||
| FORMAT_VERSION = 0x1007; | |||
| FORMAT_VERSION = 0x1008; | |||
There was a problem hiding this comment.
I'm wondering whether version increment is really required, if the only change - is an end of try-catch blocks and not number and placement of probes? Otherwise I'm afraid of consequences of this incompatible change for downstream projects as already mentioned in #626 (comment) - when we did increment last time several years ago, it was quite disruptive.
Also in backlog we have some other changes that require increment, but were considered minor to justify it. So if underlying issue here justifies increment, then we'd better do them all together.
There was a problem hiding this comment.
Actually not obvious that number and placement do not change - I was not and is not sure about this and that was a reason to ask.
Also I have feeling that our unit tests unfortunately are not exhaustive in regards to number and placement of probes and especially in case of complex CFGs involving try-catch blocks, and unfortunately we don't have assertions about number of probes in validation tests and they are probably not exhaustive too.
And since you're also unsure, we'll need to study this more carefully - for example by counting number of probes on a big set of class files before and after this change as an addition to a more careful code review.
There was a problem hiding this comment.
@allenhair in fact after merge of #266 @marchof reverted increment of version in 82f40d7 prior to release 😉
And here I come to following conclusions: move of start and end labels of try-catch blocks between end and beginning of probe doesn't affect original labels and so doesn't affect MethodInstrumenter, i.e. placement and number of probes, as well as MethodAnalyzer, i.e. result of analysis, and affects only final class after instrumentation. I guess this is exactly what is described as "leads to semantically identical probes" in 82f40d7.
@marchof Am I right? WDYT?
There was a problem hiding this comment.
@Godin @allenhair If the change in #266 did not change the number of probes this new handling for the end labels will also not change the number of probes. So no need to change exec version.
Also if we would do so we should combine this with other changes in our backlog (see @Godin 's comment).
| "java/lang/Exception"); | ||
| expectedVisitor.visitTryCatchBlock(start, end, handler2, | ||
| expectedVisitor.visitTryCatchBlock(probe, end, handler2, |
There was a problem hiding this comment.
This test verifies that in case of probe at the beginning two invocations of visitTryCatchBlock will use the same first label, i.e. second invocation uses label created during first invocation. But there is no test to verify second label in case of probe at the end.
Godin
force-pushed
the
master
branch
2 times, most recently
from
03c269a
to
917989c
Compare
|
@marchof I restored version of exec-file, polished tests and comments and added changelog entry. Could you please re-review? If all fine, I'll squash and merge. |
|
@allenhair hope you don't mind that we took over finalization of this, thanks again for your valuable contribution! ❤️ |
|
No problem @Godin, thanks for pushing this through. |
Fixes #626