Re: RFR: 8287745 jni/nullCaller/NullCallerTest.java fails with "exitValue = 1"

[Mandy Chung]

On Fri, 3 Jun 2022 07:56:38 GMT, Tim Prinzing  wrote:

> Fixed JtregNativeJdk.gmk to include c++ libs for NullCallerTest

There are a few C++ tests under `test/jdk/java/foreign` as well.

-

PR: https://git.openjdk.java.net/jdk/pull/9010

Re: RFR: 8287745 jni/nullCaller/NullCallerTest.java fails with "exitValue = 1"

[Mandy Chung]

On Fri, 3 Jun 2022 22:40:52 GMT, Tim Prinzing  wrote:

>> make/test/JtregNativeJdk.gmk line 67:
>> 
>>> 65:   BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeNullCallerTest := $(LIBCXX) 
>>> jvm.lib
>>> 66:   BUILD_JDK_JTREG_EXECUTABLES_LIBS_exerevokeall := advapi32.lib
>>> 67:   BUILD_JDK_JTREG_EXECUTABLES_CFLAGS_exeNullCallerTest := /EHsc
>> 
>> Does this test need this flag?  or the default exception handling behavior 
>> is adequate?
>
> iostreams don't compile without it.  While the tests currently have printf, 
> the 8281001 restores back to using iostream and this file doesn't have to 
> change.

Ok, thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/9010

Re: RFR: 8287745 jni/nullCaller/NullCallerTest.java fails with "exitValue = 1"

[Mandy Chung]

On Fri, 3 Jun 2022 07:56:38 GMT, Tim Prinzing  wrote:

> Fixed JtregNativeJdk.gmk to include c++ libs for NullCallerTest

Thanks for doing the run, Dan.

-

PR: https://git.openjdk.java.net/jdk/pull/9010

Re: RFR: 8287745 jni/nullCaller/NullCallerTest.java fails with "exitValue = 1"

[Mandy Chung]

On Fri, 3 Jun 2022 07:56:38 GMT, Tim Prinzing  wrote:

> Fixed JtregNativeJdk.gmk to include c++ libs for NullCallerTest

Marked as reviewed by mchung (Reviewer).

make/test/JtregNativeJdk.gmk line 67:

> 65:   BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeNullCallerTest := $(LIBCXX) jvm.lib
> 66:   BUILD_JDK_JTREG_EXECUTABLES_LIBS_exerevokeall := advapi32.lib
> 67:   BUILD_JDK_JTREG_EXECUTABLES_CFLAGS_exeNullCallerTest := /EHsc

Does this test need this flag?  or the default exception handling behavior is 
adequate?

-

PR: https://git.openjdk.java.net/jdk/pull/9010

Re: RFR: 8287171: Refactor null caller tests to a single directory [v8]

[Mandy Chung]

On Wed, 1 Jun 2022 19:27:33 GMT, Tim Prinzing  wrote:

>> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates 
>> a test module with some resources in it for the actual tests that occur at 
>> the native level. The native part was switched to c++ instead of c to make 
>> it easier to create helper objects that reduce the redundant code performing 
>> error checking. The two helper classes InstanceCall and StaticCall were 
>> placed in an include file called CallHelper.hpp. The main part of the tests 
>> are in exeNullCallerTest.cpp, and there is a separate function for each of 
>> the bug reports.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   comment changes

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8934

Re: RFR: 8287171: Refactor null caller tests to a single directory [v6]

[Mandy Chung]

On Wed, 1 Jun 2022 03:01:35 GMT, Tim Prinzing  wrote:

>> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates 
>> a test module with some resources in it for the actual tests that occur at 
>> the native level. The native part was switched to c++ instead of c to make 
>> it easier to create helper objects that reduce the redundant code performing 
>> error checking. The two helper classes InstanceCall and StaticCall were 
>> placed in an include file called CallHelper.hpp. The main part of the tests 
>> are in exeNullCallerTest.cpp, and there is a separate function for each of 
>> the bug reports.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   move jni/nullCaller tests to jdk_lang group

test/jdk/jni/nullCaller/exeNullCallerTest.cpp line 27:

> 25: 
> 26: /* Test for JDK-8280902
> 27:  * ResourceBundle::getBundle may throw NPE when invoked by JNI code with 
> no caller frame

This comment isn't correct.  `ResourceBundle::getBundle` defaults to the system 
class loader if invoked by a null caller and I don't expect it may throw NPE.   
Looks like you copy the summary of the JBS issue here, which isn't helpful to 
the readers.I expect it a description of the test case.

Same comments to all other test cases.

-

PR: https://git.openjdk.java.net/jdk/pull/8934

Re: RFR: 8287171: Refactor null caller tests to a single directory [v4]

[Mandy Chung]

On Wed, 1 Jun 2022 00:59:33 GMT, Tim Prinzing  wrote:

>> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates 
>> a test module with some resources in it for the actual tests that occur at 
>> the native level. The native part was switched to c++ instead of c to make 
>> it easier to create helper objects that reduce the redundant code performing 
>> error checking. The two helper classes InstanceCall and StaticCall were 
>> placed in an include file called CallHelper.hpp. The main part of the tests 
>> are in exeNullCallerTest.cpp, and there is a separate function for each of 
>> the bug reports.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Moved the tests test/jdk/jni/nullCaller.  Added to the jdk_other group
>   which runs as part of tier2.  Made the other requested changes such as
>   not using the bugid as the function name for the test and using the
>   name of the main method being tested instead.

jdk_other is for other modules.  jdk_lang may be an option  since this is to 
test when attached with JNI thread.

-

PR: https://git.openjdk.java.net/jdk/pull/8934

Re: RFR: 8287171: Refactor null caller tests to a single directory [v3]

[Mandy Chung]

On Mon, 30 May 2022 05:37:16 GMT, Alan Bateman  wrote:

> I don't think jdk/nullCaller is right location for this. Maybe jni/nullCaller 
> could work. You'll probably need to add the location to an appropriate 
> group/tier in test/jdk/TEST.groups, otherwise it won't be run.

I also think `test/jdk/jni/nullCaller` is better.

-

PR: https://git.openjdk.java.net/jdk/pull/8934

Re: RFR: 8287171: Refactor null caller tests to a single directory [v3]

[Mandy Chung]

On Mon, 30 May 2022 00:10:50 GMT, Tim Prinzing  wrote:

>> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates 
>> a test module with some resources in it for the actual tests that occur at 
>> the native level. The native part was switched to c++ instead of c to make 
>> it easier to create helper objects that reduce the redundant code performing 
>> error checking. The two helper classes InstanceCall and StaticCall were 
>> placed in an include file called CallHelper.hpp. The main part of the tests 
>> are in exeNullCallerTest.cpp, and there is a separate function for each of 
>> the bug reports.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   problem with iostream on Windows, use printf

test/jdk/jdk/nullCaller/CallHelper.hpp line 176:

> 174: }
> 175: 
> 176: // call an method returning an object checking for exceptions and

`s/an method/a method/`

test/jdk/jdk/nullCaller/exeNullCallerTest.cpp line 29:

> 27:  * ResourceBundle::getBundle may throw NPE when invoked by JNI code with 
> no caller frame
> 28:  */
> 29: void JDK_8280902(JNIEnv* env) {

A descriptive method name would be more helpful than the bug ID, for example, 
`getResourceBundle` and `registerClassLoaderAsParallelCapable`

test/jdk/jdk/nullCaller/exeNullCallerTest.cpp line 49:

> 47: 
> 48: // check the message
> 49: if (std::string("Hello!") != env->GetStringUTFChars(msg,NULL)) {

nit: a space after the comma `(msg, NULL)` consistent with the format in this 
local file.

-

PR: https://git.openjdk.java.net/jdk/pull/8934

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v41]

[Mandy Chung]

On Fri, 6 May 2022 16:48:11 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java
>   
>   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7888

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]

[Mandy Chung]

On Thu, 5 May 2022 20:54:53 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI
>   * Tweak restricted method check to work when there's no caller class

src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 161:

> 159: ClassLoader.getSystemClassLoader();
> 160: MemorySession loaderSession = (loader == null) ?
> 161: MemorySession.global() : // boot loader never goes away

The other built-in class loaders (`ClassLoaders::appClassLoader` and 
`ClassLoaders::platformClassLoader` are both instance of `BuiltinClassLoader`) 
will never be unloaded.   Should they use the globel memory session?

src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 120:

> 118: // if there is no caller class, act as if the call came from an 
> unnamed module
> 119: Module module = currentClass != null ?
> 120: currentClass.getModule() : Holder.FALLBACK_MODULE;

This can be simplified to:

Module module = currentClass != null ?
   currentClass.getModule() : 
ClassLoader::getSystemClassLoader().getUnnamedModule();


No need to have the Holder class.

-

PR: https://git.openjdk.java.net/jdk/pull/7888

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]

[Mandy Chung]

On Thu, 5 May 2022 16:22:41 GMT, Mandy Chung  wrote:

>> Looking deeper, `System::loadLibrary` seems to search the boot loader 
>> libraries when invoked with a `null` caller class, so replicating that 
>> behavior should be safe.
>
> `BootLoader` is what you want in this case - it defines the static methods to 
> access resources, packages etc defined to the boot loader (aka null 
> `ClassLoader`).
> 
> To find a symbol defined in the native libraries loaded by the boot loader, 
> you can call `BootLoader.getNativeLibraries().find(name)`.

When a caller-sensitive method is invoked from a thread attached via JNI, the 
caller class returned from `Reflection::getCallerClass` is `null`.I would 
recommend the default to be a class in the unnamed module defined to the system 
class loader; hence it defaults to the system class loader.  This is consistent 
with JNI `FindClass` when invoked with no caller frame.

It's an open issue [1] to revisit the default for `System::load` and 
`System::loadLibrary` when invoked with null caller class.   However, the 
existing behavior may likely be unchanged because of the compatibility risk.

[1] https://bugs.openjdk.java.net/browse/JDK-8281001

-

PR: https://git.openjdk.java.net/jdk/pull/7888

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]

[Mandy Chung]

On Wed, 4 May 2022 23:44:08 GMT, Maurizio Cimadamore  
wrote:

>> Another option would be to treat calls to `ensureNativeAccess` with `null` 
>> caller class as coming from unnamed module.
>
> Looking deeper, `System::loadLibrary` seems to search the boot loader 
> libraries when invoked with a `null` caller class, so replicating that 
> behavior should be safe.

`BootLoader` is what you want in this case - it defines the static methods to 
access resources, packages etc defined to the boot loader (aka null 
`ClassLoader`).

To find a symbol defined in the native libraries loaded by the boot loader, you 
can call `BootLoader.getNativeLibraries().find(name)`.

-

PR: https://git.openjdk.java.net/jdk/pull/7888

Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v5]

[Mandy Chung]

On Wed, 20 Apr 2022 01:03:23 GMT, Tim Prinzing  wrote:

>> Created a test called NullCallerGetResource to test 
>> Module::getResourceAsStream and Class::getResourceAsStream from the native 
>> level.
>> 
>> At the java level the test builds a test module called 'n' which opens the 
>> package 'open' to everyone. There is also a package 'closed' which is 
>> neither opened or exported. Both packages have a text file called 
>> 'test.txt'. The open package has a class called OpenResources and the closed 
>> package has a class called ClosedResources. The native test is launched with 
>> the test module n added.
>> 
>> At the native level the test tries to read both the open and closed resource 
>> from both the classes and the module. It performs the equivalent of the 
>> following java operations:
>> 
>> Class c = open.OpenResources.fetchClass();
>> InputStream in = c.getResourceAsStream("test.txt");
>> assert(in != null); in.close();
>> 
>> Module n = c.getModule();
>> in = n.getResourceAsStream("open/test.txt");
>> assert(in != null); in.close();
>> 
>> Class closed = closed.ClosedResources.fetchClass();
>> assert(closedsStream("test.txt") == null);
>> assert(n.getResourceAsStream("closed/test.txt") == null);
>> 
>> The test initially threw an NPE when trying to fetch the open resource. The 
>> Module class was fixed by removing the fragment with the (caller == null) 
>> test in getResourceAsStream, and changing the call to isOpen(String,Module) 
>> to use EVERYONE_MODULE if the caller module is null.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   some cleanup of the test

Thanks for the test update.  Looks good.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8134

Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v4]

[Mandy Chung]

On Mon, 11 Apr 2022 00:48:34 GMT, Tim Prinzing  wrote:

>> Created a test called NullCallerGetResource to test 
>> Module::getResourceAsStream and Class::getResourceAsStream from the native 
>> level.
>> 
>> At the java level the test builds a test module called 'n' which opens the 
>> package 'open' to everyone. There is also a package 'closed' which is 
>> neither opened or exported. Both packages have a text file called 
>> 'test.txt'. The open package has a class called OpenResources and the closed 
>> package has a class called ClosedResources. The native test is launched with 
>> the test module n added.
>> 
>> At the native level the test tries to read both the open and closed resource 
>> from both the classes and the module. It performs the equivalent of the 
>> following java operations:
>> 
>> Class c = open.OpenResources.fetchClass();
>> InputStream in = c.getResourceAsStream("test.txt");
>> assert(in != null); in.close();
>> 
>> Module n = c.getModule();
>> in = n.getResourceAsStream("open/test.txt");
>> assert(in != null); in.close();
>> 
>> Class closed = closed.ClosedResources.fetchClass();
>> assert(closedsStream("test.txt") == null);
>> assert(n.getResourceAsStream("closed/test.txt") == null);
>> 
>> The test initially threw an NPE when trying to fetch the open resource. The 
>> Module class was fixed by removing the fragment with the (caller == null) 
>> test in getResourceAsStream, and changing the call to isOpen(String,Module) 
>> to use EVERYONE_MODULE if the caller module is null.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   requested changes

Marked as reviewed by mchung (Reviewer).

test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c 
line 95:

> 93: jclass class_OpenResources = (*env)->FindClass(env, 
> "open/OpenResources");
> 94: assert(class_OpenResources != NULL);
> 95: jmethodID mid_OpenResources_fetchClass = 
> (*env)->GetStaticMethodID(env, class_OpenResources, "fetchClass", 
> "()Ljava/lang/Class;" );

It seems that invoking `fetchClass` isn't necessary since you can simply use 
`class_OpenResources`.  Similarly for `class_ClosedResources`

test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c 
line 183:

> 181: exit(-1);
> 182: }
> 183: assert(in == NULL);

assert is typically used for sanity test.   As part of the test validation, 
e.g. in this case `in == NULL` or `in != NULL` in line 157,  it may be clearer 
if it's an explicit check and throw exception to indicate test failure 
especially in case `#undef NDEBUG` may not be set in the test.

-

PR: https://git.openjdk.java.net/jdk/pull/8134

Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v2]

[Mandy Chung]

On Thu, 14 Apr 2022 13:59:57 GMT, Claes Redestad  wrote:

