Re: RFR: JDK-8142907 Integration of minor fixes from the build-infra project

[David Holmes]

Hi Magnus,

On 12/12/2015 8:24 AM, Magnus Ihse Bursie wrote:

On 2015-12-11 22:43, Magnus Ihse Bursie wrote:

Hi David,

Resurrecting the second part of the build-infra integrations. :)
Fortunately, no major code changes in the meantime has affected this
patch. (I did need to update the new
hotspot/make/lib/Lib-jdk.hotspot.agent.gmk though.)


It seems I forgot to include the link to the new webrev:

http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.05


hotspot.m4:

 54 #server: normal interpreter and a tiered C1/C2 compiler
  55 #client: normal interpreter and C1 (no C2 compiler) (only 
32-bit platforms)


Neither of these statements are necessarily 100% true - not all 
platforms have tiered enable; some 64-bit platforms do (or will) support 
client.


Otherwise I didn't see anything else that I obviously needed to comment 
on. The proof of this is in the building. So as long as this gets 
through JPRT with -testset hotspot we should be okay. :)


Thanks,
David



/Magnus




On 2015-11-16 05:33, David Holmes wrote:

Hi Magnus,

I had a flick through most of the files. Overall seems okay but I
have a few queries below.


Replies inline.



On 13/11/2015 12:43 PM, Magnus Ihse Bursie wrote:

The build-infra project has collected a number of minor fixes and
changes during the new hotspot build development. It's a mix of
code
cleanup and new capabilities.

Not all of these new features are immediately beneficial to the JDK,
but
they will be needed for the upcoming new Hotspot build, and it will not
hurt to have them in mainline. (In fact, it will tremendously help
merging between mainline and build-infra.)

The fix addresses these issues:

In general:
* Break out hotspot configuration into hotspot.m4
* Long link lines uses @-files
* Consistently use -Wl instead of -Xlinker
* Improve clang on linux compilation
* Set shared library name explicitely on solaris
* Set correct shared library flag on Windows (-dll)
* Consistency fixes for build toolchain
* Bring compare script up to date
* General code/whitespace cleanup
* Additional functionality in MakeBase

In NativeCompilation.gmk:
* More efficient vardeps for per-file CFLAGS
* Fewer shell executions (means better performance on Windows)
* EXCLUDE_PATTERN and EXTRA_OBJECT_FILES
* Debug symbols on macosx (disabled for existing code to keep current
behavior)

Enabling debug info on macosx on existing jdk should be treated in a
follow-up bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8142907
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.01



(It turned out that WebRev could not at the same time include files
from
multiple repos and track the history of a "hg cp":ied file. So I
created
an alternative revision here:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.02/



It does not include the jdk files, but hotspot.m4 might be easier to
understand)


flags.m4:

  60   AC_SUBST(LEGACY_EXTRA_CFLAGS)
  61   AC_SUBST(LEGACY_EXTRA_CXXFLAGS)
  62   AC_SUBST(LEGACY_EXTRA_LDFLAGS)
  63
  64   AC_SUBST(EXTRA_CFLAGS)
  65   AC_SUBST(EXTRA_CXXFLAGS)
  66   AC_SUBST(EXTRA_LDFLAGS)

IIRC we added the legacy flags purely to pass the cross-compilation
args through to hotspot. Not sure why we need both legacy and
non-legancy variants now ??


This is part of an ongoing effort to split CFLAGS_JDK into more
reasonable parts. The current model is that we throw all possible
flags together in CFLAGS_JDK (and then further on to CFLAGS_JDKLIB et
al). While it was questionable even before, it did work when we only
built the JDK native libraries. Now that we need to build libjvm.so as
well, which has (for historical reason) a set of flags that are both
partially the same and partially different, this is not so good.

We would like to see a change to a situation where the different
"parts" that we build CFLAGS_JDK from would be handled separately, and
then combined as needed for JDK libraries, and for Hotspot. So,
currently we use EXTRA_CFLAGS just to add them to CFLAGS_JDK, and then
drop them. In the new Hotspot build, we use the EXTRA_CFLAGS to add
them to the Hotspot flags. Of course, we could have used
LEGACY_EXTRA_CFLAGS, but we'd like to avoid using "LEGACY" for new stuff.

With that being said, I now realize that maybe this change will not be
needed anyway, at least not right now. Since doing a proper flag
cleanup is a major task, we decided to copy the CFLAGS_JDK behavior
for Hotspot flags as well in the current implementation of the new
hotspot build, and saving the cleanup for another day (so it will not
block the new build system). So I'll revert this part of the change
for now.


On Windows -LD is a superset of -dll, so it isn't obvious the change
is correct.

I hope Erik's reply to that was satisfactory.


---

jdk/make/lib/LibCommon.gmk

+ # Disable it here for the jdk binaries until we decide to enable them.

