RFR(S) 8074010: followup to 8072383

[Dean Long]

Thanks Vladimir.

dl

On 3/3/2015 12:13 PM, Vladimir Kozlov wrote:
> Good.
>
> Thanks,
> Vladimir
>
> On 3/3/15 11:54 AM, Dean Long wrote:
>> Ping.  I think I still need another review for this.
>>
>> dl
>>
>> On 2/27/2015 10:11 AM, Dean Long wrote:
>>> Thanks for the review, David.  Can I get another review for this, 
>>> please?
>>>
>>> dl
>>>
>>> On 2/27/2015 12:43 AM, David Holmes wrote:
>>>> Thanks Dean - looks good to me.
>>>>
>>>> David
>>>>
>>>> On 27/02/2015 4:32 PM, Dean Long wrote:
>>>>> Thanks David for looking at this. Round two no longer tries to 
>>>>> simplify
>>>>> the
>>>>> makefile logic, but instead keeps existing variables names and 
>>>>> minimizes
>>>>> the number of lines changed.
>>>>>
>>>>> dl
>>>>>
>>>>> http://cr.openjdk.java.net/~dlong/8074010/8u60.01/
>>>>>
>>>>> On 2/26/2015 10:09 PM, David Holmes wrote:
>>>>>> Hi Dean,
>>>>>>
>>>>>> On 27/02/2015 3:32 PM, Dean Long wrote:
>>>>>>> This changeset removes references to closed platforms from 
>>>>>>> gcc.make,
>>>>>>> allows $(HS_ALT_MAKE)/linux/makefiles/gcc.make to override the
>>>>>>> default values, and simplifies the logic a bit.  It is targeted for
>>>>>>> 8u60.
>>>>>>
>>>>>> For the benefit of other readers the problem with the existing 
>>>>>> code is
>>>>>> that we use platform specific variables to define an additional 
>>>>>> flag eg:
>>>>>>
>>>>>> DEBUG_CFLAGS/amd64 = -g
>>>>>>
>>>>>> which is then added to the primary flag variable ie:
>>>>>>
>>>>>> DEBUG_CFLAGS += $(DEBUG_CFLAGS/$(BUILDARCH))
>>>>>>
>>>>>> That is all good. The problem is the code used to set a default:
>>>>>>
>>>>>>    ifeq ($(DEBUG_CFLAGS/$(BUILDARCH)),)
>>>>>>        ifeq ($(USE_CLANG), true)
>>>>>>          # Clang doesn't understand -gstabs
>>>>>>          DEBUG_CFLAGS += -g
>>>>>>        else
>>>>>>          DEBUG_CFLAGS += -gstabs
>>>>>>        endif
>>>>>>    endif
>>>>>>
>>>>>> Here we check the current value of $(DEBUG_CFLAGS/$(BUILDARCH)) and
>>>>>> use it's emptiness to hard-wire a specific flag onto the primary 
>>>>>> flag
>>>>>> variable. The problem with that is when we include
>>>>>> $(HS_ALT_MAKE)/linux/makefiles/gcc.make at the end of the file, it
>>>>>> wants to set a value of DEBUG_CFLAGS/$(BUILDARCH) for another
>>>>>> BUILDARCH and consequently the main flags variable will get that 
>>>>>> value
>>>>>> AND the default that was hard-wired - and we don't won't that.
>>>>>>
>>>>>> However, a cleaner solution that Dean and I have discussed is to
>>>>>> simply adjust the default-setting as follows:
>>>>>>
>>>>>>    ifeq ($(DEBUG_CFLAGS/$(BUILDARCH)),)
>>>>>>        ifeq ($(USE_CLANG), true)
>>>>>>          # Clang doesn't understand -gstabs
>>>>>>          DEBUG_CFLAGS/$(BUILDARCH) = -g
>>>>>>        else
>>>>>>          DEBUG_CFLAGS/$(BUILDARCH) = -gstabs
>>>>>>        endif
>>>>>>    endif
>>>>>>
>>>>>> so now if we later set DEBUG_CFLAGS/$(BUILDARCH) for a hitherto 
>>>>>> unseen
>>>>>> BUILDARCH it will be expanded in the primary flags variable as
>>>>>> expected and the "default" will not be used.
>>>>>>
>>>>>> Note there's also an existing bug in setting of the FASTDEBUG_CFLAGS
>>>>>> which has since been fixed in 9 and is now fixed here as well:
>>>>>>
>>>>>> -    FASTDEBUG_CFLAGS += $(DEBUG_CFLAGS/$(BUILDARCH))
>>>>>> +    FASTDEBUG_CFLAGS += $(FASTDEBUG_CFLAGS/$(BUILDARCH))
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> dl
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8074010
>>>>>>> http://cr.openjdk.java.net/~dlong/8074010/8u60.00/
>>>>>>>
>>>>>
>>>
>>