RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX
volker.simonis at gmail.com
Thu Nov 21 15:54:07 UTC 2013
thanks a lot for the fast review and your valuable comments. Please find my
On Thu, Nov 21, 2013 at 1:01 PM, Alan Bateman <Alan.Bateman at oracle.com>wrote:
> On 20/11/2013 18:26, Volker Simonis wrote:
> 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
> 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 +=
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
48 ifeq ($(OPENJDK_TARGET_OS), aix) 49 LIBNET_SRC_DIRS +=
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 (
"...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
> 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
> 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
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
wanted to blow up this change unnecessarily.
More information about the core-libs-dev