s/binaries/libraries/ ?

Sure, I'll fix.

Re: RFR: JDK-8142907 Integration of minor fixes from the build-infra project

[Magnus Ihse Bursie]

On 2015-12-14 09:12, David Holmes wrote:

Hi Magnus,

On 12/12/2015 8:24 AM, Magnus Ihse Bursie wrote:

On 2015-12-11 22:43, Magnus Ihse Bursie wrote:

Hi David,

Resurrecting the second part of the build-infra integrations. :)
Fortunately, no major code changes in the meantime has affected this
patch. (I did need to update the new
hotspot/make/lib/Lib-jdk.hotspot.agent.gmk though.)


It seems I forgot to include the link to the new webrev:

http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.05 



hotspot.m4:

 54 #server: normal interpreter and a tiered C1/C2 compiler
  55 #client: normal interpreter and C1 (no C2 compiler) (only 
32-bit platforms)


Neither of these statements are necessarily 100% true - not all 
platforms have tiered enable; some 64-bit platforms do (or will) 
support client.


This code was directly moved from jdk-options.m4. But I can update the 
text to better fit reality. I suggest updating the comment about server to:
# server: normal interpreter, and a C2 or tiered C1/C2 compiler 
depending on platform

Ok?

However, a few lines below we explicitely block enabling of client on 
64-bit platforms, so I believe this statement is correct, at least for 
the code as it looks now. It might be the case that the configure check 
is too strict, and if so we should modify it, but as long as the check 
remains I believe the comment should remain. Ok?


Otherwise I didn't see anything else that I obviously needed to 
comment on. The proof of this is in the building. So as long as this 
gets through JPRT with -testset hotspot we should be okay. :)

Great. Thanks for the review!

/Magnus


Thanks,
David



/Magnus




On 2015-11-16 05:33, David Holmes wrote:

Hi Magnus,

I had a flick through most of the files. Overall seems okay but I
have a few queries below.


Replies inline.



On 13/11/2015 12:43 PM, Magnus Ihse Bursie wrote:

The build-infra project has collected a number of minor fixes and
changes during the new hotspot build development. It's a mix of
code
cleanup and new capabilities.

Not all of these new features are immediately beneficial to the JDK,
but
they will be needed for the upcoming new Hotspot build, and it 
will not

hurt to have them in mainline. (In fact, it will tremendously help
merging between mainline and build-infra.)

The fix addresses these issues:

In general:
* Break out hotspot configuration into hotspot.m4
* Long link lines uses @-files
* Consistently use -Wl instead of -Xlinker
* Improve clang on linux compilation
* Set shared library name explicitely on solaris
* Set correct shared library flag on Windows (-dll)
* Consistency fixes for build toolchain
* Bring compare script up to date
* General code/whitespace cleanup
* Additional functionality in MakeBase

In NativeCompilation.gmk:
* More efficient vardeps for per-file CFLAGS
* Fewer shell executions (means better performance on Windows)
* EXCLUDE_PATTERN and EXTRA_OBJECT_FILES
* Debug symbols on macosx (disabled for existing code to keep current
behavior)

Enabling debug info on macosx on existing jdk should be treated in a
follow-up bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8142907
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.01 





(It turned out that WebRev could not at the same time include files
from
multiple repos and track the history of a "hg cp":ied file. So I
created
an alternative revision here:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.02/ 





It does not include the jdk files, but hotspot.m4 might be easier to
understand)


flags.m4:

  60   AC_SUBST(LEGACY_EXTRA_CFLAGS)
  61   AC_SUBST(LEGACY_EXTRA_CXXFLAGS)
  62   AC_SUBST(LEGACY_EXTRA_LDFLAGS)
  63
  64   AC_SUBST(EXTRA_CFLAGS)
  65   AC_SUBST(EXTRA_CXXFLAGS)
  66   AC_SUBST(EXTRA_LDFLAGS)

IIRC we added the legacy flags purely to pass the cross-compilation
args through to hotspot. Not sure why we need both legacy and
non-legancy variants now ??


This is part of an ongoing effort to split CFLAGS_JDK into more
reasonable parts. The current model is that we throw all possible
flags together in CFLAGS_JDK (and then further on to CFLAGS_JDKLIB et
al). While it was questionable even before, it did work when we only
built the JDK native libraries. Now that we need to build libjvm.so as
well, which has (for historical reason) a set of flags that are both
partially the same and partially different, this is not so good.

We would like to see a change to a situation where the different
"parts" that we build CFLAGS_JDK from would be handled separately, and
then combined as needed for JDK libraries, and for Hotspot. So,
currently we use EXTRA_CFLAGS just to add them to CFLAGS_JDK, and then
drop them. In the new Hotspot build, we use the EXTRA_CFLAGS to add
them to the Hotspot flags. Of course, we could have used
LEGACY_EXTRA_CFLAGS, but we'd like to avoid using "LEGACY" for new 
stuff.


