RFR: JDK-8075176 DISABLED_WARNINGS caused C++ compiler flags to get lost

[Tim Bell]

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
>