Keep ConstantPool.getConstant(int) backward compatible with v6.5.0 #157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Kengo TODA <skypencil@gmail.com>
full stack trace for referenceNote that there is another problem on the spotbugs side: SpotBugs iterates on the constant pool with But even after I applied a patch to replace it with |
|
Hi @KengoTODA There must be a better way to deal with this issue though because this PR will cause NPEs instead of a useful ClassFormatException message in all the BCEL call sites. Any ideas? |
|
I do not see what you are seeing, if I create a class |
|
Really interesting, it could be compiler dependent? I can reproduce it with AdoptOpenJDK-11.0.11+9. I'll try to add a unit test to reproduce. Thanks. |
|
full |
Signed-off-by: Kengo TODA <skypencil@gmail.com>
|
Confirmed that this problem can be reproduced with Java version |
Signed-off-by: Kengo TODA <skypencil@gmail.com>
KengoTODA
force-pushed
the
keep-backward-compatible
branch
from
170f5b2
to
767760f
Compare
|
I was using Eclipse |
| @@ -122,6 +123,7 @@ public void testB307() { | |||
| * BCEL-308: NullPointerException in Verifier Pass 3A | |||
| */ | |||
| @Test | |||
| @Disabled("ConstantPool item could be null even when class file is valid") | |||
There was a problem hiding this comment.
This test failed after I removed the verification logic, but my new suggestion is enough to pass this case so I reverted this @Disabled annotation. 馃
Signed-off-by: Kengo TODA <skypencil@gmail.com>
I see, then this problem should be OpenJDK-based compiler specific.
I proposed bc0f1ac which throws an exception only when the class file format is really invalid. Could you review it when you have time? Thanks in advance! |
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
=========================================
Coverage 44.45% 44.46%
- Complexity 2518 2520 +2
=========================================
Files 362 362
Lines 15835 15838 +3
Branches 1988 1990 +2
=========================================
+ Hits 7040 7042 +2
Misses 8090 8090
- Partials 705 706 +1
馃摚 We鈥檙e building smart automated test selection to slash your CI/CD build times. Learn more |
asfgit
pushed a commit
that referenced
this pull request
Oct 16, 2022
|
Hi @KengoTODA |
|
I'll check it, thanks for reminding me! |
|
note: I've checked the implementation of OpenJDK. I found the code that skips classfile entry after long constants and double constants, but cannot find its intention because the first commit already contains this implementation. |
|
@KengoTODA |
1 task
|
Hello @KengoTODA |
|
Wmm, I don't think so. BCEL 6.6.1 has no other big changes, but they tried with BCEL 6.6.2-SNAPSHOT, then it could be necessary to investigate rel/commons-bcel-6.6.1...master? |
|
Thanks @KengoTODA |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello, thanks for keeping commons-bcel maintained! It surely helps the Java community a lot. 馃檶
I'm using commons-bcel to maintain bytecode analysis tool like SpotBugs.
Today I found that BCEL v6.6.0 makes test cases in SpotBugs failed: spotbugs/spotbugs#2213
The reason is that,
ConstantPool#getConstant(int)throws aClassFormatExceptionifconstantPool[index]isnull. But it is possible even when the class file is valid: you can verify it by making a simple class like below:Then
javac ClassWithNullConstantPoolItem.java && javap -v ClassWithNullConstantPoolItemwill print a constant pool like below:We can confirm that there is no
#3, and in such case,getConstant(3)becomes null.In our repository, Issue389.java is one of such cases.
So in my opinion, it is not so good to throw
ClassFormatException. Here I suggest rollback this change made in v6.6.0. Thanks for checking my PR!