>> This patch examines and optimizes `Wrapper` lookups.
>> 
>> First wrote a few simple microbenchmarks to verify there are actual speedups 
>> from using perfect hash tables in `sun.invoke.util.Wrapper` compared to 
>> simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ 
>> a speed-up for the case of `char` -> `Wrapper`, but not when mapping from 
>> `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case 
>> didn't use the `FROM_CHAR` table for some reason, which is remedied.  
>> 
>> Micros show benefits across the board for warmed up case:
>> 
>> 
>> Baseline, OOTB
>> Benchmark  Mode  Cnt   Score   Error  Units
>> Wrappers.forBasicType  avgt5  14.387 ? 0.127  ns/op
>> Wrappers.forPrimitive  avgt5  38.818 ? 0.592  ns/op
>> Wrappers.forPrimitiveType  avgt5  26.085 ? 2.291  ns/op
>> Wrappers.forWrapperavgt5  44.459 ? 1.635  ns/op
>> 
>> Patch, OOTB
>> Benchmark  Mode  Cnt   Score   Error  Units
>> Wrappers.forBasicType  avgt5  14.357 ? 0.133  ns/op
>> Wrappers.forPrimitive  avgt5  23.930 ? 0.071  ns/op
>> Wrappers.forPrimitiveType  avgt5  14.343 ? 0.017  ns/op
>> Wrappers.forWrapperavgt5  27.622 ? 0.022  ns/op
>> 
>> 
>> For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when 
>> spinning up of MHs) there are decent or even great wins in all cases but 
>> `forPrimitiveType` - which was changed from a simple switch to use the hash 
>> lookup. Since the interpreter penalty is small in absolute terms and the win 
>> on JITed code is significant this seems like a reasonable trade-off:
>> 
>> 
>> Baseline, -Xint
>> Benchmark  Mode  Cnt Score Error  Units
>> Wrappers.forBasicType  avgt5  1246.144 ? 149.933  ns/op
>> Wrappers.forPrimitive  avgt5  4955.297 ? 329.869  ns/op
>> Wrappers.forPrimitiveType  avgt5   716.840 ?  62.568  ns/op
>> Wrappers.forWrapperavgt5  5774.700 ? 367.627  ns/op
>> 
>> Patch, -Xint
>> Benchmark  Mode  Cnt Score Error  Units
>> Wrappers.forBasicType  avgt5  1068.096 ? 101.728  ns/op
>> Wrappers.forPrimitive  avgt5  1146.670 ?  59.142  ns/op
>> Wrappers.forPrimitiveType  avgt5   998.037 ? 118.144  ns/op
>> Wrappers.forWrapperavgt5  3581.226 ?  20.167  ns/op
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add line break in make/test/BuildMicrobenchmark.gmk
>   
>   Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>

Looks good.The copyright header end-year needs update before you push.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8242

Re: RFR: 8257733: Move module-specific data from make to respective module [v13]

[Mandy Chung]

On Mon, 21 Mar 2022 16:29:25 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains 20 commits:
> 
>  - Merge branch 'master' into shuffle-data
>  - Correct name for bfc files
>  - Merge tag 'jdk-19+14' into shuffle-data
>
>Added tag jdk-19+14 for changeset 08cadb47
>  - Move x11wrappergen from share/data to unix/data
>  - Fix fontconfig according to review request
>  - Fix typos
>  - Restore charsetmapping and cldr to make/data
>  - Update copyright year
>  - Merge branch 'master' into shuffle-data-reborn
>  - Fix merge
>  - ... and 10 more: 
> https://git.openjdk.java.net/jdk/compare/19d34bdf...1c47d82d

I reviewed java.base and java.se.  I agree that java.se is a better home for 
jdwp.spec.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v4]

[Mandy Chung]

On Thu, 3 Mar 2022 16:55:46 GMT, Tim Prinzing  wrote:

>> The caller class returned by Reflection::getCallerClass was used to gain 
>> access to it's module in most cases and class loader in one case. I added a 
>> method to translate the caller class to caller module so that the decision 
>> of what module represents the caller with no stack frame is made in a single 
>> place. Calls made to caller.getModule() were replaced with 
>> getCallerModule(caller) which returns the system class loader unnamed module 
>> if the caller is null.
>> 
>> The one place a class loader was produced from the caller in getBundleImpl 
>> it was rewritten to route through the getCallerModule method:
>> 
>> final ClassLoader loader = (caller != null) ?
>> caller.getClassLoader() : getLoader(getCallerModule(caller));
>> 
>> A JNI test was added which calls getBundle to fetch a test bundle from a 
>> location added to the classpath, fetches a string out of the bundle and 
>> verifies it, and calls clearCache.
>> 
>> The javadoc was updated for the caller sensitive methods changed.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   more suggested changes

Marked as reviewed by mchung (Reviewer).

src/java.base/share/classes/java/util/ResourceBundle.java line 1570:

> 1568: Module callerModule = (caller != null) ? caller.getModule()
> 1569: : ClassLoader.getSystemClassLoader().getUnnamedModule();
> 1570: return callerModule;

nit: `callerModule` variable is not needed.  It can simply do:

return (caller != null) ? caller.getModule()
: 
ClassLoader.getSystemClassLoader().getUnnamedModule();

-

PR: https://git.openjdk.java.net/jdk/pull/7663

Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v3]

[Mandy Chung]

On Wed, 2 Mar 2022 21:53:01 GMT, Tim Prinzing  wrote:

>> The caller class returned by Reflection::getCallerClass was used to gain 
>> access to it's module in most cases and class loader in one case. I added a 
>> method to translate the caller class to caller module so that the decision 
>> of what module represents the caller with no stack frame is made in a single 
>> place. Calls made to caller.getModule() were replaced with 
>> getCallerModule(caller) which returns the system class loader unnamed module 
>> if the caller is null.
>> 
>> The one place a class loader was produced from the caller in getBundleImpl 
>> it was rewritten to route through the getCallerModule method:
>> 
>> final ClassLoader loader = (caller != null) ?
>> caller.getClassLoader() : getLoader(getCallerModule(caller));
>> 
>> A JNI test was added which calls getBundle to fetch a test bundle from a 
>> location added to the classpath, fetches a string out of the bundle and 
>> verifies it, and calls clearCache.
>> 
>> The javadoc was updated for the caller sensitive methods changed.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   suggested changes

Thanks for the update.

src/java.base/share/classes/java/util/ResourceBundle.java line 1567:

> 1565:  * module will be used.
> 1566:  * @param caller
> 1567:  * @return

Maybe you intended to make this `/*  */` instead of javadoc?  no need to 
have `@param` and `@return` for this simple method.

src/java.base/share/classes/java/util/ResourceBundle.java line 2251:

> 2249: @CallerSensitive
> 2250: public static final void clearCache() {
> 2251: final Module callerModule = 
> getCallerModule(Reflection.getCallerClass());

nit: final can be dropped here.

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
 line 52:

> 50: import java.nio.file.Paths;
> 51: import java.util.Properties;
> 52: import java.util.ResourceBundle;

nit: good to keep the imports in alphabetic order.

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
 line 74:

> 72: 
> 73: // set up shared library path
> 74: final String sharedLibraryPathEnvName = 
> Platform.sharedLibraryPathVariableName();

Nit: `final` adds noise but no other value to the test.  Can consider dropping 
it.

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/NullCallerResourceBundle.java
 line 89:

> 87: 
> 88: 
> 89: private static void makePropertiesFile() throws IOException {

This can be removed?

test/jdk/java/util/ResourceBundle/exeNullCallerResourceBundle/exeNullCallerResourceBundle.c
 line 28:

> 26: 
> 27: #include "jni.h"
> 28: #undef NDEBUG

is this line unintended?

-

PR: https://git.openjdk.java.net/jdk/pull/7663

Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame

[Mandy Chung]

On Wed, 2 Mar 2022 18:56:40 GMT, Tim Prinzing  wrote:

> The caller class returned by Reflection::getCallerClass was used to gain 
> access to it's module in most cases and class loader in one case. I added a 
> method to translate the caller class to caller module so that the decision of 
> what module represents the caller with no stack frame is made in a single 
> place. Calls made to caller.getModule() were replaced with 
> getCallerModule(caller) which returns the system class loader unnamed module 
> if the caller is null.
> 
> The one place a class loader was produced from the caller in getBundleImpl it 
> was rewritten to route through the getCallerModule method:
> 
> final ClassLoader loader = (caller != null) ?
> caller.getClassLoader() : getLoader(getCallerModule(caller));
> 
> A JNI test was added which calls getBundle to fetch a test bundle from a 
> location added to the classpath, fetches a string out of the bundle and 
> verifies it, and calls clearCache.
> 
> The javadoc was updated for the caller sensitive methods changed.

Instead of sprinkling the null caller text in the javadoc in each `getBundle` 
method, we can specify that that in the class specification, for example, after 
the "Resource bundles in other modules and class path" section.


In cases where the {@code getBundle} factory method is called from a context 
where there is no caller frame on the stack (e.g. when called directly from
a JNI attached thread), the caller module is default to the unnamed module for 
the
{@linkplain ClassLoader#getSystemClassLoader system class loader}.


What do you think?

src/java.base/share/classes/java/util/ResourceBundle.java line 1588:

> 1586: Control control) {
> 1587: final ClassLoader loader = (caller != null) ?
> 1588: caller.getClassLoader() : 
> getLoader(getCallerModule(caller));

Suggestion:

  ClassLoader loader = getLoader(getCallerModule(caller));


This can be simplified as above.  I would drop `final` too as it only adds 
noise to the code.

-

PR: https://git.openjdk.java.net/jdk/pull/7663

Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v7]

[Mandy Chung]

On Wed, 16 Feb 2022 21:39:20 GMT, Tim Prinzing  wrote:

>> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is 
>> null
>
> Tim Prinzing has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge master
>  - fix test breakage from rename
>  - reformat test summary
>  - reformat test summary
>  - improve test name and remove experimental test code
>  - fix copyright date
>  - More changes from feedback.
>
>The javadoc is improved to reflect InvalidCallerException is thrown with
>a caller that can't be assigned to a ClassLoader as well as a null
>caller frame.  Added a test IsParallelCapableBadCaller that uses
>reflection hackery to create a case where called with an invalid caller
>on the call stack.
>  - Changes from feedback.
>
>- Copyright dates fixed
>- IllegalCallerException thrown for no caller frame, and associated
>javadoc changes
>- test changed to look for IllegalCallerException thrown.
>  - Merge branch 'master' into JDK-8281000
>  - JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is 
> null

Looks good.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7448

Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v2]

[Mandy Chung]

On Tue, 15 Feb 2022 23:05:30 GMT, Mandy Chung  wrote:

>> Tim Prinzing has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Changes from feedback.
>>   
>>   - Copyright dates fixed
>>   - IllegalCallerException thrown for no caller frame, and associated
>>   javadoc changes
>>   - test changed to look for IllegalCallerException thrown.
>
> src/java.base/share/classes/java/lang/ClassLoader.java line 1626:
> 
>> 1624: protected static boolean registerAsParallelCapable() {
>> 1625: final Class caller = Reflection.getCallerClass();
>> 1626: if (caller == null) {
> 
> Suggestion:
> 
> if (caller == null || !ClassLoader.class.isAssignableFrom(caller)) {
>  throw new IllegalCallerException(caller + " not a subclass of 
> ClassLoader");
> } 
> 
> 
> What we suggested is to throw IllegalCallerException if the caller is not a 
> class loader and that will include null caller case.

This also needs a test case for non-class loader caller.

I looked at the existing tests testing `registerAsParallelCapable` but I can't 
find a good one to add the new test case.   The closest one could be 
`test/jdk/java/lang/ClassLoader/IsParallelCapable.java`.   I'm okay to include 
a new test case in `IsParallelCapable.java` to verify `IllegalCallerException` 
if called by a non-class loader.

-

PR: https://git.openjdk.java.net/jdk/pull/7448

Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null [v2]

[Mandy Chung]

On Tue, 15 Feb 2022 22:17:53 GMT, Tim Prinzing  wrote:

>> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is 
>> null
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Changes from feedback.
>   
>   - Copyright dates fixed
>   - IllegalCallerException thrown for no caller frame, and associated
>   javadoc changes
>   - test changed to look for IllegalCallerException thrown.

src/java.base/share/classes/java/lang/ClassLoader.java line 1612:

> 1610:  * In cases where {@code ClassLoader.registerAsParallelCapable} is 
> called from a context where
> 1611:  * there is no caller frame on the stack (e.g. when called directly
> 1612:  * from a JNI attached thread), {@code IllegalCallerException} is 
> thrown.

Suggestion:

 * In cases where this method is called from a context where the caller is 
not a subclass
 * {@code ClassLoader} or there is no caller frame on the stack (e.g. when 
called directly
 * from a JNI attached thread), {@code IllegalCallerException} is thrown.


Should mention the non-class loader caller as well.

src/java.base/share/classes/java/lang/ClassLoader.java line 1617:

> 1615:  * @return  {@code true} if the caller is successfully registered as
> 1616:  *  parallel capable and {@code false} if otherwise.
> 1617:  * @throws IllegalCallerException if there is no caller frame on 
> the stack.

Suggestion:

 * @throws IllegalCallerException if the caller is not a subclass of {@code 
ClassLoader}

src/java.base/share/classes/java/lang/ClassLoader.java line 1626:

> 1624: protected static boolean registerAsParallelCapable() {
> 1625: final Class caller = Reflection.getCallerClass();
> 1626: if (caller == null) {

Suggestion:

if (caller == null || !ClassLoader.class.isAssignableFrom(caller)) {
 throw new IllegalCallerException(caller + " not a subclass of 
ClassLoader");
} 


What we suggested is to throw IllegalCallerException if the caller is not a 
class loader and that will include null caller case.

test/jdk/java/lang/ClassLoader/exeNullCallerClassLoaderTest/NullCallerClassLoaderTest.java
 line 30:

> 28:  * @summary Test uses custom launcher that starts VM using JNI that 
> verifies
> 29:  *  ClassLoader.registerAsParallelCapable with null caller class 
> does not throw a NullPointerException,
> 30:  *  and instead returns false.

`@summary` needs update to reflect the new change.

-

PR: https://git.openjdk.java.net/jdk/pull/7448

Re: RFR: JDK-8281003 - MethodHandles::lookup throws NPE if caller is null [v4]

[Mandy Chung]

On Tue, 15 Feb 2022 00:10:44 GMT, Tim Prinzing  wrote:

>> JDK-8281003 - MethodHandles::lookup throws NPE if caller is null
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove excess description

Looks good.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7447

Re: RFR: JDK-8281003 - MethodHandles::lookup throws NPE if caller is null

[Mandy Chung]

On Fri, 11 Feb 2022 20:32:46 GMT, Tim Prinzing  wrote:

> JDK-8281003 - MethodHandles::lookup throws NPE if caller is null

This needs a CSR and the spec needs update.  

The test name could be shortened to 
`s/exeNullCallerMethodHandlesLookup/exeNullCallerLookup/`.

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 121:

> 119: Class c = Reflection.getCallerClass();
> 120: if (c == null) {
> 121: throw new IllegalCallerException();

Suggestion:

throw new IllegalCallerException("no caller frame");

-

PR: https://git.openjdk.java.net/jdk/pull/7447

Re: RFR: JDK-8281003 - MethodHandles::lookup throws NPE if caller is null

[Mandy Chung]

On Mon, 14 Feb 2022 18:28:09 GMT, Mandy Chung  wrote:

>> JDK-8281003 - MethodHandles::lookup throws NPE if caller is null
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 121:
> 
>> 119: Class c = Reflection.getCallerClass();
>> 120: if (c == null) {
>> 121: throw new IllegalCallerException();
> 
> Suggestion:
> 
> throw new IllegalCallerException("no caller frame");

The javadoc needs to be updated to specify this `IllegalCallerException` be 
thrown.


* @throws IllegalCallerException if there is no caller frame on the stack 
when called
* directly from a JNI attached thread

-

PR: https://git.openjdk.java.net/jdk/pull/7447

Re: RFR: JDK-8281003 - MethodHandles::lookup throws NPE if caller is null

[Mandy Chung]

On Mon, 14 Feb 2022 11:56:16 GMT, Alan Bateman  wrote:

>> JDK-8281003 - MethodHandles::lookup throws NPE if caller is null
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 121:
> 
>> 119: Class c = Reflection.getCallerClass();
>> 120: if (c == null) {
>> 121: throw new IllegalCallerException();
> 
> Throwing ICE is probably okay here, I just wonder if there is any practical 
> advantage to having it return publicLookup instead, e.g. is there any 
> scenario where a JNI attached thread might want to invoke a method with a 
> Lookup parameter?

`MethodHandles::publicLookup` can be called instead to get a public Lookup to 
invoke a method with a Lookup parameter.   The dilemma here is whether the API 
should be made null-caller friendly or using a proper API 
`MethodHandles::publicLookup` for such case.

-

PR: https://git.openjdk.java.net/jdk/pull/7447

Re: RFR: JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is null

[Mandy Chung]

On Fri, 11 Feb 2022 20:36:26 GMT, Tim Prinzing  wrote:

> JDK-8281000 ClassLoader::registerAsParallelCapable throws NPE if caller is 
> null

Thanks for taking on these null caller issue.

To give more context to this issue, the spec of 
`ClassLoader::isRegisteredAsParallelCapable` returns true if this class loader 
is registered as parallel capable, otherwise false.  The current spec does not 
specify what if the caller class is not a class loader.  The current 
implementation throws NPE if caller is null.  I initially proposed to return 
false for simplicity.   However, if the caller is not a subclass of 
`ClassLoader`, the current implementation throws `ClassCastException`.  Both 
cases are invalid caller and they should be handled in the same way, either 
return false or throw an exception.

Having a second thought, since this API expects to be called by a class loader, 
I think throwing `IllegalCallerException` to indicate this method is called by 
an illegal caller.This will need a CSR due to the spec change.

What do you think?

-

PR: https://git.openjdk.java.net/jdk/pull/7448

Re: RFR: 8275512: Upgrade required version of jtreg to 6.1 [v2]

[Mandy Chung]

On Tue, 19 Oct 2021 17:24:17 GMT, Weijun Wang  wrote:

>> As a follow up of JEP 411, we will soon disallow security manager by 
>> default. jtreg 6.1 does not set its own security manager if JDK version is 
>> >= 18.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   upgrade the version in GHA config
>   
>   only in patch2:
>   unchanged:

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6012

Integrated: JDK-8274311: Make build.tools.jigsaw.GenGraphs more configurable

[Mandy Chung]

On Fri, 24 Sep 2021 23:07:33 GMT, Mandy Chung  wrote:

> GenGraphs tool generates the module graph. It currently supports the 
> configuration via javadoc-graphs.properties. However, 
> `make/jdk/src/classes/build/tools/jigsaw/javadoc-graphs.properties` only 
> documents two properties. It should be updated to cover all configurable 
> properties.
> 
> There are a couple other properties not configurable such as nodesep and node 
> margin. This extends the configuration to allow to set additional properties. 
> 
> This also fixes `requiresMandatedColor` in javadoc-graphs.properties to light 
> gray to match the default configuration in the implementation, i.e. the color 
> of the edge to java.base.  It seems a bug that was unnoticed until Alex and 
> Iris spotted it.

This pull request has now been integrated.

Changeset: daaa47e2
Author:Mandy Chung 
URL:   
https://git.openjdk.java.net/jdk/commit/daaa47e2005cfa1d72f94a32e7756255f24c4d1f
Stats: 167 lines in 3 files changed: 100 ins; 33 del; 34 mod

8274311: Make build.tools.jigsaw.GenGraphs more configurable

Reviewed-by: alanb, iris

-

PR: https://git.openjdk.java.net/jdk/pull/5690

RFR: JDK-8274311: Make build.tools.jigsaw.GenGraphs more configurable

[Mandy Chung]

GenGraphs tool generates the module graph. It currently supports the 
configuration via javadoc-graphs.properties. However, 
`make/jdk/src/classes/build/tools/jigsaw/javadoc-graphs.properties` only 
documents two properties. It should be updated to cover all configurable 
properties.

There are a couple other properties not configurable such as nodesep and node 
margin. This extends the configuration to allow to set additional properties. 

This also fixes `requiresMandatedColor` in javadoc-graphs.properties to light 
gray to match the default configuration in the implementation, i.e. the color 
of the edge to java.base.  It seems a bug that was unnoticed until Alex and 
Iris spotted it.

-

Commit messages:
 - JDK-8274311: Make build.tools.jigsaw.GenGraphs more configurable

Changes: https://git.openjdk.java.net/jdk/pull/5690/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5690=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8274311
  Stats: 167 lines in 3 files changed: 100 ins; 33 del; 34 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5690.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5690/head:pull/5690

PR: https://git.openjdk.java.net/jdk/pull/5690

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]

[Mandy Chung]

On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda  wrote:

>> This is a preview of a patch implementing JEP 406: Pattern Matching for 
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>> 
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>> 
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common 
>> superinterface for expressions and patterns, `CaseLabelTree`, which 
>> expressions and patterns implement, and a list of case labels is returned 
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree` 
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent 
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be 
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary 
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed 
>> primitives, as there is no value that is not part of the input domain so 
>> that could be used to represent `case null`. Requires a bit shuffling with 
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It 
>> produces code that delegates to a new bootstrap method, that will classify 
>> the input value to the switch and return the case number, to which the 
>> switch then jumps. To support guards, the switches (and the bootstrap 
>> method) are restartable. The bootstrap method as such is written very simply 
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String 
>> s`/`case null: case String s:`/`case String s: case null:`, handling of 
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>> 
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixing typo.

I reviewed the `java.base` change namely, SwitchBootstraps.java.  Looks good.

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 47:

> 45:  * of the {@code switch}, implicitly numbered sequentially from {@code 
> [0..N)}.
> 46:  *
> 47:  * The bootstrap call site accepts a single parameter of the type of 
> the

It takes 2 parameters (not single parameter).   Perhaps you can take out this 
paragraph since it's specified in the `typeSwitch` method.

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 110:

> 108:  * @return a {@code CallSite} returning the first matching element 
> as described above
> 109:  *
> 110:  * @throws NullPointerException if any argument is null

Suggestion:

 * @throws NullPointerException if any argument is {@code null}


same formatting nit for other occurrenace of "null"

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3863

Re: RFR: JDK-8266254: Update to use jtreg 6

[Mandy Chung]

On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons  wrote:

> Please review the change to update to using jtreg 6. 
> 
> The primary change is to the jib-profiles.js file, which specifies the 
> version of jtreg to use, for those systems that rely on this file. In 
> addition, the `requiredVersion` has been updated in the various `TEST.ROOT` 
> files.
> 
> All the tests that could be updated ahead of time have been updated. There 
> are a few tests remaining that need to be done at this time, because of the 
> change in the module name for TestNG 7.3. It changed from a default of 
> `testng` to and explicit `org.testng`.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4315

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Mandy Chung]

On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang  wrote:

> Please review this implementation of [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> The code change is divided into 3 commits. Please review them one by one.
> 
> 1. 
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>  The essential change for this JEP, including the `@Deprecate` annotations 
> and spec change. It also update the default value of the 
> `java.security.manager` system property to "disallow", and necessary test 
> change following this update.
> 2. 
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>  Manual changes to several files so that the next commit can be generated 
> programatically.
> 3. 
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>  Automatic changes to other source files to avoid javac warnings on 
> deprecation for removal
> 
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
> generated programmatically, see the comment below for more details. If you 
> are only interested in a portion of the 3rd commit and would like to review 
> it as a separate file, please comment here and I'll generate an individual 
> webrev.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> any file to minimize unnecessary merge conflict.
> 
> Furthermore, since the default value of `java.security.manager` system 
> property is now "disallow", most of the tests calling 
> `System.setSecurityManager()` need to launched with 
> `-Djava.security.manager=allow`. This is covered in a different PR at 
> https://github.com/openjdk/jdk/pull/4071.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8267112: JVMCI compiler modules should be kept upgradable

[Mandy Chung]

On Thu, 13 May 2021 16:37:38 GMT, Vladimir Kozlov  wrote:

> [JDK-8264806](https://bugs.openjdk.java.net/browse/JDK-8264806) changes 
> removed sources and also removed JVMCI compiler from list of upgradable 
> modules. JVMCI compiler modules should be upgradable in JDK to work with 
> GraalVM. 
> 
> Make these modules upgradable again and empty by leaving only reference to 
> JVMCI (jdk.internal.vm.ci) module. It does not restore sources - only 
> `module-info.java` files are kept.
> 
> Note, we continue discussion about 
> [JDK-8265091](https://bugs.openjdk.java.net/browse/JDK-8265091): "Use Module 
> API to export JVMCI packages at runtime" to see if we can remove these 
> `module-info.java` files.
> 
> Changes were proposed by @dougxc after testing 
> [JDK-8264806](https://bugs.openjdk.java.net/browse/JDK-8264806) changes with 
> GraalVM.
> I restored related code in some tests for them to pass.
> 
> Testing: full tier1-tier3.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4014

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Mandy Chung]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

I reviewed the module-loader-map and non-hotspot non-AOT tests.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v5]

[Mandy Chung]

On Wed, 16 Dec 2020 18:36:25 GMT, Magnus Ihse Bursie  wrote:

>> The hard-coded list of modules in `make/common/Modules.gmk` has always been 
>> a wart in the build system. We pride ourself on using discovery instead of 
>> hard-coded list. In this case, it is not possible to do do auto-discovery, 
>> since the different module sets are configured, not determined.
>> 
>> Thus the actual lists of module sets should move to the `make/conf` 
>> directory.
>> 
>> This is the first patch in a series where I will move configuration values 
>> spread over the build system into the designated `make/conf` directory.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Changes as requested by reviewers

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1781

Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]

[Mandy Chung]

On Wed, 16 Dec 2020 14:40:14 GMT, Alan Bateman  wrote:

> javadoc-modules.conf is probably okay although someone finding this in the 
> repo might initially think it's the configuration for the javadoc modules. 
> That plus it sets DOCS_MODULES, so maybe it should be apidocs-modules.conf.

I'm okay with apidocs-modules.conf or docs-modules.conf.   It's good to match 
the make target name which makes it easier to understand what this 
configuration is for.

> module-loader-map.conf works as the configuration file that defines 
> BOOT_MODULES and PLATFORM_MODULES. I think AGGREGATOR_MODULES should be 
> dropped and "java.se" added to PLATFORM_MODULES. If I remember correctly, 
> this was separated out in JDK 9 and 10 because of the java.se.ee aggregator 
> module (that one was removed in Java 11 by JEP 320).

module-loader-map.conf works for me too.   We can drop AGGREGATOR_MODULES  
since java.se is the sole aggregator module in JDK now.

-

PR: https://git.openjdk.java.net/jdk/pull/1781

Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]

[Mandy Chung]

On Tue, 15 Dec 2020 22:58:47 GMT, Mandy Chung  wrote:

>> `JRE_TOOL_MODULES` started with more than one modules in JDK 9:
>> 
>> JRE_TOOL_MODULES += \
>> jdk.jdwp.agent \
>> jdk.pack \
>> jdk.scripting.nashorn.shell \
>> #
>> 
>> Since only `jdk.jdwp.agent` one module is left for `JRE_TOOL_MODULES`, as 
>> you are refactoring this file, I suggest to get rid of `JRE_TOOL_MODULES` 
>> and explicitly name `jdk.jdwp.agent` in `JRE_MODULES`.
>
> Do you see a way to get rid of `DOCS_MODULES` but determine this set at build 
> time?  IIRC it was added for expediency for 
> [JDK-8172312](https://bugs.openjdk.java.net/browse/JDK-8172312).   This is 
> the set of Java SE + JDK modules that excludes `jdk.internal.*` modules and 
> `jdk.unsupported` and also platform-specific modules.   (History: the docs 
> bundle generated prior to JDK 9 only included platform-neutral APIs.)
> 
> As for the conf file for module to class loader mapping, I actually like one 
> single file `jdk-modules.conf` to enumerate all JDK modules.   Currently it 
> only defines the list of modules defined to boot and platform class loader 
> but excludes any modules defined to application class loaders.  I consider to 
> enumerate all modules in this file and the build can verify if any module is 
> missing.
> 
> `module-sets-build.conf` is a bit awkward and I will give more thought on 
> naming ideas.

Can any of `INTERIM_IMAGE_MODULES` , `HOTSPOT_MODULES` and `LANGTOOLS_MODULES` 
be inlined in the appropriate .gmk file?

`INTERIM_IMAGE_MODULES` is for building interim image.  If it has to be defined 
in a conf file, I like its name be explicit and match the target or makefile, 
for example, `interim-images.conf` or `InterimImages.conf`.This way I can 
tell what this conf file intends for.  What do you think?

-

PR: https://git.openjdk.java.net/jdk/pull/1781

Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]

[Mandy Chung]

On Tue, 15 Dec 2020 22:47:16 GMT, Mandy Chung  wrote:

>> As for `JRE_TOOL_MODULES`, I understand what you mean but it is at least 
>> kind of a "sibling" to the others. After all, we use these sets of modules 
>> together to form the set of modules for the JRE:
>> 
>> JRE_MODULES += $(filter $(ALL_MODULES), $(BOOT_MODULES) \
>> $(PLATFORM_MODULES) $(JRE_TOOL_MODULES))
>> 
>> So given that `BOOT_MODULES` and `PLATFORM_MODULE` has a role to play here 
>> as well, I think it would be odd *not* to have `JRE_TOOL_MODULES` defined at 
>> the same place.
>
> `JRE_TOOL_MODULES` started with more than one modules in JDK 9:
> 
> JRE_TOOL_MODULES += \
> jdk.jdwp.agent \
> jdk.pack \
> jdk.scripting.nashorn.shell \
> #
> 
> Since only `jdk.jdwp.agent` one module is left for `JRE_TOOL_MODULES`, as you 
> are refactoring this file, I suggest to get rid of `JRE_TOOL_MODULES` and 
> explicitly name `jdk.jdwp.agent` in `JRE_MODULES`.

Do you see a way to get rid of `DOCS_MODULES` but determine this set at build 
time?  IIRC it was added for expediency for 
[JDK-8172312](https://bugs.openjdk.java.net/browse/JDK-8172312).   This is the 
set of JDK (non-Java SE) modules (excluding `jdk.internal.*` modules and 
`jdk.unsupported` and also platform-specific modules). 

As for the conf file for module to class loader mapping, I actually like one 
single file `jdk-modules.conf` to enumerate all JDK modules.   Currently it 
only defines the list of modules defined to boot and platform class loader but 
excludes any modules defined to application class loaders.  I consider to 
enumerate all modules in this file and the build can verify if any module is 
missing.

`module-sets-build.conf` is a bit awkward and I will give more thought on 
naming ideas.

-

PR: https://git.openjdk.java.net/jdk/pull/1781

Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]

[Mandy Chung]

On Tue, 15 Dec 2020 20:32:05 GMT, Magnus Ihse Bursie  wrote:

>> I thought it was a consistent and clear naming scheme. :-) But I guess to 
>> each their own...
>> 
>> Would `classloader-modules.conf`, `docs-modules.conf` and 
>> `build-modules.con` be better? Otherwise you'll need to come up with any 
>> better solutions yourself, since I'm starting to run out of ideas.
>
> As for `JRE_TOOL_MODULES`, I understand what you mean but it is at least kind 
> of a "sibling" to the others. After all, we use these sets of modules 
> together to form the set of modules for the JRE:
> 
> JRE_MODULES += $(filter $(ALL_MODULES), $(BOOT_MODULES) \
> $(PLATFORM_MODULES) $(JRE_TOOL_MODULES))
> 
> So given that `BOOT_MODULES` and `PLATFORM_MODULE` has a role to play here as 
> well, I think it would be odd *not* to have `JRE_TOOL_MODULES` defined at the 
> same place.

`JRE_TOOL_MODULES` started with more than one modules in JDK 9:

JRE_TOOL_MODULES += \
jdk.jdwp.agent \
jdk.pack \
jdk.scripting.nashorn.shell \
#

Since only `jdk.jdwp.agent` one module is left for `JRE_TOOL_MODULES`, as you 
are refactoring this file, I suggest to get rid of `JRE_TOOL_MODULES` and 
explicitly name `jdk.jdwp.agent` in `JRE_MODULES`.

-

PR: https://git.openjdk.java.net/jdk/pull/1781

Re: RFR: 8257450: Start of release updates for JDK 17 [v5]

[Mandy Chung]

On Tue, 8 Dec 2020 17:10:30 GMT, Joe Darcy  wrote:

>> Start of JDK 17 updates.
>
> Joe Darcy has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains 12 additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into JDK-8257450
>  - Update symbol files for JDK 16b27.
>  - Merge branch 'master' into JDK-8257450
>  - Merge branch 'master' into JDK-8257450
>  - Merge branch 'master' into JDK-8257450
>  - Update tests.
>  - Merge branch 'master' into JDK-8257450
>  - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into 
> JDK-8257450
>  - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into 
> JDK-8257450
>  - JDK-8257450
>  - ... and 2 more: 
> https://git.openjdk.java.net/jdk/compare/c3d62067...ff9b78ec

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1531

Re: RFR: 8257733: Move module-specific data from make to respective module [v2]

[Mandy Chung]

On Tue, 8 Dec 2020 18:16:15 GMT, Mandy Chung  wrote:

>> @wangweij Moving build tools is a related, but separate, question, which is 
>> addressed by https://bugs.openjdk.java.net/browse/JDK-8241463.
>> 
>> I send out a review earlier this year (see 
>> https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), 
>> but there were differing opinions on what the proper placement of these 
>> tools should be, and the discussion kind of fizzled out without reaching a 
>> conclusion.
>
> @magicus @erikj79 it's now clearly stated that you want everything under 
> `make` owned by the build team, which is fair.  You are right that `make` has 
> been easily considered as a dumping ground and it's time to define a clean 
> structure.
> 
> I reviewed this patch and this looks okay to me.   Some follow-up questions 
> and follow-on cleanup for example "should jdwp.spec belong to `specs` 
> directory vs `data`?  There are platform-specific data (such as charsets) 
> which has been special-case in the makefile and they need follow-on clean up. 
>   I agree that this should be cleaned up by the component teams and not part 
> of this PR.

With these new files showing up in under `src` directory, should `bin/idea.sh` 
be changed to exclude `data` to avoid incurring costs in loading JDK project 
from IDE that many of us use for development?

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8257733: Move module-specific data from make to respective module [v2]

[Mandy Chung]

On Tue, 8 Dec 2020 16:19:05 GMT, Magnus Ihse Bursie  wrote:

>> Is there a plan to move make/jdk/src/classes/build/tools/intpoly into 
>> java.base as well?
>> 
>> Update: I see all subdirs in tools are still there.
>
> @wangweij Moving build tools is a related, but separate, question, which is 
> addressed by https://bugs.openjdk.java.net/browse/JDK-8241463.
> 
> I send out a review earlier this year (see 
> https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), 
> but there were differing opinions on what the proper placement of these tools 
> should be, and the discussion kind of fizzled out without reaching a 
> conclusion.

@magicus @erikj79 it's now clearly stated that you want everything under `make` 
owned by the build team, which is fair.  You are right that `make` has been 
easily considered as a dumping ground and it's time to define a clean structure.

I reviewed this patch and this looks okay to me.   Some follow-up questions and 
follow-on cleanup for example "should jdwp.spec belong to `specs` directory vs 
`data`?  There are platform-specific data (such as charsets) which has been 
special-case in the makefile and they need follow-on clean up.   I agree that 
this should be cleaned up by the component teams and not part of this PR.

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8257733: Move module-specific data from make to respective module [v2]

[Mandy Chung]

On Mon, 7 Dec 2020 19:31:59 GMT, Jonathan Gibbons  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Move to share/data, and move jdwp.spec to java.se
>
> I have reviewed all lines in the patch file with or near instances of 
> `jdk.compiler`

Hi Magnus,

I see the motivation of moving these build files for better identification of 
ownership.   Placing them under
`src/$MODULE/{share,$OS}/data` is one option.  Given that skara will 
automatically determine appropriate mailing lists of a PR, it seems that 
`make/modules/$MODULE/data` can be another alternative that skara can include 
this pattern in the mailing list configuration??   I don't yet have a strong 
preference while I don't consider everything under `make` must be owned by the 
build team though.  Do you see one option is better than the other?

-

PR: https://git.openjdk.java.net/jdk/pull/1611

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

[Mandy Chung]

On Wed, 4 Nov 2020 10:13:59 GMT, Kim Barrett  wrote:

>> Hello!
>> 
>> As an IDE developer, I'm thinking about IDE inspection that may suggest the 
>> new method. My idea is to suggest replacing every `ref.get() == obj` with 
>> `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() 
>> == obj` could be preferred? What do you think?
>
>> Hello!
>> 
>> As an IDE developer, I'm thinking about IDE inspection that may suggest the 
>> new method. My idea is to suggest replacing every `ref.get() == obj` with 
>> `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() 
>> == obj` could be preferred? What do you think?
> 
> Those have different behaviors when ref's class overrides get.  Sometimes 
> that might be intentional (PhantomReference, where get blocks access to the 
> referent, and SoftReference, where get may update heuristics for recent 
> accesses delaying GC clearing).  But if some further subclass overrides get 
> for some reason, such a change might not be appropriate.

Checking if a reference has been cleared i.e. `ref.get() == null` or `ref.get() 
!= null` may benefit with IDE giving a hint.

-

PR: https://git.openjdk.java.net/jdk/pull/498

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v5]

[Mandy Chung]

On Tue, 20 Oct 2020 05:22:57 GMT, Kim Barrett  wrote:

>> @kimbarrett  your reworded text is okay.  I think "if it initially had some 
>> other referent value" can be dropped.
>> 
>> For a `Reference` constructed with a `null` referent, we can clarify in the 
>> spec that such reference object will never
>> get cleared and enqueued.  I suggest to file a separate issue to follow up.
>
>> @kimbarrett your reworded text is okay. I think "if it initially had some 
>> other referent value" can be dropped.
>> 
>> For a `Reference` constructed with a `null` referent, we can clarify in the 
>> spec that such reference object will never
>> get cleared and enqueued. I suggest to file a separate issue to follow up.
> 
> I don't think that clause can be dropped, because of explicit clearing (by 
> clear() or enqueue()) rather than by the
> GC.  If the reference was constructed with a null referent, 
> ref.refersTo(null) cannot tell whether ref.clear() has been
> called.

> Mandy's comment implied that references with a null referent never get 
> enqueued. Otherwise when would they get
> enqueued? There would be nothing to trigger it.

Sorry I should have been clearer.What I try to say is that `Reference(null)`
will never be cleared and enqueued by GC since its referent is `null`.
Kim is right that `Reference(null)` can be explicitly cleared and enqueued
via `Reference::enqueue`.`Reference::clear` on such an "empty" reference
object is essentially a no-op.

> > But the more we discuss this the more I think allowing an initial null 
> > referent was a mistake in the first place. :(
> 
> I agree, but here we are. Very hard to know what the compatibility impact of 
> changing that would be.

There are existing use cases depending on `Reference(null)` for example as a 
special
instance be an empty reference or the head of a doubly-linked list of 
references.
This was discussed two years ago [1].

[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054325.html

-

PR: https://git.openjdk.java.net/jdk/pull/498

Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v16]

[Mandy Chung]

On Sat, 10 Oct 2020 00:08:26 GMT, Yumin Qi  wrote:

>> This patch is reorganized after 8252725, which is separated from this patch 
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at: 
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 
>> integrated, the
>> regeneration of holder classes is simply to call the new added 
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function.  Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - Correct typo for check message
>  - Removed try/catch from java side and moved output to vm.
>  - Make change to original indent

I reviewed the files under `src/java.base`.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/193

Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]

[Mandy Chung]

On Tue, 6 Oct 2020 20:46:17 GMT, Yumin Qi  wrote:

>> This patch is reorganized after 8252725, which is separated from this patch 
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at: 
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 
>> integrated, the
>> regeneration of holder classes is simply to call the new added 
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function.  Tests: tier1-4
>
> Yumin Qi has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains
> 23 commits:
>  - Added new separate function to CDS for logging species and modified the 
> existing function to log lambda form invokers.
>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as 
> read only in CDS.
>  - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>  - Removed unused imports.
>  - Fixed comments with correct class and method name in CDS, removed unused 
> variables after last change.
>  - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to 
> CDS as generateLambdaFormHolderClasses. Added
>input verification function in CDS before class generation. Added more 
> test scenarios. Removed trailing unused ending
>words for output of lambda form trace line in case of DumpLoadedClassList.
>  - Move the check work to java, restore code in VM. Modified test code 
> according to the changes. The invoke name
>verififcation is not implemented since not all the holder class are 
> processed, not all the functions of processed
>holder classes are added. For holder class with DirectMethodHandle in its 
> name, only the name in the
>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets 
> skipped silently. This makes the verification
>on invoke names difficul, a name not in the keyset should not fail the 
> test. Also add a boolean to
>cdsGenerateHolderClasses to indicate call path.
>  - Remove trailing word of line which is not used in holder class 
> regeneration. There is a trailing LF (Line Feed) so trim
>white spaces from both front and end of the line or it will fail method 
> type validation.
>  - In case of exception happens during reloading class, CHECK will return 
> without free the allocated buffer for class
>bytes so moved the buffer allocation and freeing to caller. Also removed 
> test 6 since there is not guarantee that we
>can give a signature which will always fail. Additional changes to 
> GenerateJLIClassesHelper according to review
>suggestion.
>  - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>  - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
>  - ... and 13 more: 
> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf

src/java.base/share/classes/jdk/internal/misc/CDS.java line 40:

> 38:   * indicator for dumping class list.
> 39:   */
> 40: static public final boolean isDumpingClassList;

what about making this a private static field and adding a public static 
`isDumpingClassList()` method (which was in
the previous version).

src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:

> 142: String line = s.trim();
> 143: if (!line.startsWith("[LF_RESOLVE]") && 
> !line.startsWith("[SPECIES_RESOLVE]")) {
> 144: System.out.println("Wrong prefix: " + line);

Should this throw an exception instead?

src/java.base/share/classes/jdk/internal/misc/CDS.java line 155:

> 153: System.out.println("Incorrecct number of items in 
> the line: " + parts.length);
> 154: System.out.println("line: " + line);
> 155: return null;

I think these error cases should throw `IllegalArgumentException` and VM 
decides how to handle the exception.

src/java.base/share/classes/jdk/internal/misc/CDS.java line 140:

> 138: // return null for invalid input
> 139: private static Stream  validateInputLines(String[] lines) {
> 140: ArrayList list = new ArrayList(lines.length);

Nit: this can use diamond operatior like this: `new ArrayList<>(lines.length)`.

src/java.base/share/classes/jdk/internal/misc/CDS.java line 184:

> 182: Objects.requireNonNull(lines);
> 183: try {
> 184: Stream lineStream = validateInputLines(lines);

It seems clearer to have `validateInputLines` do validation only and convert 
this line into:

  validateInputLines(lines);
  Stream lineStream = Arrays.stream(lines);

src/java.base/share/classes/jdk/internal/misc/CDS.java line 178:

> 176: /**
> 177:  * called from vm to generate MethodHandle holder classes
> 178:  * @return @code { Object[] } if holder classes can be generated.

type: `s/@code { Object[]/{@code Object[]}`

src/java.base/share/classes/jdk/internal/misc/CDS.java line 198:

> 196: return retArray;
> 197: } 

Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v11]

[Mandy Chung]

On Tue, 6 Oct 2020 17:35:18 GMT, Yumin Qi  wrote:

>> This patch is reorganized after 8252725, which is separated from this patch 
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at: 
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 
>> integrated, the
>> regeneration of holder classes is simply to call the new added 
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function.  Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed unused imports.

src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line 
63:

> 61: if (TRACE_RESOLVE) {
> 62: System.out.println(traceLF + (resolvedMember != null ? " 
> (success)" : " (fail)"));
> 63: }

I suggest not to change the existing code.  Instead, have 
`CDS::traceLambdaFormInvoker`
to take individual parameters `Class holder, String name, String 
shortenSignature`
(rather than the formatted string).   Something like:

if (CDS.isDumpLoadedClassList()) {
  CDS.traceLambdaFormInvoker(holder, name, 
shortenSignature(basicTypeSignature(type));
}

This also gives flexibility to CDS to decide on what format to write to the 
class list (like this case, you drop the
text "success/fail")

In addition, the conditional check on `CDS.isDumpLoadedClassList()` is hard to 
relate to why CDS traces these events.
I see Ioi's comment on this method name too.   I agree with Ioi that 
`isDumpingClassList` makes more sense.

src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line 
74:

> 72: System.out.println(traceSP + (salvage != null ? " 
> (salvaged)" : " (generated)"));
> 73: }
> 74: CDS.traceLambdaFormInvoker(traceSP);

I suggest leaving the existing code unchanged.  Instead, add the following:
if (CDS.isDumpingClassList()) {
 CDS.traceSpeciesType(cn);
}

The above uses Ioi's suggested method name which reads better.

src/java.base/share/classes/jdk/internal/misc/CDS.java line 83:

> 81:  * check if -XX:+DumpLoadedClassList and given file is open
> 82:  */
> 83: public static boolean isDumpLoadedClassList() {

I agree with Ioi's suggestion to rename this to `isDumpingClassList` which 
describes what the VM is doing.

-

PR: https://git.openjdk.java.net/jdk/pull/193

Re: RFR: 8253500: [REDO] JDK-8253208 Move CDS related code to a separate class

[Mandy Chung]

On Wed, 23 Sep 2020 23:05:59 GMT, Yumin Qi  wrote:

> This patch is a REDO for JDK-8253208 which was backed out since it caused 
> runtime/cds/DeterministicDump.java failed,
> see JDK-8253495. Since the failure is another issue and only triggered by 
> this patch, the test case now is put on
> ProblemList.txt. The real root cause for the failure is detailed in 
> JDK-8253495.  When JDK-8253208 was backed out,
> CDS.java remained without removed from that patch (see JDK-8253495 subtask), 
> so this patch does not include CDS.java
> (this is why you will see CDS.c without CDS.java in this patch).  Tests 
> tier1-4 passed.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/327

Re: RFR: 8253208: Move CDS related code to a separate class [v3]

[Mandy Chung]

On Mon, 21 Sep 2020 22:24:15 GMT, Yumin Qi  wrote:

>> With more CDS related code added to VM, it is time to move CDS code to a 
>> separate class. CDS is the new class which is
>> specific to CDS.
>> Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8253208: Move CDS related code to a separate class

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/261

Re: RFR: 8253208: Move CDS related code to a separate class [v2]

[Mandy Chung]

On Mon, 21 Sep 2020 18:17:55 GMT, Yumin Qi  wrote:

>> With more CDS related code added to VM, it is time to move CDS code to a 
>> separate class. CDS is the new class which is
>> specific to CDS.
>> Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8253208: Move CDS related code to a separate class

This patch looks okay.   Please add the javadoc for 
`CDS::getRandomSeedForDumping` and `CDS::defineArchiveModules` for
completeness.

-

PR: https://git.openjdk.java.net/jdk/pull/261

Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive

[Mandy Chung]

src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java 
367 /** 368 * called from vm to generate MethodHandle holder classes 369 
* @return @code { Object[] } if holder classes can be generated. 370 * 
@param lines the output lines from @code { VM.cdsTraceResolve } 371 */ 
@code {} is wrong syntax. It should be {@code } 372 static 
Object[] cdsGenerateHolderClasses(String[] lines) { this can be made 
private as it's invoked by VM only. Can you move it to the end of the 
file. 373 try { 374 Map result = 
generateHolderClasses(Arrays.stream(lines)); 375 if (result == null) { 
376 return null; 377 }



generateHolderClasses should never return null and so line 371-377 can 
be dropped.   I also suggest to add in the generateHolderClasses method 
to add Objects.requireNonNull(traces).


379 Object[] ret_array = new Object[size * 2];


Rename `ret_array` to retArray to follow the naming convention using camel case.


src/java.base/share/classes/jdk/internal/misc/VM.java
457 isDumpLoadedClassListSetAndOpen = 
isDumpLoadedClassListSetAndOpen0();


This should be cached in the caller who checks if -XX:+DumpLoadedClassList
instead of every VM initialization.

Since there are many CDS-related methods, I think it's time to introduce
a new CDS class for these methods to reside (maybe jdk.internal.vm.CDS?).

I suggest to add CDS:logTraceResolve(String line) method that
will check if isDumpLoadedClassListSetAndOpen is true, then call the
native cdsTraceResolve (or whatever name).  This way,
GenerateJLIClassesHelper can simply call CDS::logTraceResolve.


493  * Output to DumpLoadedClassList, format is simimar to LF_RESOLVE


s/simimar/similar


494  * @see InvokerBytecodeGenerator
495  * @param line the line to output.


@see is typically placed after @param.


Should it say @see java.lang.invoke.GenerateJLIClassesHelper::traceLambdaForm
instead of InvokerBytecodeGenerator?


Mandy



On 9/15/20 12:15 PM, Yumin Qi wrote:

This patch is reorganized after 8252725, which is separated from this patch to 
refactor jlink glugin code. The previous
webrev with hg can be found at: 
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 
integrated, the
regeneration of holder classes is simply to call the new added 
GenerateJLIClassesHelper.cdsGenerateHolderClasses
function.

Tests: tier1-4

-

Commit messages:
  - 8247536: Support for pre-generated java.lang.invoke classes in CDS static 
archive
  - Merge remote-tracking branch 'upstream/master' into jdk-8247536
  - Merge remote-tracking branch 'upstream/master' into jdk-8247536
  - Merge remote-tracking branch 'origin/jdk-8252689'
  - 8252689: Classes are loaded from jrt:/java.base even when CDS is used
  - 8252689: Classes are loaded from jrt:/java.base even when CDS is used
  - 8252689: Classes are loaded from jrt:/java.base even when CDS is used

Changes: https://git.openjdk.java.net/jdk/pull/193/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk=193=00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8247536
   Stats: 368 lines in 18 files changed: 344 ins; 13 del; 11 mod
   Patch: https://git.openjdk.java.net/jdk/pull/193.diff
   Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193

PR: https://git.openjdk.java.net/jdk/pull/193

Re: RFR: JDK-8244021 Hide warning from jlink about incubating modules

[Mandy Chung]

Looks good.

Mandy

On 4/28/20 7:19 AM, Magnus Ihse Bursie wrote:
The last remaining "WARNING" that is printed while building the JDK 
stems from jlink. This is due to the fact that jmod is run with the 
flag --warn-if-resolved=incubating, which will cause future processing 
of the jmod to print a warning.


This is by design to get the warning to display for the end users.

However, this also has the side-effect that our call to jlink will 
display this warning, unconditionally. This warning from jlink cannot 
be disabled, per design.


Since we know that we have incubating modules from time to time, and 
it is not a warning for us, and the message cannot be removed using 
flags to jlink, I propose to just filter it out, so we can achieve a 
fully warning-free build.


The filtering can always be disabled by running like this "make images 
JLINK_DISABLE_WARNINGS="


Bug: https://bugs.openjdk.java.net/browse/JDK-8244021
Patch inline:
diff --git a/make/Images.gmk b/make/Images.gmk
--- a/make/Images.gmk
+++ b/make/Images.gmk
@@ -85,6 +85,8 @@
   JLINK_JDK_EXTRA_OPTS := --keep-packaged-modules $(JDK_IMAGE_DIR)/jmods
 endif

+JLINK_DISABLE_WARNINGS := | ( $(GREP) -v -e "WARNING: Using incubator 
module" || test "$$?" = "1" )

+
 $(eval $(call SetupExecute, jlink_jdk, \
 WARN := Creating jdk image, \
 DEPS := $(JMODS) $(BASE_RELEASE_FILE) \
@@ -93,7 +95,8 @@
 SUPPORT_DIR := $(SUPPORT_OUTPUTDIR)/images/jdk, \
 PRE_COMMAND := $(RM) -r $(JDK_IMAGE_DIR), \
 COMMAND := $(JLINK_TOOL) --add-modules $(JDK_MODULES_LIST) \
-    $(JLINK_JDK_EXTRA_OPTS) --output $(JDK_IMAGE_DIR), \
+    $(JLINK_JDK_EXTRA_OPTS) --output $(JDK_IMAGE_DIR) \
+    $(JLINK_DISABLE_WARNINGS), \
 ))

Re: RFR 8241749: Remove the Nashorn JavaScript Engine

[Mandy Chung]

This looks okay to me.

Can you add the bug ID next to @ignore that will make it easier to find 
the JBS issue?   These test bugs are filed with P4 but I think they 
should be fixed in 15.


Mandy

On 4/15/20 8:56 AM, sundararajan.athijegannat...@oracle.com wrote:

Please review.

Nashorn script engine modules (jdk.scripting.nashorn, 
jdk.scripting.nashorn.shell) and jjs tool are removed.


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

JEP: https://openjdk.java.net/jeps/372

CSR: https://bugs.openjdk.java.net/browse/JDK-8241751

Webrev: http://cr.openjdk.java.net/~sundar/8241749/webrev.00/index.html

Few tests that use nashorn are worked around by @ignore or deleting 
relevant nashorn test sections. Separate test bugs have been filed to 
handle these.


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

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

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

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

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

Thanks,

-Sundar

Re: RFR (S): 8241947: Minor comment fixes for system property handling

[Mandy Chung]

+1

Mandy

On 3/31/20 11:57 AM, Langer, Christoph wrote:


Hi Mandy,

this is a good suggestion. The listing of system properties at the 
props field declaration seems somewhat redundant, given that it 
already exists more exactly and with API normativity in the doc of 
System::getProperties().


So what do you think of this version: 
http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ 
<http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/> ?


Thanks

Christoph

*From:* Mandy Chung 
*Sent:* Dienstag, 31. März 2020 19:34
*To:* Langer, Christoph ; 
core-libs-...@openjdk.java.net

*Cc:* build-dev 
*Subject:* Re: RFR (S): 8241947: Minor comment fixes for system 
property handling


On 3/31/20 7:56 AM, Langer, Christoph wrote:

Hi,

please review a small fix that updates two comments.

The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property 
"vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it 
must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 
(Minimize JNI upcalls in system-properties initialization).

The second one is the code comment attached to "private static Properties props;" in 
java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be 
undefined), "java.vendor.version" can be undefined, so this should be reflected in the 
comment. I also took the liberty to remove an unneeded import statement.

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

Webrev:http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/


I wonder if we still want to keep this block of comments listing a 
subset of system properties.  Anyway the minor comment looks okay.


Mandy

Re: RFR (S): 8241947: Minor comment fixes for system property handling

[Mandy Chung]

On 3/31/20 7:56 AM, Langer, Christoph wrote:

Hi,

please review a small fix that updates two comments.

The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property 
"vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it 
must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 
(Minimize JNI upcalls in system-properties initialization).

The second one is the code comment attached to "private static Properties props;" in 
java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be 
undefined), "java.vendor.version" can be undefined, so this should be reflected in the 
comment. I also took the liberty to remove an unneeded import statement.

Bug: https://bugs.openjdk.java.net/browse/JDK-8241947
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/



I wonder if we still want to keep this block of comments listing a 
subset of system properties.  Anyway the minor comment looks okay.


Mandy

Re: RFR: JDK-8241463 Move build tools to respective modules

[Mandy Chung]

Hi Magnus,

On 3/24/20 4:16 AM, Magnus Ihse Bursie wrote:


At the core, we'd like to "invert" the current structure where we have 
files like:

make/lib/Lib-java.base.gmk
make/lib/Lib-java.desktop.gmk
make/gensrc/Gensrc-java.base.gmk
make/gensrc/Gensrc-java.desktop.gmk
... etc

and instead have like:
make/modules/java.base/lib.gmk
make/modules/java.base/gensrc.gmk
make/modules/java.desktop/lib.gmk
make/modules/java.desktop/gensrc.gmk

:

We already have collected everything else that belongs to a module 
under src/$module/share. Apart from classes, and native, we have:

* conf
* lib
* legal
* man
for those modules that require them.

My suggestion is that we add, for those module that require them:
* data
* tools



I think we should take at modularizing make/data, build tools, 
make/gensrc etc as a whole and put down other options considered.


I haven't had time looking closely but I try to understand how make/data 
are used.  Here are some examples of it that produces either 
gensrc/$MODULE/$PACKAGE/*.java files or files in jdk build output.


   - generate source files from gensrc/$MODULE/$PACKAGE/*.java.template
   - generate resource bundle from 
gensrc/$MODULE/$PACKAGE/resource/*.properties

   - generate CLDR locale data from make/data/cldr files
   - generate jdk/lib/tzdb.dat from make/data/tzdata
   - generate jdk/lib/ct.sym from make/data/symbol
   - generate jdk/lib/security/cacerts from make/data/cacerts
   - generate jdk/lib/security/backlisted.certs from 
make/data/blacklistedcertsconverter
   - generate jdk/lib/security/public_suffix_list.data from 
make/data/publicsuffixlist


I can see why you propose the data files are moved to the source. There 
could be other options to explore.


Another observation:  Some build tools are module-specific (e.g. 
generate icons .png.java files) that is clearly used only by one 
module.   There are other build tools that can be used by any module 
e.g. generate resource bundle .java source.


And regarding JEP 201, as far as I can tell the additional "man" and 
"lib" directories do not seem to be reflected in JEP 201. I do not 
know if this is considered an oversight, or just reflecting the fact 
that the directory structure will continue to evolve after JEP 201 
were delivered. But if you think JEP 201 needs to be updated, fine, 
I'll gladly help with that.




JEP 299 discusses the reorganization of man pages and specification and 
specifies the man directory.  When JEP 201 was written, the man 
directory was not present (part of moving the specs effort to the 
openjdk source).   It was added in JDK 12.  "lib" directory is an oversight.


JEP 201 may need update for this build modularization work too.

Mandy

Re: RFR: JDK-8241463 Move build tools to respective modules

[Mandy Chung]

Hi Magnus,

Modularizing the build tools is a good move.    This patch suggests to 
place the build tools under

    src/$MODULE/share/tools/$PACKAGE/*.java

I think the modular source location of the build tools needs more 
discussion, including jigsaw-dev for this discussion.


The JDK source as specified in JEP 201 is under:
    src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java

Compiling the source files from the `src` directory are the intermediate 
input to build the resulting image.    Build tools are used to generate 
additional intermediate input (that is not part of the `src` directory) 
to build the image.   So I wonder if make/$MODULE/share/tools or 
make/tools/$MODULE  may be better location for the build tools.


Mandy

On 3/23/20 12:03 PM, Magnus Ihse Bursie wrote:
The build tools (small java tools that are run during the build to 
generate source code, or data, needed in the JDK) have historically 
been placed in the "make" directory. This maybe made sense long time 
ago, but does not do so anymore.


Instead, the build tools source code should move the the module that 
needs them. For instance, compilefontconfig should move to 
java.desktop, etc.


There are multiple reasons for this:

* Currently we build *all* build tools at once, which mean that we 
cannot compile java.base until e.g. the compilefontconfig tool is 
compiled, even though it is not needed.


* If a build tool, e.g. compilefontconfig is modified, all build tools 
are recompiled, which triggers a rebuild of more or less the entire 
JDK. This makes development of the build tools unnecessary tedious.


* When the build tools are modified, the group owning the 
corresponding module is the proper review instance, not the build 
team. But since they reside under "make", the review mails often 
include build-dev, but this is mostly noise for us. With this move, 
the ownership is made clear.


In this patch, I have not modified how and when the build tools are 
compiled, but this shuffle is the prerequisite for continuing with 
that in a follow-up patch.


I have also moved the build tools to the org.openjdk.buildtools.* 
package name space (inspired by Skara), instead of the strangely named 
build.tools.* name space.


A few build tools are not moved in this patch. Two of them, 
charsetmapping and cldrconverter, are shared between two modules. (I 
think they should move to modules nevertheless, but they need some 
more thought to make sure I do this right.) The rest are tools that 
are needed for the build in general, like linking or javadoc support. 
I'll move this to a better location too, but in a separate patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241463
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241463-move-build-tools-to-modules/webrev.01


/Magnus

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Mandy Chung]

On 12/5/19 12:41 AM, Alan Bateman wrote:

On 05/12/2019 08:16, Henry Jen wrote:

Hi,

Updated webrev[1] reflect comments since last webrev. Vicente had 
done all the heavy-lifting and hand over to me to finish up.


Changes to symbols is reverted, as we expect that only need to be 
updated in next release by running make/scripts/generate-symbol-data.sh.


The jar files are confusing in the webrev, but those files are 
removed. The whole test/jdk/tools/pack200 is removed.


Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/

The update webrev looks okay to me, except this part of the comment in 
flags-cflags.m4


"Now that unpack200 has been removed we should consider setting it for 
windows too. but this could be done as a follow-up effort. It could be 
that other other clients are relying on the current configuration for 
windows".


I think it would be best to create an infrastructure/build issue and 
leave most of this  confusing comment out.




I also think keeping flags-cflags.m4 as is and file a new build issue as 
a follow-up would be better.


Otherwise, this updated webrev looks okay to me.

Mandy

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Mandy Chung]

On 11/22/19 1:30 PM, Vicente Romero wrote:

[1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/


make/lib/Lib-jdk.pack.gmk should be removed.

make/scripts/compare_exceptions.sh.incl
    Should ./lib/libunpack.so also be removed?

make/scripts/compare.sh
    line 535-543 invokes unpack200.   You should get the advice from 
the build team what to be done with this file (or file a JBS issue as a 
follow-up)


Mandy

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Mandy Chung]

CSR needs to mention that jar -n option is removed.  I made minor edit 
to the CSR to state that jdk.pack module is removed.


Mandy

On 11/21/19 2:22 PM, Vicente Romero wrote:
please wait, I found some additional dependencies on module jdk.pack, 
will submit another webrev, sorry


Vicente

On 11/21/19 2:53 PM, Vicente Romero wrote:

Hi,

I think I have covered all the proposed fixes so far. This is the 
last iteration of the webrev [1], all the current changes are in this 
one, the code hasn't been split into different webrevs. I'm also 
forwarding to build-dev as there are some build related changes too. 
The CSR for this change is at [2]


Thanks for all the comment so far,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8234542/webrev.02/
[2] https://bugs.openjdk.java.net/browse/JDK-8234596



On 11/20/19 8:21 PM, David Holmes wrote:

Correction ...

On 21/11/2019 9:10 am, David Holmes wrote:

Hi Vicente,

Not sure the best mailing list for this review ... jdk-dev may not 
be well monitored.


Is there a separate review thread for the actual tool removal 
(jdk.pack) 


I overlooked the removal of jdk.pack (scrolling too fast through the 
webrev) - apologies.


David
-


and build system changes?

This removal seems okay, but I found one additional reference:

./src/utils/IdealGraphVisualizer/nbproject/project.properties:auxiliary.org-netbeans-modules-apisupport-installer.pack200-enabled=false 



Thanks,
David
-

On 21/11/2019 8:54 am, Vicente Romero wrote:

Hi,

I need a reviewer for the changes to remove pack200 and unpack200 
from the JDK. The webrev with the removal is at [1]. This patch is 
the "implementation" of JEP 367 [2]. The patch is basically 
removing the Pack200 related APIs plus its implementation plus any 
reference to it in other tools like `jar`. In the case of `jar`, 
Pack200 was only used if the `-n` flag was passed to the tool. I 
have removed the code that was executed when that flag was passed. 
I have also removed all the tests for Pack200.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8234542/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8232022

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[Mandy Chung]

On 10/22/19 3:37 PM, mark.reinh...@oracle.com wrote:

2019/10/22 14:12:18 -0700, mandy.ch...@oracle.com:

On 10/21/19 8:22 PM, mark.reinh...@oracle.com wrote:

RFE: https://bugs.openjdk.java.net/browse/JDK-8232080
CSR: https://bugs.openjdk.java.net/browse/JDK-8232753
Webrev: https://cr.openjdk.java.net/~mr/rev/8232080/

Looks good to me. One minor comment:

AddResourcePlugin isn't really a FILTER, probably best to add a new
category for new resource file. jlink hardcodes the order of the
categories that determines the order of the plugins to be executed (I
see the existing implementation could be cleaned up in the future).  I
think we didn't want to the options resource to be filtered by
--exclude-resources plugin and so the add-options plugin should run
after all filter plugins.

Good point -- that makes sense.

Relative patch below; original webrev updated in place.


Looks good.

Mandy

Re: 14 RFR (M) 8232080: jlink plugins for vendor information and run-time options

[Mandy Chung]

On 10/21/19 8:22 PM, mark.reinh...@oracle.com wrote:

RFE: https://bugs.openjdk.java.net/browse/JDK-8232080
CSR: https://bugs.openjdk.java.net/browse/JDK-8232753
Webrev: https://cr.openjdk.java.net/~mr/rev/8232080/



Looks good to me.  One minor comment:

AddResourcePlugin isn't really a FILTER, probably best to add a new 
category for new resource file.   jlink hardcodes the order of the 
categories that determines the order of the plugins to be executed (I 
see the existing implementation could be cleaned up in the future).   I 
think we didn't want to the options resource to be filtered by 
--exclude-resources plugin and so the add-options plugin should run 
after all filter plugins.


Mandy

Re: RFR: JDK-8232639: Change module graph images to use SVG instead of PNG format.

[Mandy Chung]

Looks good to me.

Mandy

On 10/18/19 3:12 PM, Jonathan Gibbons wrote:
Please review a small change in the build, to use .svg files instead 
of .png files,

for the generated module graph images.

The underlying generator, GraphViz, already supports .svg output.

In the proposed patch, the filename is changed in the generator tool,
and all references to "png" in make/Docs.gmk are converted to "svg".

-- Jon


JBS: https://bugs.openjdk.java.net/browse/JDK-8232639
Webrev: http://cr.openjdk.java.net/~jjg/8232639/webrev.00/index.html
Sample API docs: 
http://cr.openjdk.java.net/~jjg/8232639/api.00/api/index.html

Re: RFR: JDK-8231974: Build fails if no common legal notices are present

[Mandy Chung]

Looks okay to me.
Mandy

On 10/8/19 12:24 PM, Erik Joelsson wrote:
In certain Oracle build configurations, the common legal output 
directory is never created, because there are no legal files to put in 
it. This should not make the build fail. This patch makes adding the 
--legal-notices parameter to jmod optional.


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

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

/Erik

Re: RFR: JDK-8150741: make not equivalent to make

[Mandy Chung]

This looks right to me.

Mandy

On 9/20/19 11:11 AM, Erik Joelsson wrote:
This patch adds the currently missing dependencies between the top 
level meta-targets for each module (named just $module). Currently, we 
only add dependencies between the java targets for each module (as 
those are the only ones needed for compilation to work). With the 
added dependencies, it is now possible to do:


$ make java.compiler
$ build/linux-x86_64-normal-server-release/jdk/bin/javac

Where before you had to manually add at least "java.base" to that make 
command line.


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

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

/Erik

Re: RFR: 13,docs JDK-8228494 Update nroff version of man pages

[Mandy Chung]

jabswitch is a windows-only tool and so no need for nroff page. 
Otherwise looks fine.


Mandy

On 7/23/19 10:40 AM, Jonathan Gibbons wrote:

Please review an update to the nroff pages for JDK 13.

(There are 3 new pages and 2 modified pages.)

This is noreg-doc, so no further approvals are required for RDP2.

-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8228494
Webrev: 
http://cr.openjdk.java.net/~jjg/8228494-update-nroff-pages/webrev.00/

Re: RFR: XXS,doc JDK-8225496: typo in fix for JDK-8224257

[Mandy Chung]

+1.  Missed this in the review.

Mandy

On 6/7/19 4:36 PM, Jonathan Gibbons wrote:
The fix for  JDK-8224257 had a transposition typo, putting a ';' 
outside a quoted string instead of inside it.


JBS: https://bugs.openjdk.java.net/browse/JDK-8225496

-- Jon


$ hg diff -R open
diff -r fd61ef6c4091 
make/jdk/src/classes/build/tools/fixuppandoc/Main.java
--- a/make/jdk/src/classes/build/tools/fixuppandoc/Main.java Fri Jun 
07 14:32:48 2019 -0700
+++ b/make/jdk/src/classes/build/tools/fixuppandoc/Main.java Fri Jun 
07 16:31:17 2019 -0700

@@ -558,7 +558,7 @@
 out.write(style);
 out.write(m.group("after"));
 } else {
-    out.write(" style=\"font-weight: normal; 
text-align:left\"; ");
+    out.write(" style=\"font-weight: normal; 
text-align:left;\" ");

 out.write(attrs);
 }
 out.write(" scope=\"row\"");

Re: RFR: JDK-8224257: fix issues in files generated by pandoc

[Mandy Chung]

This patch looks fine to me.  Nothing is obviously not right.  I picked 
a couple of specs to sanity check and they look right to me.


Mandy

On 6/7/19 1:58 PM, Jonathan Gibbons wrote:
Please review an update to the tool used to post-process files 
generated by pandoc.


In HTML 5, it is not enough to put `scope="row` on selected cells in 
the table,
the cells must also be converted to ``. To counteract any 
potential change
in the visual appearance, the style attribute for the cell is also 
updated.


Tested by doing a visual comparison with meld of the files generated 
before/after

the change.

-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8224257
Webrev: http://cr.openjdk.java.net/~jjg/8225489-pandoc-tables/webrev.00/
Docs: http://cr.openjdk.java.net/~jjg/8225489-pandoc-tables/docs/

Re: RFR: docs JDK-8225134: Update man-page files

[Mandy Chung]

Bringing these pages to more recent version is better than
the current so outdated version even it's just a snapshot.

The list of new pages look okay (but I didn't validate the
content.)

Mandy

On 5/31/19 2:02 PM, Jonathan Gibbons wrote:
Please review a change to refresh the content of the man pages (*.1 
files) in the main repo,
to contain essentially the exact same content as found in the Oracle 
JDK man pages.


Going forward, the intent is to bulk-update these pages towards the 
end of each release
cycle, until we can eventually open source the underlying files from 
which the man pages

are derived, at which time these files will be deleted.

This email is to build-dev@ojn, even though there are no Makefile 
changes, because that
list includes the folk mostly likely to make direct use of these 
files.  It is also cc jdk-dev@ojn

so that the OpenJDK community is generally aware of this change.

Note, although the current man pages share a history with the previous 
versions of these
files, the transformations that have occurred between then and now 
make it all but
impossible to compare the versions side by side with diff. For those 
folk wanting to review
the content, it is best to look at the "New" or "Raw" files in the 
webrev.


-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8225134
Webrev: http://cr.openjdk.java.net/~jjg/8225134/webrev.00/webrev/

Re: RFR: P2: JDK-8225066 Add missing file

[Mandy Chung]

+1

Mandy

On 5/30/19 10:54 AM, Jonathan Gibbons wrote:
In creating the changeset for JDK-8224257, I forgot to 'hg add' a new 
file.


Please review this changeset to add the file;  the content of the file 
were previously reviewed as part of JDK-8224257.


JBS: https://bugs.openjdk.java.net/browse/JDK-8225066
Webrev: http://cr.openjdk.java.net/~jjg/8225066/webrev.00/

-- Jon

Re: RFR (XXS) 8224790: Remove Xusage.txt file

[Mandy Chung]

Looks good to me.

Mandy

On 5/25/19 1:49 PM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8224790
webrev: http://cr.openjdk.java.net/~dholmes/8224790/webrev/

Before Java 7 the Xusage.txt file was used to print the "java -X" help 
information in English. From Java 7 this information was provided in 
localised format by the launcher. The Xusage.txt file was not removed 
at the time because it was also used by the gamma launcher. But the 
gamma launcher was removed in java 8 - JDK-8008772. The information in 
Xusage.txt has become stale and incomplete (despite several edits post 
Java 7). It should just be removed.


Also removed the build logic that copied the file into the JDK image.

Thanks,
David

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Mandy Chung]

