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

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 OPTCFLAGS. 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 "$(OPTCFLAGS/NOOPT) $(OPTCFLAGS/$(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 OPTCFLAGS/$(BUILDARCH) and I can only assume the intention was to have it added to all compilation command lines. However, the OPTCFLAGS/ 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 OPTFLAGS/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 DEBUGCFLAGS. I thought -g was in the DEBUGCFLAGS ?? 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 DEBUGCFLAGS, FASTDEBUGCFLAGS and OPTCFLAGS. These get picked depending on the debug level of the build. For example in product.make, $(OPTCFLAGS/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 ENABLEFULLDEBUGSYMBOLS is selected, on a subset of architectures. As a convenience we define it thus (BSD version): OPTCFLAGS/ia64 = -g OPTCFLAGS/amd64 = -g OPTCFLAGS/arm = -g OPTCFLAGS/ppc = -g OPTCFLAGS += (OPTCFLAGS/(OPTCFLAGS/(OPTCFLAGS/(BUILDARCH)) so (OPTCFLAGS/(OPTCFLAGS/(OPTCFLAGS/(BUILDARCH)) happens to contain -g. I would be happy if the above were changed to: FDSCFLAGS/ia64 = -g FDSCFLAGS/amd64 = -g FDSCFLAGS/arm = -g FDSCFLAGS/ppc = -g OPTCFLAGS += (FDSCFLAGS/(FDSCFLAGS/(FDSCFLAGS/(BUILDARCH)) and then you could use (FDSCFLAGS/(FDSCFLAGS/(FDSCFLAGS/(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 OPTCFLAGS/ 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 OPTCFLAGS/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 (FDSCFLAGS/(FDSCFLAGS/(FDSCFLAGS/(BUILDARCH) is not set you still have to somehow handle the rest of the above logic: 507 ifeq ($(OPTCFLAGS/$(BUILDARCH)),) 508 ifeq ($(USECLANG), true) 509 # Clang doesn't understand -gstabs 510 OPTCFLAGS += -g 511 else 512 OPTCFLAGS += -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: OPTCFLAGS/generateOptoStub.o = -xO2 and added the generic OPTCFLAGS value: OPTCFLAGS/generateOptoStub.o = $(OPTCFLAGS) -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 libprocimpl.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/createobjfiles.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