----- Original Message ----- > > On Jul 5, 2016, at 11:22 AM, Andrew Hughes <gnu.and...@redhat.com> wrote: > > > > ----- Original Message ----- > >> Hello, > >> > >> In general it looks good. It's nice to see that this also fixes that > >> warning output from configure. My only nit is the comment describing the > >> parameters for FLAGS_SETUP_GCC6_COMPILER_FLAGS. It indicates that it > >> takes named parameters while it actually just takes positional. Please > >> either change it to named parameters or fix the comment. > >> > > > > Ah, that's because it was named parameters for a while, then I changed it > > because it was easier than trying to get ARG_PREFIX instead of $1 into > > [$]$1CFLAGS_JDK and friends. > > > > Fixed in: > > > > http://cr.openjdk.java.net/~andrew/8156980/webrev.02/ > > > > I'll let someone on your side push it through so you can regenerate > > your internal configure script at the same time. > > I've also been looking at this area. It looks like we've found pretty > much the same set of problems and have similar solutions, so that's > good. I have a few suggestions or possible improvements. > > ------------------------------------------------------------------------------ > common/autoconf/flags.m4 > 716 $2JVM_CFLAGS="${$2JVM_CFLAGS} ${$2CXXSTD_CXXFLAG}" > > There is a pre-existing bug in the setup of ${$2CXXSTD_CXXFLAG}. It > is using FLAGS_CXX_COMPILER_CHECK_ARGUMENTS, which doesn't know about > the BUILD/TARGET distinction.
This seems to be a problem with both FLAGS_C_COMPILER_CHECK_ARGUMENTS and FLAGS_CXX_COMPILER_CHECK_ARGUMENTS, and thus with the FLAGS_COMPILER_CHECK_ARGUMENTS macro that calls them both, which are used in several places. I think this is outside the scope of this patch and something which should be fixed by completing the current half-hearted cross-compilation support. My aim here is just to fix the regression which breaks the GCC 6 support on build==target builds, and I'd rather whoever was working on the cross-compilation build continued that work. There is a solution already in the handling of the warning argument check, but it needs to be generalised to the other cases. > > Note that this also doesn't seem to affect some non-VM chunks of code, > such as adlc (in hotspot/src/share/vm/adlc). I don't know whether > that matters. Ugh, yes, so I see. It seems a couple of parts of the HotSpot build just blithely ignore all this and set their own flags unconditionally. For example, they also set -m64 regardless of whether it was successfully tested for or not. I've added a patch to pass the std setting down to these parts of the HotSpot build again, but they more generally need to be brought in line with everything else and respect the configure checks. > > ------------------------------------------------------------------------------ > common/autoconf/flags.m4 > 771 if test "x$1" = xTARGET; then > 772 TOOLCHAIN_PREPARE_FOR_VERSION_COMPARISONS > 773 else > 774 TOOLCHAIN_PREPARE_FOR_VERSION_COMPARISONS(BUILD_,$2) > 775 fi > > This is a fix for > https://bugs.openjdk.java.net/browse/JDK-8157358 > Syntax error in TOOLCHAIN_CHECK_COMPILER_VERSION > > I think the change I suggested in that CR is better. > I agree. I've reverted this part of my patch. I didn't realise it was already being worked on and it was getting in the way of working on this fix. I just went with the most local fix that got rid of the broken test invocations. > ------------------------------------------------------------------------------ > common/autoconf/flags.m4 > 1458 AC_DEFUN([FLAGS_SETUP_GCC6_COMPILER_FLAGS], > > Changing from AC_DEFUN_ONCE to AC_DEFUN is good. > > It's needed in order to pass in the prefix. Even ignoring that issue, > something needed to be done because the defun_once defined later was > not being expanded where used. It was being expanded once before, which was appropriate for a block of code that didn't take any arguments and was only used because it made the source file easier to read than trying to add that logic inside the TOOLCHAIN_CHECK_COMPILER_VERSION invocation. Now we do need to call it for both the build and target compilers, AC_DEFUN is more appropriate. > > ------------------------------------------------------------------------------ > common/autoconf/flags.m4 > 1470 $1CFLAGS_JDK="[$]$1CFLAGS_JDK ${NO_NULL_POINTER_CHECK_CFLAG} > ${NO_LIFETIME_DSE_CFLAG}" > 1471 $1JVM_CFLAGS="[$]$1JVM_CFLAGS ${NO_NULL_POINTER_CHECK_CFLAG} > ${NO_LIFETIME_DSE_CFLAG}" > > Adding the prefix to the CFLAGS variables is good. > > Since we've already determined we're using gcc6+ to get here, I don't > think there's any benefit to the pre-existing argument checks. > > More importantly, FLAGS_COMPILER_CHECK_ARGUMENTS doesn't account for > the BUILD / TARGET distinction, so may produce the wrong answer if the > build and target compilers are different versions of gcc. > > So I think those checks should be eliminated. As you may recall, these checks were originally called in all instances and we decided to limit it to GCC 6 or later in review. I still think keeping the checks is wise, as these options may not work with future compilers. If I was to eliminate anything, it would be the version check, but I realise HotSpot developers are pretty conservative about turning off these optimisations on existing builds. FLAGS_COMPILER_CHECK_ARGUMENTS does need fixing for the cross compilation case as mentioned above, and that's a more general issue that affects other calls too: $ cat common/autoconf/flags.m4|grep 'FLAGS_COMPILER_CHECK_ARGUMENTS('|grep -v '^#' FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$STACK_PROTECTOR_CFLAG -Werror], IF_FALSE: [STACK_PROTECTOR_CFLAG=""]) FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$ZERO_ARCHFLAG], IF_FALSE: [ZERO_ARCHFLAG=""]) FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [${COMPILER_TARGET_BITS_FLAG}${OPENJDK_TARGET_CPU_BITS}], FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [-Wno-this-is-a-warning-that-do-not-exist], FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [-Wno-this-is-a-warning-that-do-not-exist], FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$NO_NULL_POINTER_CHECK_CFLAG -Werror], FLAGS_COMPILER_CHECK_ARGUMENTS(ARGUMENT: [$NO_LIFETIME_DSE_CFLAG -Werror], In the case of -Wno-this-is-a-warning-that-do-not-exist, there are two invocations because the target and build compilers are both checked by switching the value of CXX: # Repeate the check for the BUILD_CC and BUILD_CXX. Need to also reset # CFLAGS since any target specific flags will likely not work with the # build compiler CC_OLD="$CC" CXX_OLD="$CXX" CC="$BUILD_CC" CXX="$BUILD_CXX" CFLAGS_OLD="$CFLAGS" CFLAGS="" That really needs to be factored into a macro and applied to all these calls. > > Also, I think the variables capturing the options probably also need > to be prefixed, if they are retained. > With AC_SUBST gone, these variables are only temporary. They are reset at the start and their values are not used again once they are added to the CFLAGS. > Aside: I also think the variable name for > -fno-delete-null-pointer-checks is mildly confusing, as the absence of > "DELETE" inverts the apparent meaning. I was trying to keep it short for brevity, but I agree and have renamed it. > > ------------------------------------------------------------------------------ > common/autoconf/spec.gmk > [removed] > 395 NO_NULL_POINTER_CHECK_FLAG=@NO_NULL_POINTER_CHECK_CFLAG@ > 396 NO_LIFETIME_DSE_CFLAG=@NO_LIFETIME_DSE_CFLAG@ > 397 CXXSTD_CXXFLAG=@CXXSTD_CXXFLAG@ > > These removals are good. This also eliminates the "FLAG" for "CFLAG" > typo in NO_NULL_POINTER_CHECK_FLAG=. > > ------------------------------------------------------------------------------ > > I've had to bring back CXXSTD_CXXFLAG=@CXXSTD_CXXFLAG@ for the renegade parts of the HotSpot build. New webrevs: http://cr.openjdk.java.net/~andrew/8156980/webrev.03/root/ http://cr.openjdk.java.net/~andrew/8156980/webrev.03/hotspot/ -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