On 2/13/19 1:04 AM, Severin Gehwolf wrote:

 --strip-native-debug-symbols no-keep-debuginfo

--strip-native-debug-symbols strip-debuginfo
--strip-native-debug-symbols remove-debuginfo

It would avoid using negation.


What about 'omit', i.e.

--strip-native-debug-symbols  omit-debuginfo
--strip-native-debug-symbols  keep-debuginfo=


Sure. I've filed JDK-8218913 for doing this first. Would a name of --
strip-java-debug-symbols be acceptable? My thinking is that it would
have nice symmetry with --strip-native-debug-symbols. Thoughts?


--strip-native-debug-symbols is okay.

Having a second thought, javac -g and gcc -g both say "debugging
information".  I think we could go with:

--strip-java-debug-info
--strip-native-debug-info  omit-debuginfo
--strip-native-debug-info  keep-debuginfo=

unless anyone thinks `-debug-symbols` is better?

The options are getting pretty good now.

Mandy

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Mandy Chung]

On 2/12/19 11:52 AM, Severin Gehwolf wrote:

Hi Mandy, Alan,

Please find the proposal for CLI option of --strip-native-debug-symbols
below.

The current implementation here has the following options:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/

[i]   --strip-native-debug-symbols defaults
[ii]  --strip-native-debug-symbols options:objcopy-cmd=
[iii] --strip-native-debug-symbols options:debuginfo-file-ext=
[iv]  --strip-native-debug-symbols options:include-debug-syms=true