With that being said, I n

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

[Erik Joelsson]

Looks good to me.

/Erik

On 2015-12-11 21:20, 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/ 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/ 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 

Re: RFR: JDK-8142907 Integration of minor fixes from the build-infra project

[Erik Joelsson]

Looks good to me.

/Erik

On 2015-12-11 23:24, Magnus Ihse Bursie wrote:

On 2015-12-11 22:43, Magnus Ihse Bursie wrote:

Hi David,

Resurrecting the second part of the build-infra integrations. :) 
Fortunately, no major code changes in the meantime has affected this 
patch. (I did need to update the new 
hotspot/make/lib/Lib-jdk.hotspot.agent.gmk though.)


It seems I forgot to include the link to the new webrev:

http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.05 



/Magnus




On 2015-11-16 05:33, David Holmes wrote:

Hi Magnus,

I had a flick through most of the files. Overall seems okay but I 
have a few queries below.


Replies inline.



On 13/11/2015 12:43 PM, Magnus Ihse Bursie wrote:

The build-infra project has collected a number of minor fixes and
changes during the new hotspot build development. It's a mix of 
code

cleanup and new capabilities.

Not all of these new features are immediately beneficial to the 
JDK, but
they will be needed for the upcoming new Hotspot build, and it will 
not

hurt to have them in mainline. (In fact, it will tremendously help
merging between mainline and build-infra.)

The fix addresses these issues:

In general:
* Break out hotspot configuration into hotspot.m4
* Long link lines uses @-files
* Consistently use -Wl instead of -Xlinker
* Improve clang on linux compilation
* Set shared library name explicitely on solaris
* Set correct shared library flag on Windows (-dll)
* Consistency fixes for build toolchain
* Bring compare script up to date
* General code/whitespace cleanup
* Additional functionality in MakeBase

In NativeCompilation.gmk:
* More efficient vardeps for per-file CFLAGS
* Fewer shell executions (means better performance on Windows)
* EXCLUDE_PATTERN and EXTRA_OBJECT_FILES
* Debug symbols on macosx (disabled for existing code to keep current
behavior)

Enabling debug info on macosx on existing jdk should be treated in a
follow-up bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8142907
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.01 




(It turned out that WebRev could not at the same time include files 
from
multiple repos and track the history of a "hg cp":ied file. So I 
created

an alternative revision here:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.02/ 




It does not include the jdk files, but hotspot.m4 might be easier to
understand)


flags.m4:

  60   AC_SUBST(LEGACY_EXTRA_CFLAGS)
  61   AC_SUBST(LEGACY_EXTRA_CXXFLAGS)
  62   AC_SUBST(LEGACY_EXTRA_LDFLAGS)
  63
  64   AC_SUBST(EXTRA_CFLAGS)
  65   AC_SUBST(EXTRA_CXXFLAGS)
  66   AC_SUBST(EXTRA_LDFLAGS)

IIRC we added the legacy flags purely to pass the cross-compilation 
args through to hotspot. Not sure why we need both legacy and 
non-legancy variants now ??


This is part of an ongoing effort to split CFLAGS_JDK into more 
reasonable parts. The current model is that we throw all possible 
flags together in CFLAGS_JDK (and then further on to CFLAGS_JDKLIB et 
al). While it was questionable even before, it did work when we only 
built the JDK native libraries. Now that we need to build libjvm.so 
as well, which has (for historical reason) a set of flags that are 
both partially the same and partially different, this is not so good.


We would like to see a change to a situation where the different 
"parts" that we build CFLAGS_JDK from would be handled separately, 
and then combined as needed for JDK libraries, and for Hotspot. So, 
currently we use EXTRA_CFLAGS just to add them to CFLAGS_JDK, and 
then drop them. In the new Hotspot build, we use the EXTRA_CFLAGS to 
add them to the Hotspot flags. Of course, we could have used 
LEGACY_EXTRA_CFLAGS, but we'd like to avoid using "LEGACY" for new 
stuff.


With that being said, I now realize that maybe this change will not 
be needed anyway, at least not right now. Since doing a proper flag 
cleanup is a major task, we decided to copy the CFLAGS_JDK behavior 
for Hotspot flags as well in the current implementation of the new 
hotspot build, and saving the cleanup for another day (so it will not 
block the new build system). So I'll revert this part of the change 
for now.


On Windows -LD is a superset of -dll, so it isn't obvious the change 
is correct.

I hope Erik's reply to that was satisfactory.


---

jdk/make/lib/LibCommon.gmk

+ # Disable it here for the jdk binaries until we decide to enable 
them.


s/binaries/libraries/ ?

Sure, I'll fix.

