RFR(L) : 8199382 : [TESTBUG] Open source VM testbase JDI tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8199382/webrev.00/index.html
> 577169 lines changed: 577169 ins; 0 del; 0 mod; 

Hi all,

could you please review the patch which open sources JDI tests from vm 
testbase? These tests cover different aspects of JDI implementation.

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199382
webrev:  http://cr.openjdk.java.net/~iignatyev/8199382/webrev.00/index.html
testing: vmTestbase_nsk_jdi test group

Thanks,
-- Igor

Re: Fwd: RFR: JDK-8202319: Fix compilation warnings in Solaris debug builds for DevStudio 12.6

[David Holmes]

Looks okay.

Thanks,
David

On 4/05/2018 8:01 AM, gary.ad...@oracle.com wrote:

Attached an updated patch for 8202319.
Kim or David could you please sponsor the push of the patch.

On 4/26/18 6:49 PM, gary.ad...@oracle.com wrote:


Adding build-dev and hotspot-runtime-dev aliases.

 Forwarded Message 
Subject: RFR: JDK-8202319: Fix compilation warnings in Solaris 
debug builds for DevStudio 12.6

Date: Thu, 26 Apr 2018 12:35:28 -0400
From: Gary Adams 
Reply-To: gary.ad...@oracle.com
To: OpenJDK Serviceability 



Getting the sources ready for the next Solaris developer studio 
toolchain.

Some additional warnings were found in the debug build.

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

This update conditionally disables some new error checks, if the
new toolchain is used.

Re: Fwd: RFR: JDK-8202319: Fix compilation warnings in Solaris debug builds for DevStudio 12.6

[gary.ad...@oracle.com]

Attached an updated patch for 8202319.
Kim or David could you please sponsor the push of the patch.

On 4/26/18 6:49 PM, gary.ad...@oracle.com wrote:


Adding build-dev and hotspot-runtime-dev aliases.

 Forwarded Message 
Subject: 	RFR: JDK-8202319: Fix compilation warnings in Solaris debug 
builds for DevStudio 12.6

Date:   Thu, 26 Apr 2018 12:35:28 -0400
From:   Gary Adams 
Reply-To:   gary.ad...@oracle.com
To: OpenJDK Serviceability 



Getting the sources ready for the next Solaris developer studio toolchain.
Some additional warnings were found in the debug build.

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

This update conditionally disables some new error checks, if the
new toolchain is used.



# HG changeset patch
# User gadams
# Date 1525176684 14400
#  Tue May 01 08:11:24 2018 -0400
# Node ID c2219a364d79a09d5de1480ee1fc280e6fcb0f16
# Parent  2ace90aec48858c5d82fd64b546c29b395ea5524
8202319: Fix compilation warnings in Solaris debug builds for DevStudio 12.6
Reviewed-by: kbarrett, dholmes