The first option is a work-around for JDK-8218761. AFAIUI, fixing it
will need rework of the Plugin interface and probably of the options
parsing. Hence, I'd like to defer this post integration of the initial
version of --strip-native-debug-symbols plugin.

Cases [iii] and [iv] can be folded into one as suggested by Mandy with:

--strip-native-debug-symbols keep-debuginfo
--strip-native-debug-symbols keep-debuginfo=

Case [ii] would become:

--strip-native-debug-symbols objcopy=


we could relax this to a command that can contain arguments.


So in summary I'd propose these, where a) and b) may be combined, c)
and a) or c) and b) combined would be an error:

[a] --strip-native-debug-symbols keep-debuginfo[=]
[b] --strip-native-debug-symbols objcopy=
[c] --strip-native-debug-symbols defaults


This is a good compromise.  When JDK-8218761 is implemented,
[c] can become `--strip-native-debug-symbols`

"defaults" is unclear to what it does.  What about
   --strip-native-debug-symbols no-keep-debuginfo


As a follow-up to an initial implementation of the above, I'd propose
to hook it up with the current --strip-debug by a follow-up patch. It
would first rename --strip-debug to --strip-debug-attribute or perhaps
--strip-java-debug-symbols, and then let --strip-debug perform java and
native debug symbols stripping as Alan suggested.


