Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
- Original Message - On 8/23/12 5:12 PM, Andrew Hughes wrote: Dan Xu wrote: Please review the fix of CR 7193406 at http://cr.openjdk.java.net/~dxu/7193406/webrev/. And it enables fatal warning flag in the following make file. make/java/jar/Makefile Please don't do this, at least not unconditionally. Right, we had been enabling fatal warnings in the various makefiles but we've stopped doing so since it has started to cause problems. The intent was to enable fatal warnings by default in individual makefiles, as those areas of the build were cleared of warnings. That way, the introduction of a new warning into a warnings-free area **would** break the build. I understand the motivation and why the current setup would be needed in transition. However, once the whole build is warning free, would it not be preferable to remove these and just set JAVAC_WARNINGS_FATAL=true when doing development builds? The problem I see is someone new coming to OpenJDK and not being able to simply build it (with no changes) because a new warnings has appeared and is being treated as an error. This is less of a problem with javac as we control its development, and the JDK will use the javac built in the langtools step in most cases. But, generally, -Werror is something you should choose to enable, with the intention of fixing failures, not something that should be forced on everyone building the code. On a related topic, it would be nice if javadoc could also support -Werror as I constantly see warnings reappearing in the documentation. The problem is that because of implicit compilation, as code is modified, files can shift around being compiled by different Makefiles. Thus an apparently innocuous change might cause a file with warnings to be built by a different javac run from a makefile that has fatal warnings enabled, causing an unexpected build breakage. Anyway, Dan, please don't enable this flag in this (or any other) Makefile. Sorry, I thought I had mentioned this to you before. At the very least, these should not be forced on if the user has explicitly built with them set to false. If I set JAVAC_WARNINGS_FATAL=false, I don't expect part of the build to ignore this and use -Werror, possibly then causing build failures. Yes, it should always be possible to override this by specifying JAVAC_WARNINGS_FATAL=false on the make command line. This overriding should work if the value is specified directly in a Makefile, e.g. JAVAC_WARNINGS_FATAL = true However, there are several cases where the following occurs: SUBDIRS_MAKEFLAGS += JAVAC_WARNINGS_FATAL=true and this is **not** overridable on the command line. That's wrong. If these are causing problems for you, please do submit patches. Yes, that's what we have in java/tools and is why JAVAC_WARNINGS_FATAL=false doesn't completely remove -Werror at present. I'll post a webrev of the change I have for this. Although we still occasionally run into problems with implicit compilation causing unexpected build failures, I don't want to remove the fatal warnings settings wholesale. The settings in place now do seem to be keeping at least some parts of the build warning-free. The new build system will change all of this completely, of course. I don't think they have a solution yet for applying different flags to different parts of the build. In FilePermission.java file, I make one change to its method signature, public Enumeration elements() == public EnumerationPermission elements() It's in a package-private class so I doubt it. Even if it wasn't, a new major release is the perfect time to fix these issues. Yes, this one is fine because it's a private class. For warnings fixes we're trying to avoid making any API changes, since those have to go through some additional process steps. s'marks -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
- Original Message -
Hi Dan,
On 24/08/2012 7:46 AM, Dan Xu wrote:
Please review the fix of CR 7193406 at
http://cr.openjdk.java.net/~dxu/7193406/webrev/.
It cleans up warnings in the following java files.
src/share/classes/java/io/FilePermission.java
I'm surprised that you need this:
426 @SuppressWarnings(fallthrough)
...
436 switch (actions) {
437 case SecurityConstants.FILE_READ_ACTION:
438 return READ;
If this generates a fallthrough warning then I think whatever tool
generates that warning needs fixing!
src/share/classes/java/util/ArrayDeque.java
src/share/classes/java/util/Arrays.java
src/share/classes/java/util/Collections.java
src/share/classes/java/util/HashMap.java
src/share/classes/java/util/JumboEnumSet.java
src/share/classes/java/util/PriorityQueue.java
src/share/classes/java/util/PropertyResourceBundle.java
Here:
! @SuppressWarnings({ unchecked, rawtypes })
! MapString,Object result = new HashMap(properties);
! lookup = result;
why do you keep the raw type instead of using HashMap(properties) ?
src/share/classes/java/util/jar/JarVerifier.java
src/share/classes/java/util/jar/Pack200.java
src/share/classes/sun/util/PreHashedMap.java
And it enables fatal warning flag in the following make file.
make/java/jar/Makefile
I'm not sure what Andrew's objection is to this change as it only
affects how java/util/jar classes get compiled and is consistent with
the usage in all the other Makefiles. As far as I can see you can
override this on the make invocation anyway.
Well, it's a bigger problem with HotSpot where the compiler is outside
the remit of the OpenJDK build, but enabling -Werror by default increases
the chance of build breakage when no changes have even been made to the
source code. I've lost count of the number of times we've had to patch
HotSpot due to -Werror failures. It's happened twice this summer alone.
I'm not so much worried about us encountering failures (we'd probably
still turn it on during development anyway), but it makes OpenJDK less
approachable and puts off potential new developers.
With javac, as the compiler is usually built prior to the jdk build, it's
less this issue and more the implicit compilation issue Stuart mentioned
i.e. javac tries to compile a file outside util/jar which has not yet
been declared warning free. This problem will disappear as the entire
source code base is cleaned up.
I'm unclear how this is handled in the new build.
In FilePermission.java file, I make one change to its method
signature,
public Enumeration elements() == public EnumerationPermission
elements()
I am not sure whether it will cause an issue of backward
compatibility.
Please advise. Thanks!
As Andrew stated this is a package-private class so I don't see any
issue.
Cheers,
David
- Dan
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
Hi Mandy,
--- old/src/share/classes/com/sun/jmx/remote/internal/IIOPHelper.java Thu Aug
23 12:29:01 2012
+++ new/src/share/classes/com/sun/jmx/remote/internal/IIOPHelper.java Thu Aug
23 12:29:00 2012
@@ -52,7 +52,7 @@
AccessController.doPrivileged(new PrivilegedActionIIOPProxy() {
public IIOPProxy run() {
try {
-Class? c = Class.forName(IMPL_CLASS, true, null);
+Class? c = Class.forName(IMPL_CLASS);
Why not:
Class? c = Class.forName(IMPL_CLASS, true,
IIOPHelper.class.getClassLoader());
to avoid traversing up the call stack to obtain the calling class.
Paul.
On Aug 23, 2012, at 9:33 PM, Mandy Chung mandy.ch...@oracle.com wrote:
On 8/23/2012 11:58 AM, Alan Bateman wrote:
On 23/08/2012 18:43, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw repo [1]
to jdk8. This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/
This looks good to me and it's good to have these changes in jdk8.
One suggestion for ReflectUtil is to add a private static boolean isAncestor
method as I think that would make the checks in needsPackageAccessCheck
easier to read. Also in ClassLoader you could use just use
needsClassLoaderPermissionCheck(from,to).
Done. This is a good cleanup I considered to do too but missed in the
previous webrev.
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/
Thanks for the review.
Mandy
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 08/23/2012 11:46 PM, Dan Xu wrote: Please review the fix of CR 7193406 at http://cr.openjdk.java.net/~dxu/7193406/webrev/. It cleans up warnings in the following java files. src/share/classes/java/io/FilePermission.java src/share/classes/java/util/ArrayDeque.java src/share/classes/java/util/Arrays.java src/share/classes/java/util/Collections.java src/share/classes/java/util/HashMap.java src/share/classes/java/util/JumboEnumSet.java src/share/classes/java/util/PriorityQueue.java src/share/classes/java/util/PropertyResourceBundle.java src/share/classes/java/util/jar/JarVerifier.java src/share/classes/java/util/jar/Pack200.java src/share/classes/sun/util/PreHashedMap.java And it enables fatal warning flag in the following make file. make/java/jar/Makefile In FilePermission.java file, I make one change to its method signature, public Enumeration elements() == public EnumerationPermission elements() I am not sure whether it will cause an issue of backward compatibility. Please advise. Thanks! - Dan Hi Dan, I'm not sure to like the fact that you introduce some local variables just to get ride of some warnings given that Hotspot compilers are sometimes sensitive to that. I think this practice should be discussed on this list before committing this changeset. so is it a good idea to add a temporary local variable to fix a generics warning or should @SuppressWarnings should be set on the whole method ? Rémi
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Hi all, On 08/23/2012 11:46 PM, Dan Xu wrote: Please review the fix of CR 7193406 at http://cr.openjdk.java.net/~dxu/7193406/webrev/. It cleans up warnings in the following java files. src/share/classes/java/io/FilePermission.java src/share/classes/java/util/ArrayDeque.java src/share/classes/java/util/Arrays.java src/share/classes/java/util/Collections.java src/share/classes/java/util/HashMap.java src/share/classes/java/util/JumboEnumSet.java src/share/classes/java/util/PriorityQueue.java src/share/classes/java/util/PropertyResourceBundle.java src/share/classes/java/util/jar/JarVerifier.java src/share/classes/java/util/jar/Pack200.java src/share/classes/sun/util/PreHashedMap.java And it enables fatal warning flag in the following make file. make/java/jar/Makefile In FilePermission.java file, I make one change to its method signature, public Enumeration elements() == public EnumerationPermission elements() I am not sure whether it will cause an issue of backward compatibility. Please advise. Thanks! - Dan Hi Dan, I'm not sure to like the fact that you introduce some local variables just to get ride of some warnings given that Hotspot compilers are sometimes sensitive to that. I think this practice should be discussed on this list before committing this changeset. so is it a good idea to add a temporary local variable to fix a generics warning or should @SuppressWarnings should be set on the whole method ? Rémi Is it worth analysing the byte code to see the potential impact of this extra local variable? We did that for a bunch of the javac warnings cleanup in London and had a defacto rule of 'no byte code changes' or if there was one we'd get a few Hotspot enthusiasts to analyse it. Cheers, Martijn
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
I also vote for adding this overload, a lot of the usage of Class.forName with three parameters are in fact just a way to load a class without initialize it. Rémi On 08/24/2012 07:21 AM, serguei.spit...@oracle.com wrote: I was thinking about the same. But a CCC request is needed for such a change in public API. It can be done separately if any other API changes are necessary. Thanks, Serguei On 8/23/12 6:27 PM, David Holmes wrote: Hi Mandy, While I understand what you are doing and that it works it is far from obvious upon code inspection that where you replace null with Foo.class.getClassLoader(), that the result would always be null. Another way to simplify this would be to add a new overload of Class.forName(): public static Class? forName(String name, boolean initialize) That way the loader argument could be dropped completely from a number of the use-cases. David On 24/08/2012 3:43 AM, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ Summary: The VM bootstrap class loader (null) has been the defining class loader for all system classes (i.e. rt.jar and any classes on the bootclasspath). In modular world, the system classes are going to be modularized into multiple modulesand and many of which will be loaded by its own (non-null) module loader. The JDK implementation has the assumption that the defining class loader of system classes is null and it'll no longer be true when running as modules. Typical patterns in the JDK code include: Class.forName(classname, false, null) should be changed to: Class.forName(classname, false,loader of the current class) if (loader == null) condition check should be modified to check if the loader is responsible for loading system classes. This is the first set of change for this problem we identified. Similar change in other components will be made in the future. Testing: run all jdk core test targets on all platforms. Thanks Mandy P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in the java.lang.management.
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Rémi Forax wrote: Hi Dan, I'm not sure to like the fact that you introduce some local variables just to get ride of some warnings given that Hotspot compilers are sometimes sensitive to that. I think this practice should be discussed on this list before committing this changeset. Yes. We discussed this during the last warnings cleanup. If javac cannot provide a means of shutting up these kinds of warnings without changing the emitted bytecode, then warning-free-compiles should not be required within JDK. (We hit a lot of warnings in j.u.c anyway because of uses of Unsafe that have no pure-java equivalents.) so is it a good idea to add a temporary local variable to fix a generics warning or should @SuppressWarnings should be set on the whole method ? We end up doing this this too often as a workaround. -Doug
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
In response to a similar question a few months ago, Tom Rodriguez mentioned that c2 is not really susceptible to these redundant stores when making inlining decisions. I guess c1 (and interpreter of course) might be but then in those cases one's not getting max optimization anyway. John Rose mentioned that the inlining decision making should be fixed for cases where it's susceptible to these types of bytecodes (presumably c1). So it sounds like avoiding these locals is basically trying to work around current compiler limitations, rather than something more fundamental. I'm also curious if someone has actually noticed any perf degradation in hot code when adding locals like this (Doug? :)). If not (but perf tests were done), then I'm not sure it's worth worrying about. Sent from my phone On Aug 24, 2012 6:45 AM, Doug Lea d...@cs.oswego.edu wrote: Rémi Forax wrote: Hi Dan, I'm not sure to like the fact that you introduce some local variables just to get ride of some warnings given that Hotspot compilers are sometimes sensitive to that. I think this practice should be discussed on this list before committing this changeset. Yes. We discussed this during the last warnings cleanup. If javac cannot provide a means of shutting up these kinds of warnings without changing the emitted bytecode, then warning-free-compiles should not be required within JDK. (We hit a lot of warnings in j.u.c anyway because of uses of Unsafe that have no pure-java equivalents.) so is it a good idea to add a temporary local variable to fix a generics warning or should @SuppressWarnings should be set on the whole method ? We end up doing this this too often as a workaround. -Doug
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Vitaly Davidovich wrote: So it sounds like avoiding these locals is basically trying to work around current compiler limitations, rather than something more fundamental. If javac did even a smidgen of optimization, this problem would also go away. I'm also curious if someone has actually noticed any perf degradation in hot code when adding locals like this (Doug? :)). If not (but perf tests were done), then I'm not sure it's worth worrying about. Of course, I have measured actual performance problems -- otherwise I would not be bringing up this issue :-) Not every case is a problem, but there are some cases that matter, and it is not a good idea to invite people to rewrite, just for the sake of warning suppression, code that explicitly chose not to introduce redundant locals. -Doug
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
I figured you did, but wanted to check. :) So the perf hit was with c2 compilation? Were you able to check the assembly (or enable inlining printing in hotspot) and see that lack of inlining (and whatever further opto that enabled) was the difference by simply adding a local or two? I'm legitimately curious here because if that's the case and this was on a somewhat recent hotspot build, it sort of goes against what TomR seemed to have been saying. I agree with you that even javac should be able to remove this code, but it's disappointing if an optimizing JIT cannot handle it. Thanks Sent from my phone On Aug 24, 2012 9:02 AM, Doug Lea d...@cs.oswego.edu wrote: Vitaly Davidovich wrote: So it sounds like avoiding these locals is basically trying to work around current compiler limitations, rather than something more fundamental. If javac did even a smidgen of optimization, this problem would also go away. I'm also curious if someone has actually noticed any perf degradation in hot code when adding locals like this (Doug? :)). If not (but perf tests were done), then I'm not sure it's worth worrying about. Of course, I have measured actual performance problems -- otherwise I would not be bringing up this issue :-) Not every case is a problem, but there are some cases that matter, and it is not a good idea to invite people to rewrite, just for the sake of warning suppression, code that explicitly chose not to introduce redundant locals. -Doug
More updates to the ProblemList
I need a reviewer to update the ProblemList to exclude a number of tests, see attached. Minimally I'd like to get sun/management/jmxremote/startstop/JMXStartStopTest.sh excluded. It was removed from the exclude list recently [1] but has been causing havoc on Windows since its release. I believe Dmitry Samersoff is working on this. A number of tests are excluded on Mac because they don't run headless, they seem to have been missed when other tests were added for the same reason. Jason Uh had an initial patch to fix a couple of these but I think it has been pushed yet. The others are intermittent failures with bugs tracking the issues. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4fb8792725d5 diff --git a/test/ProblemList.txt b/test/ProblemList.txt --- a/test/ProblemList.txt +++ b/test/ProblemList.txt @@ -141,6 +141,9 @@ javax/management/loading/LibraryLoader/L # 7144846 javax/management/remote/mandatory/connection/ReconnectTest.java generic-all +# 7158614, locks up Windows machines at least +sun/management/jmxremote/startstop/JMXStartStopTest.sh windows-all + # jdk_math @@ -222,8 +225,10 @@ java/net/URLClassLoader/closetest/CloseT # 6962637 java/io/File/MaxPathLength.java windows-all -# 7145435 - Test needs AWT window server, does not work headless +# 7162111 - these tests need to be updated to run headless java/io/Serializable/resolveClass/deserializeButton/run.sh macosx-all +java/io/Serializable/serialver/classpath/run.sh macosx-all +java/io/Serializable/serialver/nested/run.shmacosx-all @@ -246,6 +251,9 @@ java/nio/channels/DatagramChannel/Changi # 7132677 java/nio/channels/Selector/OutOfBand.java macosx-all +# 7142919 +java/nio/channels/AsyncCloseAndInterrupt.java solaris-all + # jdk_rmi @@ -257,6 +265,13 @@ java/rmi/transport/rapidExportUnexport/R # jdk_security +# 7193792 +sun/security/pkcs11/ec/TestECDSA.java solaris-all +sun/security/pkcs11/ec/TestECDSA.java linux-all + +# 7193793 +sun/security/pkcs11/ec/TestECDH.java linux-all + # 7147060 com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java generic-all @@ -265,7 +280,6 @@ sun/security/pkcs11/ec/ReadCertificates. sun/security/pkcs11/ec/ReadCertificates.java generic-all sun/security/pkcs11/ec/ReadPKCS12.java generic-all sun/security/pkcs11/ec/TestCurves.java solaris-i586 -sun/security/pkcs11/ec/TestECDSA.java solaris-i586 #sun/security/pkcs11/ec/TestECGenSpec.java solaris-i586 #sun/security/pkcs11/ec/TestKeyFactory.java solaris-i586 sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java generic-all @@ -362,6 +376,9 @@ tools/launcher/UnicodeTest.java # jdk_util +# 7162111 - test needs to be changed to run headless +java/util/ResourceBundle/Control/Bug6530694.java macosx-all + # Filed 6933803 java/util/concurrent/ThreadPoolExecutor/CoreThreadTimeOut.java generic-all
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 24/08/2012 11:25, Rémi Forax wrote: I also vote for adding this overload, a lot of the usage of Class.forName with three parameters are in fact just a way to load a class without initialize it. Rémi I think this is worth looking at too. It would only change two or three usages in this patch but there likely be a lot more as other areas of the JDK are updated. -Alan.
Re: More updates to the ProblemList
Looks good Alan; approved, -Joe On 8/24/2012 6:47 AM, Alan Bateman wrote: I need a reviewer to update the ProblemList to exclude a number of tests, see attached. Minimally I'd like to get sun/management/jmxremote/startstop/JMXStartStopTest.sh excluded. It was removed from the exclude list recently [1] but has been causing havoc on Windows since its release. I believe Dmitry Samersoff is working on this. A number of tests are excluded on Mac because they don't run headless, they seem to have been missed when other tests were added for the same reason. Jason Uh had an initial patch to fix a couple of these but I think it has been pushed yet. The others are intermittent failures with bugs tracking the issues. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4fb8792725d5 diff --git a/test/ProblemList.txt b/test/ProblemList.txt --- a/test/ProblemList.txt +++ b/test/ProblemList.txt @@ -141,6 +141,9 @@ javax/management/loading/LibraryLoader/L # 7144846 javax/management/remote/mandatory/connection/ReconnectTest.javageneric-all +# 7158614, locks up Windows machines at least +sun/management/jmxremote/startstop/JMXStartStopTest.sh windows-all + # jdk_math @@ -222,8 +225,10 @@ java/net/URLClassLoader/closetest/CloseT # 6962637 java/io/File/MaxPathLength.java windows-all -# 7145435 - Test needs AWT window server, does not work headless +# 7162111 - these tests need to be updated to run headless java/io/Serializable/resolveClass/deserializeButton/run.sh macosx-all +java/io/Serializable/serialver/classpath/run.sh macosx-all +java/io/Serializable/serialver/nested/run.sh macosx-all @@ -246,6 +251,9 @@ java/nio/channels/DatagramChannel/Changi # 7132677 java/nio/channels/Selector/OutOfBand.java macosx-all +# 7142919 +java/nio/channels/AsyncCloseAndInterrupt.java solaris-all + # jdk_rmi @@ -257,6 +265,13 @@ java/rmi/transport/rapidExportUnexport/R # jdk_security +# 7193792 +sun/security/pkcs11/ec/TestECDSA.java solaris-all +sun/security/pkcs11/ec/TestECDSA.java linux-all + +# 7193793 +sun/security/pkcs11/ec/TestECDH.java linux-all + # 7147060 com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java generic-all @@ -265,7 +280,6 @@ sun/security/pkcs11/ec/ReadCertificates. sun/security/pkcs11/ec/ReadCertificates.java generic-all sun/security/pkcs11/ec/ReadPKCS12.java generic-all sun/security/pkcs11/ec/TestCurves.java solaris-i586 -sun/security/pkcs11/ec/TestECDSA.java solaris-i586 #sun/security/pkcs11/ec/TestECGenSpec.java solaris-i586 #sun/security/pkcs11/ec/TestKeyFactory.java solaris-i586 sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java generic-all @@ -362,6 +376,9 @@ tools/launcher/UnicodeTest.java # jdk_util +# 7162111 - test needs to be changed to run headless +java/util/ResourceBundle/Control/Bug6530694.java macosx-all + # Filed 6933803 java/util/concurrent/ThreadPoolExecutor/CoreThreadTimeOut.java generic-all
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
Hi, Here is a modified patch: http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/ The factory finders contain some format changes, a NetBeans format work. The real changes are as the follows: 1) In factory classes: reinstated the implementation resolution mechanism, the 3rd step mainly 2) In factory finders: replaced findJarServiceProvider with findServiceProvider 3) In factory finders: removed ConfigurationError class, using FactoryConfigurationError instead Please review. Thanks, Joe On 7/29/2012 11:59 PM, Joe Wang wrote: Hi Paul, Alan, I'm back on this change. On 6/25/2012 12:19 PM, Joe Wang wrote: On 6/25/2012 9:34 AM, Paul Sandoz wrote: H Joe, I just focused on javax.xml.datatype and assumed the rest follows the same pattern :-) Yeah, they are mostly similar, except Schema and XPath that do a little extra check. I said too quick. Yes, the steps are the same except for the validation and XPath. But it happened that DatatypeFactory is the only one that had defined Exception in the 3rd step. All others were made to ignore any error, since in the regular JDK, we can always fall back to the default impl unless requested not to. I've made all of the changes as we've discussed, to catch the ServiceConfigurationError and pass on the cause for all of the factories/finders. I wanted to post the change so that you could see them on Mon. But I had to read the definitions again when I got to the SchemaFactory and noticed that it was the only one that did not have its own exception. Instead, it was defined to always throw IAE if no impl is available. As I thought through these definitions, I felt I would probably change them back although I started to hate the tedious duplications as you expected :) Indeed, I have to argue again, in the regular JDK, there's no need to catch any errors since they would be considered abnormal (Windows' blue screen came to my mind). And in jigsaw, we shall only need to figure out a different error message when no provider is available, only if we wanted it to be more end-user friendly. -Joe
Re: More updates to the ProblemList
Looks fine to me. -Chris. On 24/08/12 14:47, Alan Bateman wrote: I need a reviewer to update the ProblemList to exclude a number of tests, see attached. Minimally I'd like to get sun/management/jmxremote/startstop/JMXStartStopTest.sh excluded. It was removed from the exclude list recently [1] but has been causing havoc on Windows since its release. I believe Dmitry Samersoff is working on this. A number of tests are excluded on Mac because they don't run headless, they seem to have been missed when other tests were added for the same reason. Jason Uh had an initial patch to fix a couple of these but I think it has been pushed yet. The others are intermittent failures with bugs tracking the issues. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4fb8792725d5 diff --git a/test/ProblemList.txt b/test/ProblemList.txt --- a/test/ProblemList.txt +++ b/test/ProblemList.txt @@ -141,6 +141,9 @@ javax/management/loading/LibraryLoader/L # 7144846 javax/management/remote/mandatory/connection/ReconnectTest.java generic-all +# 7158614, locks up Windows machines at least +sun/management/jmxremote/startstop/JMXStartStopTest.sh windows-all + # jdk_math @@ -222,8 +225,10 @@ java/net/URLClassLoader/closetest/CloseT # 6962637 java/io/File/MaxPathLength.java windows-all -# 7145435 - Test needs AWT window server, does not work headless +# 7162111 - these tests need to be updated to run headless java/io/Serializable/resolveClass/deserializeButton/run.sh macosx-all +java/io/Serializable/serialver/classpath/run.sh macosx-all +java/io/Serializable/serialver/nested/run.shmacosx-all @@ -246,6 +251,9 @@ java/nio/channels/DatagramChannel/Changi # 7132677 java/nio/channels/Selector/OutOfBand.java macosx-all +# 7142919 +java/nio/channels/AsyncCloseAndInterrupt.java solaris-all + # jdk_rmi @@ -257,6 +265,13 @@ java/rmi/transport/rapidExportUnexport/R # jdk_security +# 7193792 +sun/security/pkcs11/ec/TestECDSA.java solaris-all +sun/security/pkcs11/ec/TestECDSA.java linux-all + +# 7193793 +sun/security/pkcs11/ec/TestECDH.java linux-all + # 7147060 com/sun/org/apache/xml/internal/security/transforms/ClassLoaderTest.java generic-all @@ -265,7 +280,6 @@ sun/security/pkcs11/ec/ReadCertificates. sun/security/pkcs11/ec/ReadCertificates.java generic-all sun/security/pkcs11/ec/ReadPKCS12.java generic-all sun/security/pkcs11/ec/TestCurves.java solaris-i586 -sun/security/pkcs11/ec/TestECDSA.java solaris-i586 #sun/security/pkcs11/ec/TestECGenSpec.java solaris-i586 #sun/security/pkcs11/ec/TestKeyFactory.java solaris-i586 sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java generic-all @@ -362,6 +376,9 @@ tools/launcher/UnicodeTest.java # jdk_util +# 7162111 - test needs to be changed to run headless +java/util/ResourceBundle/Control/Bug6530694.java macosx-all + # Filed 6933803 java/util/concurrent/ThreadPoolExecutor/CoreThreadTimeOut.java generic-all
hg: jdk8/tl/jdk: 7193601: Build breakage with the fix to 6336885 (build-infra build)
Changeset: faf4528aea4e Author:naoto Date: 2012-08-24 10:13 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/faf4528aea4e 7193601: Build breakage with the fix to 6336885 (build-infra build) Reviewed-by: mduigou ! makefiles/CompileJavaClasses.gmk
hg: jdk8/tl/jdk: 7193933: More ProblemList.txt updates (8/2012)
Changeset: a7cdfd16e36e Author:alanb Date: 2012-08-24 19:35 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a7cdfd16e36e 7193933: More ProblemList.txt updates (8/2012) Reviewed-by: darcy, chegar ! test/ProblemList.txt
hg: jdk8/tl/jdk: 7168172: (fs) Files.isReadable slow on Windows
Changeset: bd91a601265c Author:khazra Date: 2012-08-24 11:48 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bd91a601265c 7168172: (fs) Files.isReadable slow on Windows Summary: Remove DACL checking for read access, also reviewed by ulf.zi...@cosoco.de, zhong.j...@gmail.com Reviewed-by: alanb ! src/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 08/23/2012 05:17 PM, Andrew Hughes wrote: - Original Message - - Original Message - Please review the fix of CR 7193406 at http://cr.openjdk.java.net/~dxu/7193406/webrev/. snip... And it enables fatal warning flag in the following make file. make/java/jar/Makefile Please don't do this, at least not unconditionally. At the very least, these should not be forced on if the user has explicitly built with them set to false. If I set JAVAC_WARNINGS_FATAL=false, I don't expect part of the build to ignore this and use -Werror, possibly then causing build failures. This is especially bad with both on as a new warning may be introduced into javac which then breaks everyone's build. I already have a patch I intend to upstream which fixes the other cases of this I found. I'd prefer we didn't have another. In FilePermission.java file, I make one change to its method signature, public Enumeration elements() == public EnumerationPermission elements() I am not sure whether it will cause an issue of backward compatibility. Please advise. Thanks! It's in a package-private class so I doubt it. Even if it wasn't, a new major release is the perfect time to fix these issues. - Dan I should also point out that the annotation: @SuppressWarnings(unchecked) private void readObject(ObjectInputStream in) throws IOException, could be moved down to the actual problematic assignment: VectorPermission permissions = (VectorPermission)gfields.get(permissions, null); so that warnings aren't suppressed throughout the method. Andrew, Thanks for your suggestions. I have revoked the change to the make file and added the changes to readObject method, which did not draw my attention as it already suppresses all warnings inside the method. And we should place @SuppressWarnings at narrowest possible scope. Thanks! -Dan
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Hi David,
Please see my response below. Thanks!
On 08/23/2012 06:53 PM, David Holmes wrote:
Hi Dan,
On 24/08/2012 7:46 AM, Dan Xu wrote:
Please review the fix of CR 7193406 at
http://cr.openjdk.java.net/~dxu/7193406/webrev/.
It cleans up warnings in the following java files.
src/share/classes/java/io/FilePermission.java
I'm surprised that you need this:
426 @SuppressWarnings(fallthrough)
...
436 switch (actions) {
437 case SecurityConstants.FILE_READ_ACTION:
438 return READ;
If this generates a fallthrough warning then I think whatever tool
generates that warning needs fixing!
@SuppressWarnings(fallthrough) is put to suppress warnings generated
by another switch/case statements
while (i = matchlen !seencomma) {
switch(a[i-matchlen]) {
case ',':
seencomma = true;
/*FALLTHROUGH*/
case ' ': case '\r': case '\n':
case '\f': case '\t':
break;
default:
throw new IllegalArgumentException(
invalid permission: + actions);
}
i--;
}
It is at the end of this method, getMask(String).
The change of switch/case statements starting from line 436, as you
pointed out, does not generate a fallthrough warning.
src/share/classes/java/util/ArrayDeque.java
src/share/classes/java/util/Arrays.java
src/share/classes/java/util/Collections.java
src/share/classes/java/util/HashMap.java
src/share/classes/java/util/JumboEnumSet.java
src/share/classes/java/util/PriorityQueue.java
src/share/classes/java/util/PropertyResourceBundle.java
Here:
! @SuppressWarnings({ unchecked, rawtypes })
! MapString,Object result = new HashMap(properties);
! lookup = result;
why do you keep the raw type instead of using HashMap(properties) ?
If I change to use diamond operator, the compiler will give me the
following error,
../../../src/share/classes/java/util/PropertyResourceBundle.java:132: error:
incompatible types: HashMapObject,Object cannot be converted to
MapString,Object
MapString,Object result = new HashMap(properties);
^
1 error
I think it is because the type of properties is
HashtableObject,Object, which does not fit into new
HashMapString,Object() constructor.
src/share/classes/java/util/jar/JarVerifier.java
src/share/classes/java/util/jar/Pack200.java
src/share/classes/sun/util/PreHashedMap.java
And it enables fatal warning flag in the following make file.
make/java/jar/Makefile
I'm not sure what Andrew's objection is to this change as it only
affects how java/util/jar classes get compiled and is consistent with
the usage in all the other Makefiles. As far as I can see you can
override this on the make invocation anyway.
I'm unclear how this is handled in the new build.
In FilePermission.java file, I make one change to its method signature,
public Enumeration elements() == public EnumerationPermission
elements()
I am not sure whether it will cause an issue of backward compatibility.
Please advise. Thanks!
As Andrew stated this is a package-private class so I don't see any
issue.
Cheers,
David
- Dan
Thanks for your comments.
-Dan
Re: hg: jdk8/tl/jdk: 7168172: (fs) Files.isReadable slow on Windows
Kurchi, thanks for listing me. BTW, my official openJDK ID is ulfzibis -Ulf Am 24.08.2012 20:49, schrieb kurchi.subhra.ha...@oracle.com: Changeset: bd91a601265c Author:khazra Date: 2012-08-24 11:48 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bd91a601265c 7168172: (fs) Files.isReadable slow on Windows Summary: Remove DACL checking for read access, also reviewed by ulf.zi...@cosoco.de, zhong.j...@gmail.com Reviewed-by: alanb ! src/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java
Re: hg: jdk8/tl/jdk: 7168172: (fs) Files.isReadable slow on Windows
It may not be enforced by tools, but I don't think you can be listed as a reviewer there unless you have the JDK 8 project reviewer attribute :- Alan does - http://openjdk.java.net/census#alanb but you don't - http://openjdk.java.net/census#ulfzibis Now maybe you should ask if you can be .. -phil. On 8/24/2012 12:20 PM, Ulf Zibis wrote: Kurchi, thanks for listing me. BTW, my official openJDK ID is ulfzibis -Ulf Am 24.08.2012 20:49, schrieb kurchi.subhra.ha...@oracle.com: Changeset: bd91a601265c Author:khazra Date: 2012-08-24 11:48 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bd91a601265c 7168172: (fs) Files.isReadable slow on Windows Summary: Remove DACL checking for read access, also reviewed by ulf.zi...@cosoco.de, zhong.j...@gmail.com Reviewed-by: alanb ! src/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java
Re: hg: jdk8/tl/jdk: 7168172: (fs) Files.isReadable slow on Windows
Oops, thanks for your correction, Phil. Sorry, I wasn't aware about that difference. -Ulf Am 24.08.2012 21:40, schrieb Phil Race: It may not be enforced by tools, but I don't think you can be listed as a reviewer there unless you have the JDK 8 project reviewer attribute :- Alan does - http://openjdk.java.net/census#alanb but you don't - http://openjdk.java.net/census#ulfzibis Now maybe you should ask if you can be .. -phil. On 8/24/2012 12:20 PM, Ulf Zibis wrote: Kurchi, thanks for listing me. BTW, my official openJDK ID is ulfzibis -Ulf Am 24.08.2012 20:49, schrieb kurchi.subhra.ha...@oracle.com: Changeset: bd91a601265c Author:khazra Date: 2012-08-24 11:48 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bd91a601265c 7168172: (fs) Files.isReadable slow on Windows Summary: Remove DACL checking for read access, also reviewed by ulf.zi...@cosoco.de, zhong.j...@gmail.com Reviewed-by: alanb ! src/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 8/24/12 2:42 AM, Andrew Hughes wrote: However, once the whole build is warning free, would it not be preferable to remove these and just set JAVAC_WARNINGS_FATAL=true when doing development builds? The problem I see is someone new coming to OpenJDK and not being able to simply build it (with no changes) because a new warnings has appeared and is being treated as an error. This is less of a problem with javac as we control its development, and the JDK will use the javac built in the langtools step in most cases. But, generally, -Werror is something you should choose to enable, with the intention of fixing failures, not something that should be forced on everyone building the code. Our experience is that when -Werror is off by default, warnings tend to be introduced inadvertently. In the time we've been working on warnings cleanup, I've noticed the warnings count creeping up in areas of the build that don't have -Werror enabled. If it weren't for the implicit compilation issue, enabling -Werror incrementally as areas of the build are cleared would have been a good way to ensure that we make steady progress without any backsliding. On a related topic, it would be nice if javadoc could also support -Werror as I constantly see warnings reappearing in the documentation. Yes, javadoc warnings are another *huge* problem. SUBDIRS_MAKEFLAGS += JAVAC_WARNINGS_FATAL=true and this is **not** overridable on the command line. That's wrong. If these are causing problems for you, please do submit patches. Yes, that's what we have in java/tools and is why JAVAC_WARNINGS_FATAL=false doesn't completely remove -Werror at present. I'll post a webrev of the change I have for this. Great, looking forward to this. s'marks
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 25/08/2012 1:53 AM, Mandy Chung wrote: On 8/23/2012 6:27 PM, David Holmes wrote: Another way to simplify this would be to add a new overload of Class.forName(): public static Class? forName(String name, boolean initialize) That way the loader argument could be dropped completely from a number of the use-cases. Paul and Remi also suggest that during the jigsaw code review on the jigsaw-dev mailing list. It's worth considering. I plan to look at the usage of Class.forName in the JDK and add this new overload of Class.forName method if there are many use of it but I haven't got to it yet. I prefer to push this changeset as it and file a new CR and target it for JDK 8. My other query with these changes is whether we are certain that using the specified loader rather than the boot loader will always be correct. David Mandy
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 8/24/2012 3:44 PM, David Holmes wrote:
My other query with these changes is whether we are certain that using
the specified loader rather than the boot loader will always be correct.
Yes I'm to my best knowledge but I'm looking to the reviewers to tell me
otherwise :)
The class being loaded is either part of the same module or expressed in
its module dependency. I ran the JCK tests in module mode with the
jigsaw modular JDK that uncovered these files to be changed. There are
other places in the JDK that I didn't touch since they were not found by
my testing.
BTW, I have created a new CR:
7194006: A new Class.forName(String cn, boolean initialize) method
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.02/
This include Paul's suggestion and slightly improved the javadoc
for Class.forName you suggested [1]:
- * @param initialize whether the class must be initialized
+ * @param initialize if {@code true} the class will be initialized.
+ * See Section 12 of emThe Java Language
Specification/em.
Thanks
Mandy
[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2012-May/002534.html
[PATCH FOR REVIEW] 7194032: update tests for upcoming changes for jtreg
Currently, jtreg incorrectly confuses the concept of the /directory/ in
which the test's class will be written with the /classpath/ used to
locate all of the test's classes, including any library classes. It
provides env variable TESTCLASSES and system property test.classes, and
tests use these in different contexts to mean both a directory and a
classpath.
jtreg will be fixed to separate the two concepts, such that
TESTCLASSES/test.classes will continue to mean the /directory/ in which
the test's class will the written, and will introduce
TESTCLASSPATH/test.class.path for the classpath used to locate all the
test's classes.
This change only affects a small number of tests, which use @library and
TESTCLASSES. Two RMI tests are affected.
FAILED: java/rmi/activation/Activatable/extLoadedImpl/ext.sh
FAILED: java/rmi/registry/readTest/readTest.sh
The fix is to update the tests where they are using
TESTCLASSES/test.classes as a classpath.
To ease migration onto the new jtreg, it is suggested that the tests
check to see if the new variables are defined (i.e. new jtreg) and to
fall back on the old variables if they are not defined (i.e. old jtreg).
In shell terms, this can be written
${TESTCLASSPATH:-${TESTCLASSES}}
The patch can be seen here:
http://cr.openjdk.java.net/~jjg/7194032/webrev.00/
See also
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7194032
hg: jdk8/tl/jdk: 7193339: Prepare system classes be defined by a non-null module loader
Changeset: d52081a08d11 Author:mchung Date: 2012-08-24 22:55 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d52081a08d11 7193339: Prepare system classes be defined by a non-null module loader Reviewed-by: alanb, dholmes, dsamersoff, sspitsyn, psandoz ! src/share/classes/com/sun/jmx/mbeanserver/MXBeanMapping.java ! src/share/classes/com/sun/jmx/remote/internal/IIOPHelper.java ! src/share/classes/java/lang/Class.java ! src/share/classes/java/lang/ClassLoader.java ! src/share/classes/java/lang/Thread.java ! src/share/classes/java/lang/management/ManagementFactory.java ! src/share/classes/java/lang/management/PlatformComponent.java ! src/share/classes/java/util/prefs/Preferences.java ! src/share/classes/javax/script/ScriptEngineManager.java ! src/share/classes/sun/management/MappedMXBeanType.java ! src/share/classes/sun/misc/Unsafe.java ! src/share/classes/sun/misc/VM.java ! src/share/classes/sun/reflect/misc/ReflectUtil.java
