On 2018-03-02 00:46, Erik Joelsson wrote:
Hello Magnus,
Nice to finally see this posted! Overall very nice improvement.
Thank you.
I would advocate a simple bash replace to remove the dots, like this:
MACOSX_VERSION_MIN_NODOTS=${MACOSX_VERSION_MIN//\./}
Ok, I updated this. I see no need to post another review for this.
* On macosx/clang, the JVM_CFLAGS for the build toolchain will now
also get an -fPIC (this was mysteriously missing before).
* On windows/microsoft, the build toolchain will now get -nologo
added as well, and also -homeparams for debug builds.
* On solaris/solstudio, the JDKEXE flags will now include -errfmt,
and CFLAGS_JDKEXE will include -errtags=yes. The duplication of
-errtags=yes in CXXFLAGS_JDKLIB is removed.
* On solaris/solstudio, we now always use -KPIC as pic flag.
(Previously, we used -xcode=pic32 on sparc, but this is equivalent to
-KPIC, so there's no reason for a special case.)
* On solaris/solstudio, we now use e.g. "-Wc,-Qrm-s" instead of
"-Qoption cg -Qrm-s" for C++ as well as for C code. The two formats
are equivalent (for passing options to a certain compilation phase),
and there was no reason to use different ones for C and C++.
And for clarity, I have also renamed some exported flags to clearly
show that they are consumed by LIBJSIG and ADLC. I have also renamed
"/STACK" on the Microsoft linker to "-stack", to match how we write
options elsewhere.
I believe none of these should have any real effect, but if anything,
they could probably be considered bug fixes.
I have considered whitespace and ordering differences as irrelevant.
In some cases ordering may be relevant, hopefully the COMPARE_BUILD
run will uncover if that's the case. If those are ok, then I'm
basically happy with this transformation.
COMPARE_BUILD runs on our internal mach5 system showed no discrepancies.
(This was the reason I fixed the COMPARE_BUILD system for no-change builds.)
I actually don't know of any cases where CFLAGS ordering is relevant.
(LIBS is another issue) It's good to be aware of such issues, so please
enlighten me. :-)
I noticed one thing, though, was that a trivial sort of the flags before
writing them to spec.gmk didn't work. I did this (and similarly on a
patched baseline) to facilitate my comparison, but it was not
compilable. The reason was that a few flags (actually, currently only on
clang/macosx) had arguments that was space separated from the options.
For most flags, we do not have this scenario, which I think is good. It
makes it easier to see what is a "single" flag.
I have manually verified that the generated spec.gmk and
buildjdk-spec.gmk matches this (no changes apart from the one listed
above) for linux/x64/gcc, solaris/sparcv9/solstudio,
windows/x64/microsoft and macosx/x64/clang, for release and fastdebug.
I am also currently running a test job using the COMPARE_BUILD
functionality on our internal build system.
I would appreciate if comments are more in the form of "think of this
for subsequent updates to the flag handling" rather than requests to
change this patch, at least for requests that are just not small
fixes. (I've been juggling this for a while, trying to get it right...)
Bug: https://bugs.openjdk.java.net/browse/JDK-8198724
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8198724-refactor-flags-step-1/webrev.01
Comments on things I saw, not necessarily needing fixes now:
In flags.m4, MACHINE_FLAG and COMPILER_TARGET_BITS_FLAG are redundant
without comment.
Oh, how I wish this was true. :-( COMPILER_TARGET_BITS_FLAG is actually
exported for use in (you guess!) GensrcX11Wrappers.gmk. (Guess why I was
targeting that for cleanup :-)).
When the GensrcX11Wrappers fix is in, I intend to clean up this.
flags-cflags.m4, FLAGS_SETUP_SHARED_LIBS, comments out of
date/irrelevant like:
# Default works for linux, might work on other platforms as well.
I'll go through those at a later date. For the moment,
FLAGS_SETUP_SHARED_LIBS was "good enough" to be left alone.
Solaris -errtags is not needed in CFLAGS_WARNINGS_ARE_ERRORS.
Yes. I'm currently working on a follow-up patch were I redesign the
warnings handling, including -errtags.
Given that TOOLCHAIN_MINIMUM_VERSION_gcc="4.7", perhaps we can remove
the check for -Wno-X on gcc 4.4.
Good find. It would be nice to get rid of it.
In fact, I wonder if it is possible to raise the minimum gcc version to
4.8 for JDK11. That would help us get rid of even more gcc version
checks for broken/bad behaviour in pre-4.8 gcc.
As you can see, there's a lot of follow-up cleaning left to do. This
refactoring was scary enough that I wanted to minimize the changes done
in this first step.
/Magnus
/Erik
/Magnus
