RFR (S): JDK-8142909 Integration of minor fixes from the build-infra project

David Holmes david.holmes at oracle.com
Mon Dec 14 07:17:36 UTC 2015 

Looks trivially fine to me :)

Thanks,
David

On 12/12/2015 6:20 AM, Magnus Ihse Bursie wrote:
> I'm trying to resurrect this review.
>
> Here is a new version:
> http://cr.openjdk.java.net/~ihse/JDK-8142909-hotspot-build-infra-integration/webrev.03
>
>
> I have dropped all the changes related to -g and OPT_CFLAGS. I'm not
> interested in that kind of large scale operations on the old build
> system, I just wanted to transfer the small patches we have in the
> build-infra forest to mainline to facilitate comparison between old and
> new build. If that was too problematic, let's skip it.
>
> Remaining changes are:
>   * Making adlc more quiet
>   * Sorting the windows build
>
> Also, in the meantime, a new difference has surfaced. In the first push
> of the new hotspot build system, the rewrite of SA, the file
> make/bsd/makefiles/saproc.make unfortunately was not deleted in
> mainline. I added the deletion of this file.
>
> I intend to push this through hs-rt. David and Erik, please confirm that
> these changes are OK for you.
>
> /Magnus
>
>
> On 2015-11-17 02:57, David Holmes wrote:
>> On 16/11/2015 10:14 PM, Erik Joelsson wrote:
>>> On 2015-11-16 12:11, David Holmes wrote:
>>>> On 16/11/2015 6:22 PM, Erik Joelsson wrote:
>>>>> On 2015-11-16 03:10, David Holmes wrote:
>>>>>> Hi Magnus,
>>>>>>
>>>>>> On 13/11/2015 9:33 PM, Magnus Ihse Bursie wrote:
>>>>>>> On 2015-11-13 09:13, Erik Joelsson wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> make/bsd/makefiles/amd64.make:
>>>>>>>> make/bsd/makefiles/gcc.make:
>>>>>>>> Perhaps the order of "$(OPT_CFLAGS/NOOPT)
>>>>>>>> $(OPT_CFLAGS/$(BUILDARCH))"
>>>>>>>> should be reversed to guarantee that NOOPT is the one used in case
>>>>>>>> BUILDARCH contains something that conflicts? The solaris file
>>>>>>>> does it
>>>>>>>> this way.
>>>>>>> You know you wrote that code originally? ;-)
>>>>>>>
>>>>>>> Yeah, I agree, it's more reasonable. Updated webrev:
>>>>>>> http://cr.openjdk.java.net/~ihse/JDK-8142909-hotspot-build-infra-integration/webrev.02
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> I don't like this particular change as it is too generic. It gives
>>>>>> the
>>>>>> appearance of being apply to apply whatever the BUILDARCH specific
>>>>>> optflags are, which makes no sense if the whole point here is to not
>>>>>> optimize these files. If all this is intended to do is include -g
>>>>>> then
>>>>>> I think that should be done explicitly.
>>>>>>
>>>>> The -g flag is added to OPT_CFLAGS/$(BUILDARCH) and I can only assume
>>>>> the intention was to have it added to all compilation command lines.
>>>>> However, the OPT_CFLAGS/<filename> variable overrides
>>>>> CFLAGS/$(BUILDARCH), effectively removing the -g flag from the files
>>>>> where we explicitly change the optimization level. The patch
>>>>> intends to
>>>>> make sure -g is used on those files too.
>>>>
>>>> I understand that, but it is far from obvious that OPT_FLAGS/BUILDARCH
>>>> means -g (why???), or that putting other things in that variable might
>>>> break files that should not be optimized.
>>>>
>>>>> A cleaner solution would be to not have -g be part of the OPT flags at
>>>>> all but its own set of DEBUG_CFLAGS.
>>>>
>>>> I thought -g was in the DEBUG_CFLAGS ?? Or maybe it used to be. Anyway
>>>> if all we are interested in is adding -g then I agree it should have
>>>> its own variable.
>>>>
>>> I looked some more at this. There is a DEBUG_CFLAGS, FASTDEBUG_CFLAGS
>>> and OPT_CFLAGS. These get picked depending on the debug level of the
>>> build. For example in product.make, $(OPT_CFLAGS/BYFILE) gets added to
>>> CFLAGS.
>>>
>>> I can't really see a better way of making sure the -g does not fall off
>>> for some files than what is proposed here. At least not without
>>> completely reworking the flags handling in the current hotspot
>>> makefiles, something I'm very uninterested in doing.
>>
>> In the -g case what we are handling is the addition of -g to product
>> builds (it is already on fastdebug, debug) when
>> ENABLE_FULL_DEBUG_SYMBOLS is selected, on a subset of architectures.
>> As a convenience we define it thus (BSD version):
>>
>> OPT_CFLAGS/ia64 = -g
>> OPT_CFLAGS/amd64 = -g
>> OPT_CFLAGS/arm = -g
>> OPT_CFLAGS/ppc = -g
>> OPT_CFLAGS += $(OPT_CFLAGS/$(BUILDARCH))
>>
>> so $(OPT_CFLAGS/$(BUILDARCH)) happens to contain -g. I would be happy
>> if the above were changed to:
>>
>> FDS_CFLAGS/ia64 = -g
>> FDS_CFLAGS/amd64 = -g
>> FDS_CFLAGS/arm = -g
>> FDS_CFLAGS/ppc = -g
>> OPT_CFLAGS += $(FDS_CFLAGS/$(BUILDARCH))
>>
>> and then you could use $(FDS_CFLAGS/$(BUILDARCH) on the "by-file"
>> definitions, where it would be much clearer what kind of flag is
>> actually being applied. However this needs to be done everywhere, and
>> I don't see this fix attempting to do that. We set
>> OPT_CFLAGS/<filename> in numerous places (open and closed) to regain
>> control over the "opt" flags applied to that file. Sometimes we use
>> predefined opt flags, like NOOPT or SPEED, and sometimes we don't eg:
>>
>> linux/makefiles/amd64.make
>>
>> OPT_CFLAGS/compactingPermGenGen.o = -O1
>>
>> This would need to be applied to every occurrence of the "byfile"
>> definitions in all the makefiles. Or else ascertained on a
>> case-by-case basis that we don't want -g for a specific file.
>>
>> Further, on architectures where $(FDS_CFLAGS/$(BUILDARCH) is not set
>> you still have to somehow handle the rest of the above logic:
>>
>>  507     ifeq ($(OPT_CFLAGS/$(BUILDARCH)),)
>>  508       ifeq ($(USE_CLANG), true)
>>  509         # Clang doesn't understand -gstabs
>>  510         OPT_CFLAGS += -g
>>  511       else
>>  512         OPT_CFLAGS += -gstabs
>>  513       endif
>>  514     endif
>>
>> else you again fail to set -g or -gstabs on the "by-file" definitions.
>>
>> Also looking at the change in make/solaris/makefiles/amd64.make you
>> have taken the file specific flags:
>>
>>  OPT_CFLAGS/generateOptoStub.o = -xO2
>>
>> and added the generic OPT_CFLAGS value:
>>
>> OPT_CFLAGS/generateOptoStub.o = $(OPT_CFLAGS) -xO2
>>
>> which seems extremely dubious from a correctness perspective.
>>
>> Thanks,
>> David
>> -----
>>
>>>>>> ---
>>>>>>
>>>>>> make/bsd/makefiles/saproc.make
>>>>>>
>>>>>> ! # Order src files alfabetically
>>>>>>
>>>>>> That would be "alphabetically". But that list doesn't seem alphabetic
>>>>>> anyway: MacosxDebuggerLocal.m would come after libproc_impl.c; and
>>>>>> $(AGENTDIR) should come first. I really see no point in forcing such
>>>>>> lists to be in alphabetic order.
>>>>>>
>>>>> This is to make comparison of binaries between the old hotspot
>>>>> build and
>>>>> the new hotspot build easier. In build-infra, we $(sort ) the object
>>>>> files before linking to get a reproducible order. The linker output is
>>>>> affected by the order of the object files. By making sure the old and
>>>>> the new build sorts them the same way, we get cleaner comparisons and
>>>>> can more easily detect other types of differences through those
>>>>> comparisons. The sort order isn't strictly alphabetical, it's on byte
>>>>> order so upper case goes before lower case.
>>>>
>>>> I saw the change for the ordered object files on Windows, but this
>>>> simply orders source files in a list. Not sure how that has any affect
>>>> on the linking ??
>>>>
>>> It has an effect on the linking because that list is used verbatim on
>>> the combined compile/link command line.
>>>
>>> /Erik
>>>> Cheers,
>>>> David
>>>>
>>>>> /Erik
>>>>>> make/linux/makefiles/saproc.make doesn't have the comment that was
>>>>>> added for bsd.
>>>>>>
>>>>>> ----
>>>>>>
>>>>>> make/solaris/makefiles/amd64.make
>>>>>>
>>>>>> Similar issue with bsd amd64.make. Not sure what you are trying to
>>>>>> achieve here - is it some kind of last option wins situation ?
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> make/windows/create_obj_files.sh
>>>>>>
>>>>>> Harmless I guess but not sure about relevance of sort order here.
>>>>>>
>>>>>> ----
>>>>>>
>>>>>>  src/share/vm/adlc/adlparse.cpp
>>>>>>
>>>>>> So that's where that comes from! :)
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> src/share/vm/gc/g1/g1EvacStats.cpp
>>>>>>
>>>>>> I don't see anything in the cpp files that uses anything from the
>>>>>> atomic class ???
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> /Magnus
>>>>>>>
>>>>>>>>
>>>>>>>> Otherwise looks good.
>>>>>>>>
>>>>>>>> /Erik
>>>>>>>>
>>>>>>>> On 2015-11-13 03:34, Magnus Ihse Bursie wrote:
>>>>>>>>> In the new hotspot build project, we have made a few changes to
>>>>>>>>> the
>>>>>>>>> existing build. In preparation for the new build, I'd like to
>>>>>>>>> integrate these into mainline.
>>>>>>>>>
>>>>>>>>> These changes are:
>>>>>>>>> * When overriding optimization, do not lose current debug (-g)
>>>>>>>>> setting (!)
>>>>>>>>> * Make adlc actually quiet in quiet mode
>>>>>>>>> * Make g1EvacStats.cpp compile in all cases without precompiled
>>>>>>>>> headers
>>>>>>>>> * Sort saproc object files when linking (facilitates comparison to
>>>>>>>>> new build)
>>>>>>>>>
>>>>>>>>> Unless someone suggests otherwise, I intend to push this (using
>>>>>>>>> JPRT)
>>>>>>>>> to hs-rt.
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8142909
>>>>>>>>> WebRev:
>>>>>>>>> http://cr.openjdk.java.net/~ihse/JDK-8142909-hotspot-build-infra-integration/webrev.01
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> /Magnus
>>>>>>>>
>>>>>>>
>>>>>
>>>
>

More information about the build-dev mailing list