Re: RFR: JDK-8200115: System property java.vm.vendor value includes quotation marks

[Abhijit Saha]

> On Jun 19, 2018, at 2:40 PM, Tim Bell  wrote:
> 
> Erik:
> 
>> Since JDK-8189761, the java.vm.vendor string for an OracleJDK build
>> contains quotation marks. This would also be true for any other build of
>> OpenJDK where the user set the --with-vendor-name configure option.
>> 
>> I found the culprit of this to be the combination of the build setting
>> the preprocessor flag as -DVENDOR='"value"' (double quotes inside single
>> quotes) and vm_version.cpp using the XSTR macro to convert the value of
>> VENDOR into a C string. The -DVENDOR argument is also given to libjava,
>> where System.c does not apply a similar conversion, so it requires the
>> double quotes to be supplied on the command line.
>> 
>> I found the simplest solution for unifying the behavior between libjvm
>> and libjava to be to remove the XSTR macro call in vm_version.cpp.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189761
> 
> Er ... the follow-on bug report is:
>   https://bugs.openjdk.java.net/browse/JDK-8200115
> 
>> Webrev: http://cr.openjdk.java.net/~erikj/8200115/webrev.01/

Looks good to me too.

/Abhijit

>> 
>> Testing: mach5 tier1
> 
> Looks good.
> 
> /Tim
> 

Re: RFR: JDK-8200115: System property java.vm.vendor value includes quotation marks

[Tim Bell]

Erik:


Since JDK-8189761, the java.vm.vendor string for an OracleJDK build
contains quotation marks. This would also be true for any other build of
OpenJDK where the user set the --with-vendor-name configure option.

I found the culprit of this to be the combination of the build setting
the preprocessor flag as -DVENDOR='"value"' (double quotes inside single
quotes) and vm_version.cpp using the XSTR macro to convert the value of
VENDOR into a C string. The -DVENDOR argument is also given to libjava,
where System.c does not apply a similar conversion, so it requires the
double quotes to be supplied on the command line.

I found the simplest solution for unifying the behavior between libjvm
and libjava to be to remove the XSTR macro call in vm_version.cpp.

Bug: https://bugs.openjdk.java.net/browse/JDK-8189761


Er ... the follow-on bug report is:
   https://bugs.openjdk.java.net/browse/JDK-8200115


Webrev: http://cr.openjdk.java.net/~erikj/8200115/webrev.01/

Testing: mach5 tier1


Looks good.

/Tim

RFR: JDK-8200115: System property java.vm.vendor value includes quotation marks

[Erik Joelsson]

Hello,

Since JDK-8189761, the java.vm.vendor string for an OracleJDK build 
contains quotation marks. This would also be true for any other build of 
OpenJDK where the user set the --with-vendor-name configure option.


I found the culprit of this to be the combination of the build setting 
the preprocessor flag as -DVENDOR='"value"' (double quotes inside single 
quotes) and vm_version.cpp using the XSTR macro to convert the value of 
VENDOR into a C string. The -DVENDOR argument is also given to libjava, 
where System.c does not apply a similar conversion, so it requires the 
double quotes to be supplied on the command line.


I found the simplest solution for unifying the behavior between libjvm 
and libjava to be to remove the XSTR macro call in vm_version.cpp.


Bug: https://bugs.openjdk.java.net/browse/JDK-8189761

Webrev: http://cr.openjdk.java.net/~erikj/8200115/webrev.01/

Testing: mach5 tier1

/Erik

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Doug Simon]

> On 19 Jun 2018, at 23:00, Erik Joelsson  wrote:
> 
> Hello,
> 
> Please always include build-dev when reviewing build changes.
> 
> In general this looks pretty good but there are some details that need fixing.
> 
> (Aside from this particular review, I must state my reservation against all 
> the special treatment graal is enjoying from a source and test layout and 
> build perspective. My understanding here is that someone will have to 
> regularly update the wrapper jtreg tests manually using the script. This in 
> addition to having to implement this very convoluted redirection logic 
> because the tests are in the wrong place. Wouldn't it make a lot more sense 
> to put the complicated logic in the import procedure for graal source so that 
> it would fit nicely into the OpenJDK source/build/test model, instead of 
> adding more and more complicated workarounds in the OpenJDK build and test 
> procedures?)

Yes, the updategraalinopenjdk command[1] can be modified in anyway seen fit to 
make Graal a better citizen when copied to JDK. It would also be ok to add the 
jtreg commands in test source headers.

-Doug

[1] 
https://github.com/oracle/graal/blob/27288e546392f44ebf8107795647e0db155faf38/compiler/mx.compiler/mx_compiler.py#L1161

> 
> On 2018-06-18 22:26, Ekaterina Pavlova wrote:
>> Hi All,
>> 
>> please review porting of Graal unit tests under jtreg. The main idea of this 
>> porting is to keep Graal unit tests as is
>> (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run them 
>> similar way they are run in Graal project.
>> To achieve this 
>> test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java 
>> helper class has been written
>> which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run 
>> specified set of Graal unit tests. The groups of
>> Graal unit tests to run are defined in auto-generated 
>> test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests.
>> 
>> New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into 
>> jdk.vm.compiler.tests.jar and then install
>> it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/.
>> 
> GRAALUNIT_LIB is never defined (in the open). Since this is used in open 
> makefiles, we need an open configure option to set it.
> 
> To make things clearer, I would rename LIB_DIR to something like 
> LIB_OUTPUTDIR to signal that this is a location to put files in, rather than 
> read them from.
> 
> FLATTEN has no effect in the SetupCopyFiles call since all the jar files 
> found by wildcard can only be in one directory anyway.
> 
> BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests classes 
> and jars should be built into $(SUPPORT_OUTPUTDIR)/test/... Please create a 
> suitable sub directory there for the output from this makefile.
> 
> The all and test-image targets are never called so no need to declare them.
> 
> There are some style issues too. (Please see 
> http://openjdk.java.net/groups/build/doc/code-conventions.html for details.)
> * 43 logic indent 2 spaces
> * 52 we like to put the ending )) on a new line with ', \' at the end of the 
> line before
> * 58 continuation indent 4 spaces
> * 88, 89 please break long lines
> * 90 continuation indent 4 spaces
> * 99 continuation indent 4 spaces
> * 120 break before ))
>> make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal 
>> and test-image-hotspot-jtreg-graal.
>> 
> The target build-test-hotspot-jtreg-graal only needs to depend on 
> exploded-image-optimize.
> 
> The new graal specific targets should be guarded by INCLUDE_GRAAL in 
> Main.gmk. Otherwise those targets will silently do nothing if invoked without 
> graal. That means adding them to JVM_TEST_IMAGE_TARGETS needs to be 
> conditional too.
>> test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg knows 
>> where to find Graal tests and libs.
>> 
> This needs to be duplicated for make/RunTest.gmk so that these tests work 
> with "make run-test" as well.
> 
> /Erik
>> test/hotspot/jtreg/compiler/graalunit/TestPackages.txt file defines 
>> "testName -> testPrefix [requiresStatement]" mapping
>> which is used by test/hotspot/jtreg/compiler/graalunit/generateTests.sh 
>> script to generate jtreg tests.
>> 
>> test/hotspot/jtreg/compiler/graalunit/com.oracle.mxtool.junit was ported 
>> from mx project without any changes.
>> 
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
>>  webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.00/index.html
>> testing: new tests were executed as part of automatic HS testing for several 
>> months.
>> 
>> 
>> thanks,
>> -katya
>> 
>> p.s.
>>  Igor Ignatyev volunteered to sponsor this change. 
> 