Actually both this fragment and the one in 
jdk/make/launcher/LauncherCommon.gmk I find confusing - what is the 
relation with hotspot here, and the role of SetupNativeCompilation?
I hope Erik's reply to that is acceptable as well. As for the other 
changes in the jdk repo, most of them is to either standardize on 
-Wl, or to fix some places were we didn't do a proper $$ (instead of 
$) in an macro that was supposed to be eval'd. (This was incorrect 
before as well, but

Re: RFR: JDK-8142907 Integration of minor fixes from the build-infra project

[David Holmes]

On 14/12/2015 6:30 PM, Magnus Ihse Bursie wrote:

On 2015-12-14 09:12, David Holmes wrote:

Hi Magnus,

On 12/12/2015 8:24 AM, Magnus Ihse Bursie wrote:

On 2015-12-11 22:43, Magnus Ihse Bursie wrote:

Hi David,

Resurrecting the second part of the build-infra integrations. :)
Fortunately, no major code changes in the meantime has affected this
patch. (I did need to update the new
hotspot/make/lib/Lib-jdk.hotspot.agent.gmk though.)


It seems I forgot to include the link to the new webrev:

http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.05



hotspot.m4:

 54 #server: normal interpreter and a tiered C1/C2 compiler
  55 #client: normal interpreter and C1 (no C2 compiler) (only
32-bit platforms)

Neither of these statements are necessarily 100% true - not all
platforms have tiered enable; some 64-bit platforms do (or will)
support client.


This code was directly moved from jdk-options.m4. But I can update the
text to better fit reality. I suggest updating the comment about server to:
# server: normal interpreter, and a C2 or tiered C1/C2 compiler
depending on platform
Ok?


Ok.


However, a few lines below we explicitely block enabling of client on
64-bit platforms, so I believe this statement is correct, at least for
the code as it looks now. It might be the case that the configure check
is too strict, and if so we should modify it, but as long as the check
remains I believe the comment should remain. Ok?


OK. As long as whomever removes the check knows to remove the comment. 
Both the arm32 port and the mobile project will be relaxing this afaik.


Thanks,
David


Otherwise I didn't see anything else that I obviously needed to
comment on. The proof of this is in the building. So as long as this
gets through JPRT with -testset hotspot we should be okay. :)

Great. Thanks for the review!

/Magnus


Thanks,
David



/Magnus




On 2015-11-16 05:33, David Holmes wrote:

Hi Magnus,

I had a flick through most of the files. Overall seems okay but I
have a few queries below.


Replies inline.



On 13/11/2015 12:43 PM, Magnus Ihse Bursie wrote:

The build-infra project has collected a number of minor fixes and
changes during the new hotspot build development. It's a mix of
code
cleanup and new capabilities.

Not all of these new features are immediately beneficial to the JDK,
but
they will be needed for the upcoming new Hotspot build, and it
will not
hurt to have them in mainline. (In fact, it will tremendously help
merging between mainline and build-infra.)

The fix addresses these issues:

In general:
* Break out hotspot configuration into hotspot.m4
* Long link lines uses @-files
* Consistently use -Wl instead of -Xlinker
* Improve clang on linux compilation
* Set shared library name explicitely on solaris
* Set correct shared library flag on Windows (-dll)
* Consistency fixes for build toolchain
* Bring compare script up to date
* General code/whitespace cleanup
* Additional functionality in MakeBase

In NativeCompilation.gmk:
* More efficient vardeps for per-file CFLAGS
* Fewer shell executions (means better performance on Windows)
* EXCLUDE_PATTERN and EXTRA_OBJECT_FILES
* Debug symbols on macosx (disabled for existing code to keep current
behavior)

Enabling debug info on macosx on existing jdk should be treated in a
follow-up bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8142907
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.01




(It turned out that WebRev could not at the same time include files
from
multiple repos and track the history of a "hg cp":ied file. So I
created
an alternative revision here:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.02/




It does not include the jdk files, but hotspot.m4 might be easier to
understand)


flags.m4:

  60   AC_SUBST(LEGACY_EXTRA_CFLAGS)
  61   AC_SUBST(LEGACY_EXTRA_CXXFLAGS)
  62   AC_SUBST(LEGACY_EXTRA_LDFLAGS)
  63
  64   AC_SUBST(EXTRA_CFLAGS)
  65   AC_SUBST(EXTRA_CXXFLAGS)
  66   AC_SUBST(EXTRA_LDFLAGS)

IIRC we added the legacy flags purely to pass the cross-compilation
args through to hotspot. Not sure why we need both legacy and
non-legancy variants now ??


This is part of an ongoing effort to split CFLAGS_JDK into more
reasonable parts. The current model is that we throw all possible
flags together in CFLAGS_JDK (and then further on to CFLAGS_JDKLIB et
al). While it was questionable even before, it did work when we only
built the JDK native libraries. Now that we need to build libjvm.so as
well, which has (for historical reason) a set of flags that are both
partially the same and partially different, this is not so good.