The renaming can be done separately.  I would prefer changing
--strip-debug to invoke --strip-native-debug-symbols, if present,
at the same time with this new strip native debug symbols plugin
to ensure that they all go in the same release.

In other words, the renaming should be done before this new plugin.
That's my opinion.

Mandy


Does that sound reasonable to you?

Thanks,
Severin

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Mandy Chung]

On 2/12/19 12:05 PM, Severin Gehwolf wrote:

Hi Mandy,

Thanks for the review!

OK. Here is the summary:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-February/014132.html

Personally, I find --strip-native-debug-info or --strip-native-debug-
symbols clearer than just --strip-native-debug.


Fine with me and we can stick with --strip-native-debug-symbols


W.r.t. the test:

  > The test currently only runs on Linux
  > and requires objcopy to be available. Otherwise the test is being
  > skipped.

We can create a fake objcopy utility for testing purpose
such that the test will validate if the options are passed
properly to the test utility.


AFAIK, objcopy is a build requirement for OpenJDK already, so I'm
wondering whether it makes sense to stub objcopy for the tests. Perhaps
you meant that to be used in addition to existing tests?


Test machines may not have objcopy.  It'd increase the test
coverage to include a test that can run on all test machines.
Yes it's an additional test in addition to a test you have
that only runs if objcopy utility is present.


