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
