> On Jul 5, 2016, at 1:33 PM, Andrew Hughes <gnu.and...@redhat.com> wrote:
>
> ----- Original Message -----
>>> On Jul 5, 2016, at 11:22 AM, Andrew Hughes <gnu.and...@redhat.com> wrote:
>> 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.
I totally agree that fixing the xxx_CHECK_ARGUMENTS is out of scope
for this patch.
What I'm worried about is that by keeping those checks we might get
and use the wrong answer in some cases where the BUILD and TARGET
compilers are of different vintage. Maybe that will just encourage
someone to fix them...
In the specific case of -std=gnu++98, it seems unlikely we'll see such
a misconfiguration any time soon. That option was introduced in
gcc3.3, and it seems unlikely to me that anyone is building the JDK
with such an old compiler (though it wouldn't be the first time I've
been surprised in that way). OTOH, if the compiler is very new and has
dropped support for that standard (e.g. some as yet not even announced
version of gcc), we actually want a build failure, since our code was
written for that standard and not some later one. So we're unlikely to
be hurt by the use of xxx_CHECK_ARGUMENTS here.
For the gcc6-specific options, see below.
>> 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.
I think there are some more that are outside of HotSpot.
Searching the build directory for *.o.cmdline files that don’t contain
-std=gnu++98, e.g.
find . -name "*.o.cmdline" ! -exec egrep -q -e "-std=gnu\+\+98" {} \; -print |
xargs dirname | uniq
produces a lot of stuff in ./support/native, the afore mentioned adlc, and a
smattering of others.
I think those might be better addressed by more followups, to get what we’ve
got so far in.
>> 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.
I wasn't planning to work on any of this, but was feeling annoyed
about the situation, and then events conspired to leave me at loose
ends for a bit this weekend. Though if I'd remembered how much I
dislike autoconf, I might have looked harder for alternatives.
But you will need that fix for JDK-8157358. I guess I should post an
RFR for it... or you can just incorporate it into this patch if I don’t get
that RFR done soon.
>> 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.
You are right about it being expanded once. I must have been
remembering one of the unsuccessful attempts I tried, where no code
was generated at all for it.
> Now we do need to call it for both the build and target compilers, AC_DEFUN
> is more appropriate.
Yes.
>> 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.
I'm much more worried about getting and using the wrong answer for
these options. Since we're already in a context where we're known to
be using gcc6+, we know we're not using a too-old compiler that
doesn't support them for that reason.
-f[no-]delete-null-pointer-checks has been around since gcc 3.0, and
seems unlikely to go away, since it has a real documented purpose
(address zero isn't so special on some platforms).
-f[no-]lifetime-dse is fairly new (introduced in gcc 4.9), so seems
unlikely to go away soon, and if it did we would want a build failure
anyway, at least until we've fixed the code that gets broken by that
optimization.
> 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:
Yes.
>
> $ 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.
Yes.
>> 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.
Oh, right, no more AC_SUBST here.
> 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/
------------------------------------------------------------------------------
hotspot/make/lib/CompileDtracePostJvm.gmk
90 JVM_OFFSETS_CFLAGS := $(CXXSTD_CXXFLAG) -m64
This appears to be in a solaris-specific block, so I don't think this
change should be made.
------------------------------------------------------------------------------
hotspot/make/gensrc/GensrcAdlc.gmk
54 # Set the C++ standard if supported
55 ADLC_CFLAGS += $(CXXSTD_CXXFLAG)
This appears to be in a platform-independent block; I think it needs
to be moved to a different location, probably the linux block starting
at line 37.
