RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX
Hi, this is the second review round for 8024854: Basic changes and files to build the class library on AIXhttps://bugs.openjdk.java.net/browse/JDK-8024854. The previous reviews can be found at the end of this mail in the references section. I've tried to address all the comments and suggestions from the first round and to further streamline the patch (it perfectly builds on Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). The biggest change compared to the first review round is the new aix/ subdirectory which I've now created under jdk/src and which contains AIX-only code. The webrev is against our http://hg.openjdk.java.net/ppc-aix-port/stagerepository which has been recently synchronised with the jdk8 master: http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/ Below (and in the webrev) you can find some comments which explain the changes to each file. In order to be able to push this big change, I need the approval of reviewers from the lib, net, svc, 2d, awt and sec groups. So please don't hesitate to jump at it - there are enough changes for everybody:) Thank you and best regards, Volker *References:* The following reviews have been posted so far (thanks Iris for collecting them :) - Alan Bateman (lib): Initial comments (16 Sep [2]) - Chris Hegarty (lib/net): Initial comments (20 Sep [3]) - Michael McMahon (net): Initial comments (20 Sept [4]) - Steffan Larsen (svc): APPROVED (20 Sept [5]) - Phil Race (2d): Initial comments (18 Sept [6]); Additional comments (15 Oct [7]) - Sean Mullan (sec): Initial comments (26 Sept [8]) [2]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html [3]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html [4]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html [5]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html [6]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html [7]: http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html [8]: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html *Detailed change description:* The new jdk/src/aix subdirectory contains the following new and AIX-specific files for now: src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties src/aix/classes/sun/nio/ch/AixAsynchronousChannelProvider.java src/aix/classes/sun/nio/ch/AixPollPort.java src/aix/classes/sun/nio/fs/AixFileStore.java src/aix/classes/sun/nio/fs/AixFileSystem.java src/aix/classes/sun/nio/fs/AixFileSystemProvider.java src/aix/classes/sun/nio/fs/AixNativeDispatcher.java src/aix/classes/sun/tools/attach/AixAttachProvider.java src/aix/classes/sun/tools/attach/AixVirtualMachine.java src/aix/native/java/net/aix_close.c src/aix/native/sun/nio/ch/AixPollPort.c src/aix/native/sun/nio/fs/AixNativeDispatcher.c src/aix/native/sun/tools/attach/AixVirtualMachine.c src/aix/porting/porting_aix.c src/aix/porting/porting_aix.h src/aix/porting/porting_aix.c src/aix/porting/porting_aix.h - Added these two files for AIX relevant utility code. - Currently these files only contain an implementation of dladdr which is not available on AIX. - In the first review round the existing dladdr users in the code either called the version from the HotSpot on AIX ( src/solaris/native/sun/awt/awt_LoadLibrary.c) or had a private copy of the whole implementation (src/solaris/demo/jvmti/hprof/hprof_md.c). This is now not necessary any more. The new file layout required some small changes to the makefiles to make them aware of the new directory locations: makefiles/CompileDemos.gmk - Add an extra argument to SetupJVMTIDemo which can be used to pass additional source locations. - Currently this is only used on AIX for the AIX porting utilities which are required by hprof. makefiles/lib/Awt2dLibraries.gmk makefiles/lib/ServiceabilityLibraries.gmk - On AIX add the sources of the AIX porting utilities to the build. They are required by src/solaris/native/sun/awt/awt_LoadLibrary.c and src/solaris/demo/jvmti/hprof/hprof_md.c because dladdr is not available on AIX. makefiles/lib/NioLibraries.gmk - Make the AIX-build aware of the new NIO source locations under src/aix/native/sun/nio/. makefiles/lib/NetworkingLibraries.gmk - Make the AIX-build aware of the new aix_close.c source location under src/aix/native/java/net/. src/share/bin/jli_util.h - Define JLI_Lseek on AIX. src/share/lib/security/java.security-aix - Provide default java.security-aix for AIX based on the latest Linux version as suggested by Sean Mullan. src/share/native/common/check_code.c - On AIX malloc(0) and calloc(0, ...) return a NULL pointer, which is legal, but the code in check_code.c does not handles this properly. So we wrap the two methods on AIX and return a non-NULL pointer even if we allocate 0
hg: jdk8/tl/corba: 8028215: ORB.init fails with SecurityException if properties select the JDK default ORB
Changeset: fe781b3badd6 Author:msheppar Date: 2013-11-21 11:30 + URL: http://hg.openjdk.java.net/jdk8/tl/corba/rev/fe781b3badd6 8028215: ORB.init fails with SecurityException if properties select the JDK default ORB Summary: check for default ORBImpl and ORBSingleton set via properties or System properties Reviewed-by: alanb, coffeys, mchung ! src/share/classes/org/omg/CORBA/ORB.java
Re: RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX
On 20/11/2013 18:26, Volker Simonis wrote: Hi, this is the second review round for 8024854: Basic changes and files to build the class library on AIX https://bugs.openjdk.java.net/browse/JDK-8024854. The previous reviews can be found at the end of this mail in the references section. I've tried to address all the comments and suggestions from the first round and to further streamline the patch (it perfectly builds on Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). The biggest change compared to the first review round is the new aix/ subdirectory which I've now created under jdk/src and which contains AIX-only code. Thanks for the update and addressing all the original comments and suggestions. In particular, moving most of the AIX specific files to src/aix and including an implementation of dladdr, make a big difference and makes this much easier to review. I've skimmed through all the non-client files in the webrev and just have a few comments: NetworkLibraries.gmk - is the exclude of bsd_close.c right? It looks like it will add this to LIBNET_EXCLUDE_FILES even when building on Mac. In the old verifier code (check_code.c) then it's not clear to me that the caller wrapper is needed but in any case the change suggests to me that we should look at the malloc usages so that they better handle the size==0 case. I realize the wrappers are to avoid changing too much and it should be okay to handle this via a separate bug. In net_util.c then it's a bit ugly to be calling aix_close_init. Michael/Chris - what you would think about the JNI_OnLoad calling into a platform specific function to do platform specific initialization? The changes to java_md_solinux.c look okay to me but it makes me wonder if this should be renamed as it no longer exclusively Solaris + Linux. Port.java - one suggestion for unregisterImpl is to rename it to preUnregister and change it to protected so that it's more obvious that it supposed to be overridden. UnixNativeDispatcher.c - this looks okay (must reduced since the first round), I just wonder if the changes to *_getpwuid and *_getgrgid are really needed as this just impacts the error message. Also might be good to indent the #ifdef to be consistent with the other usages in these functions. That's mostly it. I notice that only a small number of tests have been updated. Are there more test updates to come? I'm pretty sure we have a lot more tests that may require update (searching for SunOS might give some hints). -Alan.
hg: jdk8/tl/jdk: 8028628: java/nio/channels/FileChannel/Size.java failed once in the same binary run
Changeset: 81708985c0a2 Author:dxu Date: 2013-11-21 14:23 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/81708985c0a2 8028628: java/nio/channels/FileChannel/Size.java failed once in the same binary run Reviewed-by: alanb, chegar, mchung, lancea ! test/java/nio/channels/FileChannel/Size.java
hg: jdk8/tl/jdk: 8028632: Update jdk/test/ProblemList.txt to reflect fix JDK-8024423
Changeset: fc9f24b9408e Author:sla Date: 2013-11-21 12:57 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/fc9f24b9408e 8028632: Update jdk/test/ProblemList.txt to reflect fix JDK-8024423 Summary: Removed 5 testcases from the ProblemList Reviewed-by: sla Contributed-by: balchandra.vai...@oracle.com ! test/ProblemList.txt
hg: jdk8/tl/jdk: 7065902: (file) test/java/nio/file/Files/Misc.java fails on Solaris 11 when run as root
Changeset: a74d6aa51654 Author:dxu Date: 2013-11-21 14:16 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a74d6aa51654 7065902: (file) test/java/nio/file/Files/Misc.java fails on Solaris 11 when run as root Reviewed-by: alanb ! test/java/nio/file/Files/Misc.java
hg: jdk8/tl/jdk: 8028215: ORB.init fails with SecurityException if properties select the JDK default ORB
Changeset: d5d4b9a63174 Author:msheppar Date: 2013-11-21 11:36 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d5d4b9a63174 8028215: ORB.init fails with SecurityException if properties select the JDK default ORB Summary: check for default ORBImpl and ORBSingleton set via properties or System properties Reviewed-by: alanb, coffeys, mchung + test/com/sun/corba/se/impl/orb/SetDefaultORBTest.java
hg: jdk8/tl/jdk: 7174936: several String methods claim to always create new String
Changeset: 4bc37b6c4133 Author:smarks Date: 2013-11-21 16:02 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4bc37b6c4133 7174936: several String methods claim to always create new String Reviewed-by: dholmes, bchristi, alanb, lancea ! src/share/classes/java/lang/String.java
hg: jdk8/tl/jdk: 6402201: ProcessAttachTest.sh needs better synchronization
Changeset: 91ec3bc92793 Author:egahlin Date: 2013-11-21 13:46 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/91ec3bc92793 6402201: ProcessAttachTest.sh needs better synchronization Reviewed-by: alanb ! test/ProblemList.txt ! test/com/sun/jdi/ProcessAttachDebuggee.java ! test/com/sun/jdi/ProcessAttachTest.sh
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 2972241cf7eb Author:tyan Date: 2013-11-21 13:37 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2972241cf7eb 703: test/com/sun/net/httpserver/Test9a.java fails intermittently Summary: Additional stacktrace information is printed on failure Reviewed-by: alanb, dfuchs, chegar ! test/ProblemList.txt ! test/com/sun/net/httpserver/Test9a.java Changeset: ed979f9b40cd Author:tyan Date: 2013-11-21 13:42 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ed979f9b40cd 8022212: Intermittent test failures in java/net Reviewed-by: chegar ! test/java/net/NetworkInterface/IndexTest.java
hg: jdk8/tl/jdk: 6703075: (process) java/lang/ProcessBuilder/Basic.java fails with fastdebug
Changeset: 89fccc5a7469 Author:martin Date: 2013-11-21 16:06 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/89fccc5a7469 6703075: (process) java/lang/ProcessBuilder/Basic.java fails with fastdebug Reviewed-by: alanb ! test/java/lang/ProcessBuilder/Basic.java
Re: RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX
Hi Alan, thanks a lot for the fast review and your valuable comments. Please find my answers inline: On Thu, Nov 21, 2013 at 1:01 PM, Alan Bateman alan.bate...@oracle.comwrote: On 20/11/2013 18:26, Volker Simonis wrote: Hi, this is the second review round for 8024854: Basic changes and files to build the class library on AIXhttps://bugs.openjdk.java.net/browse/JDK-8024854. The previous reviews can be found at the end of this mail in the references section. I've tried to address all the comments and suggestions from the first round and to further streamline the patch (it perfectly builds on Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). The biggest change compared to the first review round is the new aix/ subdirectory which I've now created under jdk/src and which contains AIX-only code. Thanks for the update and addressing all the original comments and suggestions. In particular, moving most of the AIX specific files to src/aix and including an implementation of dladdr, make a big difference and makes this much easier to review. I've skimmed through all the non-client files in the webrev and just have a few comments: NetworkLibraries.gmk - is the exclude of bsd_close.c right? It looks like it will add this to LIBNET_EXCLUDE_FILES even when building on Mac. You're right, that's a typo. That should have read: 48 ifneq ($(OPENJDK_TARGET_OS), aix) 49 LIBNET_EXCLUDE_FILES += aix_close.c 50 else 51 LIBNET_SRC_DIRS += $(JDK_TOPDIR)/src/aix/native/java/net/ 52 endif But actually I've just realized that it is not need at all, because 'aix_close.c' isn't in the PATH for any other OS than AIX (that could be probably called a feature of the new file layout:) So I'll simply change it to: 48 ifeq ($(OPENJDK_TARGET_OS), aix) 49 LIBNET_SRC_DIRS += $(JDK_TOPDIR)/src/aix/native/java/net/ 50 endif In the old verifier code (check_code.c) then it's not clear to me that the caller wrapper is needed but in any case the change suggests to me that we should look at the malloc usages so that they better handle the size==0 case. I realize the wrappers are to avoid changing too much and it should be okay to handle this via a separate bug. Yes, exactly. I didn't wanted to change too much code. But as the C-Standard states ( http://pubs.opengroup.org/onlinepubs/95399/functions/malloc.html) ...If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned... it is perfectly legal that malloc/calloc return a NULL pointer if called with a zero argument. This case is currently not handled (i.e. it's handled as an 'out of memory' error) in check_code.c and I agree that this should be fixed via a separate bug. In net_util.c then it's a bit ugly to be calling aix_close_init. Michael/Chris - what you would think about the JNI_OnLoad calling into a platform specific function to do platform specific initialization? What about renaming 'initLocalAddrTable()' into something like 'platformInit()' and moving the call to 'aix_close_init' to a AIX-specific version of 'platformInit()' in net_util_md.c? The changes to java_md_solinux.c look okay to me but it makes me wonder if this should be renamed as it no longer exclusively Solaris + Linux. You're right - we could rename it to something like 'java_md_unix.c'. But no matter how fancy the name would be, the file would still be in the 'src/solaris/bin' subdirectory:( So I think we'd better leave this for a later change when we completely factor out the Linux/Mac code from the 'solaris/' directory. Port.java - one suggestion for unregisterImpl is to rename it to preUnregister and change it to protected so that it's more obvious that it supposed to be overridden. Done. Also changed the comment to JavaDoc style to be more consistent with the other comments in that file. UnixNativeDispatcher.c - this looks okay (must reduced since the first round), I just wonder if the changes to *_getpwuid and *_getgrgid are really needed as this just impacts the error message. Also might be good to indent the #ifdef to be consistent with the other usages in these functions. You're right. This change was done before you fixed 7043788: (fs) PosixFileAttributes.owner() or group() throws NPE if owner/group not in passwd/group database ( http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/f91c799f7bfb). After you're fix it was automatically adapted. I've removed the special AIX handling as suggested because I think as well that another error message in the exception won't have any impact. That's mostly it. I notice that only a small number of tests have been updated. Are there more test updates to come? I'm pretty sure we have a lot more tests that may require update (searching for SunOS might give some hints). I'm currently working on it and created 8028537: PPC64: Updated jdk/test scripts to understand the AIX os and environment for it because I didn't
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 93826827e8b4 Author:valeriep Date: 2013-11-19 15:29 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/93826827e8b4 8026943: SQE test jce/Global/Cipher/SameBuffer failed Summary: Always use different input/output buffers when calling FeedbackCipher objects Reviewed-by: mullan ! src/share/classes/com/sun/crypto/provider/CipherBlockChaining.java ! src/share/classes/com/sun/crypto/provider/CipherCore.java ! src/share/classes/com/sun/crypto/provider/DESedeWrapCipher.java + test/com/sun/crypto/provider/Cipher/AES/TestCopySafe.java Changeset: 06d155a7c9b0 Author:valeriep Date: 2013-11-21 11:58 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/06d155a7c9b0 Merge
Re: 答复: RFR for JDK-8022212 Intermittent test failures in java/net
Pushed: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ed979f9b40cd -Chris. On 11/20/2013 12:55 PM, Tristan Yan wrote: Hi Chris I don't see this has been pushed, could you do this for me. Thank you very much. Tristan -邮件原件- 发件人: Tristan Yan 发送时间: Saturday, November 16, 2013 11:25 AM 收件人: Chris Hegarty; net-dev@openjdk.java.net 主题: 答复: RFR for JDK-8022212 Intermittent test failures in java/net Thanks Chris for sponsoring this. Tristan -邮件原件- 发件人: Chris Hegarty 发送时间: Thursday, November 14, 2013 9:34 PM 收件人: Tristan Yan; net-dev@openjdk.java.net 主题: Re: RFR for JDK-8022212 Intermittent test failures in java/net Thank you Tristan. I can sponsor this change for you. -Chris. On 13/11/13 12:53, Tristan Yan wrote: Thank you Chris This is the webrev http://cr.openjdk.java.net/~ewang/tristan/JDK-8022212/webrev.00/ Thank you Tristan On 11/13/2013 06:07 PM, Chris Hegarty wrote: On 11/13/2013 06:00 AM, Tristan Yan wrote: Hi Everyone I am working on bug https://bugs.openjdk.java.net/browse/JDK-8022212. This is the same root cause as bug JDK-8022963, which is we should not test Teredo Tunneling Pseudo-Interface interface in windows. So suggested fix is doing the same as we did in JDK-8022963, remove Teredo Tunneling Pseudo-Interface from this test. Please let me know if you have any comments or suggestions. If you have confirmed that these tests are failing for the same reason, then I think it is reasonable to apply similar changes to that of 8022963. Webrev? Thanks, -Chris. Tristan/ /
