Bugzilla – Full Text Bug Listing
| Summary: | [null][compiler] 'Adding potential null mark in unexpected state' assertion while compiling | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Ayushman Jain <amj87.iitr> | ||||||||
| Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> | ||||||||
| Status: | VERIFIED INVALID | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | amj87.iitr, jarthana, satyam.kandula, srikanth_sankaran, stephan.herrmann | ||||||||
| Version: | 3.8 | ||||||||||
| Target Milestone: | 3.8 M6 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Ayushman Jain
Because of the new null analysis for fields, on compiling jdt.core with the option turned on we get Internal compiler error: org.eclipse.jdt.internal.compiler.flow.UnconditionalFlowInfo$AssertionFailedException: assertion failed: Adding 'potentially null' mark in unexpected state at org.eclipse.jdt.internal.compiler.flow.UnconditionalFlowInfo.isTrue(UnconditionalFlowInfo.java:1044) ClasspathEntry.java /org.eclipse.jdt.core/src/org/eclipse/jdt/internal/core line 0 Java Problem This needs to be investigated. Lowering priority since the option is off by default and compliation is ok out of the box I think the fix is straightforward. In the fix for bug 247564, we had marked non-constant fields as pot. null to consider modification in different threads. But we used markNullStatus, which only marks the potentially non null bit, keeping the other bits intact. What we should do here is to change the whole bit stream to 0100, which stands for pot. null. Oops, there's also an NPE :) java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.ast.FieldReference.variableBinding(FieldReference.java:686) Today's my lucky day java.lang.ArrayIndexOutOfBoundsException at org.eclipse.jdt.internal.compiler.flow.UnconditionalFlowInfo.addConstantFieldsMask(UnconditionalFlowInfo.java:1643) Are these due to some last minute changes ? I thought we were using eclipse SDK as a test case to mine for problems originally ? (In reply to comment #5) > Are these due to some last minute changes ? I thought we > were using eclipse SDK as a test case to mine for > problems originally ? While compiling the SDK, several other errors (viz. Buildpath issues, API issues) would've short-circuited the code paths which are now getting executed. Hence these errors. Not to worry though, fix is simple. Its a (few) silly mistake(s) :) Actually, the NPE was a false alarm because it doesn't have with the latest build. It occured in a runtime workbench because I had changes from bug 237236 in my workspace. However, its best to protect the condition with a null check. NPE happened on SourceTypeBinding:931 at this.superInterfaces.length == 1, where the declaring class of length is null Another interesting observation. The bug in comment 1 is actually in the resource leaks handling. In org.eclipse.jdt.internal.compiler.ast.FakedTrackingVariable.analyseCloseableExpression(..), we have used markPotentiallyNonNullBit(..) . I think we should have used markNullStatus(..) and go by the proper route. This will solve this unexpected state issue (In reply to comment #8) > Another interesting observation. The bug in comment 1 is actually in the > resource leaks handling. > In > org.eclipse.jdt.internal.compiler.ast.FakedTrackingVariable.analyseCloseableExpression(..), > we have used markPotentiallyNonNullBit(..) . I think we should have used > markNullStatus(..) and go by the proper route. This will solve this unexpected > state issue Confirmed that this fixes the bug. Running tests now. Both comment 3 and comment 4 were problems exposed by the fix for bug 237236 in my workspace. They don't occur with the current code, but I'm anyway including the fix for them as well. Releasing a disabled test meanwhile to check the AIOOBE which should be enabled in bug 237236's fix Created attachment 209925 [details]
proposed fix
This changes the markPotentialNullBit(..) to markNullStatus(..).
Stephan, since this patch touches your code, can you please see if everything looks ok? The patch is quite small.
Created attachment 209948 [details]
counter proposal
That is in interesting one indeed!
Yes, markPotentialXBit() must be used with care, ideally only when the null status of the variable is known to be the start state (0000).
However, the two locations in question meet this demand:
AbstractMethodDeclaration: before analysing any statements we init the status for the method's arguments. Impossible to be different from 0000 at this point.
FakedTrackingVariable: two lines above the line in question we see: "tracker = new FakedTrackingVariable(local, location)", which internally creates a new LocalVariableBinding into tracker.binding. No null status yet.
So changing any of these lines would only mask an error that must have happened earlier.
And indeed I found a bug that caused initialization to 1001 into local variables that hadn't even been created at that time. Next the fresh local from FakedTrackingVariable would find this 1001, try to set the pot.nul bit and bail out because our assumption above is broken.
How did we get non-existent locals initialized to 1001? By a simple loop overrun in resetNullInfoForFields(). For details see the patch (change in UFI plus test).
BTW: for using markNullStatus(var) another requirement must be met: the previous null status of this var must be irrelevant at this point. If that's not the case some kind of flow merging is necessary.
The change in FieldDeclaration looks OK. Yes one keeps forgetting that we have this one field (Array.length) that doesn't have a declaring class, sigh. Actually I would include (this.binding.declaringClass == null) into one of the conditions above for an early exit, no need to keep looping.
I did not fully evaluate the change in UFI#addConstantFieldsMask(). Do you have a test that would fail in HEAD without this change?
(In reply to comment #12) > Created attachment 209948 [details] > counter proposal > > And indeed I found a bug that caused initialization to 1001 into local > variables that hadn't even been created at that time. Next the fresh local from > FakedTrackingVariable would find this 1001, try to set the pot.nul bit and bail > out because our assumption above is broken. Yes your analysis is correct. :) > I did not fully evaluate the change in UFI#addConstantFieldsMask(). Do you have > a test that would fail in HEAD without this change? Yup there's a way to reproduce. Apply the patch from bug 237236 and then use the diasbled test case I added in the comment 11 patch. You get an AIOOBE. This is because although I handled the case when the length of the current mask is smaller than that of the one being added, I did not handle the opposite case. (In reply to comment #12) > BTW: for using markNullStatus(var) another requirement must be met: the > previous null status of this var must be irrelevant at this point. If that's > not the case some kind of flow merging is necessary. So in the two places where markPotnetiallyNullBit is used, we shouldn't change it to markNullStatus(..)? Created attachment 209955 [details] combined fix + tests This patch combines my fixes for NPE and AIOOBE, and Stephan's fix for 'adding pot. null mark'. I've also added regression tests for all these cases. One test is disabled for now, which will be enabled when bug 237236 is released. (In reply to comment #15) > Created attachment 209955 [details] > combined fix + tests Released in master via commit acc6e9dd91bf6b774847a32dd6da3bb481276405 (In reply to comment #14) > (In reply to comment #12) > > BTW: for using markNullStatus(var) another requirement must be met: the > > previous null status of this var must be irrelevant at this point. If that's > > not the case some kind of flow merging is necessary. > So in the two places where markPotnetiallyNullBit is used, we shouldn't change > it to markNullStatus(..)? Right. Leaving as is has two advantages: performance and catching bugs like this one. Verified for 3.8M5 using I20120124-2000 Reopening to synch with the base bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564 As this bug is implementation specific and since https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564 has been reopened, this can be closed as INVALID. Verified for 3.8M6 |