diff --git a/src/hotspot/share/runtime/frame.cpp 
b/src/hotspot/share/runtime/frame.cpp
--- a/src/hotspot/share/runtime/frame.cpp
+++ b/src/hotspot/share/runtime/frame.cpp
@@ -1103,6 +1103,9 @@
 
 void frame::oops_do_internal(OopClosure* f, CodeBlobClosure* cf, RegisterMap* 
map, bool use_interpreter_oop_map_cache) {
 #ifndef PRODUCT
+#if defined(__SUNPRO_CC) && __SUNPRO_CC >= 0x5140
+#pragma error_messages(off, SEC_NULL_PTR_DEREF)
+#endif
   // simulate GC crash here to dump java thread in error report
   if (CrashGCForDumpingJavaThread) {
 char *t = NULL;
diff --git a/src/hotspot/share/utilities/vmError.cpp 
b/src/hotspot/share/utilities/vmError.cpp
--- a/src/hotspot/share/utilities/vmError.cpp
+++ b/src/hotspot/share/utilities/vmError.cpp
@@ -1606,6 +1606,9 @@
 }
 
 #ifndef PRODUCT
+#if defined(__SUNPRO_CC) && __SUNPRO_CC >= 0x5140
+#pragma error_messages(off, SEC_NULL_PTR_DEREF)
+#endif
 typedef void (*voidfun_t)();
 // Crash with an authentic sigfpe
 static void crash_with_sigfpe() {

Re: Windows: problem with msvxxx.dll locations containing spaces

[Erik Joelsson]

Hello,

In my case, VCINSTALLDIR is in short form already. Could you try 
changing line 543 from BASIC_WINDOWS_REWRITE_AS_UNIX_PATH to 
BASIC_FIXUP_PATH?


/Erik


On 2018-05-03 11:17, Thomas Stüfe wrote:

Hi Erik,

I see the error on two very different machines. One is my private
machine - windows 7, VS2013 and VS2017 installed. The second one is my
corporate Laptop, Windows 10 and only VS2013.

In both cases I use a very recent cygwin64 installation.

Both cases run in TOOLCHAIN_SETUP_MSVC_DLL into the "# Probe: Using
well-known location from Visual Studio 12.0 and older" case, and in
the for loop at line 559 attempt to tokenize the POSSIBLE_MSVC_DLL
variable content. Here the 64bit build on my corporate laptop:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/WINDOWS/system32/msvcr120.dll using well-known location in
SYSTEMROOT
checking found msvcr120.dll architecture... ok
checking for msvcr120.dll... /cygdrive/c/WINDOWS/system32/msvcr120.dll
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcp120.dll
configure: Found msvcp120.dll at
/cygdrive/c/WINDOWS/system32/msvcp120.dll using well-known location in
SYSTEMROOT

As you can see, we fall back to the default in windows/system32, which
happens to be working for 64bit. On 32bit, we get this error:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/WINDOWS/system32/msvcr120.dll using well-known location in
SYSTEMROOT
checking found msvcr120.dll architecture... incorrect, ignoring
configure: The file type of the located msvcr120.dll is PE32+
executable (DLL) (GUI) x86-64, for MS Windows
configure: Found msvcr120.dll at /cygdrive/c/Program Files
(x86)/Microsoft Visual Studio
12.0/VC/redist/arm/Microsoft.VC120.CRT/msvcr120.dll using search of
VCINSTALLDIR
checking found msvcr120.dll architecture... incorrect, ignoring
configure: The file type of the located msvcr120.dll is PE32
executable (DLL) (GUI) ARMv7 Thumb, for MS Windows
checking for msvcr120.dll... no
configure: error: Could not find msvcr120.dll. Please specify using
--with-msvcr-dll.
configure exiting with result code 1

Best Regards, Thomas

On Thu, May 3, 2018 at 7:18 PM, Erik Joelsson  wrote:

Also, on my windows system, if I try using my local Visual Studio, both MSVC
dll's are found in the Visual Studio dir, and the spaces are all cleaned up.
I wonder what's different about your setup, Thomas.

/Erik



On 2018-05-03 10:13, Erik Joelsson wrote:

Hello,

On 2018-05-03 03:41, Magnus Ihse Bursie wrote:

I think the main issue here is that BASIC_FIXUP_PATH should be called for
the located msvcr*.dll files. I don't have time to look at it now, but see
if you can do a rewrite using BASIC_FIXUP_PATH when the potential file is
located. This should remove all spaces from the path.


No, that is not a good solution. I intentionally removed that
BASIC_FIXUP_PATH because in VS2017, one of the files has a file name longer
than 8 characters and BASIC_FIXUP_PATH rewrites that filename to short
style. This in turn has weird consequences in make when we copy that file
using generated copy rules. The target file then gets the short style name
literally.

When I made that change, I looked at all the calls for potential locations
and thought all of them had the directory prepared properly already. It
seems I was wrong. I think the correct solution is to either get rid of
spaces earlier for all the input directories we search, or maybe tweak
BASIC_FIXUP_PATH to not touch the actual filename.

/Erik

/Magnus

On 2018-05-02 20:46, Thomas Stüfe wrote:

Hi,

My 32bit builds on Windows were failing since quite a while and I
finally had some minutes to look into that.

See prior discussion here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021150.html

My output used to look like this:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/Windows/system32/msvcr120.dll using well-known location in
SYSTEMROOT

So basically, build does not correctly search for msvcr120.dll in
"/cygdrive/c/Program Files (x86)/Microsoft Visual Studio
12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll" - instead, it

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[Kim Barrett]

> On May 3, 2018, at 1:52 PM, B. Blaser  wrote:
> 
> Hi,
> 
> On 2 May 2018 at 22:40, Kim Barrett  wrote:
>>> On May 2, 2018, at 5:10 AM, Michal Vala  wrote:
>>> 
>>> 
>>> 
>>> On 05/01/2018 07:59 PM, Kim Barrett wrote:
> On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
> Someone to sponsor this please?
 Do you have a sponsor yet?  If not, I’ll do it.
>>> 
>>> No, I don't. I'd really appreciate if you sponsor it.
>> 
>> Will do.  I should be able to take care of it in the next day or so.
>> 
> 
> I've noted some similar issues at several other places which makes the
> JDK build fail on Linux with glibc = 2.26, see comments here:
> https://bugs.openjdk.java.net/browse/JDK-8202353
> 
> Here is a patch for that, does this look reasonable (tier1 seems OK)?

I think we should move in the direction of eliminating the use of readdir_r 
entirely,
either under JDK-8202353, or leave that as only the HotSpot part and have 
another
RFE for the uses in java.base/unix/native.

Re: Windows: problem with msvxxx.dll locations containing spaces

[Thomas Stüfe]

Hi Erik,

I see the error on two very different machines. One is my private
machine - windows 7, VS2013 and VS2017 installed. The second one is my
corporate Laptop, Windows 10 and only VS2013.

In both cases I use a very recent cygwin64 installation.

Both cases run in TOOLCHAIN_SETUP_MSVC_DLL into the "# Probe: Using
well-known location from Visual Studio 12.0 and older" case, and in
the for loop at line 559 attempt to tokenize the POSSIBLE_MSVC_DLL
variable content. Here the 64bit build on my corporate laptop:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/WINDOWS/system32/msvcr120.dll using well-known location in
SYSTEMROOT
checking found msvcr120.dll architecture... ok
checking for msvcr120.dll... /cygdrive/c/WINDOWS/system32/msvcr120.dll
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcp120.dll
configure: Found msvcp120.dll at
/cygdrive/c/WINDOWS/system32/msvcp120.dll using well-known location in
SYSTEMROOT

As you can see, we fall back to the default in windows/system32, which
happens to be working for 64bit. On 32bit, we get this error:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/WINDOWS/system32/msvcr120.dll using well-known location in
SYSTEMROOT
checking found msvcr120.dll architecture... incorrect, ignoring
configure: The file type of the located msvcr120.dll is PE32+
executable (DLL) (GUI) x86-64, for MS Windows
configure: Found msvcr120.dll at /cygdrive/c/Program Files
(x86)/Microsoft Visual Studio
12.0/VC/redist/arm/Microsoft.VC120.CRT/msvcr120.dll using search of
VCINSTALLDIR
checking found msvcr120.dll architecture... incorrect, ignoring
configure: The file type of the located msvcr120.dll is PE32
executable (DLL) (GUI) ARMv7 Thumb, for MS Windows
checking for msvcr120.dll... no
configure: error: Could not find msvcr120.dll. Please specify using
--with-msvcr-dll.
configure exiting with result code 1

Best Regards, Thomas

On Thu, May 3, 2018 at 7:18 PM, Erik Joelsson  wrote:
> Also, on my windows system, if I try using my local Visual Studio, both MSVC
> dll's are found in the Visual Studio dir, and the spaces are all cleaned up.
> I wonder what's different about your setup, Thomas.
>
> /Erik
>
>
>
> On 2018-05-03 10:13, Erik Joelsson wrote:
>>
>> Hello,
>>
>> On 2018-05-03 03:41, Magnus Ihse Bursie wrote:
>>>
>>> I think the main issue here is that BASIC_FIXUP_PATH should be called for
>>> the located msvcr*.dll files. I don't have time to look at it now, but see
>>> if you can do a rewrite using BASIC_FIXUP_PATH when the potential file is
>>> located. This should remove all spaces from the path.
>>>
>> No, that is not a good solution. I intentionally removed that
>> BASIC_FIXUP_PATH because in VS2017, one of the files has a file name longer
>> than 8 characters and BASIC_FIXUP_PATH rewrites that filename to short
>> style. This in turn has weird consequences in make when we copy that file
>> using generated copy rules. The target file then gets the short style name
>> literally.
>>
>> When I made that change, I looked at all the calls for potential locations
>> and thought all of them had the directory prepared properly already. It
>> seems I was wrong. I think the correct solution is to either get rid of
>> spaces earlier for all the input directories we search, or maybe tweak
>> BASIC_FIXUP_PATH to not touch the actual filename.
>>
>> /Erik
>>>
>>> /Magnus
>>>
>>> On 2018-05-02 20:46, Thomas Stüfe wrote:

 Hi,

 My 32bit builds on Windows were failing since quite a while and I
 finally had some minutes to look into that.

 See prior discussion here:
 http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021150.html

 My output used to look like this:

 checking if fixpath.exe works... yes
 POSSIBLE_MSVC_DLL /cygdrive/c/Program
 POSSIBLE_MSVC_DLL Files
 POSSIBLE_MSVC_DLL (x86)/Microsoft
 POSSIBLE_MSVC_DLL Visual
 POSSIBLE_MSVC_DLL Studio
 POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
 configure: Found msvcr120.dll at
 /cygdrive/c/Windows/system32/msvcr120.dll using well-known location in
 SYSTEMROOT

 So basically, build does not correctly search for msvcr120.dll in
 "/cygdrive/c/Program Files (x86)/Microsoft Visual Studio
 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll" - 

Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

[B. Blaser]

Hi,

On 2 May 2018 at 22:40, Kim Barrett  wrote:
>> On May 2, 2018, at 5:10 AM, Michal Vala  wrote:
>>
>>
>>
>> On 05/01/2018 07:59 PM, Kim Barrett wrote:
 On Apr 27, 2018, at 4:26 PM, Michal Vala  wrote:
 Someone to sponsor this please?
>>> Do you have a sponsor yet?  If not, I’ll do it.
>>
>> No, I don't. I'd really appreciate if you sponsor it.
>
> Will do.  I should be able to take care of it in the next day or so.
>

I've noted some similar issues at several other places which makes the
JDK build fail on Linux with glibc = 2.26, see comments here:
https://bugs.openjdk.java.net/browse/JDK-8202353

Here is a patch for that, does this look reasonable (tier1 seems OK)?

Thanks,
Bernard


diff -r 1871c5d07caf src/java.base/unix/native/libjava/TimeZone_md.c
--- a/src/java.base/unix/native/libjava/TimeZone_md.cFri Apr 27
15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libjava/TimeZone_md.cThu May 03
17:59:31 2018 +0200
@@ -122,7 +122,9 @@
 DIR *dirp = NULL;
 struct stat statbuf;
 struct dirent64 *dp = NULL;
+#ifndef __linux__
 struct dirent64 *entry = NULL;
+#endif
 char *pathname = NULL;
 int fd = -1;
 char *dbuf = NULL;
@@ -140,7 +142,7 @@
 if (name_max < 1024) {
 name_max = 1024;
 }
-
+#ifndef __linux__
 entry = (struct dirent64 *)malloc(offsetof(struct dirent64,
d_name) + name_max + 1);
 if (entry == NULL) {
 (void) closedir(dirp);
@@ -148,6 +150,9 @@
 }

 while (readdir64_r(dirp, entry, ) == 0 && dp != NULL) {
+#else
+while ((dp = readdir64(dirp)) != NULL) {
+#endif
 /*
  * Skip '.' and '..' (and possibly other .* files)
  */
@@ -213,10 +218,11 @@
 free((void *) pathname);
 pathname = NULL;
 }
-
+#ifndef __linux__
 if (entry != NULL) {
 free((void *) entry);
 }
+#endif
 if (dirp != NULL) {
 (void) closedir(dirp);
 }
diff -r 1871c5d07caf src/java.base/unix/native/libjava/UnixFileSystem_md.c
--- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cFri Apr
27 15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cThu May
03 17:59:31 2018 +0200
@@ -312,7 +312,9 @@
 {
 DIR *dir = NULL;
 struct dirent64 *ptr;
+#ifndef __linux__
 struct dirent64 *result;
+#endif
 int len, maxlen;
 jobjectArray rv, old;
 jclass str_class;
@@ -324,14 +326,14 @@
 dir = opendir(path);
 } END_PLATFORM_STRING(env, path);
 if (dir == NULL) return NULL;
-
+#ifndef __linux__
 ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1));
 if (ptr == NULL) {
 JNU_ThrowOutOfMemoryError(env, "heap allocation failed");
 closedir(dir);
 return NULL;
 }
-
+#endif
 /* Allocate an initial String array */
 len = 0;
 maxlen = 16;
@@ -339,7 +341,11 @@
 if (rv == NULL) goto error;

 /* Scan the directory */
+#ifndef __linux__
 while ((readdir64_r(dir, ptr, ) == 0)  && (result != NULL)) {
+#else
+while ((ptr = readdir64(dir)) != NULL) {
+#endif
 jstring name;
 if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, ".."))
 continue;
@@ -360,7 +366,9 @@
 (*env)->DeleteLocalRef(env, name);
 }
 closedir(dir);
+#ifndef __linux__
 free(ptr);
+#endif

 /* Copy the final results into an appropriately-sized array */
 old = rv;
@@ -375,7 +383,9 @@

  error:
 closedir(dir);
+#ifndef __linux__
 free(ptr);
+#endif
 return NULL;
 }

diff -r 1871c5d07caf src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
--- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
Fri Apr 27 15:55:29 2018 -0700
+++ b/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
Thu May 03 17:59:31 2018 +0200
@@ -726,12 +726,18 @@
 char name_extra[PATH_MAX + 1 - sizeof result->d_name];
 } entry;
 struct dirent64* ptr = 