Anyhow, I'll look into it. It'll likely have to use the '--strip-
native-debug-symbols objcopy=case. 


What I was thinking is:
--strip-native-debug-symbols objcopy="java fakeobjcopy"

where objcopy accepts a command that can contain arguments.

It'll have to be some native code which will require some custom

make machinery to get compiled, no? If you have pointers to examples in
the JDK already, I'd appreciate it.


See the idea above.


  > webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/

I haven't reviewed the new files.  Just notice that very long
lines in the new files that you may want to fix the formatting
that will help further side-by-side review.


Some of the longer lines have been cleaned up in the lates webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/05/webrev/


The --list-plugin output is very very long.  The existing plugins
didn't set a good example to keep the well formatted (I recorded
that they were cleaned up at one point to keep 80 chars width).


Right. I'll make sure --list-plugins looks right for --strip-native-
debug-symbols once we've agreed on the options.


One other question: should this plugin be moved to linux/classes
rather than unix/classes given that that's the platform it
intends to support?


Done in version 05.


Thanks for making the change.  I will reply the other thread.

Mandy

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Mandy Chung]

On 2/8/19 2:08 AM, Alan Bateman wrote:
I agree with Mandy that the CLI options need examination. The proposed 
`--strip-native-debug-symbols` seems reasonable but it will be 
inconsistent with the existing `--strip-debug` option. Mandy - what you 
would think about renaming the existing option to something that makes 
it clear that it strips the debug attribute from class files? (we would 
of course need to do something to keep the existing option working).