Re: RFR(M): 8205207: Port Graal unit tests under jtreg

[Erik Joelsson]

Hello,

Please always include build-dev when reviewing build changes.

In general this looks pretty good but there are some details that need 
fixing.


(Aside from this particular review, I must state my reservation against 
all the special treatment graal is enjoying from a source and test 
layout and build perspective. My understanding here is that someone will 
have to regularly update the wrapper jtreg tests manually using the 
script. This in addition to having to implement this very convoluted 
redirection logic because the tests are in the wrong place. Wouldn't it 
make a lot more sense to put the complicated logic in the import 
procedure for graal source so that it would fit nicely into the OpenJDK 
source/build/test model, instead of adding more and more complicated 
workarounds in the OpenJDK build and test procedures?)


On 2018-06-18 22:26, Ekaterina Pavlova wrote:

Hi All,

please review porting of Graal unit tests under jtreg. The main idea 
of this porting is to keep Graal unit tests as is
(located in src/jdk.internal.vm.compiler/share/classes/*.test) and run 
them similar way they are run in Graal project.
To achieve this 
test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java 
helper class has been written
which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run 
specified set of Graal unit tests. The groups of
Graal unit tests to run are defined in auto-generated 
test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests.


New make/test/JtregGraalUnit.gmk is used to build Graal unit tests 
into jdk.vm.compiler.tests.jar and then install

it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/.

GRAALUNIT_LIB is never defined (in the open). Since this is used in open 
makefiles, we need an open configure option to set it.


To make things clearer, I would rename LIB_DIR to something like 
LIB_OUTPUTDIR to signal that this is a location to put files in, rather 
than read them from.


FLATTEN has no effect in the SetupCopyFiles call since all the jar files 
found by wildcard can only be in one directory anyway.


BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests 
classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/... 
Please create a suitable sub directory there for the output from this 
makefile.


The all and test-image targets are never called so no need to declare them.

There are some style issues too. (Please see 
http://openjdk.java.net/groups/build/doc/code-conventions.html for details.)

* 43 logic indent 2 spaces
* 52 we like to put the ending )) on a new line with ', \' at the end of 
the line before

* 58 continuation indent 4 spaces
* 88, 89 please break long lines
* 90 continuation indent 4 spaces
* 99 continuation indent 4 spaces
* 120 break before ))
make/Main.gmk adds proper dependencies for 
build-test-hotspot-jtreg-graal and test-image-hotspot-jtreg-graal.


The target build-test-hotspot-jtreg-graal only needs to depend on 
exploded-image-optimize.


The new graal specific targets should be guarded by INCLUDE_GRAAL in 
Main.gmk. Otherwise those targets will silently do nothing if invoked 
without graal. That means adding them to JVM_TEST_IMAGE_TARGETS needs to 
be conditional too.
test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg 
knows where to find Graal tests and libs.


This needs to be duplicated for make/RunTest.gmk so that these tests 
work with "make run-test" as well.


/Erik
test/hotspot/jtreg/compiler/graalunit/TestPackages.txt file defines 
"testName -> testPrefix [requiresStatement]" mapping
which is used by 
test/hotspot/jtreg/compiler/graalunit/generateTests.sh script to 
generate jtreg tests.


test/hotspot/jtreg/compiler/graalunit/com.oracle.mxtool.junit was 
ported from mx project without any changes.



    JBS: https://bugs.openjdk.java.net/browse/JDK-8205207
 webrev: 
http://cr.openjdk.java.net/~epavlova//8205207/webrev.00/index.html
testing: new tests were executed as part of automatic HS testing for 
several months.



thanks,
-katya

p.s.
 Igor Ignatyev volunteered to sponsor this change. 

Re: RFR: 8205197: Never default to using libc++ on Linux

[Gerard Ziemski]

Thank you for fixing this.


> On Jun 18, 2018, at 7:36 PM, Martin Buchholz  wrote:
> 
> 8205197: Never default to using libc++ on Linux
> http://cr.openjdk.java.net/~martin/webrevs/jdk/stlib-default/
> https://bugs.openjdk.java.net/browse/JDK-8205197
> 

Re: [8u] RFR: 8148351: Only display resolved symlink for compiler, do not change path

[Kevin Walls]

Thanks Severin,

Yes, keeping the special case about ccache should keep our own builds 
happy.  I ran this in JPRT earlier today and the builds went OK, so if 
it fixes your issue also then it looks good to me.


Thanks
Kevin


On 19/06/2018 17:57, Severin Gehwolf wrote:

Hi,

After discussion in [1], please review this revised JDK 8u backport of
JDK-8148351. When the compiler is a symlink to ccache it'll continue to
use it's work-around to keep the Oracle JDK 8 build working. If the
compiler isn't a symlink it'll print this as a configure message and
won't do the ccache work-around (which it did prior this patch).

Example:

Before:

checking for gcc... /usr/bin/gcc
checking resolved symbolic links for CC... /usr/bin/gcc
checking if CC is disguised ccache... no, keeping CC
configure: Using gcc C compiler version 8.1.1 [gcc (GCC) 8.1.1 20180502 (Red 
Hat 8.1.1-1)]

After:

checking for gcc... /usr/bin/gcc
checking resolved symbolic links for CC... no symlink
configure: Using gcc C compiler version 8.1.1 [gcc (GCC) 8.1.1 20180502 (Red 
Hat 8.1.1-1)]

webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/webrev.02/

Thoughts?

Thanks,
Severin

[1] http://mail.openjdk.java.net/pipermail/build-dev/2018-June/022444.html

Re: RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

[Jiangli Zhou]

> On Jun 19, 2018, at 5:21 AM, Volker Simonis  wrote:
> 
> On Tue, Jun 19, 2018 at 9:25 AM, David Holmes  wrote:
>> Hi Volker,
>> 
>> On 19/06/2018 4:50 PM, Volker Simonis wrote:
>>> 
>>> On Tue, Jun 19, 2018 at 6:54 AM, David Holmes 
>>> wrote:
 
 Hi Volker,
 
 v3 looks much cleaner - thanks.
 
 But AFAICS the change to jvmtiEnv.cpp is also not needed.
 ClassLoaderExt::append_boot_classpath exists regardless of INCLUDE_CDS
 but
 operates differently (just calling
 ClassLoader::add_to_boot_append_entries).
 
>>> 
>>> That's not entirely true because the whole compilation unit (i.e.
>>> classLoaderExt.cpp) which contains
>>> 'ClassLoaderExt::append_boot_classpath()' is excluded from the
>>> compilation if CDS is disabled (see make/hotspot/lib/JvmFeatures.gmk).
>> 
>> 
>> Hmmm. There's a CDS bug there. Either classLoaderExt.cpp should not be
>> excluded from a non-CDS build, or it should not contain any INCLUDE_CDS
>> guards! I suspect it should not be excluded.
>> 
>>> So I can either move the whole implementation of
>>> 'ClassLoaderExt::append_boot_classpath()' into classLoaderExt.hpp in
>>> which case things would work as you explained and my changes to
>>> jvmtiEnv.cpp could be removed or leave the whole code and change as
>>> is. Please let me know what you think?
>> 
>> 
>> In the interest of moving forward you can push what you have and I will file
>> a bug against CDS to sort out classLoaderExt.cpp.
>> 
> 
> Thanks! As the current version also passed the submit-repo tests I've pushed 
> it.
> 
> Regarding classLoaderExt.cpp, I think it is OK to exclude it for
> non-CDS builds. If my IDE doesn't cheat on me (see [1]),
> ClassLoaderExt is mostly used from other CDS-only files
> (classListParser.cpp, systemDictionaryShared.cpp, filemap.cpp,
> metaspaceShared.cpp). The only references from non-CDS files are from
> classLoader.cpp an jvmtiEnv.cpp. The ones from classLoader.cpp are all
> guarded with 'INCLUDE_CDS' or they only use functions defined in
> classLoaderExt.hpp. The single remaining reference from jvmtiEnv.cpp
> has been guarded with 'INCLUDE_CDS' by my change.
> 
> I think it is a matter of taste if we leave this as is or move the
> offending function from classLoaderExt.cpp to classLoaderExt.hpp and
> remove the new guard from jvmtiEnv.cpp.

For the classLoaderExt.cpp bug, we could use a private function, 
ClassLoaderExt::disable_shared_platform_and_app_classes, which does the logic 
in the original ClassLoaderExt::append_boot_classpath #INCLUDE_CDS. 
ClassLoaderExt::append_boot_classpath could be defined in classLoaderExt.hpp as:

void disable_shared_platform_and_app_classes() NOT_CDS_RETURN;

void append_boot_classpath(ClassPathEntry* new_entry) {
  disable_shared_platform_and_app_classes();
  ClassLoader::add_to_boot_append_entries(new_entry);
}

The new guard can be removed from jvmtiEnv.cpp with those. Reducing CDS 
specifics from general code probably is cleaner.

Thanks,
Jiangli


> 
> Regards,
> Volker
> 
> [1] http://cr.openjdk.java.net/~simonis/webrevs/2018/ClassLoaderExt.html
> 
>> Thanks,
>> David
>> 
>> 
>>> Regards,
>>> Volker
>>> 
 Thanks,
 David
 
 
 On 19/06/2018 2:04 AM, Volker Simonis wrote:
> 
> 
> On Mon, Jun 18, 2018 at 8:17 AM, David Holmes 
> wrote:
>> 
>> 
>> Hi Volker,
>> 
>> src/hotspot/share/runtime/globals.hpp
>> 
>> This change should not be needed! We do minimal VM builds without CDS
>> and
>> we
>> don't have to touch the UseSharedSpaces defaults (unless recent change
>> have
>> broken this - in which case that needs to be addressed in its own
>> right!)
>> 
> 
> Yes, you're right, CDS_ONLY/NOT_CDS isn't really required here,
> because UseSharedSpaces is reseted later on at the end of
> Arguments::parse(). I just thought it would be cleaner to disable it
> statically, if the VM doesn't support it. But anyway I don't really
> mind and I've reverted that change in globals.hpp.
> 
>> src/hotspot/share/classfile/javaClasses.cpp
>> 
>> AFAICS you should be using INCLUDE_CDS in the ifdefs not
>> INCLUDE_CDS_JAVA_HEAP. But again I'm unclear (as was Thomas) why this
>> should
>> be needed as we have not needed it before. As Thomas notes we have:
>> 
>> ./hotspot/share/memory/metaspaceShared.hpp:  static bool
>> is_archive_object(oop p) NOT_CDS_JAVA_HEAP_RETURN_(false);
>> ./hotspot/share/classfile/stringTable.hpp:  static oop
>> create_archived_string(oop s, Thread* THREAD)
>> NOT_CDS_JAVA_HEAP_RETURN_(NULL);
>> 
>> so these methods should be defined when CDS is not available.
>> 
> 
> Thomas and you are right. Must have been a mis-configuration on AIX
> where I saw undefined symbols at link time. I've removed the ifdefs
> from javaClasses.cpp now.
> 
> Finally, I've also wrapped all the FileMapInfo fields in vmStructs.cpp
>

Re: [8u] RFR: 8148351: Only display resolved symlink for compiler, do not change path

[Erik Joelsson]

This looks good to me.

/Erik


On 2018-06-19 09:57, Severin Gehwolf wrote:

Hi,

After discussion in [1], please review this revised JDK 8u backport of
JDK-8148351. When the compiler is a symlink to ccache it'll continue to
use it's work-around to keep the Oracle JDK 8 build working. If the
compiler isn't a symlink it'll print this as a configure message and
won't do the ccache work-around (which it did prior this patch).

Example:

Before:

checking for gcc... /usr/bin/gcc
checking resolved symbolic links for CC... /usr/bin/gcc
checking if CC is disguised ccache... no, keeping CC
configure: Using gcc C compiler version 8.1.1 [gcc (GCC) 8.1.1 20180502 (Red 
Hat 8.1.1-1)]

After:

checking for gcc... /usr/bin/gcc
checking resolved symbolic links for CC... no symlink
configure: Using gcc C compiler version 8.1.1 [gcc (GCC) 8.1.1 20180502 (Red 
Hat 8.1.1-1)]

webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/webrev.02/

Thoughts?

Thanks,
Severin

[1] http://mail.openjdk.java.net/pipermail/build-dev/2018-June/022444.html

[8u] RFR: 8148351: Only display resolved symlink for compiler, do not change path

[Severin Gehwolf]

Hi,

After discussion in [1], please review this revised JDK 8u backport of
JDK-8148351. When the compiler is a symlink to ccache it'll continue to
use it's work-around to keep the Oracle JDK 8 build working. If the
compiler isn't a symlink it'll print this as a configure message and
won't do the ccache work-around (which it did prior this patch). 

Example:

Before:

checking for gcc... /usr/bin/gcc
checking resolved symbolic links for CC... /usr/bin/gcc
checking if CC is disguised ccache... no, keeping CC
configure: Using gcc C compiler version 8.1.1 [gcc (GCC) 8.1.1 20180502 (Red 
Hat 8.1.1-1)]

After:

checking for gcc... /usr/bin/gcc
checking resolved symbolic links for CC... no symlink
configure: Using gcc C compiler version 8.1.1 [gcc (GCC) 8.1.1 20180502 (Red 
Hat 8.1.1-1)]

webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/webrev.02/

Thoughts?

Thanks,
Severin

[1] http://mail.openjdk.java.net/pipermail/build-dev/2018-June/022444.html

Re: RFA: 8031668: TOOLCHAIN_FIND_COMPILER unexpectedly resolves symbolic links AND 8148351: Only display resolved symlink for compiler, do not change path

[Severin Gehwolf]

Hi Erik,

On Tue, 2018-06-19 at 07:06 -0700, Erik Joelsson wrote:
> Hello,
> 
> On 2018-06-19 06:30, Severin Gehwolf wrote:
> > Hi Kevin,
> > 
> > Thanks for your help! Adding in build-dev for additional input.
> > 
> > On Tue, 2018-06-19 at 13:51 +0100, Kevin Walls wrote:
> > > Hi Severin -
> > > 
> > > Ah, actually there's a conflict with the way we build, where we use
> > > ccache in the path, and it no longer likes it with this change:
> > > 
> > > Our build system jprt fails, with the configure output:
> > > ...
> > > checking flags for boot jdk java command for small workloads...
> > > -XX:+UseSerialGC -Xms32M -Xmx512M
> > > configure: Using default toolchain gcc (GNU Compiler Collection)
> > > checking for gcc... /opt/jprt/products/P1/ccache2.4/bin/gcc
> > > checking resolved symbolic links for CC...
> > > /opt/jprt/products/P1/ccache2.4/bin/ccache
> > > configure: error: /opt/jprt/products/P1/ccache2.4/bin/gcc is a symbolic
> > > link to ccache. This is not supported.
> > > configure: Please use --enable-ccache instead of providing a wrapped
> > > compiler.
> > > configure exiting with result code 1
> > 
> > Erik, Magnus, how do you do this for your JDK 9+ builds? Is there a way
> > to use "--enable-ccache" for JDK 8 builds?
> 
> We don't. The problem here is that the build machines we have for 8 have 
> ccache installed as a gcc wrapper. This was done back in 7 with the 
> intention of transparently improving build performance (but in reality 
> it didn't). Because of this situation, we implemented a workaround for 8 
> where we explicitly don't accept ccache as gcc wrapper and look for gcc 
> elsewhere instead. Our build machines for 9 do not have cache installed 
> at all (it's still in the devkit, but not as a wrapper, and since we saw 
> no benefit for our use case we don't use ccache anyway).
> 
> The change being backported here removes the workaround that allows the 
> build to function with the ccache wrapper on the system. I don't think 
> that's a good idea for 8. We need to retain that workaround so the patch 
> needs to be modified before going into 8u.

Sigh. Back to the drawing board ;-)

> Reproducing this issue locally should be pretty simple. Create a dummy 
> script named ccache somewhere. Put a symlink named gcc, pointing to that 
> ccache script first in your path. Then run configure.

Thanks, yes. I've got that reproduced.

Cheers,
Severin

> /Erik
> > > It's the second change, 8148351, which triggers this failure, as it
> > > removes lines at 549 onwards which previously tried to handle the ccache
> > > in the path case.
> > 
> > Right. It's 8148351 which we'd be interested in.
> > 
> > > Yes, in 9 onward we abort the build if ccache is found at the end of a
> > > link, we haven't done that in jdk 8 before... Would retaining the ccache
> > > handling code in toolchain.m4 still give you the behaviour you want from
> > > doing the backport?
> > 
> > Thanks,
> > Severin
> > 
> > > Thanks
> > > Kevin
> > > 
> > > 
> > > 
> > > 
> > > 
> > > On 19/06/2018 07:23, Severin Gehwolf wrote:
> > > > Hi Kevin,
> > > > 
> > > > On Mon, 2018-06-18 at 21:11 +0100, Kevin Walls wrote:
> > > > > Hi -- I've been doing various 8u build changes recently -- would you
> > > > > like me to push this, with the regenerated autogen-generated file?
> > > > 
> > > > It would be very much appreciated!
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > > On 18/06/2018 17:45, Rob McKenna wrote:
> > > > > > Approved. Please work with the appropriate team to find a sponsor.
> > > > > > 
> > > > > >-Rob
> > > > > > 
> > > > > > On 18/06/18 16:36, Severin Gehwolf wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > Please approve these two backports to JDK 8u-dev which are 
> > > > > > > already in
> > > > > > > JDK 9 and better.
> > > > > > > 
> > > > > > > 8031668: TOOLCHAIN_FIND_COMPILER unexpectedly resolves symbolic 
> > > > > > > links
> > > > > > > webrev: 
> > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8031668/webrev.01/
> > > > > > > hg export: 
> > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8031668/JDK-8031668.jdk8.export.patch
> > > > > > > 
> > > > > > > 8148351: Only display resolved symlink for compiler, do not 
> > > > > > > change path
> > > > > > > webrev: 
> > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/webrev.01/
> > > > > > > hg export: 
> > > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/JDK-8148351.jdk8.export.patch
> > > > > > > 
> > > > > > > Bug 8031668 is a dependency for 8148351 which actually fixes the
> > > > > > > wrapped compiler issue on the JDK 8 tree. The JDK 8 patch of 
> > > > > > > 8031668 is
> > > > > > > the same as for JDK 9, modulo some context changes. After the JDK 
> > > > > > > 8
> > > > > > > patch of 8031668, the JDK 9 patch of 8148351 imports as is.
> > > > > > > 
> > > > > > > The issue at hand is that one cannot build the JDK 8 tree with 
> > > > > > > certain
> > > > > > > 

Re: RFA: 8031668: TOOLCHAIN_FIND_COMPILER unexpectedly resolves symbolic links AND 8148351: Only display resolved symlink for compiler, do not change path

[Erik Joelsson]

Hello,

On 2018-06-19 06:30, Severin Gehwolf wrote:

Hi Kevin,

Thanks for your help! Adding in build-dev for additional input.

On Tue, 2018-06-19 at 13:51 +0100, Kevin Walls wrote:

Hi Severin -

Ah, actually there's a conflict with the way we build, where we use
ccache in the path, and it no longer likes it with this change:

Our build system jprt fails, with the configure output:
...
checking flags for boot jdk java command for small workloads...
-XX:+UseSerialGC -Xms32M -Xmx512M
configure: Using default toolchain gcc (GNU Compiler Collection)
checking for gcc... /opt/jprt/products/P1/ccache2.4/bin/gcc
checking resolved symbolic links for CC...
/opt/jprt/products/P1/ccache2.4/bin/ccache
configure: error: /opt/jprt/products/P1/ccache2.4/bin/gcc is a symbolic
link to ccache. This is not supported.
configure: Please use --enable-ccache instead of providing a wrapped
compiler.
configure exiting with result code 1

Erik, Magnus, how do you do this for your JDK 9+ builds? Is there a way
to use "--enable-ccache" for JDK 8 builds?
We don't. The problem here is that the build machines we have for 8 have 
ccache installed as a gcc wrapper. This was done back in 7 with the 
intention of transparently improving build performance (but in reality 
it didn't). Because of this situation, we implemented a workaround for 8 
where we explicitly don't accept ccache as gcc wrapper and look for gcc 
elsewhere instead. Our build machines for 9 do not have cache installed 
at all (it's still in the devkit, but not as a wrapper, and since we saw 
no benefit for our use case we don't use ccache anyway).


The change being backported here removes the workaround that allows the 
build to function with the ccache wrapper on the system. I don't think 
that's a good idea for 8. We need to retain that workaround so the patch 
needs to be modified before going into 8u.


Reproducing this issue locally should be pretty simple. Create a dummy 
script named ccache somewhere. Put a symlink named gcc, pointing to that 
ccache script first in your path. Then run configure.


/Erik

It's the second change, 8148351, which triggers this failure, as it
removes lines at 549 onwards which previously tried to handle the ccache
in the path case.

Right. It's 8148351 which we'd be interested in.


Yes, in 9 onward we abort the build if ccache is found at the end of a
link, we haven't done that in jdk 8 before... Would retaining the ccache
handling code in toolchain.m4 still give you the behaviour you want from
doing the backport?

Thanks,
Severin


Thanks
Kevin





On 19/06/2018 07:23, Severin Gehwolf wrote:

Hi Kevin,

On Mon, 2018-06-18 at 21:11 +0100, Kevin Walls wrote:

Hi -- I've been doing various 8u build changes recently -- would you
like me to push this, with the regenerated autogen-generated file?

It would be very much appreciated!

Thanks,
Severin


On 18/06/2018 17:45, Rob McKenna wrote:

Approved. Please work with the appropriate team to find a sponsor.

   -Rob

On 18/06/18 16:36, Severin Gehwolf wrote:

Hi,

Please approve these two backports to JDK 8u-dev which are already in
JDK 9 and better.

8031668: TOOLCHAIN_FIND_COMPILER unexpectedly resolves symbolic links
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8031668/webrev.01/
hg export: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8031668/JDK-8031668.jdk8.export.patch

8148351: Only display resolved symlink for compiler, do not change path
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/webrev.01/
hg export: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/JDK-8148351.jdk8.export.patch

Bug 8031668 is a dependency for 8148351 which actually fixes the
wrapped compiler issue on the JDK 8 tree. The JDK 8 patch of 8031668 is
the same as for JDK 9, modulo some context changes. After the JDK 8
patch of 8031668, the JDK 9 patch of 8148351 imports as is.

The issue at hand is that one cannot build the JDK 8 tree with certain
compiler wrappers such as cscppc. It currently fails with:

configure: Using default toolchain gcc (GNU Compiler Collection)
checking for gcc... /usr/lib64/cscppc/gcc
checking resolved symbolic links for CC... /usr/bin/cscppc
checking if CC is disguised ccache... no, keeping CC
configure: The C compiler (located as /usr/bin/cscppc) does not seem to be the 
required gcc compiler.
configure: The result from running with --version was: ""
configure: error: A gcc compiler is required. Try setting --with-tools-dir.
configure exiting with result code 1

After both backports configure with a wrapped GCC succeeds.

Please note that I'd need an Oracle sponsor for getting this pushed to
jdk8u-dev since both changes require re-generation of generated-
configure.sh

Thanks,
Severin

Re: RFR: 8205197: Never default to using libc++ on Linux

[Erik Joelsson]

Looks ok to me.

/Erik


On 2018-06-18 17:36, Martin Buchholz wrote:

8205197: Never default to using libc++ on Linux
http://cr.openjdk.java.net/~martin/webrevs/jdk/stlib-default/ 


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

Re: RFA: 8031668: TOOLCHAIN_FIND_COMPILER unexpectedly resolves symbolic links AND 8148351: Only display resolved symlink for compiler, do not change path

[Severin Gehwolf]

Hi Kevin,

Thanks for your help! Adding in build-dev for additional input.

On Tue, 2018-06-19 at 13:51 +0100, Kevin Walls wrote:
> Hi Severin -
> 
> Ah, actually there's a conflict with the way we build, where we use 
> ccache in the path, and it no longer likes it with this change:
> 
> Our build system jprt fails, with the configure output:
> ...
> checking flags for boot jdk java command for small workloads... 
> -XX:+UseSerialGC -Xms32M -Xmx512M
> configure: Using default toolchain gcc (GNU Compiler Collection)
> checking for gcc... /opt/jprt/products/P1/ccache2.4/bin/gcc
> checking resolved symbolic links for CC... 
> /opt/jprt/products/P1/ccache2.4/bin/ccache
> configure: error: /opt/jprt/products/P1/ccache2.4/bin/gcc is a symbolic 
> link to ccache. This is not supported.
> configure: Please use --enable-ccache instead of providing a wrapped 
> compiler.
> configure exiting with result code 1

Erik, Magnus, how do you do this for your JDK 9+ builds? Is there a way
to use "--enable-ccache" for JDK 8 builds?

> It's the second change, 8148351, which triggers this failure, as it 
> removes lines at 549 onwards which previously tried to handle the ccache 
> in the path case.

Right. It's 8148351 which we'd be interested in.

> Yes, in 9 onward we abort the build if ccache is found at the end of a 
> link, we haven't done that in jdk 8 before... Would retaining the ccache 
> handling code in toolchain.m4 still give you the behaviour you want from 
> doing the backport?

Thanks,
Severin

> Thanks
> Kevin
> 
> 
> 
> 
> 
> On 19/06/2018 07:23, Severin Gehwolf wrote:
> > Hi Kevin,
> > 
> > On Mon, 2018-06-18 at 21:11 +0100, Kevin Walls wrote:
> > > Hi -- I've been doing various 8u build changes recently -- would you
> > > like me to push this, with the regenerated autogen-generated file?
> > 
> > It would be very much appreciated!
> > 
> > Thanks,
> > Severin
> > 
> > > 
> > > On 18/06/2018 17:45, Rob McKenna wrote:
> > > > Approved. Please work with the appropriate team to find a sponsor.
> > > > 
> > > >   -Rob
> > > > 
> > > > On 18/06/18 16:36, Severin Gehwolf wrote:
> > > > > Hi,
> > > > > 
> > > > > Please approve these two backports to JDK 8u-dev which are already in
> > > > > JDK 9 and better.
> > > > > 
> > > > > 8031668: TOOLCHAIN_FIND_COMPILER unexpectedly resolves symbolic links
> > > > > webrev: 
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8031668/webrev.01/
> > > > > hg export: 
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8031668/JDK-8031668.jdk8.export.patch
> > > > > 
> > > > > 8148351: Only display resolved symlink for compiler, do not change 
> > > > > path
> > > > > webrev: 
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/webrev.01/
> > > > > hg export: 
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8148351/JDK-8148351.jdk8.export.patch
> > > > > 
> > > > > Bug 8031668 is a dependency for 8148351 which actually fixes the
> > > > > wrapped compiler issue on the JDK 8 tree. The JDK 8 patch of 8031668 
> > > > > is
> > > > > the same as for JDK 9, modulo some context changes. After the JDK 8
> > > > > patch of 8031668, the JDK 9 patch of 8148351 imports as is.
> > > > > 
> > > > > The issue at hand is that one cannot build the JDK 8 tree with certain
> > > > > compiler wrappers such as cscppc. It currently fails with:
> > > > > 
> > > > > configure: Using default toolchain gcc (GNU Compiler Collection)
> > > > > checking for gcc... /usr/lib64/cscppc/gcc
> > > > > checking resolved symbolic links for CC... /usr/bin/cscppc
> > > > > checking if CC is disguised ccache... no, keeping CC
> > > > > configure: The C compiler (located as /usr/bin/cscppc) does not seem 
> > > > > to be the required gcc compiler.
> > > > > configure: The result from running with --version was: ""
> > > > > configure: error: A gcc compiler is required. Try setting 
> > > > > --with-tools-dir.
> > > > > configure exiting with result code 1
> > > > > 
> > > > > After both backports configure with a wrapped GCC succeeds.
> > > > > 
> > > > > Please note that I'd need an Oracle sponsor for getting this pushed to
> > > > > jdk8u-dev since both changes require re-generation of generated-
> > > > > configure.sh
> > > > > 
> > > > > Thanks,
> > > > > Severin
> 
> 

Re: RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

[Volker Simonis]

On Tue, Jun 19, 2018 at 9:25 AM, David Holmes  wrote:
> Hi Volker,
>
> On 19/06/2018 4:50 PM, Volker Simonis wrote:
>>
>> On Tue, Jun 19, 2018 at 6:54 AM, David Holmes 
>> wrote:
>>>
>>> Hi Volker,
>>>
>>> v3 looks much cleaner - thanks.
>>>
>>> But AFAICS the change to jvmtiEnv.cpp is also not needed.
>>> ClassLoaderExt::append_boot_classpath exists regardless of INCLUDE_CDS
>>> but
>>> operates differently (just calling
>>> ClassLoader::add_to_boot_append_entries).
>>>
>>
>> That's not entirely true because the whole compilation unit (i.e.
>> classLoaderExt.cpp) which contains
>> 'ClassLoaderExt::append_boot_classpath()' is excluded from the
>> compilation if CDS is disabled (see make/hotspot/lib/JvmFeatures.gmk).
>
>
> Hmmm. There's a CDS bug there. Either classLoaderExt.cpp should not be
> excluded from a non-CDS build, or it should not contain any INCLUDE_CDS
> guards! I suspect it should not be excluded.
>
>> So I can either move the whole implementation of
>> 'ClassLoaderExt::append_boot_classpath()' into classLoaderExt.hpp in
>> which case things would work as you explained and my changes to
>> jvmtiEnv.cpp could be removed or leave the whole code and change as
>> is. Please let me know what you think?
>
>
> In the interest of moving forward you can push what you have and I will file
> a bug against CDS to sort out classLoaderExt.cpp.
>

Thanks! As the current version also passed the submit-repo tests I've pushed it.

Regarding classLoaderExt.cpp, I think it is OK to exclude it for
non-CDS builds. If my IDE doesn't cheat on me (see [1]),
ClassLoaderExt is mostly used from other CDS-only files
(classListParser.cpp, systemDictionaryShared.cpp, filemap.cpp,
metaspaceShared.cpp). The only references from non-CDS files are from
classLoader.cpp an jvmtiEnv.cpp. The ones from classLoader.cpp are all
guarded with 'INCLUDE_CDS' or they only use functions defined in
classLoaderExt.hpp. The single remaining reference from jvmtiEnv.cpp
has been guarded with 'INCLUDE_CDS' by my change.

I think it is a matter of taste if we leave this as is or move the
offending function from classLoaderExt.cpp to classLoaderExt.hpp and
remove the new guard from jvmtiEnv.cpp.

Regards,
Volker

[1] http://cr.openjdk.java.net/~simonis/webrevs/2018/ClassLoaderExt.html

> Thanks,
> David
>
>
>> Regards,
>> Volker
>>
>>> Thanks,
>>> David
>>>
>>>
>>> On 19/06/2018 2:04 AM, Volker Simonis wrote:


 On Mon, Jun 18, 2018 at 8:17 AM, David Holmes 
 wrote:
>
>
> Hi Volker,
>
> src/hotspot/share/runtime/globals.hpp
>
> This change should not be needed! We do minimal VM builds without CDS
> and
> we
> don't have to touch the UseSharedSpaces defaults (unless recent change
> have
> broken this - in which case that needs to be addressed in its own
> right!)
>

 Yes, you're right, CDS_ONLY/NOT_CDS isn't really required here,
 because UseSharedSpaces is reseted later on at the end of
 Arguments::parse(). I just thought it would be cleaner to disable it
 statically, if the VM doesn't support it. But anyway I don't really
 mind and I've reverted that change in globals.hpp.

> src/hotspot/share/classfile/javaClasses.cpp
>
> AFAICS you should be using INCLUDE_CDS in the ifdefs not
> INCLUDE_CDS_JAVA_HEAP. But again I'm unclear (as was Thomas) why this
> should
> be needed as we have not needed it before. As Thomas notes we have:
>
> ./hotspot/share/memory/metaspaceShared.hpp:  static bool
> is_archive_object(oop p) NOT_CDS_JAVA_HEAP_RETURN_(false);
> ./hotspot/share/classfile/stringTable.hpp:  static oop
> create_archived_string(oop s, Thread* THREAD)
> NOT_CDS_JAVA_HEAP_RETURN_(NULL);
>
> so these methods should be defined when CDS is not available.
>

 Thomas and you are right. Must have been a mis-configuration on AIX
 where I saw undefined symbols at link time. I've removed the ifdefs
 from javaClasses.cpp now.

 Finally, I've also wrapped all the FileMapInfo fields in vmStructs.cpp
 into CDS_ONLY macros as suggested by Jiangli because the really only
 make sense for a CDS-enabled VM.

 Here's the new webrev:

 http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965.v3/

 Please let me know if you think there's still something missing.

 Regards,
 Volker


> ??
>
> Thanks,
> David
> -
>
>
>
>
>
> On 15/06/2018 12:26 AM, Volker Simonis wrote:
>>
>>
>>
>> Hi,
>>
>> can I please have a review for the following fix:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/
>> https://bugs.openjdk.java.net/browse/JDK-8204965
>>
>> CDS does currently not work on AIX because of the way how we
>> reserve/commit memory on AIX. The problem is that we're using a
>> combination of shmat/mmap depending on the page size a

Re: JDK Build on Fedora 28

[Patrick Reinhart]

I went thru the remaining warnings, that show up when building under gcc 
8.1.1 on Fedora 28:


/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c: 
In function ‘jniFatalError’:
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:650:24: 
warning: passing argument 1 to restrict-qualified parameter aliases with 
argument 4 [-Wrestrict]

 (void)snprintf(buf, sizeof(buf), "JDWP %s", buf);
^~~  ~~~
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c: 
In function ‘jniFatalError.constprop’:
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:650:48: 
warning: ‘%s’ directive output may be truncated writing up to 511 bytes 
into a region of size 507 [-Wformat-truncation=]

 (void)snprintf(buf, sizeof(buf), "JDWP %s", buf);
^~   ~~~
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:650:15: 
note: ‘snprintf’ output between 6 and 517 bytes into a destination of 
size 512

 (void)snprintf(buf, sizeof(buf), "JDWP %s", buf);
   ^~
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c: 
In function ‘log_message_end’:
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c:75:24: 
warning: ‘%.3d’ directive output may be truncated writing between 3 and 
11 bytes into a region of size between 0 and 80 [-Wformat-truncation=]

"%s.%.3d %s", timestamp_prefix,
^~~~
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c:75:20: 
note: using the range [-2147483648, 2147483647] for directive argument

"%s.%.3d %s", timestamp_prefix,
^~~~
/home/rep/sources/jdk/src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c:74:11: 
note: ‘snprintf’ output between 6 and 174 bytes into a destination of 
size 81

 (void)snprintf(tbuf, ltbuf,
   ^
"%s.%.3d %s", timestamp_prefix,
~~~
(int)(millisecs), timestamp_postfix);



and a lot of those:

/home/rep/sources/jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-debug.hh:130:1: 
warning: explicit specialization ‘void _hb_debug_msg_va(const char*, 
const void*, const char*, bool, unsigned int, int, const char*, 
__va_list_tag*) [with int max_level = 0]’ may be missing attributes 
[-Wmissing-attributes]

 _hb_debug_msg_va<0> (const char *what HB_UNUSED,
 ^~~




On 2018-06-19 11:20, Andrew Haley wrote:

On 06/19/2018 03:30 AM, David Holmes wrote:

I've filed:

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

for the underlying issue. Hotspot folk need to check if the warnings 
are
legitmate or not. If not then we may need to disable this warning in 
the

build.


The warnings are legitimate: it's unnecessarily hairy code.  It could
easily be rewritten to use memcpy(), would be obviously guaranteed not
to exceed array bounds, and might even be slightly faster.  Fixing
these warnings is an opportunity to clean up some of this code.

Re: JDK Build on Fedora 28

[Andrew Haley]

On 06/19/2018 03:30 AM, David Holmes wrote:
> I've filed:
> 
> https://bugs.openjdk.java.net/browse/JDK-8205201
> 
> for the underlying issue. Hotspot folk need to check if the warnings are 
> legitmate or not. If not then we may need to disable this warning in the 
> build.

The warnings are legitimate: it's unnecessarily hairy code.  It could
easily be rewritten to use memcpy(), would be obviously guaranteed not
to exceed array bounds, and might even be slightly faster.  Fixing
these warnings is an opportunity to clean up some of this code.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR(M): 8204965: Fix '--disable-cds' and disable CDS on AIX by default

[David Holmes]

Hi Volker,

On 19/06/2018 4:50 PM, Volker Simonis wrote:

On Tue, Jun 19, 2018 at 6:54 AM, David Holmes  wrote:

Hi Volker,

v3 looks much cleaner - thanks.

But AFAICS the change to jvmtiEnv.cpp is also not needed.
ClassLoaderExt::append_boot_classpath exists regardless of INCLUDE_CDS but
operates differently (just calling ClassLoader::add_to_boot_append_entries).



That's not entirely true because the whole compilation unit (i.e.
classLoaderExt.cpp) which contains
'ClassLoaderExt::append_boot_classpath()' is excluded from the
compilation if CDS is disabled (see make/hotspot/lib/JvmFeatures.gmk).


Hmmm. There's a CDS bug there. Either classLoaderExt.cpp should not be 
excluded from a non-CDS build, or it should not contain any INCLUDE_CDS 
guards! I suspect it should not be excluded.



So I can either move the whole implementation of
'ClassLoaderExt::append_boot_classpath()' into classLoaderExt.hpp in
which case things would work as you explained and my changes to
jvmtiEnv.cpp could be removed or leave the whole code and change as
is. Please let me know what you think?


In the interest of moving forward you can push what you have and I will 
file a bug against CDS to sort out classLoaderExt.cpp.


Thanks,
David


Regards,
Volker


Thanks,
David


On 19/06/2018 2:04 AM, Volker Simonis wrote:


On Mon, Jun 18, 2018 at 8:17 AM, David Holmes 
wrote:


Hi Volker,

src/hotspot/share/runtime/globals.hpp

This change should not be needed! We do minimal VM builds without CDS and
we
don't have to touch the UseSharedSpaces defaults (unless recent change
have
broken this - in which case that needs to be addressed in its own right!)



Yes, you're right, CDS_ONLY/NOT_CDS isn't really required here,
because UseSharedSpaces is reseted later on at the end of
Arguments::parse(). I just thought it would be cleaner to disable it
statically, if the VM doesn't support it. But anyway I don't really
mind and I've reverted that change in globals.hpp.


src/hotspot/share/classfile/javaClasses.cpp

AFAICS you should be using INCLUDE_CDS in the ifdefs not
INCLUDE_CDS_JAVA_HEAP. But again I'm unclear (as was Thomas) why this
should
be needed as we have not needed it before. As Thomas notes we have:

./hotspot/share/memory/metaspaceShared.hpp:  static bool
is_archive_object(oop p) NOT_CDS_JAVA_HEAP_RETURN_(false);
./hotspot/share/classfile/stringTable.hpp:  static oop
create_archived_string(oop s, Thread* THREAD)
NOT_CDS_JAVA_HEAP_RETURN_(NULL);

so these methods should be defined when CDS is not available.



Thomas and you are right. Must have been a mis-configuration on AIX
where I saw undefined symbols at link time. I've removed the ifdefs
from javaClasses.cpp now.

Finally, I've also wrapped all the FileMapInfo fields in vmStructs.cpp
into CDS_ONLY macros as suggested by Jiangli because the really only
make sense for a CDS-enabled VM.

Here's the new webrev:

http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965.v3/

Please let me know if you think there's still something missing.

Regards,
Volker



??

Thanks,
David
-





On 15/06/2018 12:26 AM, Volker Simonis wrote:



Hi,

can I please have a review for the following fix:

http://cr.openjdk.java.net/~simonis/webrevs/2018/8204965/
https://bugs.openjdk.java.net/browse/JDK-8204965

CDS does currently not work on AIX because of the way how we
reserve/commit memory on AIX. The problem is that we're using a
combination of shmat/mmap depending on the page size and the size of
the memory chunk to reserve. This makes it impossible to reliably
reserve the memory for the CDS archive and later on map the various
parts of the archive into these regions.

In order to fix this we would have to completely rework the memory
reserve/commit/uncommit logic on AIX which is currently out of our
scope because of resource limitations.

Unfortunately, I could not simply disable CDS in the configure step
because some of the shared code apparently relies on parts of the CDS
code which gets excluded from the build when CDS is disabled. So I
also fixed the offending parts in hotspot and cleaned up the configure
logic for CDS.

Thank you and best regards,
Volker

PS: I did run the job through the submit forest
(mach5-one-simonis-JDK-8204965-20180613-1946-26349) but the results
weren't really useful because they mention build failures on linux-x64
which I can't reproduce locally.