We would like to see a change to a situation where the different
"parts" that we build CFLAGS_JDK from would be handled separately, and
then combined as needed for JDK libraries, and for Hotspot. So,
currently we use EXTRA_CFLAGS just to add them to CFLAGS_JDK, and then
drop them. In the new Hotspo

Re: RFR: JDK-8142907 Integration of minor fixes from the build-infra project

[Gary Adams]

On 12/14/15 07:34, David Holmes wrote:

On 14/12/2015 6:30 PM, Magnus Ihse Bursie wrote:

On 2015-12-14 09:12, David Holmes wrote:

Hi Magnus,

On 12/12/2015 8:24 AM, Magnus Ihse Bursie wrote:

On 2015-12-11 22:43, Magnus Ihse Bursie wrote:

Hi David,

Resurrecting the second part of the build-infra integrations. :)
Fortunately, no major code changes in the meantime has affected this
patch. (I did need to update the new
hotspot/make/lib/Lib-jdk.hotspot.agent.gmk though.)


It seems I forgot to include the link to the new webrev:

http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.05 





hotspot.m4:

 54 #server: normal interpreter and a tiered C1/C2 compiler
  55 #client: normal interpreter and C1 (no C2 compiler) (only
32-bit platforms)

Neither of these statements are necessarily 100% true - not all
platforms have tiered enable; some 64-bit platforms do (or will)
support client.


This code was directly moved from jdk-options.m4. But I can update the
text to better fit reality. I suggest updating the comment about 
server to:

# server: normal interpreter, and a C2 or tiered C1/C2 compiler
depending on platform
Ok?


Ok.


However, a few lines below we explicitely block enabling of client on
64-bit platforms, so I believe this statement is correct, at least for
the code as it looks now. It might be the case that the configure check
is too strict, and if so we should modify it, but as long as the check
remains I believe the comment should remain. Ok?


OK. As long as whomever removes the check knows to remove the comment. 
Both the arm32 port and the mobile project will be relaxing this afaik.
For the mobile port we have relaxed the constraint and allow a 64 bit 
client and

minimal vm to be built. I'll make sure the comment is updated in our repos.
I don't think there was a fundamental reason those variations were not 
support,
just a question of which platforms were being built and tested on a 
regular basis.




Thanks,
David


Otherwise I didn't see anything else that I obviously needed to
comment on. The proof of this is in the building. So as long as this
gets through JPRT with -testset hotspot we should be okay. :)

Great. Thanks for the review!

/Magnus


Thanks,
David



/Magnus




On 2015-11-16 05:33, David Holmes wrote:

Hi Magnus,

I had a flick through most of the files. Overall seems okay but I
have a few queries below.


Replies inline.



On 13/11/2015 12:43 PM, Magnus Ihse Bursie wrote:

The build-infra project has collected a number of minor fixes and
changes during the new hotspot build development. It's a mix of
code
cleanup and new capabilities.

Not all of these new features are immediately beneficial to the 
JDK,

but
they will be needed for the upcoming new Hotspot build, and it
will not
hurt to have them in mainline. (In fact, it will tremendously help
merging between mainline and build-infra.)

The fix addresses these issues:

In general:
* Break out hotspot configuration into hotspot.m4
* Long link lines uses @-files
* Consistently use -Wl instead of -Xlinker
* Improve clang on linux compilation
* Set shared library name explicitely on solaris
* Set correct shared library flag on Windows (-dll)
* Consistency fixes for build toolchain
* Bring compare script up to date
* General code/whitespace cleanup
* Additional functionality in MakeBase

In NativeCompilation.gmk:
* More efficient vardeps for per-file CFLAGS
* Fewer shell executions (means better performance on Windows)
* EXCLUDE_PATTERN and EXTRA_OBJECT_FILES
* Debug symbols on macosx (disabled for existing code to keep 
current

behavior)

Enabling debug info on macosx on existing jdk should be treated 
in a

follow-up bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8142907
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.01 






(It turned out that WebRev could not at the same time include files
from
multiple repos and track the history of a "hg cp":ied file. So I
created
an alternative revision here:
http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.02/ 






It does not include the jdk files, but hotspot.m4 might be 
easier to

understand)


flags.m4:

  60   AC_SUBST(LEGACY_EXTRA_CFLAGS)
  61   AC_SUBST(LEGACY_EXTRA_CXXFLAGS)
  62   AC_SUBST(LEGACY_EXTRA_LDFLAGS)
  63
  64   AC_SUBST(EXTRA_CFLAGS)
  65   AC_SUBST(EXTRA_CXXFLAGS)
  66   AC_SUBST(EXTRA_LDFLAGS)

IIRC we added the legacy flags purely to pass the cross-compilation
args through to hotspot. Not sure why we need both legacy and
non-legancy variants now ??


