Hi Artem, thanks a lot for your review. Please find my comments inline:
On Monday, November 25, 2013, Artem Ananiev wrote: > Hi, Volker, > > just a few very minor comments about the client changes: > > 1. mlib_sys.c: the change is fine, but it makes the following comment > obsolete. You're right. I'll update the comment and try to find a corresponding reference for the AIX compiler. > 2. XRBackendNative.c: the same comment here. Again, I'll update the comment accordingly. > 3. Awt2dLibraries.gmk: $(JDK_TOPDIR)/src/aix/porting/porting_aix.c would > be better than just porting_aix.c Good catch! I think the right solution would be to add $(JDK_TOPDIR)/src/aix/porting to LIBAWT_DIRS. This will also add the directory automatically to LIBAWT_CFLAGS. I'll prepare a final webrev with all the proposed changes tomorrow. Thanks once again, Volker > > Thanks, > > Artem > > On 11/20/2013 10:26 PM, 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. >> >> The webrev is against our http://hg.openjdk.java.net/ppc-aix-port/stage >> repository 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 bytes. >> >> >> src/share/native/sun/awt/medialib/mlib_sys.c >> >> * |malloc| always returns 8-byte aligned pointers on AIX as well. >> >> >> src/share/native/sun/awt/medialib/mlib_types.h >> >> * Add AIX to the list of known platforms. >> >> >> src/share/native/sun/font/layout/KernTable.cpp >> >> * Rename the macro |DEBUG| to |DEBUG_KERN_TABLE| because |DEBUG| is >> too common and may be defined in other headers as well as on the >> command line and |xlc| bails out on macro redefinitions with a >> different value. >> >> >> src/share/native/sun/security/ec/impl/ecc_impl.h >> >> * Define required types and macros on AIX. >> >> >> src/solaris/back/exec_md.c >> >> * AIX behaves like Linux in this case so check for it in the Linux >> branch. >> >> >> src/solaris/bin/java_md_solinux.c, >> src/solaris/bin/java_md_solinux.h >> >> * On AIX |LD_LIBRARY_PATH| is called |LIBPATH| >> * Always use |LD_LIBRARY_PATH| macro instead of using the string >> "|LD_LIBRARY_PATH|" directly to cope with different library path >> names. >> * Add |jre/lib/<arch>/jli| to the standard library search path on AIX >> because the AIX linker doesn't support the |-rpath| option. >> * Replace |#ifdef __linux__| by |#ifndef __solaris__| because in this >> case, AIX behaves like Linux. >> * Removed the definition of |JVM_DLL|, |JAVA_DLL| and >> |LD_LIBRARY_PATH| from |java_md_solinux.h| because the macros are >> redefined in the corresponding |.c| files anyway. >> >> >> src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties >> >> * Provide basic |fontconfig.properties|for AIX. >> >> >> src/solaris/classes/java/lang/UNIXProcess.java.aix, >> src/aix/classes/sun/tools/attach/AixAttachProvider.java, >> src/aix/classes/sun/tools/attach/AixVirtualMachine.java, >> src/aix/native/sun/tools/attach/AixVirtualMachine.c >> >> * Provide AIX specific Java versions, mostly based on the >> corresponding Linux versions. >> >> >> src/solaris/classes/sun/nio/ch/DefaultAsynchronousChannelProv >> ider.java >> src/solaris/classes/sun/nio/fs/DefaultFileSystemProvider.java >> >> * Detect AIX operating system and return the corresponding channel and >> file system providers. >> >> >> src/solaris/classes/sun/nio/ch/Port.java >> >> * Add a callback function |unregisterImpl(int fd)| for implementations >> that need special handling when |fd| is removed and call it from >> |unregister(int fd)|. By default the implementation of >> |unregisterImpl(int fd)| is empty except for the derived >> |AixPollPort| class on AIX. >> >> >> src/solaris/demo/jvmti/hprof/hprof_md.c >> >> * Add AIX support. AIX mostly behaves like Linux, with the one >> exception that it has no |dladdr| functionality. >> * Use the |dladdr| implementation from |porting_aix.{c,h}| on AIX. >> >> >> src/solaris/native/com/sun/management/UnixOperatingSystem_md.c >> >> * Adapt for AIX (i.e. use |libperfstat| on AIX to query OS memory). >> >> >> src/solaris/native/common/jdk_util_md.h >> >> * Add AIX definitions of the |ISNANF| and |ISNAND| macros. >> >> >> src/solaris/native/java/io/io_util_md.c >> >> * AIX behaves like Linux in this case so check for it in the Linux >> branch. >> >> >> src/solaris/native/java/lang/UNIXProcess_md.c >> >> * AIX behaves mostly like Solraris in this case so adjust the ifdefs >> accordingly. >> >> >> src/solaris/native/java/lang/childproc.c >> >> * AIX does not understand '/proc/self' - it requires the real process >> ID to query the proc file system for the current process. >> >> >> src/solaris/native/java/net/NetworkInterface.c >> >> * Add AIX support into the Linux branch because AIX mostly behaves >> like AIX for IPv4. >> * AIX needs a special function to enumerate IPv6 interfaces and to >> query the MAC address. >> * Moved the declaration of |siocgifconfRequest| to the beginning a the >> function (as recommend by Michael McMahon) to remain C89 compatible. >> >> >> src/solaris/native/java/net/PlainSocketImpl.c >> >> * On AIX (like on Solaris) |setsockopt| will set errno to |EINVAL| if >> the socket is closed. The default error message is then confusing. >> >> >> src/aix/native/java/net/aix_close.c, >> src/share/native/java/net/net_util.c >> >> * As recommended by Michael McMahon and Alan Bateman I've move an >> adapted version of |linux_close.c| to >> |src/aix/native/java/net/aix_close.c| because we also need the file >> and socket wrappers on AIX. >> * Compared to the Linux version, we've added the initialization of >> some previously uninitialized data structures. >> * Also, on AIX we don't have |__attribute((constructor))| so we need >> to initialize manually (from |JNI_OnLoad()| in >> |src/share/native/java/net/net_util.c|. >> >> >> src/solaris/native/java/net/net_util_md.h >> >> * AIX needs the same workaround for I/O cancellation like Linux and >> MacOSX. >> >> >> src/solaris/native/java/net/net_util_md.c >> >> * |SO_REUSEADDR| is called |SO_REUSEPORT| on AIX. >> * On AIX we have to ignore failures due to |ENOBUFS| when setting the >> |SO_SNDBUF|/|SO_RCVBUF| socket options. >> >> >> src/solaris/native/java/util/TimeZone_md.c >> >> * Currently on AIX the only way to get the platform time zone is to >> read it from the |TZ| environment variable. >> >> >> src/solaris/native/sun/awt/awt_LoadLibrary.c >> >> * Use the |dladdr| from |porting_aix.{c,h}| on AIX. >> >> >> src/solaris/native/sun/awt/fontpath.c >> >> * Changed some macros from |if !defined(__linux__)| to |if >> defined(__solaris__)| because that's their real meaning. >> * Add AIX specific fontpath settings and library search paths for >> |libfontconfig.so|. >> >> >> src/solaris/native/sun/java2d/x11/X11SurfaceData.c >> >> * Rename the |MIN| and |MAX| macros to |XSD_MIN| and |XSD_MAX| to >> avoid name clashes (|MIN| and |MAX| are much too common and thexy >> are already defined in some AIX system headers). >> >> >> src/solaris/native/sun/java2d/x11/XRBackendNative.c >> >> * Handle AIX like Solaris. >> >> >> src/solaris/native/sun/nio/ch/Net.c >> >> * Add AIX-specific includes and constant definitions. >> * On AIX "socket extensions for multicast source filters" support >> depends on the OS version. Check for this and throw appropriate >> exceptions if it is requested but not supported. This is needed to >> pass >> JCK-api/java_nio/channels/DatagramChannel/ >> DatagramChannel.html#Multicast >> >> >> src/solaris/native/sun/nio/fs/UnixNativeDispatcher.c >> >> * On AIX |strerror()| is not thread-safe so we have to use >> |strerror_r()| instead. >> * On AIX |readdir_r()| returns EBADF (i.e. '9') and sets 'result' to >> NULL for the directory stream end. Otherwise, 'errno' will contain >> the error code. >> * Handle some AIX corner cases for the results of the |statvfs64()| >> call. >> * On AIX |getgrgid_r()| returns ESRCH if group does not exist. >> >> >> src/solaris/native/sun/security/pkcs11/j2secmod_md.c >> >> * Use |RTLD_LAZY| instead of |RTLD_NOLOAD| on AIX. >> >> >> test/java/lang/ProcessBuilder/Basic.java >> test/java/lang/ProcessBuilder/DestroyTest.java >> >> * Port this test to AIX and make it more robust (i.e. recognize the >> "C" locale as |isEnglish()|, ignore VM-warnings in the output, make >> sure the "grandchild" processes created by the test don't run too >> long (60s vs. 6666s) because in the case of test problems these >> processes will pollute the process space, make sure the test fails >> with an error and doesn't hang indefinitley in the case of a problem). >> >> *Q (Michael McMahon):* Seems to be two macros _AIX and AIX. Is this >> intended? >> >> Well, |_AIX| is defined in some system headers while |AIX| is defined by >> the build system. This is already used inconsistently (i.e. |__linux__| >> vs. |LINUX|) and in general I try to be consistent with the style of the >> file where I the changes are. That said, I changes most of the >> occurences of |AIX| to |_AIX|. >> >> *Q (Alan Bateman):* There are a few changes for OS/400 in the patch, are >> they supposed to be there? >> >> We currently don't support OS/400 (later renamed into i5/OS and >> currently called IBM i) in our OpenJDK port but we do support it in our >> internel, SAP JVM build. We stripped out all the extra OS/400 >> functionality from the port but in some places where there is common >> functionality needd by both, AIX and OS/400 the OS/400 support may still >> be visible in the OpenJDK port. I don't think this is a real problem and >> it helps us to keep our internel sources in sync with the OpenJDK port. >> That said, I think I've removed all the references to OS/400 now. >> >> >>