+#ifndef __linux__
 int res;
+#endif
 DIR* dirp = jlong_to_ptr(value);

 /* EINTR not listed as a possible error */
 /* TDB: reentrant version probably not required here */
+#ifndef __linux__
 res = readdir64_r(dirp, ptr, );
+#else
+ptr = result = readdir64(dirp);
+#endif

 #ifdef _AIX
 /* On AIX, readdir_r() returns EBADF (i.e. '9') and sets 'result'
to NULL for the */
@@ -741,10 +747,12 @@
 }
 #endif

+#ifndef __linux__
 if (res != 0) {
 throwUnixException(env, res);
 return NULL;
 } else {
+#endif
 if (result == NULL) {
 return NULL;
 } else {
@@ -755,7 +763,9 @@
 }
 return bytes;
 }
+#ifndef __linux__
 }
+#endif
 }

 JNIEXPORT void JNICALL
diff -r 1871c5d07caf
src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c
--- a/src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c
   Fri Apr 27 15:55:29 2018 -0700
+++ 

Re: RFR: 8200729: Conditional compilation of GCs

[Stefan Karlsson]

Hi Vladimir,

On 2018-05-03 18:56, Vladimir Kozlov wrote:
I see that you don't guard Use*GC flags with _ONLY macros. It is hard to 
figure out from gcConfig.cpp what happened if UseG1GC is specified on 
command line for VM which does not include it. What happens?


Right. The current patch leaves the Use*GC flags available in all 
builds, so that we can accept the flag but give a warning:


$ build/slowdebug-no-g1/jdk/bin/java -XX:+UseG1GC
Java HotSpot(TM) 64-Bit Server VM warning: -XX:+UseG1GC not supported in 
this VM


This is handled in GCConfig::select_gc_ergonomically with this line:
  NOT_G1GC(  UNSUPPORTED_OPTION(UseG1GC);)

