[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(S)  8074010: followup to 8072383
From:       Dean Long <dean.long () oracle ! com>
Date:       2015-02-27 18:11:24
Message-ID: 54F0B34C.5020103 () oracle ! com
[Download RAW message or body]

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/
>>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]