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

Fri Dec 11 20:20:15 UTC 2015

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