This is part of an ongoing effort to split CFLAGS_JDK into more
reasonable parts. The current model is that we throw all possible
flags together in CFLAGS_JDK (and then further on to CFLAGS_JDKLIB et
al). While it was questionable even before, it did work when we only
built the JDK native libraries. Now that we need to build 
libjvm.so as

well, which has (for historical reason) a set o

Re: Re: RFR: JDK-8144312: Remove limitations on the default number of jobs in the build

[Mikael Gerdin]

Hi David,

On 2015-12-11 14:21, David Holmes wrote:

On 11/12/2015 11:16 PM, Magnus Ihse Bursie wrote:

On 2015-12-03 03:11, Roger Riggs wrote:

Hi,

It would be useful to figure out the number of cpus available when in
a container.
Some comments have added to:
8140793 
getAvailableProcessors may incorrectly report the number of cpus in
Docker container

But so far we haven't dug deep enough.   Suggestions are welcome?

http://serverfault.com/questions/691659/count-number-of-allowed-cpus-in-a-docker-container

suggests running nproc. I'm not sure if that can be counted on to be
present, but we could certainly check for it.


I'd like to know how nproc does it so we can try to apply the same logic
in the VM for Runtime.availableProcessors. Can someone actually confirm
that it returns the number of processors available to the container?


I don't have a container at hand but running nproc under strace suggests 
that it calls sched_getaffinity and counts the number of set bits in the 
cpu affinity mask:


$ strace -e trace=sched_getaffinity nproc
sched_getaffinity(0, 128, {f, 0, 0, 0}) = 32
4
+++ exited with 0 +++

It would be nice if anyone with access to a system where the number of 
cpus is limited in a similar manner to a docker container could run the 
above command and see if it

1) returns the correct number of cpus
2) works as I think, that is, it counts the number of set bits in the 
array which is the third syscall argument.



/Mikael




David


/Magnus



Roger


On 12/2/15 6:59 PM, Martin Buchholz wrote:

Not to say you shouldn't do this, but I worry that increasingly
computing
is being done in "containers" where e.g. the number of cpus is doubling
every year but only a small number are available to actually be used
by a
given process.  if availableProcessors reports 1 million, what
should we
do?  (no need to answer...)

On Tue, Dec 1, 2015 at 1:55 AM, Erik Joelsson

wrote:


Hello,

The current heuristic for figuring out what to default set the -j
flag to
make needs some tweaking.

In JDK 9, it looks at the amount of memory and the number of cpus in
the
system. It divides memory by 1024 to get a safe number of jobs that
will
fit into memory. The lower of that number and the number of cpus is
then
picked. The number is then scaled down to about 90% of the number of
cpus
to leave some resources for other activities. It is also capped at 16.

Since we now have the build using "nice" to make sure the build isn't
bogging down the system, I see no reason to do the 90% scaling
anymore.
Also, the performance issues that forced us to cap at 16 have long
been
fixed, and even if we don't scale well beyond 16, we do still scale.
So I
propose we remove that arbitrary limitation too.

Bug: https://bugs.openjdk.java.net/browse/JDK-8144312
Webrev: http://cr.openjdk.java.net/~erikj/8144312/webrev.01/

/Erik

RFR 8142398: IllegalAccessException Class sun.usagetracker.UsageTrackerClient$4 (module java.base) can not access a member of class java.lang.management.ManagementFactory (module java.management)

[Jaroslav Bachorik]

Please, review the following change

Issue : https://bugs.openjdk.java.net/browse/JDK-8138677
Webrev: http://cr.openjdk.java.net/~jbachorik/8138677/webrev.00

The problem is that the class UsageTrackerClient is accessing 
RuntimeMXBean.getInputArguments() method via reflection to avoid static 
dependency. However, with functional module boundaries this fails.


Since the functionality provided by RuntimeMXBean.getInputArguments() is 
not, in fact, specific to the management subsystem, it makes sense to 
move it out of jmm.h to jvm.h


Full suite of SVC tests was run against this change successfully on all 
platforms.



Thanks,

-JB-

Re: Re: RFR: JDK-8144312: Remove limitations on the default number of jobs in the build

[Martin Buchholz]

My current mental model is
configured cpus >= online cpus >= allowed cpus
In a traditional system they are all the same.

I experimented and saw that cpusets are indeed turned on in some
systems used for testing at Google.
I.e. allowed cpus is a strict subset of online cpus.

It seems likely that the following would be a better implementation of
availableProcessors on Linux:

  cpu_set_t s;
  return (sched_getaffinity(0, sizeof(s), &s) == 0) ? CPU_COUNT(&s) :
fallback_to_old_way();

with all the pain in configury.

