Magnus:
Looks good to me as well.
Tim
On 03/17/15 06:15, Erik Joelsson wrote:
Looks good. Nice to find the root cause of this.
/Erik
On 2015-03-17 13:58, Magnus Ihse Bursie wrote:
It turned out that the fix for JDK-8074796 (Disabling warnings on
clang triggers compiler bug for libunpack) did not address the core
issue.
In fact, there was no compiler bug in clang. (Surprise! :-)) Instead,
what happened was that the makefile changes that turned on the
warning flags, also affected other flags sent to the compiler. This
happened on all toolchains, but was first noticed only for clang builds.
More precisely, due to the convoluted logic in
SetupNativeCompilation, the value of "CFLAGS_release := -DPRODUCT"
which was set in libunpack should have been copied by default to
CXXFLAGS_release, so it could be used when compiling C++ code.
However, if there are additional CXX flags set, then this copy does
not happen. Due to the exact placement of the DISABLED_WARINGS flags
code in SetupNativeCompilation, the CXX flags turned out to be
non-empty when the "if CXX flags not set, then copy C flags by
default" was executed. Hence, CFLAGS_release was not transferred to
CXXFLAGS_release, and -DPRODUCT was lost when compiling the C++ files.
One could certainly argue that our entire handling of C flags vs C++
flags is not ideal. Hopefully, we can address that in the future, and
create a more robust model.
For now, moving the code in SetupNativeCompilation will solve the
problems which was introduced with the new warning option. This will
also allow us to re-enable the warning statement for clang.
Bug: https://bugs.openjdk.java.net/browse/JDK-8075176
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8075176-disabled-warnings-removed-extra-cflags/webrev.01
/Magnus