Renaming it to make it clear is good.  How about:

--strip-debug-attribute
--strip-native-debug-symbols

--strip-debug will invoke both --strip-debug-attribute and
--strip-native-debug-symbols, if supported.

Typically we want to strip both.  If only stripping debug attribute,
then it can use --strip-debug-attribute.

What do you think?

The need to specify the argument as "defaults" or "options" is a bit 
annoying. It might be time to replace hasArguments in the plugin  > interface to allow for optional arguments.


Hmm... I thought it allows optional arguments.  LegalNoticeFilePlugin
accepts an optional argument.

On the other hand, pluginToMaps will put an empty map if hasArguments
return false.  The plugin processing code (PluginsHelper) is quite
complicated that I can't quite tell without spending time.

In any case I think a plugin should support optional arguments.

The plugin interface is not 
exported/documented/supported so we have flexibility to change it. If we 
do this then it should mean the usages reduce down to:


--strip-native-debug-symbols
--strip-native-debug-symbols objcopy=:...


objcopy is fine.

It would also be nice to allow `keep-debuginfo` taking an optional
file extension.

--strip-native-debug-symbols keep-debuginfo
--strip-native-debug-symbols keep-debuginfo=dbg



I see the plugin has moved to src/jdk.jlink/unix in this iteration. It 
might be better to start out in src/jdk.jlink/linux - we can always move 
to the unix tree in the event that there is interest/support on other 
platforms.


Agree.

Mandy points out the issue with wrapping in the usage output. I agree 
that the`jlink --list-plugins` needs to be readable/tidy esp. in this 
case as there are many sub-options to read about.


I file JDK-8218685 for --list-plugins tidy up.


Mandy

Re: RFR: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Mandy Chung]

Hi Severin,

Using the plugin specific ResourceBundle is good.  Thanks for making
the change.

I see that Alan's comment [1] on the plugin options and I assume
you will investigate the plugin options and bring back a proposal.
Did I miss the discussion on these options?

Option: 
--strip-native-debug-symbols=][:debuginfo-file-ext=][:include-debug-syms=true]>


Examples: --strip-native-debug-symbols=defaults

I suggest the above be simplified and drop the "=defaults".  Simply:
   --strip-native-debug-symbols

Examples: 
--strip-native-debug-symbols=options:objcopy-cmd=/usr/bin/objcopy:debuginfo-file-ext=dbg:include-debug-syms=true 



If include-debug-syms=true then it will run
  objcopy --only-keep-debug foo foo. to create debug symbols file
  objcopy --add-gnu-debuglink=foo.dbg foo

So what about simplifying the options to:

--strip-native-debug-symbols=keep-debug-symbols=dbg,objcopy-path=/usr/bin/objcopy

We could also drop the word "symbols":


--strip-native-debug=[keep-debug=,][objcopy-path=]

By default, `--strip-native-debug` will strip native debug symbols
and don't keep debug symbols.

keep-debug= creates the debug symbols file.
 specifies the file extension.  It would be
nice to use the default `debuginfo` extension and simply
accept `keep-debug` option.

`objcopy-cmd` may be okay too.  Others may have opinion.

I think we should agree on the plugin options first before
doing the code review.

W.r.t. the test:

> The test currently only runs on Linux
> and requires objcopy to be available. Otherwise the test is being
> skipped.

We can create a fake objcopy utility for testing purpose
such that the test will validate if the options are passed
properly to the test utility.

> webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796/04/webrev/


I haven't reviewed the new files.  Just notice that very long
lines in the new files that you may want to fix the formatting
that will help further side-by-side review.

The --list-plugin output is very very long.  The existing plugins
didn't set a good example to keep the well formatted (I recorded
that they were cleaned up at one point to keep 80 chars width).

One other question: should this plugin be moved to linux/classes
rather than unix/classes given that that's the platform it
intends to support?

Mandy
[1] 
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-January/014099.html

Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Mandy Chung]

On 1/28/19 1:26 AM, Severin Gehwolf wrote:

Hi Alan, Mandy,

On Sun, 2019-01-27 at 08:14 +, Alan Bateman wrote:

On 26/01/2019 00:06, Mandy Chung wrote:

Hi Severin,

Another alternative would be to support per-jlink-plugin resource
bundle to avoid merging .properties files at build time.  The
plugin-specific messages should only be used by the plugin itself
and it would be cleaner for each plugin to manage its resource bundle.

That seems a cleaner idea than the suggestion we had on jigsaw-dev to
merge the properties. If Severin does this for this plugin then I assume
the resources for the other plugins could be moved to their own resource
files in their own time.

Alright. I'll try this then.


One simplest way is to have StripNativeDebugSymbolsPlugin loads its own 
ResourceBundle.


I would add a new PluginsResourceBundle::getMessage method taking a 
ResourceBundle object

that StripNativeDebugSymbolsPlugin can call.

This is a patch for PluginsResourceBundle that you can use.

diff --git 
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java 
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
--- 
a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java
+++ 
b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/PluginsResourceBundle.java

@@ -62,7 +62,11 @@
 }

 public static String getMessage(String key, Object... args) throws 
MissingResourceException {

-    String val = pluginsBundle.getString(key);
+    return getMessage(pluginsBundle, key, args);
+    }
+
+    public static String getMessage(ResourceBundle rb, String key, 
Object... args) throws MissingResourceException {

+    String val = rb.getString(key);
 return MessageFormat.format(val, args);
 }
 }

Mandy

Re: Help with build changes for: 8214796: Create a jlink plugin for stripping debug info symbols from native libraries

[Mandy Chung]

Hi Severin,

Another alternative would be to support per-jlink-plugin resource
bundle to avoid merging .properties files at build time.  The
plugin-specific messages should only be used by the plugin itself
and it would be cleaner for each plugin to manage its resource bundle.

Mandy

On 1/25/19 7:27 AM, Severin Gehwolf wrote:

Hi,

I'm working on an enhancement for jlink. In particular a platform
specific plugin. I.e. It would only get built on unix/linux platforms.
My trouble is getting some resouce properties set up properly. In my
example there is two versions of plugins.properties: one in
shared/classes one in unix/classes. These, need to get merged into one
via the MergeProperties build tool class. So far so good. But for the
cases where there is only one resource property it should just get
copied to support/gensrc and that source root be used for compiling
those properties into actual ListResourceBundle Java classes.

The copying from the src tree to the gensrc tree doesn't seem to work.
I've tried using $(CP) and SetupCopyFiles to no avail. Would anybody
willing to help?

WIP webrev is:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8214796-wip/01/webrev/

I thought I'd do the copying in an else branch in SetupPropertiesMerge,
where I've put in a FIXME comment. Am I on the wrong track?

Thanks,
Severin

Re: JDK 13 RFR of JDK-8205626: Start of release updates for JDK 13

[Mandy Chung]

Looks good.

Mandy

On 12/5/18 9:28 AM, joe darcy wrote:

Hello,

With the inception of JDK 13 right around the corner, please review 
the build-related changes of the update:


    http://cr.openjdk.java.net/~darcy/jdk13-fork.2

As usual, javac defines a new -source/-target 13 which is used in the 
build of the libraries. A new class file version is defined and the 
feature version is updated.


Other aspects of the changes will be reviewed on other mailing lists, 
compiler-dev, etc.


Thanks,

-Joe

Re: RFR 8214794 : java.specification.version should be only the major version number

[Mandy Chung]

The revised webrev looks okay.

Mandy

On 12/4/18 11:32 AM, Roger Riggs wrote:

Hi Mandy, Martin,

The new test is unnecessary, the case is covered by 
java/lang/System/Versions test

and uses the stronger comparison for the version numbers.

It would not detect the problem unless the version included more than 
the major version.


Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/

Thanks, Roger

On 12/04/2018 01:41 PM, Mandy Chung wrote:



On 12/4/18 8:16 AM, Roger Riggs wrote:

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/



Looks okay.   I agree with Martin to go with a stronger assertion 
without converting into a number. 
test/jdk/java/lang/System/Versions.java looks like also covering this 
test case.  At some point it'd be good to consolidate these two tests.


Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc 
are a relevant group.   VERSION_SPECIFICATION can be moved to group 
with VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.


Mandy

Re: RFR 8214794 : java.specification.version should be only the major version number

[Mandy Chung]

On 12/4/18 8:16 AM, Roger Riggs wrote:

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/



Looks okay.   I agree with Martin to go with a stronger assertion 
without converting into a number. 
test/jdk/java/lang/System/Versions.java looks like also covering this 
test case.  At some point it'd be good to consolidate these two tests.


Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are 
a relevant group.   VERSION_SPECIFICATION can be moved to group with 
VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.


Mandy

Re: RFR 4947890 : Minimize JNI upcalls in system property initialization

[Mandy Chung]

Looks good to me.

Mandy

On 11/27/18 1:00 PM, Roger Riggs wrote:

Hi,

Updated webrev:
http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw-4/index.html

- Updated defaults for VENDOR, VENDOR_URL, VENDOR_URL_BUG to:
  COMPANY_NAME=N/A
  VENDOR_URL=https://openjdk.java.net/
  VENDOR_URL_BUG=https://bugreport.java.com/bugreport/

- Added Brent's suggestion to verify FIXED_LENGTH in the test.

- Ensure that configure passes the --vendor-... arguments through to
  initialize the system properties.

Thanks to all the reviewers.

Roger

Re: RFR 4947890 : Minimize JNI upcalls in system property initialization

[Mandy Chung]

Hi Roger,

Looking good.  I have a few small comments:

I assume VM.saveAndRemoveProperties will be a separate cleanup.
SystemProps::cmdProperties adds the system properties that can
skip adding these internal properties to the Properties object.
In fact, these internal properties are the interface between VM
and libraries that can be replaced with a different mechanism
other than system properties.

Suggest to move the call to VersionProps.init(props) in
SystemProps.initProperties

The `VENDOR*` build time variables can always have a default
like the other constants in VersionProps.  Then no need to
handle if not set.

SystemProps.staticInitOnly_* are a bit odd.  They are temporarily
set with the values and then reset to null.  Perhaps leave
StaticProperty as is for now and re-visit it in the future.
Then I think SystemProps::putDefault can be removed.

Raw::xxx_NDX are initialized to 1 + previous_NDX.  It's a general
good approach to increment the index but I find it error-prone and
hard to catch mistake since the (adjacent) variable names look
so alike. Perhaps some form of verification or assertion to ensure
the indices are correctly initialized.

Mandy

On 11/16/18 10:49 AM, Roger Riggs wrote:

Updates to include the suggestions made by Mandy and Brent:

 - Move the build-time properties from native code to the 
VersionProps.java.template
   including java.vendor.* and java.specification.* properties, plus 
the java.class.version (major.minor)
   This included two changes to the build that generates source of 
VersionProps.java.


 - Updated the StaticProperty initialization to be explicitly invoked 
by initProperties.


 - Makes separate calls to native to retrieve the platform properties 
and VM/command line properties


 - (The hotspot function for JVM_GetProperties are unchanged)

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

Issue:
https://bugs.openjdk.java.net/browse/JDK-4947890

Thanks, Roger

Re: RFR: 8061282: Migrate jmh-jdk-microbenchmarks into the JDK

[Mandy Chung]

Looks good.

A small comment: make test TEST=micro always builds the native libraries 
for the tests.  It'd be
nice if running microbenchmarks does not depend on the target of 
building native test libraries.

Perhaps you can file a RFE as a follow up.

Mandy

On 11/16/18 4:43 AM, Claes Redestad wrote:

Hi Mandy,

I've updated copyright headers of all microbenchmarks to use the 
correct one, and fixed a few instances of long lines that stood out. 
We can do more cleanups as a follow-up.


Currently no microbenchmark includes any resources, and as you say the 
current convention is to keep resources co-located. I've removed the 
classes and resources folders, and updated makefiles with the trivial 
changes needed for this.


The JEP text also referred to `make run-test`, which is now just `make 
test`. Updated.


To help set thing up JDK-8061281 includes examples and setup 
instructions in docs/testing.md|html - I've added a reference to this 
documentation in the JEP text. I think using the JEP as the 
authoritative place to document usage wouldn't age well, as 
instructions typically evolve and change over time.


Combined webrev for 8061281 and 8061282 here:
http://cr.openjdk.java.net/~redestad/8061281_8061282.01/

Thanks!

/Claes

On 2018-11-15 23:50, Mandy Chung wrote:

Hi Claes,

It's good to see this JEP targeted and integrate the microbenchmarks 
to colocate with JDK.  Overall the work looks good.


The copyright headers need update to GPL.  There are some super long 
lines (mostly looking up method handles), for example:
+    MethodHandle bodyNormal = 
MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, 
"doWorkNormal", MethodType.methodType(void.class, 
MethodHandlesCatchException.class));
+    MethodHandle bodyExceptional = 
MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, 
"doWorkExceptional", MethodType.methodType(void.class, 
MethodHandlesCatchException.class));
+    MethodHandle fallback = 
MethodHandles.lookup().findStatic(MethodHandlesCatchException.class, 
"fallback", MethodType.methodType(void.class, MyException.class, 
MethodHandlesCatchException.class));
JEP 230 proposes to separate the resources from the source files in 
micro/classes and micro/resources directories.  What kinds of 
resources are expected to be placed under micro/resources directory?  
If they are java resources, then I would expect them follow the 
consistent layout as JDK source tree where the java classes and 
resources are placed together.