On Mon, Dec 14, 2015 at 6:58 AM, Mikael Gerdin  wrote:
> Hi David,
>
> On 2015-12-11 14:21, David Holmes wrote:
>>
>> On 11/12/2015 11:16 PM, Magnus Ihse Bursie wrote:
>>>
>>> On 2015-12-03 03:11, Roger Riggs wrote:

 Hi,

 It would be useful to figure out the number of cpus available when in
 a container.
 Some comments have added to:
 8140793 
 getAvailableProcessors may incorrectly report the number of cpus in
 Docker container

 But so far we haven't dug deep enough.   Suggestions are welcome?
>>>
>>>
>>> http://serverfault.com/questions/691659/count-number-of-allowed-cpus-in-a-docker-container
>>>
>>> suggests running nproc. I'm not sure if that can be counted on to be
>>> present, but we could certainly check for it.
>>
>>
>> I'd like to know how nproc does it so we can try to apply the same logic
>> in the VM for Runtime.availableProcessors. Can someone actually confirm
>> that it returns the number of processors available to the container?
>
>
> I don't have a container at hand but running nproc under strace suggests
> that it calls sched_getaffinity and counts the number of set bits in the cpu
> affinity mask:
>
> $ strace -e trace=sched_getaffinity nproc
> sched_getaffinity(0, 128, {f, 0, 0, 0}) = 32
> 4
> +++ exited with 0 +++
>
> It would be nice if anyone with access to a system where the number of cpus
> is limited in a similar manner to a docker container could run the above
> command and see if it
> 1) returns the correct number of cpus
> 2) works as I think, that is, it counts the number of set bits in the array
> which is the third syscall argument.
>
>
> /Mikael
>
>
>
>>
>> David
>>
>>> /Magnus
>>>

 Roger


 On 12/2/15 6:59 PM, Martin Buchholz wrote:
>
> Not to say you shouldn't do this, but I worry that increasingly
> computing
> is being done in "containers" where e.g. the number of cpus is doubling
> every year but only a small number are available to actually be used
> by a
> given process.  if availableProcessors reports 1 million, what
> should we
> do?  (no need to answer...)
>
> On Tue, Dec 1, 2015 at 1:55 AM, Erik Joelsson
> 
> wrote:
>
>> Hello,
>>
>> The current heuristic for figuring out what to default set the -j
>> flag to
>> make needs some tweaking.
>>
>> In JDK 9, it looks at the amount of memory and the number of cpus in
>> the
>> system. It divides memory by 1024 to get a safe number of jobs that
>> will
>> fit into memory. The lower of that number and the number of cpus is
>> then
>> picked. The number is then scaled down to about 90% of the number of
>> cpus
>> to leave some resources for other activities. It is also capped at 16.
>>
>> Since we now have the build using "nice" to make sure the build isn't
>> bogging down the system, I see no reason to do the 90% scaling
>> anymore.
>> Also, the performance issues that forced us to cap at 16 have long
>> been
>> fixed, and even if we don't scale well beyond 16, we do still scale.
>> So I
>> propose we remove that arbitrary limitation too.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8144312
>> Webrev: http://cr.openjdk.java.net/~erikj/8144312/webrev.01/
>>
>> /Erik
>>

>>>
>>
>

Re: RFR 8142398: IllegalAccessException Class sun.usagetracker.UsageTrackerClient$4 (module java.base) can not access a member of class java.lang.management.ManagementFactory (module java.management)

[David Holmes]

Hi Jaroslav,

This movement of functionality seems okay to me. And good to see the JDK 
6 code removed :)


Thanks,
David

On 15/12/2015 2:55 AM, Jaroslav Bachorik wrote:

Please, review the following change

Issue : https://bugs.openjdk.java.net/browse/JDK-8138677
Webrev: http://cr.openjdk.java.net/~jbachorik/8138677/webrev.00

The problem is that the class UsageTrackerClient is accessing
RuntimeMXBean.getInputArguments() method via reflection to avoid static
dependency. However, with functional module boundaries this fails.

Since the functionality provided by RuntimeMXBean.getInputArguments() is
not, in fact, specific to the management subsystem, it makes sense to
move it out of jmm.h to jvm.h

Full suite of SVC tests was run against this change successfully on all
platforms.


Thanks,

-JB-

Re: RFR 8142398: IllegalAccessException Class sun.usagetracker.UsageTrackerClient$4 (module java.base) can not access a member of class java.lang.management.ManagementFactory (module java.management)

[Mandy Chung]

> On Dec 14, 2015, at 8:55 AM, Jaroslav Bachorik  
> wrote:
> 
> Please, review the following change
> 
> Issue : https://bugs.openjdk.java.net/browse/JDK-8138677
> Webrev: http://cr.openjdk.java.net/~jbachorik/8138677/webrev.00
> 
> The problem is that the class UsageTrackerClient is accessing 
> RuntimeMXBean.getInputArguments() method via reflection to avoid static 
> dependency. However, with functional module boundaries this fails.
> 
> Since the functionality provided by RuntimeMXBean.getInputArguments() is not, 
> in fact, specific to the management subsystem, it makes sense to move it out 
> of jmm.h to jvm.h
> 

