Community
Participate
Working Groups
Created attachment 188652 [details] comparator log showing changed classes, when using new compiler As documented in bug 336544, a JDT compiler regression, of sorts, was found in 3.6.x maintenance stream, so a) the Eclipse SDK is fixing that for 3.6.2, and b) are using a different "base builder" with the fixed compiler to recompile their final 3.6.2 delivery. (I say "regression, of sorts", since from the comments in the bug, sounds like the generated byte codes were "always" wrong, but due to another fix, got worse.) Some of the discussion around this change was on eclipse-pmc list, in this thread: http://dev.eclipse.org/mhonarc/lists/eclipse-pmc/msg01356.html The new version of the base builder is r36x_v20110209 We in WTP were previously using R36_RC4 base builder. It sounds like there's been other fixes since the compiler included in R36_RC4. That old compiler (jdt.core) was 3.6.0.v_A58.jar. The new version, in r36x_v20110209 base builder is 3.6.2.v_A76_R36x. I am not even sure when the regression was introduced ... so, the compiler we were using might not have had the regression in it. (I'll ask on original jdt bug). So, I have done a new WTP build, with the new compiler, and our handy-dandy comparator testing shows that there are seven classes with difference in byte codes produced by new compiler ... in some critical, core class ... I'll attach the comparator log/output. My _guess_ is that none of the differences are significant ... that is, as we saw on Indigo builds, there is simply some unused code being correctly omitted, instead of jumped over, or similar. But ... you never know (and, doubt I could figure it out from looking at the byte codes, without a few weeks of work). A purist might argue that we should stick with old byte codes, simply because a "known quantity that's been tested" is better then a _possibly_ more correct version, that has not been as well tested. But, if this is going to be long-lived code ... it might be better/easier to maintain if bytes codes are "correct" (according to newest JDT compiler), rather than changed later. Put another way, in the rare event that some of the byte codes are wrong, it might lead to rare, unusual, difficult to reproduce bugs. So, the point we are at ... the new compiler produces different byte codes, but via the magic of p2 mirror operations, the old versions are included in our current build distribution, since the bundle qualifiers have not changed. So, if we want to use the "new" _possibly_ more correct byte codes, then we would have to re-tag the bundles listed in the comparator log, and re-build, to get the new bytes code in the build distribution.
The comparator logs do not show up yet, and the rest of the JUnit tests are still running (I "peek" directly on build server) but the build that will include this change is http://build.eclipse.org/webtools/committers/wtp-R3.2.3-M/20110210015622/M-3.2.3-20110210015622/ (And, for future reference, that is a short lived URL ... will change in week or so).
Once you have the name of the 7 different class files, I'd like to investigate. I would need to old .class files and the new ones. Thanks.
Sorry, just saw the attachment. I would need to "old" .class files and the new ones. I'd like to find out what is different. I need the .class files for: org/eclipse/jst/jsp/core/internal/java/jspel/Token.class org/eclipse/wst/sse/ui/internal/StorageModelProvider.class org/eclipse/wst/xsl/internal/debug/ui/tabs/main/TransformsBlock.class org/eclipse/wst/jsdt/internal/codeassist/SelectionEngine.class org/eclipse/wst/xml/core/internal/contenttype/XMLHeadTokenizer.class org/eclipse/jst/j2ee/model/internal/validation/EJBJar20VRule.class org/eclipse/wst/jsdt/core/tests/compiler/regression/AbstractRegressionTest.class I can either get the .class files isolated or the jars that contain both versions of the .class files. Thanks.
Thanks for the offer to check the byte codes. I'd probably be less error prone if I gave you links for "everything" then let you download/navigate to them in a way easiest for you. The first 6 are in "real" code, the last one in unit tests. For the real code, there is an archived repo at http://build.eclipse.org/webtools/committers/wtp-R3.2.3-M/20110210015622/M-3.2.3-20110210015622/wtp-repo-M-3.2.3-20110210015622.zip to compare to http://build.eclipse.org/webtools/committers/wtp-R3.2.3-M/20110209071934/M-3.2.3-20110209071934/wtp-repo-M-3.2.3-20110209071934.zip Similar for the unit test class: http://build.eclipse.org/webtools/committers/wtp-R3.2.3-M/20110210015622/M-3.2.3-20110210015622/wtp-tests-repo-M-3.2.3-20110210015622.zip to compare to http://build.eclipse.org/webtools/committers/wtp-R3.2.3-M/20110209071934/M-3.2.3-20110209071934/wtp-tests-repo-M-3.2.3-20110209071934.zip If easier for you to use a shell account or scp, the file locations are the same as above, but with '/shared' instead of 'http://build.eclipse.org'. Or, your third option, if easier to use a shell account to directly to the repo, instead of the final zip file segment, you would use '/repository/ and '/repositoryunittests/' Or, your fourth option ... tell me you'd want me to sort it all out :) Above, I list only the "previous build", where compiler was only change. The comparator tests name differences in 3 or 4 various reference repositories. We do use all those references in our comparator tests (they are previously declared weekly builds) but I think there must be some indeterminacy in the way the mirror task (and its associated comparator) pick out the "latest bundle with same qualifier". The same "old version" must be in the previous build, I reference above. I'd be happy to "hand pick" the jars instead of you ... but, will take me a few hours to get to. So, I'll start that task later in the day if you'd prefer.
doh, in comment #4 I listed the "new compiled jars" in /repository and /repositoryunittests, but ... now that I'm waking up :) ... I realized those repos contain the _results_ of the mirror operation, which would take the "old" bundle with same qualifier ... the newly compiled bundles are indeed only on build machine (not in zip files) in the various directories listed in comparator log, such as .../buildrepository/jst-sdk .../buildrepository/wst-sdk .../buildrepository/wst.tests
I don't have ssh access to the build machine. So unless you find a way to give me the required jars, I cannot investigate the differences.
Created attachment 188697 [details] zip containing jars to compare, in 'old' and 'new' subdirectories Here's the 'old' and 'new' jars. The zip file is 17Meg, so I marked as "big file" so it will be removed from bugzilla, at some point. (And ... I will also email to you, Olivier, so maybe one of the methods will work).
I got it. I am investigating.
org/eclipse/jst/jsp/core/internal/java/jspel/Token.class is a difference about the way the following statement is compiled: switch(ofKind) { default : return new Token(ofKind, image); } This is a consequence of the fix for bug 314830. org/eclipse/wst/sse/ui/internal/StorageModelProvider.class if (document == null) { document = model.getStructuredDocument(); } We now generate the null check. Before we skipped it as document can only be null at this location. So the change is perfect safe. org/eclipse/wst/xsl/internal/debug/ui/tabs/main/TransformsBlock.class Same code pattern than the first issue: protected void setSortColumn(int column) { switch (column) { // case 1: // sortByName(); // break; // case 2: // sortByType(); // break; } super.setSortColumn(column); } I would say the whole switch statement should be removed. org/eclipse/wst/jsdt/internal/codeassist/SelectionEngine.class Same thing again: The code is: switch (kind) { default: .... } I don't see the point of having a switch statement. org/eclipse/wst/xml/core/internal/contenttype/XMLHeadTokenizer.class Same thing again. org/eclipse/jst/j2ee/model/internal/validation/EJBJar20VRule.class This looks more aggressive as we removed some null checks. The change for the catch Throwable looks suspicious. So I'll take a second look at the EJBJar20VRule class.
(In reply to comment #9) > org/eclipse/jst/j2ee/model/internal/validation/EJBJar20VRule.class > This looks more aggressive as we removed some null checks. The change for the > catch Throwable looks suspicious. > So I'll take a second look at the EJBJar20VRule class. Everything is fine. I was confused with another null check being removed. This is the one for the logger check and not the bean inside the Throwable catch blocks. So I reviewed all changes in "real" code and none of them is wrong or a false positive. Some code could be changed to "improve" the code generation like unnecessary switch statements, but as it stands, there is no risk going with the .class files generated by the "new" compiler. Hope this help.
(In reply to comment #10) > (In reply to comment #9) > > org/eclipse/jst/j2ee/model/internal/validation/EJBJar20VRule.class > > This looks more aggressive as we removed some null checks. The change for the > > catch Throwable looks suspicious. > > So I'll take a second look at the EJBJar20VRule class. > Everything is fine. I was confused with another null check being removed. This > is the one for the logger check and not the bean inside the Throwable catch > blocks. > So I reviewed all changes in "real" code and none of them is wrong or a false > positive. Some code could be changed to "improve" the code generation like > unnecessary switch statements, but as it stands, there is no risk going with > the .class files generated by the "new" compiler. > Hope this help. Olivier, I don't understand your statement: "This is the one for the logger check". Is there unnecessary code that we can clean up in EJBJar20VRule ? If so, what?
I retagged org.eclipse.jst.j2ee.core with v201102101900 and updated the tag in the jst-j2ee-basic.map file. Nitin will have to update the tag for the org.eclipse.jst.web_core.feature, since that is in sourceediting
(In reply to comment #11) > Olivier, I don't understand your statement: "This is the one for the logger > check". Is there unnecessary code that we can clean up in EJBJar20VRule ? If > so, what? Sorry my mistake. We do keep the null check on logger. I got confused by the diff between old and new .class files. The only thing that can be cleaned is the if (bean != null) { ... } line 268. bean cannot be null at this location. Sorry for the misunderstanding.
(In reply to comment #12) > I retagged org.eclipse.jst.j2ee.core with v201102101900 and updated the tag in > the jst-j2ee-basic.map file. > > Nitin will have to update the tag for the org.eclipse.jst.web_core.feature, > since that is in sourceediting I've tagged org.eclipse.jst.web_core.feature as v201102101900 and the remainder as v201102102045.
Thanks to all. I'm marking as fixed since work has been done. The rebuild has been started. ETA: midnight?
