Bugzilla – Full Text Bug Listing
| Summary: | Compiler produces incorrect bytecode for null pointer check | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Tom Poindexter <tpoindex> | ||||||||||||
| Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> | ||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||
| Severity: | major | ||||||||||||||
| Priority: | P3 | CC: | Olivier_Thomann, srikanth_sankaran, stephan.herrmann | ||||||||||||
| Version: | 3.7 | Flags: | amj87.iitr:
review+
|
||||||||||||
| Target Milestone: | 3.6.1 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 174761 [details]
TclInputStream.java
This is the source file, see method "getsObj", line 215
Created attachment 174762 [details]
TclInputStream.asm.helios
Bytecode ("javap -c" output) as produced by Eclipse 3.6 Helios
Created attachment 174763 [details]
TclInputStream.asm.javac-1.5
Bytecode ("javap" output) as produced by JDK 1.5 javac compiler.
As a workaround you can replace: buf.BUFFER_PADDING; with: CharBuffer.BUFFER_PADDING; Simple test case:
public class X {
static class B {
public static final int CONST = 0;
int i;
}
B b;
public static void main(String[] args) {
new X().foo();
}
void foo() {
B localB = b;
int i = localB.CONST;
if (localB != null) {
i = localB.i;
}
System.out.println(i);
}
}
The problem comes from the localB.CONST. This records that localB cannot be null, but in this case it is a reference to a static field using an instance. The call is never made against the instance itself preventing a NPE from occurring on that call.
Ayushman, please investigate. We cannot optimize the null check on localB when it is used to refer to a static field. It works fine if the instance is used to access a static method.
If either localB is replaced with 'B' or the field CONST is not a static field, then the code would be fine.
This is a candidate for 3.6.1.
Created attachment 174770 [details]
Proposed fix + regression test
Patch to be reviewed. Might be too naive and miss other cases.
I didn't run the tests.
(In reply to comment #6) > Created an attachment (id=174770) [details] > Proposed fix + regression test It seems your patch doesn't prevent the null-warning on localB.CONST? Is that by intention? Otherwise I would suggest to simply guard the *calls* to checkNPE like: if (needValue) checkNPE(currentScope, flowContext, flowInfo, true); (In reply to comment #7) > It seems your patch doesn't prevent the null-warning on localB.CONST? > Is that by intention? Otherwise I would suggest to simply guard the > *calls* to checkNPE like: > > if (needValue) > checkNPE(currentScope, flowContext, flowInfo, true); I was not sure the call could be completely ignored. Created attachment 174855 [details]
Proposed fix + regression test
Previous patch modified to match Stephan's comment.
(In reply to comment #9) > Created an attachment (id=174855) [details] > Proposed fix + regression test > > Previous patch modified to match Stephan's comment. Thanks Olivier. Patch is good. I'll run all the tests and request review. Ayushman, please review the last patch. All tests pass. Patch looks good. Released for 3.6.1. Released for 3.7M1. Added regression test: org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#testBug320414 Verified for 3.7 M1 using build I20100802-1800 Verified for 3.6.1 RC2 using Build id: M20100825-0800 |
Build Identifier: 20100617-1415 The compiler produces code that seems to aggressively optimize a null object check, resulting in the body of an "if" statement to always be executed. Here is an abbreviated version of the code: class TclInputStream { ChannelBuffer inQueueHead = null; int getsObj(TclObject tobj) { GetsState gs; ChannelBuffer buf; boolean oldEncodingStart, oldEncodingEnd; int oldRemoved, skip, inEofChar; int copiedTotal, oldLength; boolean in_binary_encoding = false; int dst, dstEnd, eol, eof; final boolean debug = false; buf = inQueueHead; oldLength = 0; oldEncodingStart = encodingStart; oldEncodingEnd = encodingEnd; oldRemoved = buf.BUFFER_PADDING; if (buf != null) { oldRemoved = buf.nextRemoved; } } } The statement "if (buf != null)" is not generated in the bytecode. The body of the if statement is always executed (resulting in a NullPointerException.) See attached file "TclInputStream.asm.helios" (produced with "javap -c") Note- Eclipse 3.5 produced correct code, as does the JDK 1.5 javac. Reproducible: Always Steps to Reproduce: 1. Full project code available at: http://kenai.com/projects/jtcl/sources 2. Check out code using Mercurial (or use the Mercurial plugin): hg clone https://hg.kenai.com/hg/jtcl~jtcl-main cd jtcl~jtcl-main 3. Update to revision 41: hg update -r 41 4. Import as eclipse project, allow Eclipse to compile project 5. File that produces faulty code: src/main/java/tcl/lang/channel/TclInputStream.java 6. See line 215 in TclInputStream.java, method "getsObj()" (this file is also attached) 7. Decompile java class file: target/classes/tcl/lang/channel/TclInputStream.class 8. Examine bytecode for "getsObj" method.