Hi Artem,<div><br></div><div>thanks a lot for your review. Please find my comments inline:<br><br>On Monday, November 25, 2013, Artem Ananiev wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi, Volker,<br>
<br>
just a few very minor comments about the client changes:<br>
<br>
1. mlib_sys.c: the change is fine, but it makes the following comment obsolete.</blockquote><div><br></div><div>You're right. I'll update the comment and try to find a corresponding reference for the AIX compiler.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
2. XRBackendNative.c: the same comment here.</blockquote><div> </div><div>Again, I'll update the comment accordingly.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
3. Awt2dLibraries.gmk: $(JDK_TOPDIR)/src/aix/porting/<u></u>porting_aix.c would be better than just porting_aix.c</blockquote><div><br></div><div>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.</div>
<div><br></div><div>I'll prepare a final webrev with all the proposed changes tomorrow.</div><div><br></div><div>Thanks once again,</div><div>Volker</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
<br>
Artem<br>
<br>
On 11/20/2013 10:26 PM, Volker Simonis wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
this is the second review round for "8024854: Basic changes and files to<br>
build the class library on AIX<br>
<<a href="https://bugs.openjdk.java.net/browse/JDK-8024854" target="_blank">https://bugs.openjdk.java.<u></u>net/browse/JDK-8024854</a>>". The previous<br>
reviews can be found at the end of this mail in the references section.<br>
<br>
I've tried to address all the comments and suggestions from the first<br>
round and to further streamline the patch (it perfectly builds on<br>
Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). The<br>
biggest change compared to the first review round is the new "aix/"<br>
subdirectory which I've now created under "jdk/src" and which contains<br>
AIX-only code.<br>
<br>
The webrev is against our <a href="http://hg.openjdk.java.net/ppc-aix-port/stage" target="_blank">http://hg.openjdk.java.net/<u></u>ppc-aix-port/stage</a><br>
repository which has been recently synchronised with the jdk8 master:<br>
<br>
<a href="http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/" target="_blank">http://cr.openjdk.java.net/~<u></u>simonis/webrevs/8024854.v2/</a><br>
<br>
Below (and in the webrev) you can find some comments which explain the<br>
changes to each file. In order to be able to push this big change, I<br>
need the approval of reviewers from the lib, net, svc, 2d, awt and sec<br>
groups. So please don't hesitate to jump at it - there are enough<br>
changes for everybody:)<br>
<br>
Thank you and best regards,<br>
Volker<br>
<br>
*References:*<br>
<br>
The following reviews have been posted so far (thanks Iris for<br>
collecting them :)<br>
<br>
- Alan Bateman (lib): Initial comments (16 Sep [2])<br>
- Chris Hegarty (lib/net): Initial comments (20 Sep [3])<br>
- Michael McMahon (net): Initial comments (20 Sept [4])<br>
- Steffan Larsen (svc): APPROVED (20 Sept [5])<br>
- Phil Race (2d): Initial comments (18 Sept [6]); Additional comments<br>
(15 Oct [7])<br>
- Sean Mullan (sec): Initial comments (26 Sept [8])<br>
<br>
[2]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001045.html</a><br>
[3]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001078.html</a><br>
[4]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001079.html</a><br>
[5]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001077.html</a><br>
[6]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001069.html</a><br>
[7]: <a href="http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/2d-dev/2013-October/<u></u>003826.html</a><br>
[8]:<br>
<a href="http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html" target="_blank">http://mail.openjdk.java.net/<u></u>pipermail/ppc-aix-port-dev/<u></u>2013-September/001081.html</a><br>
<br>
<br>
*Detailed change description:*<br>
<br>
The new "jdk/src/aix" subdirectory contains the following new and<br>
AIX-specific files for now:<br>
<br>
src/aix/classes/sun/awt/<u></u>fontconfigs/aix.fontconfig.<u></u>properties<br>
src/aix/classes/sun/nio/ch/<u></u>AixAsynchronousChannelProvider<u></u>.java<br>
src/aix/classes/sun/nio/ch/<u></u>AixPollPort.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixFileStore.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixFileSystem.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixFileSystemProvider.java<br>
src/aix/classes/sun/nio/fs/<u></u>AixNativeDispatcher.java<br>
src/aix/classes/sun/tools/<u></u>attach/AixAttachProvider.java<br>
src/aix/classes/sun/tools/<u></u>attach/AixVirtualMachine.java<br>
src/aix/native/java/net/aix_<u></u>close.c<br>
src/aix/native/sun/nio/ch/<u></u>AixPollPort.c<br>
src/aix/native/sun/nio/fs/<u></u>AixNativeDispatcher.c<br>
src/aix/native/sun/tools/<u></u>attach/AixVirtualMachine.c<br>
src/aix/porting/porting_aix.c<br>
src/aix/porting/porting_aix.h<br>
<br>
<br>
src/aix/porting/porting_aix.c<br>
src/aix/porting/porting_aix.h<br>
<br>
* Added these two files for AIX relevant utility code.<br>
* Currently these files only contain an implementation of |dladdr|<br>
which is not available on AIX.<br>
* In the first review round the existing |dladdr| users in the code<br>
either called the version from the HotSpot on AIX<br>
(|src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c|) or had a private<br>
copy of the whole implementation<br>
(|src/solaris/demo/jvmti/<u></u>hprof/hprof_md.c|). This is now not<br>
necessary any more.<br>
<br>
The new file layout required some small changes to the makefiles to make<br>
them aware of the new directory locations:<br>
<br>
<br>
makefiles/CompileDemos.gmk<br>
<br>
* Add an extra argument to |SetupJVMTIDemo| which can be used to pass<br>
additional source locations.<br>
* Currently this is only used on AIX for the AIX porting utilities<br>
which are required by hprof.<br>
<br>
<br>
makefiles/lib/Awt2dLibraries.<u></u>gmk<br>
makefiles/lib/<u></u>ServiceabilityLibraries.gmk<br>
<br>
* On AIX add the sources of the AIX porting utilities to the build.<br>
They are required by |src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c|<br>
and |src/solaris/demo/jvmti/hprof/<u></u>hprof_md.c| because |dladdr| is<br>
not available on AIX.<br>
<br>
<br>
makefiles/lib/NioLibraries.gmk<br>
<br>
* Make the AIX-build aware of the new NIO source locations under<br>
|src/aix/native/sun/nio/|.<br>
<br>
<br>
makefiles/lib/<u></u>NetworkingLibraries.gmk<br>
<br>
* Make the AIX-build aware of the new |aix_close.c| source location<br>
under |src/aix/native/java/net/|.<br>
<br>
<br>
src/share/bin/jli_util.h<br>
<br>
* Define |JLI_Lseek| on AIX.<br>
<br>
<br>
src/share/lib/security/java.<u></u>security-aix<br>
<br>
* Provide default |java.security-aix| for AIX based on the latest<br>
Linux version as suggested by Sean Mullan.<br>
<br>
<br>
src/share/native/common/check_<u></u>code.c<br>
<br>
* On AIX malloc(0) and calloc(0, ...) return a NULL pointer, which is<br>
legal, but the code in |check_code.c| does not handles this<br>
properly. So we wrap the two methods on AIX and return a non-NULL<br>
pointer even if we allocate 0 bytes.<br>
<br>
<br>
src/share/native/sun/awt/<u></u>medialib/mlib_sys.c<br>
<br>
* |malloc| always returns 8-byte aligned pointers on AIX as well.<br>
<br>
<br>
src/share/native/sun/awt/<u></u>medialib/mlib_types.h<br>
<br>
* Add AIX to the list of known platforms.<br>
<br>
<br>
src/share/native/sun/font/<u></u>layout/KernTable.cpp<br>
<br>
* Rename the macro |DEBUG| to |DEBUG_KERN_TABLE| because |DEBUG| is<br>
too common and may be defined in other headers as well as on the<br>
command line and |xlc| bails out on macro redefinitions with a<br>
different value.<br>
<br>
<br>
src/share/native/sun/security/<u></u>ec/impl/ecc_impl.h<br>
<br>
* Define required types and macros on AIX.<br>
<br>
<br>
src/solaris/back/exec_md.c<br>
<br>
* AIX behaves like Linux in this case so check for it in the Linux branch.<br>
<br>
<br>
src/solaris/bin/java_md_<u></u>solinux.c,<br>
src/solaris/bin/java_md_<u></u>solinux.h<br>
<br>
* On AIX |LD_LIBRARY_PATH| is called |LIBPATH|<br>
* Always use |LD_LIBRARY_PATH| macro instead of using the string<br>
"|LD_LIBRARY_PATH|" directly to cope with different library path names.<br>
* Add |jre/lib/<arch>/jli| to the standard library search path on AIX<br>
because the AIX linker doesn't support the |-rpath| option.<br>
* Replace |#ifdef __linux__| by |#ifndef __solaris__| because in this<br>
case, AIX behaves like Linux.<br>
* Removed the definition of |JVM_DLL|, |JAVA_DLL| and<br>
|LD_LIBRARY_PATH| from |java_md_solinux.h| because the macros are<br>
redefined in the corresponding |.c| files anyway.<br>
<br>
<br>
src/aix/classes/sun/awt/<u></u>fontconfigs/aix.fontconfig.<u></u>properties<br>
<br>
* Provide basic |fontconfig.properties|for AIX.<br>
<br>
<br>
src/solaris/classes/java/lang/<u></u>UNIXProcess.java.aix,<br>
src/aix/classes/sun/tools/<u></u>attach/AixAttachProvider.java,<br>
src/aix/classes/sun/tools/<u></u>attach/AixVirtualMachine.java,<br>
src/aix/native/sun/tools/<u></u>attach/AixVirtualMachine.c<br>
<br>
* Provide AIX specific Java versions, mostly based on the<br>
corresponding Linux versions.<br>
<br>
<br>
src/solaris/classes/sun/nio/<u></u>ch/<u></u>DefaultAsynchronousChannelProv<u></u>ider.java<br>
src/solaris/classes/sun/nio/<u></u>fs/DefaultFileSystemProvider.<u></u>java<br>
<br>
* Detect AIX operating system and return the corresponding channel and<br>
file system providers.<br>
<br>
<br>
src/solaris/classes/sun/nio/<u></u>ch/Port.java<br>
<br>
* Add a callback function |unregisterImpl(int fd)| for implementations<br>
that need special handling when |fd| is removed and call it from<br>
|unregister(int fd)|. By default the implementation of<br>
|unregisterImpl(int fd)| is empty except for the derived<br>
|AixPollPort| class on AIX.<br>
<br>
<br>
src/solaris/demo/jvmti/hprof/<u></u>hprof_md.c<br>
<br>
* Add AIX support. AIX mostly behaves like Linux, with the one<br>
exception that it has no |dladdr| functionality.<br>
* Use the |dladdr| implementation from |porting_aix.{c,h}| on AIX.<br>
<br>
<br>
src/solaris/native/com/sun/<u></u>management/<u></u>UnixOperatingSystem_md.c<br>
<br>
* Adapt for AIX (i.e. use |libperfstat| on AIX to query OS memory).<br>
<br>
<br>
src/solaris/native/common/jdk_<u></u>util_md.h<br>
<br>
* Add AIX definitions of the |ISNANF| and |ISNAND| macros.<br>
<br>
<br>
src/solaris/native/java/io/io_<u></u>util_md.c<br>
<br>
* AIX behaves like Linux in this case so check for it in the Linux branch.<br>
<br>
<br>
src/solaris/native/java/lang/<u></u>UNIXProcess_md.c<br>
<br>
* AIX behaves mostly like Solraris in this case so adjust the ifdefs<br>
accordingly.<br>
<br>
<br>
src/solaris/native/java/lang/<u></u>childproc.c<br>
<br>
* AIX does not understand '/proc/self' - it requires the real process<br>
ID to query the proc file system for the current process.<br>
<br>
<br>
src/solaris/native/java/net/<u></u>NetworkInterface.c<br>
<br>
* Add AIX support into the Linux branch because AIX mostly behaves<br>
like AIX for IPv4.<br>
* AIX needs a special function to enumerate IPv6 interfaces and to<br>
query the MAC address.<br>
* Moved the declaration of |siocgifconfRequest| to the beginning a the<br>
function (as recommend by Michael McMahon) to remain C89 compatible.<br>
<br>
<br>
src/solaris/native/java/net/<u></u>PlainSocketImpl.c<br>
<br>
* On AIX (like on Solaris) |setsockopt| will set errno to |EINVAL| if<br>
the socket is closed. The default error message is then confusing.<br>
<br>
<br>
src/aix/native/java/net/aix_<u></u>close.c,<br>
src/share/native/java/net/net_<u></u>util.c<br>
<br>
* As recommended by Michael McMahon and Alan Bateman I've move an<br>
adapted version of |linux_close.c| to<br>
|src/aix/native/java/net/aix_<u></u>close.c| because we also need the file<br>
and socket wrappers on AIX.<br>
* Compared to the Linux version, we've added the initialization of<br>
some previously uninitialized data structures.<br>
* Also, on AIX we don't have |__attribute((constructor))| so we need<br>
to initialize manually (from |JNI_OnLoad()| in<br>
|src/share/native/java/net/<u></u>net_util.c|.<br>
<br>
<br>
src/solaris/native/java/net/<u></u>net_util_md.h<br>
<br>
* AIX needs the same workaround for I/O cancellation like Linux and<br>
MacOSX.<br>
<br>
<br>
src/solaris/native/java/net/<u></u>net_util_md.c<br>
<br>
* |SO_REUSEADDR| is called |SO_REUSEPORT| on AIX.<br>
* On AIX we have to ignore failures due to |ENOBUFS| when setting the<br>
|SO_SNDBUF|/|SO_RCVBUF| socket options.<br>
<br>
<br>
src/solaris/native/java/util/<u></u>TimeZone_md.c<br>
<br>
* Currently on AIX the only way to get the platform time zone is to<br>
read it from the |TZ| environment variable.<br>
<br>
<br>
src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c<br>
<br>
* Use the |dladdr| from |porting_aix.{c,h}| on AIX.<br>
<br>
<br>
src/solaris/native/sun/awt/<u></u>fontpath.c<br>
<br>
* Changed some macros from |if !defined(__linux__)| to |if<br>
defined(__solaris__)| because that's their real meaning.<br>
* Add AIX specific fontpath settings and library search paths for<br>
|libfontconfig.so|.<br>
<br>
<br>
src/solaris/native/sun/java2d/<u></u>x11/X11SurfaceData.c<br>
<br>
* Rename the |MIN| and |MAX| macros to |XSD_MIN| and |XSD_MAX| to<br>
avoid name clashes (|MIN| and |MAX| are much too common and thexy<br>
are already defined in some AIX system headers).<br>
<br>
<br>
src/solaris/native/sun/java2d/<u></u>x11/XRBackendNative.c<br>
<br>
* Handle AIX like Solaris.<br>
<br>
<br>
src/solaris/native/sun/nio/ch/<u></u>Net.c<br>
<br>
* Add AIX-specific includes and constant definitions.<br>
* On AIX "socket extensions for multicast source filters" support<br>
depends on the OS version. Check for this and throw appropriate<br>
exceptions if it is requested but not supported. This is needed to<br>
pass<br>
JCK-api/java_nio/channels/<u></u>DatagramChannel/<u></u>DatagramChannel.html#Multicast<br>
<br>
<br>
src/solaris/native/sun/nio/fs/<u></u>UnixNativeDispatcher.c<br>
<br>
* On AIX |strerror()| is not thread-safe so we have to use<br>
|strerror_r()| instead.<br>
* On AIX |readdir_r()| returns EBADF (i.e. '9') and sets 'result' to<br>
NULL for the directory stream end. Otherwise, 'errno' will contain<br>
the error code.<br>
* Handle some AIX corner cases for the results of the |statvfs64()| call.<br>
* On AIX |getgrgid_r()| returns ESRCH if group does not exist.<br>
<br>
<br>
src/solaris/native/sun/<u></u>security/pkcs11/j2secmod_md.c<br>
<br>
* Use |RTLD_LAZY| instead of |RTLD_NOLOAD| on AIX.<br>
<br>
<br>
test/java/lang/ProcessBuilder/<u></u>Basic.java<br>
test/java/lang/ProcessBuilder/<u></u>DestroyTest.java<br>
<br>
* Port this test to AIX and make it more robust (i.e. recognize the<br>
"C" locale as |isEnglish()|, ignore VM-warnings in the output, make<br>
sure the "grandchild" processes created by the test don't run too<br>
long (60s vs. 6666s) because in the case of test problems these<br>
processes will pollute the process space, make sure the test fails<br>
with an error and doesn't hang indefinitley in the case of a problem).<br>
<br>
*Q (Michael McMahon):* Seems to be two macros _AIX and AIX. Is this<br>
intended?<br>
<br>
Well, |_AIX| is defined in some system headers while |AIX| is defined by<br>
the build system. This is already used inconsistently (i.e. |__linux__|<br>
vs. |LINUX|) and in general I try to be consistent with the style of the<br>
file where I the changes are. That said, I changes most of the<br>
occurences of |AIX| to |_AIX|.<br>
<br>
*Q (Alan Bateman):* There are a few changes for OS/400 in the patch, are<br>
they supposed to be there?<br>
<br>
We currently don't support OS/400 (later renamed into i5/OS and<br>
currently called IBM i) in our OpenJDK port but we do support it in our<br>
internel, SAP JVM build. We stripped out all the extra OS/400<br>
functionality from the port but in some places where there is common<br>
functionality needd by both, AIX and OS/400 the OS/400 support may still<br>
be visible in the OpenJDK port. I don't think this is a real problem and<br>
it helps us to keep our internel sources in sync with the OpenJDK port.<br>
That said, I think I've removed all the references to OS/400 now.<br>
<br>
<br>
</blockquote>
</blockquote></div>