That expands into:
// Disable options not supported in this release, with a warning if they
// were explicitly requested on the command-line
#define UNSUPPORTED_OPTION(opt)  \
do { \
  if (opt) { \
if (FLAG_IS_CMDLINE(opt)) {  \
  warning("-XX:+" #opt " not supported in this VM"); \
}\
FLAG_SET_DEFAULT(opt, false);\
  }  \
} while(0)




I don't see C1 changes but it looks like it was changed with 8201543.


Right. There's no INCLUDE_ guards left in C1!


C2 changes seems fine but they will be also affected by 8202377.


Exactly.



What is left is AOT and JVMCI/Graal. We thought to implement "cross" 
compilation when we can specify for which configuration we should 
generate AOT code. It includes GC barriers code. Currently I see that 
you keep all GCs in build as before so it is not a issue. And we record 
VM version so we will not generate and use G1 barriers if it is not in 
VM used by jaotc.


So it seems to me compiler and AOT, Graal changes are fine.


OK. Thanks for taking a close look at this.



I would suggest in addition to hs-tier2 testing run hs-tier2-graal to 
make sure Graal still works.


I'll make sure to test with hs-tier2-graal.

Thanks for reviewing!

StefanK



Thanks,
Vladimir

On 5/2/18 7:37 AM, Stefan Karlsson wrote:

Hi all,

Please review these patches to allow for conditional compilation of 
the GCs in HotSpot.


Full patch:

http://cr.openjdk.java.net/~stefank/8200729/webrev.04/all/

(See below for a more fine-grained division into smaller patches)

Today Parallel, G1, and CMS, are all guarded by INCLUDE_ALL_GCS. 
INCLUDE_ALL_GCS becomes defined to 1 for our server 
(--with-jvm-variants=server) builds, and is defined to 0 for the 
minimal (--with-jvm-variants=minimal) builds. There are also ways to 
forcefully remove these GCs from the compilation by configuring with, 
for example, --with-jvm-features=all-gcs.


The proposed patch removes INCLUDE_ALL_GCS (and all-gcs) and replaces 
it with INCLUDE_CMSGC, INCLUDE_G1GC, and INCLUDE_PARALLELGC. In 
addition to that, INCLUDE_SERIALGC has been added to guard the Serial 
GC code.


Future GCs should adopt this scheme before they get incorporated into 
the code base. Note, that there will be some files in gc/shared that 
are going to have to know about all GCs, but the goal is to have very 
few of these INCLUDE_ checks in the non-GC parts of HotSpot.


With this patch, it's also going to be easier to stop compiling CMS 
when the time as come for it to move from deprecated to removed.


Note, that even though this adds great flexibility, and allows for 
easy inclusion / exclusion of GCs, there's no guarantee that a 
specific combination of GCs is going to be maintained in the future. 
Just like not all combinations of the different Runtime features (CDS, 
NMT, JFR) and Compiler features (AOT, COMPILER1, COMPILER2) are 
guaranteed to compile and be supported. I've sanity checked the 
different GC configurations build locally, but I've only fully tested 
the "server" variant, and "minimal" has only been built locally.


There's a more high-level discussion around the flexibility the 
--with-jvm-features flag adds, and if we really should allow it. Since 
this patch only builds upon that existing flexibility, and I don't 
change the two defined jvm variants, I'd appreciate if that discussion 
could be kept out of this review thread. For further discussion around 
this, please see:


http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021663.html

This is the patch queue:

The first patch simply cleans up some INCLUDE_ALL_GCS usages in 
platform-specific files. Some of these changes are already being 
cleaned up by other RFEs:


http://cr.openjdk.java.net/~stefank/8200729/webrev.04/00.removeUnneededIncludeAllGCs/ 




The second patch pre-cleans some include files:

http://cr.openjdk.java.net/~stefank/8200729/webrev.04/01.fixIncludes/


The following is the main patch, which include all relevant HotSpot 
changes. For a while now, we have been actively working on abstracting 
away GC specific code from non-GC 

Re: Windows: problem with msvxxx.dll locations containing spaces

[Erik Joelsson]

Also, on my windows system, if I try using my local Visual Studio, both 
MSVC dll's are found in the Visual Studio dir, and the spaces are all 
cleaned up. I wonder what's different about your setup, Thomas.


/Erik


On 2018-05-03 10:13, Erik Joelsson wrote:

Hello,

On 2018-05-03 03:41, Magnus Ihse Bursie wrote:
I think the main issue here is that BASIC_FIXUP_PATH should be called 
for the located msvcr*.dll files. I don't have time to look at it 
now, but see if you can do a rewrite using BASIC_FIXUP_PATH when the 
potential file is located. This should remove all spaces from the path.


No, that is not a good solution. I intentionally removed that 
BASIC_FIXUP_PATH because in VS2017, one of the files has a file name 
longer than 8 characters and BASIC_FIXUP_PATH rewrites that filename 
to short style. This in turn has weird consequences in make when we 
copy that file using generated copy rules. The target file then gets 
the short style name literally.


When I made that change, I looked at all the calls for potential 
locations and thought all of them had the directory prepared properly 
already. It seems I was wrong. I think the correct solution is to 
either get rid of spaces earlier for all the input directories we 
search, or maybe tweak BASIC_FIXUP_PATH to not touch the actual filename.


/Erik

/Magnus

On 2018-05-02 20:46, Thomas Stüfe wrote:

Hi,

My 32bit builds on Windows were failing since quite a while and I
finally had some minutes to look into that.

See prior discussion here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021150.html

My output used to look like this:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/Windows/system32/msvcr120.dll using well-known location in
SYSTEMROOT

So basically, build does not correctly search for msvcr120.dll in
"/cygdrive/c/Program Files (x86)/Microsoft Visual Studio
12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll" - instead, it
fails and falls back to the system default
"/cygdrive/c/Windows/system32/msvcr120.dll". That dll is a 64bit dll,
and so configure fails.

Note that 64bit build shows exactly the same behaviour! Only there it
works by accident, since the default
/cygdrive/c/Windows/system32/msvcr120.dll it finds happens to be a
64bit library too, so configure succeeds.

Part of the problem is TOOLCHAIN_SETUP_MSVC_DLL in
toolchain_windows.m4. We use a bash for loop to iterate thru a list of
one or more files, but that for expression should be quoted.

If I make this fix:

--- a/make/autoconf/toolchain_windows.m4    Mon Apr 23 18:04:17 
2018 -0700
+++ b/make/autoconf/toolchain_windows.m4    Wed May 02 18:38:04 
2018 +0200

@@ -556,7 +556,7 @@
  fi
    fi
    # In case any of the above finds more than one file, loop 
over them.

-  for possible_msvc_dll in $POSSIBLE_MSVC_DLL; do
+  for possible_msvc_dll in "$POSSIBLE_MSVC_DLL"; do
  $ECHO "POSSIBLE_MSVC_DLL $possible_msvc_dll"
  TOOLCHAIN_CHECK_POSSIBLE_MSVC_DLL([$DLL_NAME], 
[$possible_msvc_dll],

  [well-known location in VCINSTALLDIR])


the 32bit configure correctly sets the msvcrt dll:

POSSIBLE_MSVC_DLL /cygdrive/c/Program Files (x86)/Microsoft Visual
Studio 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at /cygdrive/c/Program Files
(x86)/Microsoft Visual Studio
12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll using well-known
location in VCINSTALLDIR
checking found msvcr120.dll architecture... ok

and I can start the build, but I get follow up errors:

...
Creating hotspot/variant-server/tools/adlc/adlc.exe from 13 file(s)
Compiling 2 files for BUILD_JVMTI_TOOLS
make[3]: *** No rule to make target '/cygdrive/c/Program', needed by
'/cygdrive/c/mine/projects/openjdk/jdk-jdk/output-fastdebug-32/support/modules_libs/java.base/Program'. 


Stop.
make[3]: *** Waiting for unfinished jobs
make[2]: *** [make/Main.gmk:165: java.base-copy] Error 2
make[2]: *** Waiting for unfinished jobs

I stopped looking at that point, but to me it looks like the build
cannot survive msvcrt.dll locations with spaces in them.

Kind Regards, Thomas

Re: Windows: problem with msvxxx.dll locations containing spaces

[Erik Joelsson]

Hello,

On 2018-05-03 03:41, Magnus Ihse Bursie wrote:
I think the main issue here is that BASIC_FIXUP_PATH should be called 
for the located msvcr*.dll files. I don't have time to look at it now, 
but see if you can do a rewrite using BASIC_FIXUP_PATH when the 
potential file is located. This should remove all spaces from the path.


No, that is not a good solution. I intentionally removed that 
BASIC_FIXUP_PATH because in VS2017, one of the files has a file name 
longer than 8 characters and BASIC_FIXUP_PATH rewrites that filename to 
short style. This in turn has weird consequences in make when we copy 
that file using generated copy rules. The target file then gets the 
short style name literally.


When I made that change, I looked at all the calls for potential 
locations and thought all of them had the directory prepared properly 
already. It seems I was wrong. I think the correct solution is to either 
get rid of spaces earlier for all the input directories we search, or 
maybe tweak BASIC_FIXUP_PATH to not touch the actual filename.


/Erik

/Magnus

On 2018-05-02 20:46, Thomas Stüfe wrote:

Hi,

My 32bit builds on Windows were failing since quite a while and I
finally had some minutes to look into that.

See prior discussion here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021150.html

My output used to look like this:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/Windows/system32/msvcr120.dll using well-known location in
SYSTEMROOT

So basically, build does not correctly search for msvcr120.dll in
"/cygdrive/c/Program Files (x86)/Microsoft Visual Studio
12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll" - instead, it
fails and falls back to the system default
"/cygdrive/c/Windows/system32/msvcr120.dll". That dll is a 64bit dll,
and so configure fails.

Note that 64bit build shows exactly the same behaviour! Only there it
works by accident, since the default
/cygdrive/c/Windows/system32/msvcr120.dll it finds happens to be a
64bit library too, so configure succeeds.

Part of the problem is TOOLCHAIN_SETUP_MSVC_DLL in
toolchain_windows.m4. We use a bash for loop to iterate thru a list of
one or more files, but that for expression should be quoted.

If I make this fix:

--- a/make/autoconf/toolchain_windows.m4    Mon Apr 23 18:04:17 
2018 -0700
+++ b/make/autoconf/toolchain_windows.m4    Wed May 02 18:38:04 
2018 +0200

@@ -556,7 +556,7 @@
  fi
    fi
    # In case any of the above finds more than one file, loop 
over them.

-  for possible_msvc_dll in $POSSIBLE_MSVC_DLL; do
+  for possible_msvc_dll in "$POSSIBLE_MSVC_DLL"; do
  $ECHO "POSSIBLE_MSVC_DLL $possible_msvc_dll"
  TOOLCHAIN_CHECK_POSSIBLE_MSVC_DLL([$DLL_NAME], 
[$possible_msvc_dll],

  [well-known location in VCINSTALLDIR])


the 32bit configure correctly sets the msvcrt dll:

POSSIBLE_MSVC_DLL /cygdrive/c/Program Files (x86)/Microsoft Visual
Studio 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at /cygdrive/c/Program Files
(x86)/Microsoft Visual Studio
12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll using well-known
location in VCINSTALLDIR
checking found msvcr120.dll architecture... ok

and I can start the build, but I get follow up errors:

...
Creating hotspot/variant-server/tools/adlc/adlc.exe from 13 file(s)
Compiling 2 files for BUILD_JVMTI_TOOLS
make[3]: *** No rule to make target '/cygdrive/c/Program', needed by
'/cygdrive/c/mine/projects/openjdk/jdk-jdk/output-fastdebug-32/support/modules_libs/java.base/Program'. 


Stop.
make[3]: *** Waiting for unfinished jobs
make[2]: *** [make/Main.gmk:165: java.base-copy] Error 2
make[2]: *** Waiting for unfinished jobs

I stopped looking at that point, but to me it looks like the build
cannot survive msvcrt.dll locations with spaces in them.

Kind Regards, Thomas

Re: [8u] RFR: 8196516: libfontmanager must be built with LDFLAGS allowing unresolved symbols

[Erik Joelsson]

Looks good.

/Erik


On 2018-05-03 04:33, Severin Gehwolf wrote:

Hi,

Could I please get a review of this change for JDK 8u? We are seing
build failures due to unresolved symbols when building
libfontmanager.so. The build flag to trigger this is to configure with:

--with-extra-ldflags="-Wl,-z,defs"

Attempting a build of JDK 8 with this fails. This has been fixed in JDK
11 and hasn't shown any issues so far. Due to the change in the build
system the JDK 11 patch does not apply cleanly after path unshuffeling
(different context it seems). Hence, asking here for a review before
asking for JDK 8 backport approval.

Bug: https://bugs.openjdk.java.net/browse/JDK-8196516
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8196516/webrev.jdk8/

Review thread for JDK 11:
http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021575.html

Thanks,
Severin

Re: [11] RFR(S) 8202552: [AOT][JVMCI] Incorrect usage of INCLUDE_JVMCI and INCLUDE_AOT

[Vladimir Kozlov]

Thank you, Magnus

On 5/3/18 1:40 AM, Magnus Ihse Bursie wrote:

On 2018-05-03 00:13, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8202552/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8202552


Looks good to me.

Just some thinking out loud... The way we handle the responsibility 
split between the makefiles and the hotspot source files is actually a 
bit corny. :-( It would make more sense for the makefiles to set 
-DINCLUDE_features=1 when the feature is enabled, and 
-DINCLUDE_feature=0 when it is not (or just leave it out), and skip all 
the #ifndef INCLUDE_ #define INCLUDE_ 1 ...in macros.hpp.


Yes, I thought the same thing but it was too much change for this fix.



But that is the work of a future cleanup. This patch makes sure we use 
the same pattern everywhere, which is good.


Thanks,
Vladimir K



/Magnus




Stefan K. found several places where #ifdef instead of #if is used for 
INCLUDE_JVMCI.

I also found places where we can use COMPILER2_OR_JVMCI.

An other problem surprised me that we don't set INCLUDE_AOT to 1. 
Makefile defines that variable without value. I changed code to match 
other variables setting - in makefile set it to 0 if it is not part of 
build and set it to 1 in macros.hpp if it is not defined.


Tested with tier1, tier2 and tier2 with Graal as JIT compiler.

Re: [11] RFR(S) 8202552: [AOT][JVMCI] Incorrect usage of INCLUDE_JVMCI and INCLUDE_AOT

[Vladimir Kozlov]

Thank you, Stefan

Vladimir K

On 5/3/18 12:20 AM, Stefan Karlsson wrote:

Looks good to me. Thanks for fixing.

StefanK

On 2018-05-03 00:13, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8202552/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8202552

Stefan K. found several places where #ifdef instead of #if is used for 
INCLUDE_JVMCI.

I also found places where we can use COMPILER2_OR_JVMCI.

An other problem surprised me that we don't set INCLUDE_AOT to 1. 
Makefile defines that variable without value. I changed code to match 
other variables setting - in makefile set it to 0 if it is not part of 
build and set it to 1 in macros.hpp if it is not defined.


Tested with tier1, tier2 and tier2 with Graal as JIT compiler.

Re: License and Usage Terms of generated API documentation

[Michael Rasmussen]

On 2018-05-03 13:21, Magnus Ihse Bursie wrote:
> A lot of links on the interwebz are pointing to the docs at 
> docs.oracle.com, and it would probably be quite messy if the same (or 
> even worse, almost the same) documentation were published both at 
> docs.oracle.com and java.net.

Well, until quite recently, that was the case.
The docs were available at both download.java.net and docs.oracle.com (though 
download.java.net seemed to basically just be a proxy for docs.oracle.com)

Google still returns results for download.java.net for the Java 10 docs, for 
instance this link:
https://download.java.net/java/jdk10/docs/api/java/lang/Integer.html
( https://i.imgur.com/4ZugUQa.png )

Also, the Java 11 EA docs are on download.java.net:
https://download.java.net/java/early_access/jdk11/docs/api/index.html?overview-summary.html

/Michael

Re: RFR: 8200729: Conditional compilation of GCs

[Stefan Karlsson]

On 2018-05-03 15:19, Erik Helin wrote:

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > Hi all,

Hi Stefan,

thanks for taking on this work, much appreciated!

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > The first patch simply cleans up some INCLUDE_ALL_GCS usages in 
platform-specific files. Some of these changes are already being cleaned 
up by other RFEs:

 >
 > 
http://cr.openjdk.java.net/~stefank/8200729/webrev.04/00.removeUnneededIncludeAllGCs/ 



Looks good, Reviewed.

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > The second patch pre-cleans some include files:
 >
 > http://cr.openjdk.java.net/~stefank/8200729/webrev.04/01.fixIncludes/

Also looks good, Reviewed.

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
 > The following is the main patch, which include all relevant HotSpot 
changes. For a while now, we have been actively working on abstracting 
away GC specific code from non-GC directories, but as can be seen in 
this patch, there are still a few places left. Hopefully, we will get 
rid of most of these in the near future.

 >
 > http://cr.openjdk.java.net/~stefank/8200729/webrev.04/02.mainPatch/

Very nice, just one small nit:

- barrierSetConfig.hpp:
   could you move "+  G1GC_ONLY(f(G1BarrierSet))" into
   FOR_EACH_CONCRETE_BARRIER_SET_DO ? As in

    // Do something for each concrete barrier set part of the build.
    #define FOR_EACH_CONCRETE_BARRIER_SET_DO(f)  \
  f(CardTableBarrierSet) \
  G1GC_ONLY(f(G1BarrierSet))



Yes. That's better.

As a note for people following along this thread (and to a future 
version of myself), the following work is ongoing to further clean up 
the GC specific bits and pieces sprinkled throughout the rest of the VM:

- 8202377: Modularize C2 GC barriers
- 8202547: Move G1 runtime calls used by generated code to 
G1BarrierSetRuntime

- 8201436: Replace oop_ps_push_contents with oop_iterate and closure

In addition to the above work, this patch highlights a few more places 
that needs to be taken care of:

- get rid of #if INCLUDE_CMS in defNewGeneration.cpp
- split collector specific parts of gcTrace.hpp into collector
   specific tracers
- rework Threads::create_thread_roots_tasks into something more generic
   that parallel then can use to implement its own
   create_thread_roots_task
- remove marksweep_init() and set up MarkSweep::_gc_timer and
   MarkSweep::_gc_tracer in SerialHeap and ParallelScavengeHeap
- benchmark (and measure file sizes) to see if it is still worthwhile to
   keep the INCLUDE_OOP_OOP_ITERATE_BACKWARDS guard (i.e. can we always
   compile the oop_iterate_backwards methods?)
- need to do some work to encapsulate the G1 and CDS code
   (see e.g. oop.cpp, metaspaceShared.cpp and file.cpp)
- move VM_CollectForMetadataAllocation::initiate_concurrent_GC to
   something like
   CollectedHeap::initiate_concurrent_GC_for_metadata_allocation
   (preferably with a shorter name

(the above list is _not_ complete, there will always be a need for 
cleanups :D)


I agree.



 > The last patch adds the make file support to turn on and off the 
different GCs. The content of this patch has evolved from versions 
written by myself, Per, and Magnus Ihse Bursie. Magnus last proposed 
version used the names gc-cms, gc-g1, gc-parallel, and gc-serial, but 
I've changed them to cmsgc, g1gc, parallelgc, and serialgc, so that they 
match the INCLUDE_ defines.

 >
 > 
http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ 



My Makefile knowledge is limited to smaller hacks, I will leave this 
patch for Magnus to review (which I see he already has done).


Overall, very nice work Stefan, thanks!


Thanks Erik!

StefanK


Erik

Re: RFR: 8200729: Conditional compilation of GCs

[Erik Helin]

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
> Hi all,

Hi Stefan,

thanks for taking on this work, much appreciated!

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
> The first patch simply cleans up some INCLUDE_ALL_GCS usages in 
platform-specific files. Some of these changes are already being cleaned 
up by other RFEs:

>
> 
http://cr.openjdk.java.net/~stefank/8200729/webrev.04/00.removeUnneededIncludeAllGCs/ 



Looks good, Reviewed.

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
> The second patch pre-cleans some include files:
>
> http://cr.openjdk.java.net/~stefank/8200729/webrev.04/01.fixIncludes/

Also looks good, Reviewed.

On 05/02/2018 04:37 PM, Stefan Karlsson wrote:
> The following is the main patch, which include all relevant HotSpot 
changes. For a while now, we have been actively working on abstracting 
away GC specific code from non-GC directories, but as can be seen in 
this patch, there are still a few places left. Hopefully, we will get 
rid of most of these in the near future.

>
> http://cr.openjdk.java.net/~stefank/8200729/webrev.04/02.mainPatch/

Very nice, just one small nit:

- barrierSetConfig.hpp:
  could you move "+  G1GC_ONLY(f(G1BarrierSet))" into
  FOR_EACH_CONCRETE_BARRIER_SET_DO ? As in

   // Do something for each concrete barrier set part of the build.
   #define FOR_EACH_CONCRETE_BARRIER_SET_DO(f)  \
 f(CardTableBarrierSet) \
 G1GC_ONLY(f(G1BarrierSet))

As a note for people following along this thread (and to a future 
version of myself), the following work is ongoing to further clean up 
the GC specific bits and pieces sprinkled throughout the rest of the VM:

- 8202377: Modularize C2 GC barriers
- 8202547: Move G1 runtime calls used by generated code to 
G1BarrierSetRuntime

- 8201436: Replace oop_ps_push_contents with oop_iterate and closure

In addition to the above work, this patch highlights a few more places 
that needs to be taken care of:

- get rid of #if INCLUDE_CMS in defNewGeneration.cpp
- split collector specific parts of gcTrace.hpp into collector
  specific tracers
- rework Threads::create_thread_roots_tasks into something more generic
  that parallel then can use to implement its own
  create_thread_roots_task
- remove marksweep_init() and set up MarkSweep::_gc_timer and
  MarkSweep::_gc_tracer in SerialHeap and ParallelScavengeHeap
- benchmark (and measure file sizes) to see if it is still worthwhile to
  keep the INCLUDE_OOP_OOP_ITERATE_BACKWARDS guard (i.e. can we always
  compile the oop_iterate_backwards methods?)
- need to do some work to encapsulate the G1 and CDS code
  (see e.g. oop.cpp, metaspaceShared.cpp and file.cpp)
- move VM_CollectForMetadataAllocation::initiate_concurrent_GC to
  something like
  CollectedHeap::initiate_concurrent_GC_for_metadata_allocation
  (preferably with a shorter name

(the above list is _not_ complete, there will always be a need for 
cleanups :D)


> The last patch adds the make file support to turn on and off the 
different GCs. The content of this patch has evolved from versions 
written by myself, Per, and Magnus Ihse Bursie. Magnus last proposed 
version used the names gc-cms, gc-g1, gc-parallel, and gc-serial, but 
I've changed them to cmsgc, g1gc, parallelgc, and serialgc, so that they 
match the INCLUDE_ defines.

>
> 
http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ 



My Makefile knowledge is limited to smaller hacks, I will leave this 
patch for Magnus to review (which I see he already has done).


Overall, very nice work Stefan, thanks!
Erik

Re: Windows: problem with msvxxx.dll locations containing spaces

[Thomas Stüfe]

Thank you Magnus, will do.

On Thu, May 3, 2018 at 12:41 PM, Magnus Ihse Bursie
 wrote:
> I think the main issue here is that BASIC_FIXUP_PATH should be called for
> the located msvcr*.dll files. I don't have time to look at it now, but see
> if you can do a rewrite using BASIC_FIXUP_PATH when the potential file is
> located. This should remove all spaces from the path.
>
> /Magnus
>
>
> On 2018-05-02 20:46, Thomas Stüfe wrote:
>>
>> Hi,
>>
>> My 32bit builds on Windows were failing since quite a while and I
>> finally had some minutes to look into that.
>>
>> See prior discussion here:
>> http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021150.html
>>
>> My output used to look like this:
>>
>> checking if fixpath.exe works... yes
>> POSSIBLE_MSVC_DLL /cygdrive/c/Program
>> POSSIBLE_MSVC_DLL Files
>> POSSIBLE_MSVC_DLL (x86)/Microsoft
>> POSSIBLE_MSVC_DLL Visual
>> POSSIBLE_MSVC_DLL Studio
>> POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
>> configure: Found msvcr120.dll at
>> /cygdrive/c/Windows/system32/msvcr120.dll using well-known location in
>> SYSTEMROOT
>>
>> So basically, build does not correctly search for msvcr120.dll in
>> "/cygdrive/c/Program Files (x86)/Microsoft Visual Studio
>> 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll" - instead, it
>> fails and falls back to the system default
>> "/cygdrive/c/Windows/system32/msvcr120.dll". That dll is a 64bit dll,
>> and so configure fails.
>>
>> Note that 64bit build shows exactly the same behaviour! Only there it
>> works by accident, since the default
>> /cygdrive/c/Windows/system32/msvcr120.dll it finds happens to be a
>> 64bit library too, so configure succeeds.
>>
>> Part of the problem is TOOLCHAIN_SETUP_MSVC_DLL in
>> toolchain_windows.m4. We use a bash for loop to iterate thru a list of
>> one or more files, but that for expression should be quoted.
>>
>> If I make this fix:
>>
>> --- a/make/autoconf/toolchain_windows.m4Mon Apr 23 18:04:17 2018
>> -0700
>> +++ b/make/autoconf/toolchain_windows.m4Wed May 02 18:38:04 2018
>> +0200
>> @@ -556,7 +556,7 @@
>>   fi
>> fi
>> # In case any of the above finds more than one file, loop over
>> them.
>> -  for possible_msvc_dll in $POSSIBLE_MSVC_DLL; do
>> +  for possible_msvc_dll in "$POSSIBLE_MSVC_DLL"; do
>>   $ECHO "POSSIBLE_MSVC_DLL $possible_msvc_dll"
>>   TOOLCHAIN_CHECK_POSSIBLE_MSVC_DLL([$DLL_NAME],
>> [$possible_msvc_dll],
>>   [well-known location in VCINSTALLDIR])
>>
>>
>> the 32bit configure correctly sets the msvcrt dll:
>>
>> POSSIBLE_MSVC_DLL /cygdrive/c/Program Files (x86)/Microsoft Visual
>> Studio 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll
>> configure: Found msvcr120.dll at /cygdrive/c/Program Files
>> (x86)/Microsoft Visual Studio
>> 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll using well-known
>> location in VCINSTALLDIR
>> checking found msvcr120.dll architecture... ok
>>
>> and I can start the build, but I get follow up errors:
>>
>> ...
>> Creating hotspot/variant-server/tools/adlc/adlc.exe from 13 file(s)
>> Compiling 2 files for BUILD_JVMTI_TOOLS
>> make[3]: *** No rule to make target '/cygdrive/c/Program', needed by
>>
>> '/cygdrive/c/mine/projects/openjdk/jdk-jdk/output-fastdebug-32/support/modules_libs/java.base/Program'.
>> Stop.
>> make[3]: *** Waiting for unfinished jobs
>> make[2]: *** [make/Main.gmk:165: java.base-copy] Error 2
>> make[2]: *** Waiting for unfinished jobs
>>
>> I stopped looking at that point, but to me it looks like the build
>> cannot survive msvcrt.dll locations with spaces in them.
>>
>> Kind Regards, Thomas
>
>

Re: [8u] RFR: 8196516: libfontmanager must be built with LDFLAGS allowing unresolved symbols

[Magnus Ihse Bursie]

On 2018-05-03 13:33, Severin Gehwolf wrote:

Hi,

Could I please get a review of this change for JDK 8u? We are seing
build failures due to unresolved symbols when building
libfontmanager.so. The build flag to trigger this is to configure with:

--with-extra-ldflags="-Wl,-z,defs"

Attempting a build of JDK 8 with this fails. This has been fixed in JDK
11 and hasn't shown any issues so far. Due to the change in the build
system the JDK 11 patch does not apply cleanly after path unshuffeling
(different context it seems). Hence, asking here for a review before
asking for JDK 8 backport approval.

Bug: https://bugs.openjdk.java.net/browse/JDK-8196516
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8196516/webrev.jdk8/

Looks good to me.

/Magnus


Review thread for JDK 11:
http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021575.html

Thanks,
Severin

Re: License and Usage Terms of generated API documentation

[Volker Simonis]

On Thu, May 3, 2018 at 12:21 PM, Magnus Ihse Bursie
 wrote:
> On 2018-05-02 17:03, Volker Simonis wrote:
>>
>> Hi,
>>
>> we currently build OpenJDK and make it available from various sources
>> (e.g. GitHub, apt-get server, DockerHub). We also build the API
>> documentation (i.e. JavaDoc) and would like to make it available from
>> our project page as well. However the default API doc produced by the
>> build looks as follows:
>>
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/jdk10-api-doc/overview-summary.html
>>
>> Especially the footer seems to be weired and only valid for the API
>> doc hosted by Oracle itself. It reads as follows:
>>
>> Use is subject to "license terms" and the "documentation redistribution
>> policy".
>>
>> "license terms" links to
>>
>> http://www.oracle.com/technetwork/java/javase/terms/license/java10speclicense.html
>> which redirects to
>> http://download.oracle.com/otndocs/jcp/java_se-10-final-spec/license.html
>>
>> "documentation redistribution policy" links to
>> http://www.oracle.com/technetwork/java/redist-137594.html
>>
>> Especially the "documentation redistribution policy" is very strict. It
>> states:
>>
>> "The Java SE API Specification is not redistributable."
>>
>> This is a very strong statement. While it may be fine for the API
>> documentation hosted by Oracle (under
>> https://docs.oracle.com/javase/10/docs/api/overview-summary.html) I
>> doubt this can be valid for the OpenJDK API documentation which was
>> produced exclusively from GPLv2 licensed sources (actually even GPLv2
>> plus Classpath Exception). From my understanding the whole HTML tree
>> generated by "make docs-image" should be by default licensed under
>> GPLv2 as well.
>>
>> I would therefore like to propose to make the following variables from
>> "make/Docs.gmk" configurable with corresponding configure flags:
>>
>> JAVADOC_BASE_URL :=
>> http://www.oracle.com/pls/topic/lookup?ctx=javase10id=homepage
>> BUG_SUBMIT_URL := http://bugreport.java.com/bugreport/
>> COPYRIGHT_URL := {@docroot}/../legal/copyright.html
>> LICENSE_URL :=
>> http://www.oracle.com/technetwork/java/javase/terms/license/java10speclicense.html
>> REDISTRIBUTION_URL :=
>> http://www.oracle.com/technetwork/java/redist-137594.html
>>
>> "JAVADOC_BASE_URL" should by default point to an OpenJDK site
>> (although I'm not sure which one will be best suited). It seems
>> strange that the default documentation generated from an OpenSource
>> project like OpenJDK points to some Oracle-proprietary documentation.
>>
>> "BUG_SUBMIT_URL" should use the value of the already existing
>> "--with-vendor-bug-url" if that was set at configure time.
>>
>> "COPYRIGHT_URL" currently points to "copyright.html" which doesn't
>> exist neither in the OpenJDK sources nor in the generated images. Not
>> sure what would be a useful default value here. Maybe just leave it
>> empty? "Copyright © 1993, 2018, Oracle.." already seems
>> self-explanatory and clear enough.
>>
>> "LICENSE_URL" and "REDISTRIBUTION_URL" should both by default point to
>> the GPLv2+CPE LICENSE file and this LICENSE file should be copied into
>> the API doc HTML tree (much like it is copied into the various
>> subdirectories of the "legel/" directory of an OpenJDK image)
>>
>> I can open an issue for this topic and propose an implementation if
>> there's consensus on this topic.
>>
>> Thank you and best regards,
>> Volker
>
> Since you added build-dev, I'll chip in some of my cents.
>
> The code in the make file has been around like Forever. The current
> implementation is just what has been ported between frameworks and source
> control systems and updated for changes in release version and URL schemes.
>

Thanks for confirming this. That was my impression as well :)

> I agree that is odd that the Oracle URL is hard-coded. On the other hand,
> there is no common place where generated OpenJDK documents are published.
> (The Oracle site technically speaking documents the Java Platform API, not
> the OpenJDK, even if any actual difference is minimal to non-existant wrt
> docs.)
>
> It would make sense to me to have a OpenJDK-based documentation driven by
> the community. I'm guessing it's no easy thing to create this, though. A lot
> of links on the interwebz are pointing to the docs at docs.oracle.com, and
> it would probably be quite messy if the same (or even worse, almost the
> same) documentation were published both at docs.oracle.com and java.net.
> Also, the question arises as to who should do the hosting, and how.
>
> So it's definitely a community issue to resolve, and one in which Oracle's
> legacy is important to take into consideration.
>
> Until a consensus of a better solution for hosting the generated
> documentation is reached, I'd like to avoid changing the build code. That
> will just open for an uneccessary split in what constitutes the proper
> documentation URL.
>

The JAVADOC_BASE_URL is the minor issue here and I agree that 

[8u] RFR: 8196516: libfontmanager must be built with LDFLAGS allowing unresolved symbols

[Severin Gehwolf]

Hi,

Could I please get a review of this change for JDK 8u? We are seing
build failures due to unresolved symbols when building
libfontmanager.so. The build flag to trigger this is to configure with:

--with-extra-ldflags="-Wl,-z,defs"

Attempting a build of JDK 8 with this fails. This has been fixed in JDK
11 and hasn't shown any issues so far. Due to the change in the build
system the JDK 11 patch does not apply cleanly after path unshuffeling
(different context it seems). Hence, asking here for a review before
asking for JDK 8 backport approval.

Bug: https://bugs.openjdk.java.net/browse/JDK-8196516
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8196516/webrev.jdk8/

Review thread for JDK 11:
http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021575.html

Thanks,
Severin

Re: Windows: problem with msvxxx.dll locations containing spaces

[Magnus Ihse Bursie]

I think the main issue here is that BASIC_FIXUP_PATH should be called 
for the located msvcr*.dll files. I don't have time to look at it now, 
but see if you can do a rewrite using BASIC_FIXUP_PATH when the 
potential file is located. This should remove all spaces from the path.


/Magnus

On 2018-05-02 20:46, Thomas Stüfe wrote:

Hi,

My 32bit builds on Windows were failing since quite a while and I
finally had some minutes to look into that.

See prior discussion here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021150.html

My output used to look like this:

checking if fixpath.exe works... yes
POSSIBLE_MSVC_DLL /cygdrive/c/Program
POSSIBLE_MSVC_DLL Files
POSSIBLE_MSVC_DLL (x86)/Microsoft
POSSIBLE_MSVC_DLL Visual
POSSIBLE_MSVC_DLL Studio
POSSIBLE_MSVC_DLL 12.0/VC/redist/x64/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at
/cygdrive/c/Windows/system32/msvcr120.dll using well-known location in
SYSTEMROOT

So basically, build does not correctly search for msvcr120.dll in
"/cygdrive/c/Program Files (x86)/Microsoft Visual Studio
12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll" - instead, it
fails and falls back to the system default
"/cygdrive/c/Windows/system32/msvcr120.dll". That dll is a 64bit dll,
and so configure fails.

Note that 64bit build shows exactly the same behaviour! Only there it
works by accident, since the default
/cygdrive/c/Windows/system32/msvcr120.dll it finds happens to be a
64bit library too, so configure succeeds.

Part of the problem is TOOLCHAIN_SETUP_MSVC_DLL in
toolchain_windows.m4. We use a bash for loop to iterate thru a list of
one or more files, but that for expression should be quoted.

If I make this fix:

--- a/make/autoconf/toolchain_windows.m4Mon Apr 23 18:04:17 2018 -0700
+++ b/make/autoconf/toolchain_windows.m4Wed May 02 18:38:04 2018 +0200
@@ -556,7 +556,7 @@
  fi
fi
# In case any of the above finds more than one file, loop over them.
-  for possible_msvc_dll in $POSSIBLE_MSVC_DLL; do
+  for possible_msvc_dll in "$POSSIBLE_MSVC_DLL"; do
  $ECHO "POSSIBLE_MSVC_DLL $possible_msvc_dll"
  TOOLCHAIN_CHECK_POSSIBLE_MSVC_DLL([$DLL_NAME], [$possible_msvc_dll],
  [well-known location in VCINSTALLDIR])


the 32bit configure correctly sets the msvcrt dll:

POSSIBLE_MSVC_DLL /cygdrive/c/Program Files (x86)/Microsoft Visual
Studio 12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll
configure: Found msvcr120.dll at /cygdrive/c/Program Files
(x86)/Microsoft Visual Studio
12.0/VC/redist/x86/Microsoft.VC120.CRT/msvcr120.dll using well-known
location in VCINSTALLDIR
checking found msvcr120.dll architecture... ok

and I can start the build, but I get follow up errors:

...
Creating hotspot/variant-server/tools/adlc/adlc.exe from 13 file(s)
Compiling 2 files for BUILD_JVMTI_TOOLS
make[3]: *** No rule to make target '/cygdrive/c/Program', needed by
'/cygdrive/c/mine/projects/openjdk/jdk-jdk/output-fastdebug-32/support/modules_libs/java.base/Program'.
Stop.
make[3]: *** Waiting for unfinished jobs
make[2]: *** [make/Main.gmk:165: java.base-copy] Error 2
make[2]: *** Waiting for unfinished jobs

I stopped looking at that point, but to me it looks like the build
cannot survive msvcrt.dll locations with spaces in them.

Kind Regards, Thomas

Re: License and Usage Terms of generated API documentation

[Magnus Ihse Bursie]

On 2018-05-02 17:03, Volker Simonis wrote:

Hi,

we currently build OpenJDK and make it available from various sources
(e.g. GitHub, apt-get server, DockerHub). We also build the API
documentation (i.e. JavaDoc) and would like to make it available from
our project page as well. However the default API doc produced by the
build looks as follows:

http://cr.openjdk.java.net/~simonis/webrevs/2018/jdk10-api-doc/overview-summary.html

Especially the footer seems to be weired and only valid for the API
doc hosted by Oracle itself. It reads as follows:

Use is subject to "license terms" and the "documentation redistribution policy".

"license terms" links to
http://www.oracle.com/technetwork/java/javase/terms/license/java10speclicense.html
which redirects to
http://download.oracle.com/otndocs/jcp/java_se-10-final-spec/license.html

"documentation redistribution policy" links to
http://www.oracle.com/technetwork/java/redist-137594.html

Especially the "documentation redistribution policy" is very strict. It states:

"The Java SE API Specification is not redistributable."

This is a very strong statement. While it may be fine for the API
documentation hosted by Oracle (under
https://docs.oracle.com/javase/10/docs/api/overview-summary.html) I
doubt this can be valid for the OpenJDK API documentation which was
produced exclusively from GPLv2 licensed sources (actually even GPLv2
plus Classpath Exception). From my understanding the whole HTML tree
generated by "make docs-image" should be by default licensed under
GPLv2 as well.

I would therefore like to propose to make the following variables from
"make/Docs.gmk" configurable with corresponding configure flags:

JAVADOC_BASE_URL :=
http://www.oracle.com/pls/topic/lookup?ctx=javase10id=homepage
BUG_SUBMIT_URL := http://bugreport.java.com/bugreport/
COPYRIGHT_URL := {@docroot}/../legal/copyright.html
LICENSE_URL := 
http://www.oracle.com/technetwork/java/javase/terms/license/java10speclicense.html
REDISTRIBUTION_URL := http://www.oracle.com/technetwork/java/redist-137594.html

"JAVADOC_BASE_URL" should by default point to an OpenJDK site
(although I'm not sure which one will be best suited). It seems
strange that the default documentation generated from an OpenSource
project like OpenJDK points to some Oracle-proprietary documentation.

"BUG_SUBMIT_URL" should use the value of the already existing
"--with-vendor-bug-url" if that was set at configure time.

"COPYRIGHT_URL" currently points to "copyright.html" which doesn't
exist neither in the OpenJDK sources nor in the generated images. Not
sure what would be a useful default value here. Maybe just leave it
empty? "Copyright © 1993, 2018, Oracle.." already seems
self-explanatory and clear enough.

"LICENSE_URL" and "REDISTRIBUTION_URL" should both by default point to
the GPLv2+CPE LICENSE file and this LICENSE file should be copied into
the API doc HTML tree (much like it is copied into the various
subdirectories of the "legel/" directory of an OpenJDK image)

I can open an issue for this topic and propose an implementation if
there's consensus on this topic.

Thank you and best regards,
Volker

Since you added build-dev, I'll chip in some of my cents.

The code in the make file has been around like Forever. The current 
implementation is just what has been ported between frameworks and 
source control systems and updated for changes in release version and 
URL schemes.


I agree that is odd that the Oracle URL is hard-coded. On the other 
hand, there is no common place where generated OpenJDK documents are 
published. (The Oracle site technically speaking documents the Java 
Platform API, not the OpenJDK, even if any actual difference is minimal 
to non-existant wrt docs.)


It would make sense to me to have a OpenJDK-based documentation driven 
by the community. I'm guessing it's no easy thing to create this, 
though. A lot of links on the interwebz are pointing to the docs at 
docs.oracle.com, and it would probably be quite messy if the same (or 
even worse, almost the same) documentation were published both at 
docs.oracle.com and java.net. Also, the question arises as to who should 
do the hosting, and how.


So it's definitely a community issue to resolve, and one in which 
Oracle's legacy is important to take into consideration.


Until a consensus of a better solution for hosting the generated 
documentation is reached, I'd like to avoid changing the build code. 
That will just open for an uneccessary split in what constitutes the 
proper documentation URL.


/Magnus

Re: RFR: 8200729: Conditional compilation of GCs

[Stefan Karlsson]

On 2018-05-03 11:06, Magnus Ihse Bursie wrote:

On 2018-05-02 16:37, Stefan Karlsson wrote:

Hi all,

Please review these patches to allow for conditional compilation of 
the GCs in HotSpot.


Full patch:

http://cr.openjdk.java.net/~stefank/8200729/webrev.04/all/


It's nice to see this cleanup in build logic!

I spotted one issue with the build logic, otherwise it looks good. In 
JvmFeatures.gmk, remove the  "JVM_EXCLUDE_FILES += none" lines. They are 
not needed and are technically incorrect (makes the build tries to 
exclude files named "none").



Thanks for reviewing this, and helping out writing the make files 
changes! I'll remove the += none lines.


StefanK



/Magnus



(See below for a more fine-grained division into smaller patches)

Today Parallel, G1, and CMS, are all guarded by INCLUDE_ALL_GCS. 
INCLUDE_ALL_GCS becomes defined to 1 for our server 
(--with-jvm-variants=server) builds, and is defined to 0 for the 
minimal (--with-jvm-variants=minimal) builds. There are also ways to 
forcefully remove these GCs from the compilation by configuring with, 
for example, --with-jvm-features=all-gcs.


The proposed patch removes INCLUDE_ALL_GCS (and all-gcs) and replaces 
it with INCLUDE_CMSGC, INCLUDE_G1GC, and INCLUDE_PARALLELGC. In 
addition to that, INCLUDE_SERIALGC has been added to guard the Serial 
GC code.


Future GCs should adopt this scheme before they get incorporated into 
the code base. Note, that there will be some files in gc/shared that 
are going to have to know about all GCs, but the goal is to have very 
few of these INCLUDE_ checks in the non-GC parts of HotSpot.


With this patch, it's also going to be easier to stop compiling CMS 
when the time as come for it to move from deprecated to removed.


Note, that even though this adds great flexibility, and allows for 
easy inclusion / exclusion of GCs, there's no guarantee that a 
specific combination of GCs is going to be maintained in the future. 
Just like not all combinations of the different Runtime features (CDS, 
NMT, JFR) and Compiler features (AOT, COMPILER1, COMPILER2) are 
guaranteed to compile and be supported. I've sanity checked the 
different GC configurations build locally, but I've only fully tested 
the "server" variant, and "minimal" has only been built locally.


There's a more high-level discussion around the flexibility the 
--with-jvm-features flag adds, and if we really should allow it. Since 
this patch only builds upon that existing flexibility, and I don't 
change the two defined jvm variants, I'd appreciate if that discussion 
could be kept out of this review thread. For further discussion around 
this, please see:


http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021663.html

This is the patch queue:

The first patch simply cleans up some INCLUDE_ALL_GCS usages in 
platform-specific files. Some of these changes are already being 
cleaned up by other RFEs:


http://cr.openjdk.java.net/~stefank/8200729/webrev.04/00.removeUnneededIncludeAllGCs/ 




The second patch pre-cleans some include files:

http://cr.openjdk.java.net/~stefank/8200729/webrev.04/01.fixIncludes/


The following is the main patch, which include all relevant HotSpot 
changes. For a while now, we have been actively working on abstracting 
away GC specific code from non-GC directories, but as can be seen in 
this patch, there are still a few places left. Hopefully, we will get 
rid of most of these in the near future.


http://cr.openjdk.java.net/~stefank/8200729/webrev.04/02.mainPatch/


The last patch adds the make file support to turn on and off the 
different GCs. The content of this patch has evolved from versions 
written by myself, Per, and Magnus Ihse Bursie. Magnus last proposed 
version used the names gc-cms, gc-g1, gc-parallel, and gc-serial, but 
I've changed them to cmsgc, g1gc, parallelgc, and serialgc, so that 
they match the INCLUDE_ defines.


http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ 



Thanks,
StefanK

Re: RFR: 8200729: Conditional compilation of GCs

[Magnus Ihse Bursie]

On 2018-05-02 16:37, Stefan Karlsson wrote:

Hi all,

Please review these patches to allow for conditional compilation of 
the GCs in HotSpot.


Full patch:

http://cr.openjdk.java.net/~stefank/8200729/webrev.04/all/


It's nice to see this cleanup in build logic!

I spotted one issue with the build logic, otherwise it looks good. In 
JvmFeatures.gmk, remove the  "JVM_EXCLUDE_FILES += none" lines. They are 
not needed and are technically incorrect (makes the build tries to 
exclude files named "none").


/Magnus



(See below for a more fine-grained division into smaller patches)

Today Parallel, G1, and CMS, are all guarded by INCLUDE_ALL_GCS. 
INCLUDE_ALL_GCS becomes defined to 1 for our server 
(--with-jvm-variants=server) builds, and is defined to 0 for the 
minimal (--with-jvm-variants=minimal) builds. There are also ways to 
forcefully remove these GCs from the compilation by configuring with, 
for example, --with-jvm-features=all-gcs.


The proposed patch removes INCLUDE_ALL_GCS (and all-gcs) and replaces 
it with INCLUDE_CMSGC, INCLUDE_G1GC, and INCLUDE_PARALLELGC. In 
addition to that, INCLUDE_SERIALGC has been added to guard the Serial 
GC code.


Future GCs should adopt this scheme before they get incorporated into 
the code base. Note, that there will be some files in gc/shared that 
are going to have to know about all GCs, but the goal is to have very 
few of these INCLUDE_ checks in the non-GC parts of HotSpot.


With this patch, it's also going to be easier to stop compiling CMS 
when the time as come for it to move from deprecated to removed.


Note, that even though this adds great flexibility, and allows for 
easy inclusion / exclusion of GCs, there's no guarantee that a 
specific combination of GCs is going to be maintained in the future. 
Just like not all combinations of the different Runtime features (CDS, 
NMT, JFR) and Compiler features (AOT, COMPILER1, COMPILER2) are 
guaranteed to compile and be supported. I've sanity checked the 
different GC configurations build locally, but I've only fully tested 
the "server" variant, and "minimal" has only been built locally.


There's a more high-level discussion around the flexibility the 
--with-jvm-features flag adds, and if we really should allow it. Since 
this patch only builds upon that existing flexibility, and I don't 
change the two defined jvm variants, I'd appreciate if that discussion 
could be kept out of this review thread. For further discussion around 
this, please see:


http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021663.html

This is the patch queue:

The first patch simply cleans up some INCLUDE_ALL_GCS usages in 
platform-specific files. Some of these changes are already being 
cleaned up by other RFEs:


http://cr.openjdk.java.net/~stefank/8200729/webrev.04/00.removeUnneededIncludeAllGCs/ 




The second patch pre-cleans some include files:

http://cr.openjdk.java.net/~stefank/8200729/webrev.04/01.fixIncludes/


The following is the main patch, which include all relevant HotSpot 
changes. For a while now, we have been actively working on abstracting 
away GC specific code from non-GC directories, but as can be seen in 
this patch, there are still a few places left. Hopefully, we will get 
rid of most of these in the near future.


http://cr.openjdk.java.net/~stefank/8200729/webrev.04/02.mainPatch/


The last patch adds the make file support to turn on and off the 
different GCs. The content of this patch has evolved from versions 
written by myself, Per, and Magnus Ihse Bursie. Magnus last proposed 
version used the names gc-cms, gc-g1, gc-parallel, and gc-serial, but 
I've changed them to cmsgc, g1gc, parallelgc, and serialgc, so that 
they match the INCLUDE_ defines.


http://cr.openjdk.java.net/~stefank/8200729/webrev.04/03.selectIndivudualGCsMakePatch/ 



Thanks,
StefanK

Re: [11] RFR(S) 8202552: [AOT][JVMCI] Incorrect usage of INCLUDE_JVMCI and INCLUDE_AOT

[Magnus Ihse Bursie]

On 2018-05-03 00:13, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8202552/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8202552


Looks good to me.

Just some thinking out loud... The way we handle the responsibility 
split between the makefiles and the hotspot source files is actually a 
bit corny. :-( It would make more sense for the makefiles to set 
-DINCLUDE_features=1 when the feature is enabled, and 
-DINCLUDE_feature=0 when it is not (or just leave it out), and skip all 
the #ifndef INCLUDE_ #define INCLUDE_ 1 ...in macros.hpp.


But that is the work of a future cleanup. This patch makes sure we use 
the same pattern everywhere, which is good.


/Magnus




Stefan K. found several places where #ifdef instead of #if is used for 
INCLUDE_JVMCI.

I also found places where we can use COMPILER2_OR_JVMCI.

An other problem surprised me that we don't set INCLUDE_AOT to 1. 
Makefile defines that variable without value. I changed code to match 
other variables setting - in makefile set it to 0 if it is not part of 
build and set it to 1 in macros.hpp if it is not defined.


Tested with tier1, tier2 and tier2 with Graal as JIT compiler.

Re: [11] RFR(S) 8202552: [AOT][JVMCI] Incorrect usage of INCLUDE_JVMCI and INCLUDE_AOT

[Stefan Karlsson]

Looks good to me. Thanks for fixing.

StefanK

On 2018-05-03 00:13, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8202552/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8202552

Stefan K. found several places where #ifdef instead of #if is used for 
INCLUDE_JVMCI.

I also found places where we can use COMPILER2_OR_JVMCI.

An other problem surprised me that we don't set INCLUDE_AOT to 1. 
Makefile defines that variable without value. I changed code to match 
other variables setting - in makefile set it to 0 if it is not part of 
build and set it to 1 in macros.hpp if it is not defined.


Tested with tier1, tier2 and tier2 with Graal as JIT compiler.