This change looks okay.

Mandy

Re: RFR 8142398: IllegalAccessException Class sun.usagetracker.UsageTrackerClient$4 (module java.base) can not access a member of class java.lang.management.ManagementFactory (module java.management)

[Alan Bateman]

On 14/12/2015 16:55, Jaroslav Bachorik wrote:

Please, review the following change

Issue : https://bugs.openjdk.java.net/browse/JDK-8138677
Webrev: http://cr.openjdk.java.net/~jbachorik/8138677/webrev.00

The problem is that the class UsageTrackerClient is accessing 
RuntimeMXBean.getInputArguments() method via reflection to avoid 
static dependency. However, with functional module boundaries this fails.


Since the functionality provided by RuntimeMXBean.getInputArguments() 
is not, in fact, specific to the management subsystem, it makes sense 
to move it out of jmm.h to jvm.h
Make sense to me too and the changes look okay. One small thing is that 
make/share/makefiles/mapfile-vers seems to list the JVM_ functions in 
alphabetic order, I assume you want to preserve that convention.


-Alan.

Re: RFR: JDK-8144312: Remove limitations on the default number of jobs in the build

[David Holmes]

On 15/12/2015 1:27 PM, Martin Buchholz wrote:

My current mental model is
configured cpus >= online cpus >= allowed cpus
In a traditional system they are all the same.

I experimented and saw that cpusets are indeed turned on in some
systems used for testing at Google.
I.e. allowed cpus is a strict subset of online cpus.

It seems likely that the following would be a better implementation of
availableProcessors on Linux:

   cpu_set_t s;
   return (sched_getaffinity(0, sizeof(s), &s) == 0) ? CPU_COUNT(&s) :
fallback_to_old_way();

with all the pain in configury.


Yes this is covered in this older bug that I had forgotten about:

https://bugs.openjdk.java.net/browse/JDK-6515172

David
-


On Mon, Dec 14, 2015 at 6:58 AM, Mikael Gerdin  wrote:

Hi David,

On 2015-12-11 14:21, David Holmes wrote:


On 11/12/2015 11:16 PM, Magnus Ihse Bursie wrote:


On 2015-12-03 03:11, Roger Riggs wrote:


Hi,

It would be useful to figure out the number of cpus available when in
a container.
Some comments have added to:
8140793 
getAvailableProcessors may incorrectly report the number of cpus in
Docker container

But so far we haven't dug deep enough.   Suggestions are welcome?



http://serverfault.com/questions/691659/count-number-of-allowed-cpus-in-a-docker-container

suggests running nproc. I'm not sure if that can be counted on to be
present, but we could certainly check for it.



I'd like to know how nproc does it so we can try to apply the same logic
in the VM for Runtime.availableProcessors. Can someone actually confirm
that it returns the number of processors available to the container?



I don't have a container at hand but running nproc under strace suggests
that it calls sched_getaffinity and counts the number of set bits in the cpu
affinity mask:

$ strace -e trace=sched_getaffinity nproc
sched_getaffinity(0, 128, {f, 0, 0, 0}) = 32
4
+++ exited with 0 +++

It would be nice if anyone with access to a system where the number of cpus
is limited in a similar manner to a docker container could run the above
command and see if it
1) returns the correct number of cpus
2) works as I think, that is, it counts the number of set bits in the array
which is the third syscall argument.


/Mikael





David


/Magnus



Roger


On 12/2/15 6:59 PM, Martin Buchholz wrote:


Not to say you shouldn't do this, but I worry that increasingly
computing
is being done in "containers" where e.g. the number of cpus is doubling
every year but only a small number are available to actually be used
by a
given process.  if availableProcessors reports 1 million, what
should we
do?  (no need to answer...)

On Tue, Dec 1, 2015 at 1:55 AM, Erik Joelsson

wrote:


Hello,

The current heuristic for figuring out what to default set the -j
flag to
make needs some tweaking.

In JDK 9, it looks at the amount of memory and the number of cpus in
the
system. It divides memory by 1024 to get a safe number of jobs that
will
fit into memory. The lower of that number and the number of cpus is
then
picked. The number is then scaled down to about 90% of the number of
cpus
to leave some resources for other activities. It is also capped at 16.

Since we now have the build using "nice" to make sure the build isn't
bogging down the system, I see no reason to do the 90% scaling
anymore.
Also, the performance issues that forced us to cap at 16 have long
been
fixed, and even if we don't scale well beyond 16, we do still scale.
So I
propose we remove that arbitrary limitation too.

Bug: https://bugs.openjdk.java.net/browse/JDK-8144312
Webrev: http://cr.openjdk.java.net/~erikj/8144312/webrev.01/

/Erik