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:

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.




Reply via email to