Bugzilla – Full Text Bug Listing
| Summary: | [compiler] NPE while compile a method with unused local | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Olliver Johann <ojohann> | ||||||
| Component: | Core | Assignee: | Srikanth Sankaran <srikanth_sankaran> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, jarthana, Olivier_Thomann, patric, roger.butenuth, satyam.kandula, stephan.herrmann | ||||||
| Version: | 3.8 | Flags: | daniel_megert:
pmc_approved+
stephan.herrmann: review+ Olivier_Thomann: review+ |
||||||
| Target Milestone: | 3.7.2 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Olliver Johann
Created attachment 209361 [details]
eclipse project with src
eclipse project with src
I couldn't reproduce this with 3.8M4. You are saying that the given two java files in the attached zip don't compile? What compile error do you get? I could compile and even run those files alright. Sorry I could reproduce this if I import the whole project. Individually those 2 java classes don't cause a problem. The bug happens only when "Preserve unused locals" is disabled. So the workaround is to enable that option till this gets fixed. We have found this error in our project with a tomcat v7_21 with use ecj-3.7.jar. In this envirorment it is not possible to enable "Preserve unused locals" and the default is "off". Wow. A very interesting problem indeed. Bug arises due to interplay between the two restart modes one for goto_w generation and another for unused local handling introduced for https://bugs.eclipse.org/bugs/show_bug.cgi?id=185682. Having restarted for one cause, when we are forced to restart for another, we claim it is futile to do so having attempted restart already. Fix is to encode the reason for restart so we don't attempt the same restart twice, but will accommodate disparate restart requests. Patch will follow shortly. Seems like another candidate in the works for 3.7.2! I wish we would have gotten rid of the restart from bug 328519, sigh. Somehow we never finished the discussion in bug 328830, which btw. has a lot of background info on this issue. Created attachment 209388 [details]
Patch under test
Released in 3.8 M5 via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=6d6867f51ab2593e87423b03682d06ffbcaebc01. Ayush, please review for possible 3.7.2 inclusion. Stephan, please review for 3.7.2 possible inclusion. TIA. (In reply to comment #11) > Stephan, please review for 3.7.2 possible inclusion. TIA. With memories of non-termination due to infinite restarts (see bug 328830 comment 22) I'm reluctant to approve this patch. Aren't we now abandoning our back door out of the restart loop? Are we sure that the number of restarts is finite? How? Srikanth, do you have a good story to comfort my worries? (In reply to comment #12) > (In reply to comment #11) > > Stephan, please review for 3.7.2 possible inclusion. TIA. > > With memories of non-termination due to infinite restarts > (see bug 328830 comment 22) I'm reluctant to approve this patch. > Aren't we now abandoning our back door out of the restart loop? > Are we sure that the number of restarts is finite? How? > > Srikanth, do you have a good story to comfort my worries? Hakuna matata ! It is easier to review this patch in 3.8 HEAD by comparing with previous element than by looking at the patch posted. Some observations: -- All changes are under an if of the form: if (e.compilationResult == CodeStream.RESTART_IN_WIDE_MODE) So any behavior change can come about only when the most recent abort cause is branch offset overflow. -- We want to support RESTART_IN_WIDE_MODE arising after a potential RESTART_CODE_GEN_FOR_UNUSED_LOCALS_MODE. In fact the current bug is due to our failing to do so. -- The number of RESTART_IN_WIDE_MODE's not only finite, but is exactly one. See that the throws of the form: throw new AbortMethod(CodeStream.RESTART_IN_WIDE_MODE, null); are guarded by checks to if !this.wideMode. -- codeStream.wideMode is not altered within the code generation loop after it is set to wide mode just before restart. I think it is a good idea to mark Olivier for review since he made some changes around this area recently. Olivier, if you have a moment, appreciate a quick review. TIA. (In reply to comment #13) > -- codeStream.wideMode is not altered within the code generation > loop after it is set to wide mode just before restart. BTW, the only reset that happens happens in AMD.generateCode(..) at the top. Similar resets should be there in CDi.java and Clinit.java - No ? The absence is harmless (though will generate slightly bigger class files.) (In reply to comment #13) > -- codeStream.wideMode is not altered within the code generation > loop after it is set to wide mode just before restart. (In reply to comment #14) > BTW, the only reset that happens happens in AMD.generateCode(..) > at the top. See that AMD.generateCode itself may be reentered while compiling something like: public class X { void foo() { new X() { void goo() { // reenter generate code for method } }; } } but this will be for a different class file and a different code stream. Since methods of a class don't nest and further since the wide mode flag is an instance variable, we are good. Confirmed that this problem exists on 3.7.1.
How common is this problem and what is required to
trigger it ?
-- During code generation there should be a requirement for a
goto bytecode whose target is > 7FFF in either direction.
e.g: if ( ... ) {
// huge block of code here...
}
Think of generated code.
AND
-- Later stages of code generation discovering that they need to
emit some local variable which earlier phases thought was unused.
Workarounds:
-- Per comment#4, set "Preserve unused locals" to on.
-- Tweak the code to "use" those variables - this can be hard
to figure out.
(In reply to comment #6) > Fix is to encode the reason for restart so we don't attempt the > same restart twice, but will accommodate disparate restart requests. > Patch will follow shortly. For the record, this was the not the fix adopted. We now use a simpler restart loop which recognizes that a restart in wide mode can happen but once per method. (In reply to comment #3) > Sorry I could reproduce this if I import the whole project. Individually those > 2 java classes don't cause a problem. Actually one of them always fails to compile and the other does compile fine. The broken case is enough, the good one is there just to show (I think) that some incremental changes break the compiler (by causing the branch offsets to overflow) Already released for 3.8 M5. Based on review feedback, will decide the course of action for 3.7.2. (In reply to comment #18) > (In reply to comment #3) > > Sorry I could reproduce this if I import the whole project. Individually those > > 2 java classes don't cause a problem. > > Actually one of them always fails to compile and the other does compile > fine. The broken case is enough, the good one is there just to show > (I think) that some incremental changes break the compiler (by > causing the branch offsets to overflow) This is not true, both fail (if preserve locals == false), but in different ways: - CheckCompile does not show any error in Eclipse, but the compiled class is broken (contains only a throw with "unresolved compilation problem". - CheckCompileSimple causes a NPE in the compiler (In reply to comment #20) [...] > > fine. The broken case is enough, the good one is there just to show > > (I think) that some incremental changes break the compiler (by > > causing the branch offsets to overflow) > > This is not true, both fail (if preserve locals == false), but in different > ways: > - CheckCompile does not show any error in Eclipse, but the compiled class is > broken (contains only a throw with "unresolved compilation problem". > - CheckCompileSimple causes a NPE in the compiler I see, Thanks for the clarification. I had observed this dual failure mode, but hadn't realized that the two test cases are showcase each problem. The patch fixes both issues :) (In reply to comment #21) > (In reply to comment #20) > > [...] What about 3.7.2? The bug is quite nasty: You get a corrupt class file without an error message during compile. (In reply to comment #22) > What about 3.7.2? The bug is quite nasty: You get a corrupt class file without > an error message during compile. See comment#19. We are in 3.7.2 RC2, so multiple reviews are required and have been requested. Looks good to me. (In reply to comment #24) > Looks good to me. Thanks Olivier. Stephan, sorry to nag while you are busy, this needs two reviews and needs to make it to RC2 build 18 Jan if possible. Dani, even as we are waiting for a second review, a heads up that this needs a back port. Ideally for RC2. (In reply to comment #26) > Dani, even as we are waiting for a second review, a heads up > that this needs a back port. Ideally for RC2. This is a severe regression that got introduced during 3.7. I verified that the patch fixes the problem but did not do a full code review. +1 to backport once we have the second review. To turn my vague memories into knowledge I traveled back in time to the time of bug 328830 comment 22. The issue back then did not involve restart for wide mode but arose because a restart for unused locals was not able to remedy its cause. This happened because in a method in a local class by referring to a local from the enclosing method we made that local relevant - the restart was unsuccessful because it could only influence generating locals in the current method, but would have needed to influence the enclosing method. We are still only a few line changes apart from that situation occurring again, and for restart for local generation it is not easy to show the absence of infinite regression. That's why I still think this use of the restart mechanisms should be backed out. So much for the records. For the current bug I understand that the patch only affects restart for wide mode. For this case and based on the clear reasoning in comment 13 I'm convinced that restart will happen at most once per method. Thus the change is safe. For further follow-up regarding comment 14 one should check if the comment in Clinit is still relevant ("// should never occur ..."). +1 for backporting. (In reply to comment #28) > and for restart for local generation it is not easy to show the absence of > infinite regression. That's why I still think this use of the restart > mechanisms should be backed out. Can you raise a separate defect for this ? Or is bug 328830 the right one to discuss this ? > +1 for backporting. Ayush, can you please release this in 3.7.2 stream along with the fix for bug 361963, TIA. Jay - FYI. (In reply to comment #29) > (In reply to comment #28) > > > and for restart for local generation it is not easy to show the absence of > > infinite regression. That's why I still think this use of the restart > > mechanisms should be backed out. > > Can you raise a separate defect for this ? Or is bug 328830 the right one > to discuss this ? I don't think we need a new bug, bug 328830 has all the context and all the options. (In reply to comment #29) > Ayush, can you please release this in 3.7.2 stream along with the fix for > bug 361963 http://images.fanpop.com/images/image_uploads/i-can-but-i-won-t--garfield-262532_1024_768.jpg PS: Just kidding :) Released into 3.7 maintenance via commit 8d116352a80813f7ae91cb458eb818290b4d92c0 Verified for 3.7.2RC2 using build M20120118-0800 This seem to have been missed out in our M5 verification. Marking it for M6. Verified for 3.8M6 using build I20120306-0800 |
