[8u] RFR: 8267545: [8u] Enable Xcode 12 builds on macOS
Hohensee, Paul
hohensee at amazon.com
Thu May 27 22:39:55 UTC 2021
Thanks, Volker. Tagged.
-----Original Message-----
From: Volker Simonis <volker.simonis at gmail.com>
Date: Thursday, May 27, 2021 at 12:58 PM
To: "Hohensee, Paul" <hohensee at amazon.com>
Cc: "Taylor, Ben" <benty at amazon.com>, jdk8u-dev <jdk8u-dev at openjdk.java.net>
Subject: RE: [8u] RFR: 8267545: [8u] Enable Xcode 12 builds on macOS
Hi Ben, Paul,
thanks for doing the additional tweaking.
As long as you don't forget to remove the URLClassPath.c redundancy
before pushing, there's no need for a new webrev of the JDK part from
my side.
I approve all three parts.
Best regards,
Volker
On Thu, May 27, 2021 at 9:03 PM Hohensee, Paul <hohensee at amazon.com> wrote:
>
> Thanks for the quick reviews, Volker and Ben. New webrev:
>
> https://cr.openjdk.java.net/~phh/8267545/webrev.8u.root.05/
>
> In flags.m4, the new clang MACOSX_MIN_VERSION hunk is almost an exact copy of the existing gcc one. The only difference is 10.9.0 for clang and 10.7.0 for gcc. The existing gcc hunk includes the FIXME comment, so it's just a copy-paste thing which I'd prefer to leave alone. It's the same with the second hunk that includes HAS_GNU_HASH. The new clang hunk is indeed identical to gcc hunk, so I believe Simon put it there as a placeholder in case something needed to be different for clang. I changed the condition to check for gcc or clang rather than duplicate that hunk.
>
> Re Libraries.m4 and Platform.m4, I'm not an Xcode/clang expert either. The clang -std switch appears to mean to enable certain language features and to use the libraries that support them. So, e.g., you can specify -std=c++11, which means to enable c++11 features. LIBCXX is added to LDFLAGS of various sorts, so it looks like an ld command line switch to me. I can't find any documentation for it though, so I (and Ben) think it's a typo. I've changed it to
>
> LIBCXX="-stdlib=libc++"
>
> The URLClassPath.c redundancy was indeed a copy-paste error.
>
> Paul
>
> -----Original Message-----
> From: "Taylor, Ben" <benty at amazon.com>
> Date: Thursday, May 27, 2021 at 11:02 AM
> To: Volker Simonis <volker.simonis at gmail.com>, "Hohensee, Paul" <hohensee at amazon.com>
> Cc: jdk8u-dev <jdk8u-dev at openjdk.java.net>
> Subject: Re: [8u] RFR: 8267545: [8u] Enable Xcode 12 builds on macOS
>
> > platform.m4
> > -----------
> > -stdlib=libc++
>
> Based on the man page for clang from xcode 12.5 [1] I believe Volker is correct
> and this is likely a typo carried over from Simon's original patch [2]
>
> --Ben
>
> [1] -std=<standard> Specify the language standard to compile for.
> -stdlib=<library> Specify the C++ standard library to use;
> supported options are libstdc++ and libc++. If not
> specified, platform default will be used.
>
> [2] https://github.com/stooke/jdk8u-xcode10/blob/master/jdk8u-patch/jdk8u-libcxxfix.patch
>
> On 5/27/21, 8:36 AM, "jdk8u-dev on behalf of Volker Simonis" <jdk8u-dev-retn at openjdk.java.net on behalf of volker.simonis at gmail.com> wrote:
>
> Hi Paul,
>
> in general, this change looks good to me. I have just a few comments
> and questions:
>
> TOP-LEVEL
> =========
>
> flags.m4
> --------
> Are the following lines still required to point to a problem or can
> they be removed (e.g. we don't have any "closed legacy code" in ojdk8u
> any more)?
>
> 688 # FIXME: This needs to be exported in spec.gmk due to
> closed legacy code.
> 689 # FIXME: clean this up, and/or move it elsewhere.
>
> Also don't understand the following lines:
>
> 771 elif test "x$TOOLCHAIN_TYPE" = xclang; then
> 772 # If this is a --hash-style=gnu system, use --hash-style=both, why?
> 773 # We have previously set HAS_GNU_HASH if this is the case
> 774 if test -n "$HAS_GNU_HASH"; then
>
> but "$HAS_GNU_HASH" is only defined for the gcc toolchain (see
> "toolchain.m4") so it doesn't seem to make sense here.
>
> Later we have:
>
> 771 elif test "x$TOOLCHAIN_TYPE" = xclang; then
> ...
> 777 if test "x$OPENJDK_TARGET_OS" = xlinux; then
>
> but this change is for enabling Xcode builds on Mac and not clang
> builds on Linux. So ot seems the whole "elif test "x$TOOLCHAIN_TYPE" =
> xclang; then" isn't required?
>
>
> libraries.m4
> ------------
> LIBCXX="-std=libc++"
> platform.m4
> -----------
> -stdlib=libc++
>
> I'm not an Xcode expert so this is just a question. Are the options
> for "OPENJDK_TARGET_CPU_JLI_CFLAGS" and "LIBCXX" really different?
> I.e. "-std=libc++" vs. "-stdlib=libc++".
>
>
> JDK
> ===
>
> URLClassPath.c
> ---------------
>
> +/* defined in libverify.so/verify.dll (src file common/check_format.c) */
> +extern jboolean VerifyClassname(char *utf_name, jboolean arrayAllowed);
> +extern jboolean VerifyFixClassname(char *utf_name);
> +
> #include "sun_misc_URLClassPath.h"
>
> +/* defined in libverify.so/verify.dll (src file common/check_format.c) */
> +extern jboolean VerifyClassname(char *utf_name, jboolean arrayAllowed);
> +extern jboolean VerifyFixClassname(char *utf_name);
> +
>
> Do we really need to declare these external dependecies two time?
> Looks like a copy-paste error to me?
>
>
> HOTSPOT
> =======
>
> LGTM
>
> On Wed, May 26, 2021 at 7:33 PM Hohensee, Paul <hohensee at amazon.com> wrote:
> >
> > This is the last patch in the series that will enable Xcode 12 builds of 8u. The 8u build system is sufficiently different from later versions that backports beyond what have already been done are unrealistic.
> >
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8267545
> > root webrev: https://cr.openjdk.java.net/~phh/8267545/webrev.8u.root.03/
> > jdk webrev: https://cr.openjdk.java.net/~phh/8267545/webrev.8u.jdk.02/
> > hotspot webrev: https://cr.openjdk.java.net/~phh/8267545/webrev.8u.hotspot.01/
> >
> > As detailed in the JBS issue, these are a merge/update by Ben Taylor of Simon Tooke’s proposed patches. The minimum Xcode version has been changed from 4 to 6 because recent source changes cause the 8u build to fail with Xcode 4. See
> >
> > https://mail.openjdk.java.net/pipermail/jdk8u-dev/2021-May/013791.html
> >
> > References to Xcode 4 have been removed from the root patch. Also, the regenerated generated-configure.sh isn’t in the root patch for the sake of clarity: I’ll add it to the final push.
> >
> > In the hotspot patch:
> >
> >
> > 1. The change to ostream.cpp is the fix proposed by Ziyi Luo for 8256720. I plan to close 8256720 as a dup of 8267545.
> > 2. The change to os_bsd_x86.cpp was mistakenly left out of a previous backport (which I can’t find at the moment!)
> >
> > Thanks,
> > Paul
> >
> >
>
>
More information about the jdk8u-dev
mailing list