<div dir="ltr"><div>Hi Phil,<br><br></div>thanks a lot for the review. Please find my comments inline:<br><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Nov 26, 2013 at 12:24 AM, Phil Race <span dir="ltr"><<a href="mailto:philip.race@oracle.com" target="_blank">philip.race@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
I see you've already received a ton of good feedback on this v2.<br>
<br>
I have just a few things to add.<br>
I don't know what symlinks might exist on AIX but it seems odd to<br>
me that you have :-<br>
<br>
138 static char *fullAixFontPath[] = {<br>
139 "/usr/lpp/X11/lib/X11/fonts/<u></u>Type1",<br>
..<br>
<br>
but the paths in the new file aix.fontconfig.properties look like this :-<br>
<br>
/usr/lib/X11/fonts/Type1/cour.<u></u>pfa<br>
<br></blockquote><div><br></div><div>You're absolutely right. I've updated 'aix.fontconfig.properties' to contain the same absolute path like 'fontpath.c'. I've also added a comment which describes to which AIX-package the fonts belong to and to which locations they are sym-linked to. <br>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also for the build to pick up a file called<br>
*src/aix/classes/sun/awt/<u></u>fontconfigs/aix.fontconfig.<u></u>properties<br>
<br>
*it seems like you should have added a section to handle this path to<br>
jdk/makefiles/gendata/<u></u>GenDataFontConfig.gmk<br>
<br>
That seems to be where the new build handles such files.<br>
<br>
Are you seeing the .bfc and .src files created ?<br>
<br></blockquote><div><br></div><div>You're right. But that was already added by the general AIX-build change "8024900: PPC64: Enable new build on AIX (jdk part)" (<a href="http://hg.openjdk.java.net/ppc-aix-port/stage/jdk/diff/3ac08cd5e2e8/makefiles/GendataFontConfig.gmk">http://hg.openjdk.java.net/ppc-aix-port/stage/jdk/diff/3ac08cd5e2e8/makefiles/GendataFontConfig.gmk</a>).<br>
</div><div><br></div><div>And yes, the .bfc and .src files are created and copied to the right places.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
At runtime it'll get picked up so so long as System.getProperty("<a href="http://os.name" target="_blank">os.name</a>")<br>
returns "aix" (all lower-case) so I think that's OK. Its the build time part<br>
I'd expect to see but don't.<br>
<br></blockquote><div><br></div><div>I did split the AIX change into several changes and the build changes have been reviewed separately:<br><br><span title="JDK-8024265: PPC64: Enable new build on AIX"><span class="">8024265: PPC64: Enable new build on AIX (</span></span><a href="https://bugs.openjdk.java.net/browse/JDK-8024265">https://bugs.openjdk.java.net/browse/JDK-8024265</a>)<br>
8024900: PPC64: Enable new build on AIX (jdk part) (<a href="https://bugs.openjdk.java.net/browse/JDK-8024900">https://bugs.openjdk.java.net/browse/JDK-8024900</a>)<br><br></div><div>This change only contains the additional make changes which became necessary after I started to move AIX-specific files into their own jdk/src/aix/ directory. Everything else is already in place.<br>
<br></div><div>I'll prepare and test a finaly webrev with all the changes from this review round today.<br><br></div><div>Thanks a lot,<br></div><div>Volker<br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-phil.<div class="im"><br>
<br>
On 11/20/2013 10:26 AM, Volker Simonis wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi,<br>
<br>
this is the second review round for "8024854: Basic changes and files to build the class library on AIX <<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 reviews can be found at the end of this mail in the references section.<div class="im">
<br>
<br>
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.<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> repository which has been recently synchronised with the jdk8 master:<br>
<br>
</div><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> <<a href="http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v2/" target="_blank">http://cr.openjdk.java.net/%<u></u>7Esimonis/webrevs/8024854.v2/</a>><div class="im">
<br>
<br>
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:)<br>
<br>
Thank you and best regards,<br>
Volker<br>
<br></div>
*References:*<div class="im"><br>
<br>
The following reviews have been posted so far (thanks Iris for 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 (15 Oct [7])<br>
- Sean Mullan (sec): Initial comments (26 Sept [8])<br>
<br>
[2]: <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]: <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]: <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]: <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]: <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]: <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></div>
*Detailed change description:*<div class="im"><br>
<br>
The new "jdk/src/aix" subdirectory contains the following new and AIX-specific files for now:<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></div>
* Added these two files for AIX relevant utility code.<br>
* Currently these files only contain an implementation of |dladdr|<div class="im"><br>
which is not available on AIX.<br></div>
* In the first review round the existing |dladdr| users in the code<div class="im"><br>
either called the version from the HotSpot on AIX<br></div>
(|src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c|) or had a private<div class="im"><br>
copy of the whole implementation<br></div>
(|src/solaris/demo/jvmti/<u></u>hprof/hprof_md.c|). This is now not<div class="im"><br>
necessary any more.<br>
<br>
The new file layout required some small changes to the makefiles to make them aware of the new directory locations:<br>
<br>
<br>
makefiles/CompileDemos.gmk<br>
<br></div>
* Add an extra argument to |SetupJVMTIDemo| which can be used to<br>
pass additional source locations.<br>
* Currently this is only used on AIX for the AIX porting utilities<div class="im"><br>
which are required by hprof.<br>
<br>
<br>
makefiles/lib/Awt2dLibraries.<u></u>gmk<br>
makefiles/lib/<u></u>ServiceabilityLibraries.gmk<br>
<br></div>
* On AIX add the sources of the AIX porting utilities to the build.<div class="im"><br>
They are required by<br>
|src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c| and<br>
|src/solaris/demo/jvmti/hprof/<u></u>hprof_md.c| because |dladdr| is not<br>
available on AIX.<br>
<br>
<br>
makefiles/lib/NioLibraries.gmk<br>
<br></div>
* 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<div class="im"><br>
Linux version as suggested by Sean Mullan.<br>
<br>
<br>
src/share/native/common/check_<u></u>code.c<br>
<br></div>
* On AIX malloc(0) and calloc(0, ...) return a NULL pointer, which<div class="im"><br>
is 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></div>
* |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<div class="im"><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></div>
* 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<br>
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<div class="im"><br>
"|LD_LIBRARY_PATH|" directly to cope with different library path<br>
names.<br></div>
* Add |jre/lib/<arch>/jli| to the standard library search path on<div class="im"><br>
AIX because the AIX linker doesn't support the |-rpath| option.<br></div>
* Replace |#ifdef __linux__| by |#ifndef __solaris__| because in<div class="im"><br>
this case, AIX behaves like Linux.<br></div>
* Removed the definition of |JVM_DLL|, |JAVA_DLL| and<div class="im"><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></div>
* 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<div class="im"><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></div>
* Detect AIX operating system and return the corresponding channel<div class="im"><br>
and file system providers.<br>
<br>
<br>
src/solaris/classes/sun/nio/<u></u>ch/Port.java<br>
<br></div>
* Add a callback function |unregisterImpl(int fd)| for<div class="im"><br>
implementations that need special handling when |fd| is removed<br></div>
and call it from |unregister(int fd)|. By default the<div class="im"><br>
implementation of |unregisterImpl(int fd)| is empty except for the<br>
derived |AixPollPort| class on AIX.<br>
<br>
<br>
src/solaris/demo/jvmti/hprof/<u></u>hprof_md.c<br>
<br></div>
* Add AIX support. AIX mostly behaves like Linux, with the one<div class="im"><br>
exception that it has no |dladdr| functionality.<br></div>
* 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<br>
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<div class="im"><br>
process 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></div>
* 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<div class="im"><br>
the function (as recommend by Michael McMahon) to remain C89<br>
compatible.<br>
<br>
<br>
src/solaris/native/java/net/<u></u>PlainSocketImpl.c<br>
<br></div>
* On AIX (like on Solaris) |setsockopt| will set errno to |EINVAL|<div class="im"><br>
if 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></div>
* As recommended by Michael McMahon and Alan Bateman I've move an<div class="im"><br>
adapted version of |linux_close.c| to<br>
|src/aix/native/java/net/aix_<u></u>close.c| because we also need the<br>
file and socket wrappers on AIX.<br></div>
* Compared to the Linux version, we've added the initialization of<div class="im"><br>
some previously uninitialized data structures.<br></div>
* Also, on AIX we don't have |__attribute((constructor))| so we need<div class="im"><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></div>
* 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><div class="im">
* |SO_REUSEADDR| is called |SO_REUSEPORT| on AIX.<br>
* On AIX we have to ignore failures due to |ENOBUFS| when setting<br>
the |SO_SNDBUF|/|SO_RCVBUF| socket options.<br>
<br>
<br>
src/solaris/native/java/util/<u></u>TimeZone_md.c<br>
<br></div>
* Currently on AIX the only way to get the platform time zone is to<div class="im"><br>
read it from the |TZ| environment variable.<br>
<br>
<br>
src/solaris/native/sun/awt/<u></u>awt_LoadLibrary.c<br>
<br></div>
* 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<div class="im"><br>
defined(__solaris__)| because that's their real meaning.<br></div>
* 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<div class="im"><br>
are already defined in some AIX system headers).<br>
<br>
<br>
src/solaris/native/sun/java2d/<u></u>x11/XRBackendNative.c<br>
<br></div>
* 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<div class="im"><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></div>
* 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<div class="im"><br>
NULL for the directory stream end. Otherwise, 'errno' will contain<br>
the error code.<br></div>
* Handle some AIX corner cases for the results of the |statvfs64()|<br>
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,<div class="im"><br>
make sure the "grandchild" processes created by the test don't run<br>
too long (60s vs. 6666s) because in the case of test problems<br>
these processes will pollute the process space, make sure the test<br>
fails with an error and doesn't hang indefinitley in the case of a<br>
problem).<br>
<br></div>
*Q (Michael McMahon):* Seems to be two macros _AIX and AIX. Is this intended?<br>
<br>
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|.<br>
<br>
*Q (Alan Bateman):* There are a few changes for OS/400 in the patch, are they supposed to be there?<div class="im"><br>
<br>
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.<br>
<br>
<br>
</div></blockquote>
<br>
</blockquote></div><br></div></div></div></div>