I was trying to experiment building and running the benchmarks. What 
does configure --with-jmh expect to contain?  I can't quite figure it 
out from the error message.


The JEP describes make build-microbenchmark and run-test targets to 
build and execute the microbenchmarks.


It'd also be helpful to update the JEP to include an example how to 
run a specific set of benchmarks e.g. 
org.openjdk.bench.java.lang.ObjectHashCode and how to run the 
benchmarks with JDK n and JDK n-1 and compare the result (is there a 
build target to do this)?   We can reference this JEP to get started 
running the microbenchmark and refer to JMH and other documentation 
for other details like developing a JMH benchmark.


Mandy

On 10/18/18 2:03 PM, Claes Redestad wrote:

Hi,

as the final part of JEP 230: Microbenchmarks Suite, I propose 
migrating all microbenchmarks from the codetools 
jmh-jdk-microbenchmarks project into the JDK:


http://cr.openjdk.java.net/~redestad/8061282/jdk.00/
This is built on top of the patch for JDK-8061281, and makes the 
entirety of this suite readily available to build, run and 
experiment with from the main jdk repos.


While the future of the codetools jmh-jdk-microbenchmarks project is 
out of scope for JEP 230, it has been suggested it could be kept 
alive as a stabilization target and that stable microbenchmarks 
should be kept out of the jdk. That discussion is partly out of 
scope here, but I believe it makes sense to keep a copy in the JDK 
suite precisely because the benchmark will be compiled with the 
platform javac, meaning a different set of bugs, regressions and 
improvements will be discoverable.


Two micros, org.openjdk.bench.java.lang.invoke.Indify and 
org.opendjk.bench.java.lang.reflect.GetAnnotation need special build 
treatment and will need to be dealt with in a follow-up.


Thanks!

/Claes

Re: RFR(M) 8213348: jdk.internal.vm.compiler.management service providers missing in module descriptor

[Mandy Chung]

On 11/8/18 12:37 PM, dean.l...@oracle.com wrote:

On 11/8/18 11:49 AM, Mandy Chung wrote:

This is strange.  upgrade module path precedes the application module
path in the search path.  I would not expect --module-path would help.



Sorry, the error is if I remove both --upgrade-module-path and 
--module-path.

Putting back either one makes it work again, so I can remove either one.
I don't remember why I added --module-path, but it was probably when I was
experimented with using --module-source-path instead of -sourcepath.
Should I go ahead and remove --module-path? 


+1.  No need to set --module-path.


Can you get the full javac command to see what are being compiled
and all the paths set in these options?

It's very long, but I can summarize.  -sourcepath contains source 
paths under

src/jdk.internal.vm.compiler only.  The files we are compiling come from
_gensrc_proc_files, which contains only files under 
jdk.internal.vm.compiler.




Since no change in javac command after removing --module-path, we are 
all set.


Mandy


dl


Mandy

On 11/8/18 11:28 AM, dean.l...@oracle.com wrote:

I get this without --module-path:

  Fatal Error: Unable to find package java.lang in classpath or 
bootclasspath


I guess before, when it was using the unnamed module, that it was 
picking up these

system classes from the jdk10 bootjdk, which doesn't seem ideal.

It works without --upgrade-module-path, so I can remove that.

dl

On 11/7/18 8:12 PM, Mandy Chung wrote:

Hi Dean,

On 11/7/18 7:56 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8213348
https://bugs.openjdk.java.net/browse/JDK-8211781
http://cr.openjdk.java.net/~dlong/8213348/webrev/


108 --module-path $(JDK_OUTPUTDIR)/modules \
  109 --upgrade-module-path $(JDK_OUTPUTDIR)/modules --system none \

You added `--module-path` option to javac.  What error did you
get without it?  It looks a bit suspicious.  I wonder if
the `--upgrade-module-path` is needed as --system none is set.

We need Jon to advice on this javac command.

Mandy

Re: RFR(M) 8213348: jdk.internal.vm.compiler.management service providers missing in module descriptor

[Mandy Chung]

This is strange.  upgrade module path precedes the application module
path in the search path.  I would not expect --module-path would help.

Can you get the full javac command to see what are being compiled
and all the paths set in these options?

Mandy

On 11/8/18 11:28 AM, dean.l...@oracle.com wrote:

I get this without --module-path:

  Fatal Error: Unable to find package java.lang in classpath or 
bootclasspath


I guess before, when it was using the unnamed module, that it was 
picking up these

system classes from the jdk10 bootjdk, which doesn't seem ideal.

It works without --upgrade-module-path, so I can remove that.

dl

On 11/7/18 8:12 PM, Mandy Chung wrote:

Hi Dean,

On 11/7/18 7:56 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8213348
https://bugs.openjdk.java.net/browse/JDK-8211781
http://cr.openjdk.java.net/~dlong/8213348/webrev/


108 --module-path $(JDK_OUTPUTDIR)/modules \
  109 --upgrade-module-path $(JDK_OUTPUTDIR)/modules --system none \

You added `--module-path` option to javac.  What error did you
get without it?  It looks a bit suspicious.  I wonder if
the `--upgrade-module-path` is needed as --system none is set.

We need Jon to advice on this javac command.

Mandy

Re: RFR(M) 8213348: jdk.internal.vm.compiler.management service providers missing in module descriptor

[Mandy Chung]

Hi Dean,

On 11/7/18 7:56 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8213348
https://bugs.openjdk.java.net/browse/JDK-8211781
http://cr.openjdk.java.net/~dlong/8213348/webrev/


108 --module-path $(JDK_OUTPUTDIR)/modules \
 109 --upgrade-module-path $(JDK_OUTPUTDIR)/modules --system none \

You added `--module-path` option to javac.  What error did you
get without it?  It looks a bit suspicious.  I wonder if
the `--upgrade-module-path` is needed as --system none is set.

We need Jon to advice on this javac command.

Mandy

Re: RFR: XXS: JDK-8213102: Oracle Unilinks are [301 Moved Permanently] to https://docs.oracle.com

[Mandy Chung]

+1

Mandy

On 10/29/18 11:57 AM, Jonathan Gibbons wrote:
Please review the following simple fix to change the host for Oracle 
Unilinks from www.oracle.com to docs.oracle.com to avoid unnecessary 
redirects.


JBS: https://bugs.openjdk.java.net/browse/JDK-8213102

The fix was done with the following command:

grep -rl oracle.com/pls open/make | xargs sed --in-place 
's|www.oracle.com/pls|docs.oracle.com/pls|g'


-- Jon


Patch inline:

$ hg diff -R open
diff -r 3152b928769d make/Docs.gmk
--- a/make/Docs.gmk Mon Oct 29 11:05:45 2018 -0700
+++ b/make/Docs.gmk Mon Oct 29 11:45:11 2018 -0700
@@ -61,7 +61,7 @@
 $(SUPPORT_OUTPUTDIR)/rmic/* $(TOPDIR)/src/*/share/doc/stub)

 # URLs
-JAVADOC_BASE_URL := 
https://www.oracle.com/pls/topic/lookup?ctx=javase$(VERSION_NUMBER)id=homepage
+JAVADOC_BASE_URL := 
https://docs.oracle.com/pls/topic/lookup?ctx=javase$(VERSION_NUMBER)id=homepage

 BUG_SUBMIT_URL := https://bugreport.java.com/bugreport/
 COPYRIGHT_URL := {@docroot}/../legal/copyright.html
 LICENSE_URL := 
https://www.oracle.com/technetwork/java/javase/terms/license/java$(VERSION_NUMBER)speclicense.html

diff -r 3152b928769d make/jdk/src/classes/build/tools/taglet/ExtLink.java
--- a/make/jdk/src/classes/build/tools/taglet/ExtLink.java Mon Oct 29 
11:05:45 2018 -0700
+++ b/make/jdk/src/classes/build/tools/taglet/ExtLink.java Mon Oct 29 
11:45:11 2018 -0700

@@ -48,7 +48,7 @@
  * will produce the following html
  * 
  * {@code
- * Please see href="https://www.oracle.com/pls/topic/lookup?ctx=javase10=Borealis;>a 
spectacular sight.
+ * Please see href="https://docs.oracle.com/pls/topic/lookup?ctx=javase10=Borealis;>a 
spectacular sight.

  * }
  */
 public class ExtLink implements Taglet {
@@ -63,7 +63,7 @@

 static final String TAG_NAME = "extLink";

-    static final String URL = 
"https://www.oracle.com/pls/topic/lookup?ctx=javase; +
+    static final String URL = 
"https://docs.oracle.com/pls/topic/lookup?ctx=javase; +

 SPEC_VERSION + "id=";

 static final Pattern TAG_PATTERN = 
Pattern.compile("(?s)(\\s*)(?\\w+)(\\s+)(?.*)$");

Re: RFR(S): 8211350: Remove jprt support

[Mandy Chung]

+1

Mandy

On 10/2/18 11:17 AM, Mikael Vidstedt wrote:


Thanks for the reviews. I’ve reverted the changes related to Helper 
and “just” adjusted the comments instead.


webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8211350/webrev.01/open/webrev/ 

incremental (from webrev.00): 
http://cr.openjdk.java.net/~mikael/webrevs/8211350/webrev.01.incr/open/webrev/ 




Btw, I notice that "Test not run, NO jmods directory” will be printed 
twice when jmods is missing - once in Helper::newHelper and once in 
the methods calling it. In general, the handling of a null return from 
newHelper could use some clean up, but that is out of scope for this 
change.


Cheers,
Mikael

Re: RFR(S): 8211350: Remove jprt support

[Mandy Chung]

On 10/2/18 12:21 AM, Mikael Vidstedt wrote:

Please review this change which removes support for, and references to, the 
(Oracle internal) JPRT system.

bug: https://bugs.openjdk.java.net/browse/JDK-8211350
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8211350/webrev.00/open/webrev/



test/jdk/tools/lib/tests/Helper.java is used by test/jdk/tools/jimage 
and test/jdk/tools/jlink tests which will be skipped, rather than 
failing, when running with images where jmods directory is not present 
(e.g. exploded image).


I think Helper class should continue to return null if jmods does not 
exist.  test/jdk/tools/jimage/JImageTest.java should also be reverted.


Otherwise, the clean up looks good.

Mandy

Re: RFR: JDK-8210731 PropertiesParser does not produce reproducible output

[mandy chung]

Looks okay to me.

Mandy
P.S. I cc'ed compiler-dev since I think you meant to cc compiler-dev 
instead of hotspot-compiler-dev.


On 9/13/18 3:20 PM, Magnus Ihse Bursie wrote:
The file make/langtools/tools/propertiesparser/PropertiesParser.java 
b/make/langtools/tools/propertiesparser/PropertiesParser.java is used 
to convert .properties files into .java files as part of the gensrc step.


However, due to it's use of creating it's output directly from 
HashMaps, it's not guaranteed to be stable, and is causing spurios 
differences in our cmp-baseline builds.


Bug: https://bugs.openjdk.java.net/browse/JDK-8210731
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8210731-properties-parser-is-not-stable/webrev.01


/Magnus

Re: RFR: JDK-8210704 Remove dead build tools hasher and jarreorder

[mandy chung]

On 9/13/18 1:46 AM, Magnus Ihse Bursie wrote:

On 2018-09-13 10:35, Magnus Ihse Bursie wrote:
The two build tools hasher and jarreorder is not used in the build 
anymore and should be removed.


Bug: https://bugs.openjdk.java.net/browse/JDK-8210704
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8210704-remove-hasher-and-jarreorder/webrev.01


Updated webrev that also removes the dead declaration for 
TOOL_GENMODULESXML. (The tool itself was previously removed)


http://cr.openjdk.java.net/~ihse/JDK-8210704-remove-hasher-and-jarreorder/webrev.02 



Looks good.  Thanks for cleaning up TOOL_GENMODULESXML which was a 
temporary tool that we missed to remove.


Mandy

[12] RFR JDK-8167314: Enable the check to detect duplicate provides in in GenModuleInfoSource

[mandy chung]

A simple patch to enable the check to enforce no duplicate provides
in module-info.java.extra, same checks as javac.  This check
was disabled in the fix for JDK-8202941 because of duplicated
provides generated for jdk.internal.vm.compiler.  Since JDK-8208463
is now resolved [1], we can enable this check.

Webrev:
  http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8167314/webrev.00/

Mandy
[1] http://hg.openjdk.java.net/jdk/jdk/rev/c5461fe16efb

Re: [12] RFR(XS) 8208463: jdk.internal.vm.compiler's module-info.java.extra contains duplicated provides of the same service interface

[mandy chung]

Hi Vladimir,

This looks good.  I verified with my patch that enforces non-duplicate
provides check and it builds successfully.  Thanks for fixing it.

thanks
Mandy

On 8/13/18 5:06 PM, Vladimir Kozlov wrote:

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

Before we did not care how module-info.java.extra is generated. The code 
in Gensrc-jdk.internal.vm.compiler.gmk  make file simple ordered 
'providers' files (which are packages names) by name and then use their 
contents to generate 'provides' instructions. If following file in 
sorted list reference the same package it will be added to the same 
'provides' as for previous file. But it did not guarantee that all 
referenced packages are listed in the same 'provides' command.


The fix is to sort files not by their name but by their content. It 
guarantee that files with the same content will be listed together.


Run tier1-3 testing. It included Graal JIT testing and AOT (which uses 
Graal) testing.


Thanks,
Vladimir

diff -r ae001a1deb74 make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk
--- a/make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk
+++ b/make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk
@@ -124,7 +124,7 @@
  ($(CD) $(GENSRC_DIR)/META-INF/providers && \
  p=""; \
  impl=""; \
-    for i in $$($(LS) | $(SORT)); do \
+    for i in $$($(GREP) '^' * | $(SORT) -t ':' -k 2 | $(SED) 
's/:.*//'); do \

    c=$$($(CAT) $$i | $(TR) -d '\n\r'); \
    if test x$$p != x$$c; then \
  if test x$$p != x; then \

[12] RFR JDK-8202941: GenModuleInfoSource build tool does not detect missing semicolons

[mandy chung]

GenModuleInfoSource build tool augments module-info.java to include
platform-specific qualified exports/opens and uses/provides services.
This fixes the build tool to catch syntax errors as closely as javac
does. The build tool remains to allow duplicated provides until
JDK-8208463 is resolved.

http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8202941/webrev.00/

Mandy

Re: RFR: JDK-8206323: Missing some legal notices in docs bundle

[mandy chung]

+1

Mandy

On 7/9/18 9:40 AM, Erik Joelsson wrote:

Hello,

The docs bundle is missing some of the legal notices from the 
jdk.javadoc module. It should just include all the legal files from that 
module. This patch removes the explicit list and uses wildcard to get 
all the files instead.


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

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

/Erik

Re: RFR(L): 8202035: Archive the set of ModuleDescriptor and ModuleReference objects for system modules

[mandy chung]

Hi Jiangli,

On 6/28/18 4:15 PM, Jiangli Zhou wrote:> webrev: 
http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/

RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921


Good work.  I'm glad to see a pretty good startup improvement.

I reviewed java.base change that looks good.

Mandy