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

[Alan Bateman]

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

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.

-

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

Re: pre-submit tests for github PRs

[Alan Bateman]

On 22/05/2022 23:58, David Holmes wrote:


GHA tests a range of OpenJDK ports, not just the "mainstream platforms".

The existing linux-86 failures (and others) are due to the Loom 
integration which only fully supports a couple of platforms and which 
broke all the other ports upon initial integration. Some folks in the 
community have been working through things to fix that.


GHA is configured to cross build for most of these ports/configurations 
and they should pass. Running them is another matter of course. GHA does 
run some tests on linux-x86 and the x86_32 port does need work before it 
can run the tests with --enable-preview. Aleksey Shipilëv seems to be 
making good progress on it.


-Alan

Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled

[Alan Bateman]

On Wed, 11 May 2022 12:47:08 GMT, Jaikiran Pai  wrote:

>> src/java.base/share/native/libzip/zlib/gzwrite.c line 452:
>> 
>>> 450: len = strlen(next);
>>> 451: #  else
>>> 452: #   ifdef __APPLE__ // ignore format-nonliteral warning on macOS
>> 
>> Instead of patching 3rd party code to fix a compilation warning, you should 
>> disable that warning instead.
>> 
>> In `make/modules/java.base/lib/CoreLibraries.gmk`, add 
>> 
>> DISABLED_WARNINGS_clang := format-nonliteral, \
>> 
>> as line 138.
>
> Thank you for these useful inputs Magnus. I did these changes locally but for 
> some reason this format-nonliteral is not getting picked up while building 
> that library. I will investigate and see what's going on. Will update the PR 
> once I figure it out.

I agree with Magnus and try to avoid changing the imported zlib code.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Alan Bateman]

On Wed, 11 May 2022 08:40:21 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Avoid pragma error in before GCC 12

src/java.base/unix/native/libjli/java_md_common.c line 135:

> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) return 
> 0;
> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, 
> cmd);
> 135: #pragma GCC diagnostic pop

Can we just replace this code rather than putting pragmas here?

-

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

Re: VS2017 build errors jdk/jdk

[Alan Bateman]

On 10/05/2022 13:57, erik.joels...@oracle.com wrote:


Because of how much specific logic we need in configure for each 
version of Visual Studio, we have generally kept configure support for 
older versions of VS for a long time, to not unnecessarily limit 
people. We removed 2015 and older when we moved to C++11 in Hotspot as 
2017 or later was a hard requirement.


Which compiler versions actually work depends on if anyone wants to 
keep up with the maintenance to keep them working. If VS2017 becomes 
unusable and nobody wants or is able to fix it, then I agree that we 
should remove support from configure.


I don't think the logic in the build is the main concern, instead it's 
requiring all changes to shared + windows code to be buildable with 
VS2017 is a tax. So hopefully support for this version can be dropped soon.


-Alan.

Re: VS2017 build errors jdk/jdk

[Alan Bateman]

On 10/05/2022 09:29, Baesken, Matthias wrote:

Hello, it seems jdk/jdk does not build any more with VS2017.
Should we still support this compiler ?

For the error see :

https://bugs.openjdk.java.net/browse/JDK-8286459
8286459: compile error with VS2017 in continuationFreezeThaw.cpp

In Oracle, we moved from using VS2019 16.9.3 to VS2022 17.1.0 a few 
months ago.


I checked the Microsoft site to get the status of VS2017. It says that 
the "Mainstream End Date" for that release was April 2022 so I guess 
it's not easy to get updates without extended support now.


So maybe it time to look at it, it might be easier to have a smaller set 
of VS releases to support.


-Alan

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]

[Alan Bateman]

On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken  wrote:

>> Currently we set _WIN32_WINNT at various places in the codebase; this is 
>> used to target a minimum Windows version we want to support. See also for 
>> more detailled information :
>> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
>> Macros for Conditional Declarations
>> "Certain functions that depend on a particular version of Windows are 
>> declared using conditional code. This enables you to use the compiler to 
>> detect whether your application uses functions that are not supported on its 
>> target version(s) of Windows."
>> 
>> However currently we have quite a lot of differing settings of _WIN32_WINNT 
>> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible 
>> would make sense because we have this setting already at   java_props_md.c  
>> (so targeting older Windows versions at other places is most likely not 
>> useful).
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   adjust API level to Windows 8 for security.cpp and do some cleanup

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8282060: RemoteRuntimeImageTest is not actually testing on JDK 8

[Alan Bateman]

On Thu, 5 May 2022 09:00:47 GMT, Athijegannathan Sundararajan 
 wrote:

> This test requires jdk8 to be available while running jdk tests. But 
> JDK8_HOME is defined to be BOOT_JDK and so version check fails in the test. 
> The test vacuously passes just printing a message. There are already tests 
> that exercise jrt-fs.jar on the same JDK being tested and so basic jrt-fs.jar 
> is covered for same target JDK case.

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v3]

[Alan Bateman]

On Thu, 28 Apr 2022 07:13:24 GMT, Matthias Baesken  wrote:

>> src/java.base/windows/native/libnio/ch/wepoll.c line 159:
>> 
>>> 157: 
>>> 158: #undef _WIN32_WINNT
>>> 159: #define _WIN32_WINNT 0x0601
>> 
>> This is 3rd party code and would prefer not change it if possible.
>
> Hi Alan, I agree (thats why I did not change the define in harfbuzz,  but I 
> missed that wepoll.c is 3rd party code as well).

I assume you can revert the update to the copyright header in the latest 
version as there aren't any changes to this source file now.

-

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

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

[Alan Bateman]

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

Marked as reviewed by alanb (Reviewer).

-

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

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

[Alan Bateman]

On Tue, 12 Apr 2022 10:24:47 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:
> 
>   Add @ForceInline annotation on session accessor
>   Beef up UnrolledAccess benchmark

src/java.base/share/classes/java/nio/channels/FileChannel.java line 1052:

> 1050: public MemorySegment map(MapMode mode, long offset, long size,
> 1051:   MemorySession session)
> 1052: throws IOException, UnsupportedOperationException

Just a minor here is that UOE is a runtime exception so probably shouldn't be 
in the throws.

-

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 [v4]

[Alan Bateman]

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

Thanks for the update, the src changes in c5fef46b look okay.

-

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 [v3]

[Alan Bateman]

On Thu, 7 Apr 2022 21:08: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:
> 
>   revert Module::isOpen

The updated changes to Class and Module in 912896d7 look okay, just minor 
comments.

src/java.base/share/classes/java/lang/Class.java line 3007:

> 3005: if (callerModule == null) {
> 3006: // no caller
> 3007: return thisModule.isOpen(pn);

It might be clear if the comment read "no caller, return true if the package is 
open to all modules".

src/java.base/share/classes/java/lang/Module.java line 1658:

> 1656: if (getPackages().contains(pn)) {
> 1657: if (caller == null) {
> 1658: if (! isOpen(pn)) {

Minor nit, can you remove the space in "! isOpen".

-

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 [v2]

[Alan Bateman]

On Thu, 7 Apr 2022 00:56:42 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:
> 
>   update copyright date

Tim - this creates a conflict between the spec and implementation, the changes 
to the 2-arg isOpen method need to be dropped so that it continues to throw NPE 
if module is null.

-

Changes requested by alanb (Reviewer).

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

Re: Is --with-zlib=bundled broken on MacOS aarch64 12.2.1?

[Alan Bateman]

On 28/03/2022 07:46, David Holmes wrote:

Hi Jai,

It isn't obvious to me that the bundled sources are actually intended 
to build on macOS. There's no include of unistd.h to get the lseek 
definition.
I think the context here is that Jai is chasing an issue that may be bug 
in the libz on macOS. Building the bundled version and comparing results 
would lead to useful information. AFAIK, the system zlib has always been 
used since 7u4 when the macOS was added. I think the Apple JDK did the 
same. So there may be a small bit of "porting" to do, adds include 
files, etc.


-Alan

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

[Alan Bateman]

On Thu, 17 Mar 2022 00:12:38 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 incrementally with one 
> additional commit since the last revision:
> 
>   Fix typos

Marked as reviewed by alanb (Reviewer).

-

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

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

[Alan Bateman]

On Thu, 17 Mar 2022 00:12:38 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 incrementally with one 
> additional commit since the last revision:
> 
>   Fix typos

Thanks for dropping the charset and locale data from the proposal. The updated 
proposal (b1d1e4d8) looks okay but I can't tell if you are planning to 
integrate this or wait until there is discussion on the locations proposed in 
the Informational JEP that you've just submitted. Maybe you plan to just 
integrate and then adjust/move again if needed? I suspect that JEP will need to 
includes a "specs" directory. It's okay to jdwp.spec into the java.se "data" 
directory for now I think "specs" would be a bette place for it.

-

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

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

[Alan Bateman]

On 16/03/2022 08:44, Magnus Ihse Bursie wrote:

:

If you have such strong opinions on the data files shared between 
java.base and jdk.charsets/jdk.localedata, I propose we leave them in 
make/data for the time being, clean up the associated mess, and then 
work out where they actually should be. Does that sound okay to you?


The concern, as before, is that it puts data files into src/java.base 
that are used by the build to generate classes/resources for the service 
provider modules.  We also have the complication that the charsets to 
include in java.base varies by platform so the module/package for each 
charset is decided at build time. It's always been low-priority to 
re-visit that and not clear if we could even get to an agreement easily 
because there are IBM platforms that want EBCDIC and other double byte 
charsets whereas other platforms don't want these in java.base. So yes, 
if you can drop the move of the charset data and CLDR data from the 
patch then it will make it easier to discuss.


You asked about the JDWP spec. This is the protocol spec that is used to 
generate the spec in HTML, and source files for the debugger front-end 
and backend (jdk.jdi and jdk.jdwp.agent modules). The "specs" directory 
might be right. There is another source file (jdwp-spec.md) that isn't 
in the src tree right now and they probably go together.


-Alan

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

[Alan Bateman]

Magnus,

This proposal does not address the previous concerns. As before, you are 
proposing to put the data files used to generate the classes for 
jdk.localedata and jdk.charsets into src/java.base and I don't think we 
should do that. I really think this PR needs to be withdraw n or closed 
until there is at least some agreement on placement. I know you want to 
get the files moved out of the make tree but there are many issues to 
work through before that can happen.


-Alan

On 15/03/2022 23:59, Magnus Ihse Bursie wrote:

On Tue, 15 Mar 2022 23:50:20 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 12 commits:

  - Merge branch 'master' into shuffle-data-reborn
  - Fix merge
  - Merge tag 'jdk-19+13' into shuffle-data-reborn

Added tag jdk-19+13 for changeset 5df2a057

  - Move characterdata templates to share/classes/java/lang.
  - Update comment refering to "make" dir
  - Move new symbols to jdk.compiler
  - Merge branch 'master' into shuffle-data
  - Move macosxicons from share to macosx
  - Move to share/data, and move jdwp.spec to java.se
  - Update references in test
  - ... and 2 more: https://git.openjdk.java.net/jdk/compare/83d77186...598f740f

I have carefully reviewed all PR comments, and the changes I made in response 
to them. I believe I have resolved all requests from reviewers. What remained 
to do was to create an informational JEP about the new source structure, and 
file some follow-up issues.

I have now created and submitted a new informational JEP ("JDK Source 
Structure"), available at https://bugs.openjdk.java.net/browse/JDK-8283227. When 
creating this JEP, it felt increasingly silly to just copy and extend the part about 
src/$MODULE from JEP 201, so I extended it to cover a relevant overview of the entire JDK 
source base structure. I actually think this JEP has a good merit on its own, 
notwithstanding it being a reviewer requirement for this PR.

I have also filed follow up issues for the non-standard jdk.hotspot.agent `doc` 
and `test` directories (https://bugs.openjdk.java.net/browse/JDK-8283197 and 
https://bugs.openjdk.java.net/browse/JDK-8283198, respectively).

I have filed a follow up issue for continued efforts to clean up 
charsetmapping, https://bugs.openjdk.java.net/browse/JDK-8283228.

There were two open questions:

  * should jdwp.spec belong to specs directory instead of data
  * should bin/idea.sh be changed to exclude data

but they sounded so exploratory that I decided not to open JBS issues for them.

@wangweij  @naotoj  @prrace  @erikj79 @jonathan-gibbons  You have all approved 
this PR at an older revision. Can you please reconfirm that your approval 
stands for the latest revision? (Sorry for the mass ping)

-

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

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

[Alan Bateman]

On Wed, 16 Feb 2022 17:55:55 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:
> 
>   fix copyright date

Marked as reviewed by alanb (Reviewer).

-

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

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

[Alan Bateman]

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

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

test/jdk/java/lang/invoke/MethodHandles/exeNullCallerMethodHandlesLookup/NullCallerMethodHandlesLookupTest.java
 line 2:

> 1: /*
> 2:  * Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights 
> reserved.

As this is a new file then I assume it should be 2022 only.

test/jdk/java/lang/invoke/MethodHandles/exeNullCallerMethodHandlesLookup/NullCallerMethodHandlesLookupTest.java
 line 48:

> 46: public class NullCallerMethodHandlesLookupTest {
> 47: public static void main(String[] args) throws IOException {
> 48: Path launcher = Paths.get(System.getProperty("test.nativepath"), 
> "NullCallerMethodHandlesLookupTest");

Path.of might be nicer here but doesn't matter.

-

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

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

[Alan Bateman]

On Mon, 14 Feb 2022 18:03:45 GMT, Mandy Chung  wrote:

>> 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.

You are right. If a JNI attached thread with no Java frames wants a Lookup then 
it can invoke publicLookup. I think the proposal here is good.

-

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

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

[Alan Bateman]

On Fri, 11 Feb 2022 20:32:46 GMT, Tim Prinzing  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?

-

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

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

[Alan Bateman]

On Fri, 11 Feb 2022 23:25:44 GMT, Brent Christian  wrote:

> 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.

I think this would work for both the "no caller" case and also the case where 
there is reflection hackery calling this method from somewhere other than a 
ClassLoader. So it would be a small change in behavior from CCE to ICE.

-

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

Re: RFR: 8278753: Runtime crashes with access violation during JNI_CreateJavaVM call

[Alan Bateman]

On Tue, 25 Jan 2022 00:20:19 GMT, Yumin Qi  wrote:

> Please review,
>   When jlink with --compress=2, zip is used to compress the files while doing 
> copy. The user case failed to load zip.dll, since zip.dll is not set in PATH. 
> This failure is after we get NULL from GetModuleHandle("zip.dll"), then do 
> LoadLibrary("zip.dll") will have same result.
>   The fix is calling load_zip_library of ClassLoader first --- if zip library 
> already loaded just return the cached handle for following usage, if not, 
> load zip library and cached the handle.
> 
>   Tests: tier1,4,7 in test
>Manually tested user case, and checked output of jimage list  for 
> jlinked files using --compress=2.
> 
> Thanks
> Yumin

I think this looks okay but I think @JimLaskey and/or @sundararajana should 
look at this because it creates a dependency on a JVM_* function. I'm trying to 
think if there are any interop issues when using jrtfs. Jim/Sundar can correct 
me but I think we are okay there because a tool on say JDK 8 (or 11 or 17) that 
accesses a JDK 19 run-time image will use the BasicImageReader and won't use 
libjimage in the target VM.

-

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

Re: RFR: JDK-8279636: Update JCov version to 3.0.12

[Alan Bateman]

On Wed, 26 Jan 2022 00:08:36 GMT, Alexandre Iline  
wrote:

> JDK-8279636: Update JCov version to 3.0.12

Thanks, I can confirm that 3.0-12 works with the current main line.

-

Marked as reviewed by alanb (Reviewer).

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

Re: RFR: 8279223: Define version in .jcheck/conf

[Alan Bateman]

On Thu, 23 Dec 2021 14:16:37 GMT, Erik Joelsson  wrote:

> This patch adds the version field to .jcheck/conf. By doing this we can 
> remove the corresponding configuration from the Skara bots, which in turn 
> reduces the need for timing and general complexity of starting a new JDK 
> release.

Marked as reviewed by alanb (Reviewer).

-

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

Re: [jdk18] RFR: JDK-8278967 rmiregistry fails to start because SecurityManager is disabled [v2]

[Alan Bateman]

On Wed, 22 Dec 2021 01:18:58 GMT, Stuart Marks  wrote:

>> Enable the security manager in rmiregistry's launcher arguments.
>
> Stuart Marks has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Change java.security.manager to "allow"; filter warning lines from 
> VersionCheck

This version looks okay, avoids having an attempt to set the SM in 
createRegistry always be skipped.

-

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk18/pull/45

Re: [jdk18] RFR: JDK-8278967 rmiregistry fails to start because SecurityManager is disabled

[Alan Bateman]

On Fri, 17 Dec 2021 20:01:27 GMT, Stuart Marks  wrote:

> Enable the security manager in rmiregistry's launcher arguments.

As things stand, `rmiregsitry -J-Djava.security.manager` and `rmiregistry 
-J-Djava.security.manager=allow` are equivalent because rmiregistry sets the 
default SM. Some difference may be observed if someone is running rmiregistry 
with a custom system class loader set, or custom file system provider, or 
running it with a JVM TI agent that is looking at events between VMStart and 
VMInit but these seem somewhat obscure scenarios for a command line program If 
rmiregstry worked with ToolProvider then I think there would be more to 
discuss. If the final patch is to have the launcher set the default security 
manager then we may want to change RegistryImpl.createRegistry to fail if not 
set.

The warning that is emitted for both cases is expected. JEP 411 is very clear 
that it there is no mechanism to suppress it. We may need to add a release note 
to document that rmiregistry will emit a warning a startup, that will at least 
be something to point at in the event that anyone asks or reports an issue.

-

PR: https://git.openjdk.java.net/jdk18/pull/45

Re: RFR: JDK-8278273: Remove unnecessary exclusion of doclint accessibility checks

[Alan Bateman]

On Sun, 5 Dec 2021 23:45:32 GMT, Joe Darcy  wrote:

> Exploratory builds indicate it is not currently necessary to exclude the 
> doclint accessibility checks; this patch enables them.
> 
> (Enabling the reference checks is left for future work.)

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: JDK-8272945: Use snippets in java.compiler documentation

[Alan Bateman]

On Fri, 3 Dec 2021 00:26:10 GMT, Jonathan Gibbons  wrote:

> Please review a patch to use snippets in the `java.compiler` documentation, 
> instead of a mix of raw HTML and/or `{@code ...}`.  The change is just about 
> the presentation of the code fragments; there are no changes to the normative 
> specifications for the module.
> 
> One of the examples went to extraordinary lengths to include the character 
> sequence `*/` within the example. That example has been replaced by an 
> external snippet in a separate source file, which does not have any 
> restriction on the use of `*/`. However, it does require that the file be 
> excluded from standard compilation, and that is done by setting `EXCLUDES`, 
> once for the "interim" compiler, and once again for the "product" compiler.   
>  Going forward, a better solution might be to modify the 
> `SetupJavaCompilation` macro to ignore all directories whose name is not a 
> valid Java identifier (or, if easier, contains a `-`, such as `doc-files` or 
> `snippet-files`.)

src/java.compiler/share/classes/javax/tools/JavaCompiler.java line 189:

> 187:  * source code stored in a string:
> 188:  *
> 189:  * {@snippet id=fileObject class=JavaSourceFromString }

JEP 413 uses the "file" attribute for external snippets, so using "class" here 
is new (to me anyway). Maybe the JEP should include an example that uses it.

Is this the first use of an external snippet in the src tree? I suspect files 
in the snippet-files directory will need a copyright header (downstream 
builders will report issues on this). It's not clear to me how that works if 
@replace region replacement="" isn't the first line of the external file.

-

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

Re: RFR: 8277847: Support toolGuide tag in class-level documentation

[Alan Bateman]

On Thu, 25 Nov 2021 15:58:58 GMT, Julia Boes  wrote:

> This change adds support for the `@toolGuide` tag in class-level API 
> documentation. 
> 
> (A use case is the jwebserver tool, where the 
> com.sun.net.httpserver.SimpleFileServer class provides API points for 
> extension and customization of the underlying server and its components. 
> Linking to the tool's man page in the class-level documentation would be 
> beneficial.)

Extending this to allow @toolGuide be used in class descriptions is generally 
useful.

-

Marked as reviewed by alanb (Reviewer).

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

Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v2]

[Alan Bateman]

On Mon, 22 Nov 2021 04:30:38 GMT, Joe Darcy  wrote:

>> The time to get JDK 19 underway draws nigh, please review this usual set of 
>> start-of-release updates, including CSRs for the javac and javax.lang.model 
>> updates:
>> 
>> JDK-8277512: Add SourceVersion.RELEASE_19
>> https://bugs.openjdk.java.net/browse/JDK-8277512
>> 
>> JDK-8277514: Add source 19 and target 19 to javac
>> https://bugs.openjdk.java.net/browse/JDK-8277514
>> 
>> Clean local tier 1 test runs for langtools, jdk, and hotspot.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

It's not possible to manual review the sym files but everything else looks 
okay. I searched the test tree for any additional tests that might need 
updating but didn't spot any.

-

Marked as reviewed by alanb (Reviewer).

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

Re: RFR: JDK-8276422 Add command-line option to disable finalization [v2]

[Alan Bateman]

On Thu, 18 Nov 2021 07:44:05 GMT, Aleksey Shipilev  wrote:

>> @shipilev not sure what you mean by  "a flag on the Java side". The Java 
>> code just queries the VM for the finalization enabled/disabled state and 
>> uses that to control things.
>
> Yeah, "flag" is `Holder.ENABLED` here. I mean, are Java methods 
> `registerFinalizer` and `runFinalization` called only by VM? If so, can VM 
> check the whole thing on VM side, without going to Java and asking back from 
> there?

I think @shipilev asks a good question. This could be done completely in the VM 
without the changes to j.l.ref.Finalizer. The CLI option is for experimenting, 
at least in the short term, and should be benign to have the Finalizer thread 
running, it just won't do anything.

-

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

Re: RFR: 8276400: openjdk image Jars, Zips and Jmods built from the same source are not reproducible

[Alan Bateman]

On 05/11/2021 19:15, Andrew Leonard wrote:

:
@AlanBateman @magicus thank you both for your guidance. I have now split this 
bug into the 3 mentioned:
- GenerateZip: https://bugs.openjdk.java.net/browse/JDK-8276743
- Jar/Jmod content ordering: https://bugs.openjdk.java.net/browse/JDK-8276764
- Jar/Jmod/ZipOutputStream timestamp option: 
https://bugs.openjdk.java.net/browse/JDK-8276766 **(requires CSR)**

Closing this PR to be replaced with 3 new PRs for the above bugs.

Thanks this seems a reasonable plan. When you get the 3rd item then 
we'll need to discuss whether ZipOutputStream.putEntry overrides (or 
not) the time stamps of entries that have a modification time.


-Alan

Re: RFR: 8276400: openjdk image Jars, Zips and Jmods built from the same source are not reproducible

[Alan Bateman]

On 05/11/2021 11:39, Magnus Ihse Bursie wrote:

:
I agree with Alan's comment above. First of all, to be absolutely clear, I 
think this is a very worthy goal, and much overdue. I'll do my best to help get 
this implemented.

One suggestion is to separate out the issue of ordering of entries (in 
zip/JAR and JMOD) and whether it would be the default. There may be 
trade-offs to discuss, also whether it's limited to just new zip/JAR 
files or updates to existing zip/JARs files.


-Alan

Re: RFR: 8276400: openjdk image Jars, Zips and Jmods built from the same source are not reproducible

[Alan Bateman]

On 04/11/2021 21:16, Andrew Leonard wrote:

This PR enables reproducible Jars, Jmods and openjdk image zip files 
(eg.src.zip).
It provides support for SOURCE_DATE_EPOCH for Jar, Jmod and underlying 
ZipOutputStream's.
It fixes the following keys issues relating to reproducibility:
- Jar and ZipOutputStream are not SOURCE_DATE_EPOCH aware
   - Jar and ZipOutputStream now detect SOURCE_DATE_EPOCH environment setting
- Jar and Jmod file content ordering was non-determinsitic
   - Fixes to Jar and Jmod main's to ensure sorted classes content ordering
- openjdk image zip file generation used "zip" which is non-determinsitic
   - New openjdk build tool "GenerateZip" which produces the final 
determinsitic zip files as part of the build and also detects SOURCE_DATE_EPOCH

I think this is going to require discussion, a PR may be too premature. 
Is your goal to get the JDK build and run-time images creates with jlink 
to use the SOURCE_DATE_EPOCH? Are you looking for project builds that 
use the jar/jmod/etc. tools to use it? Or are you looking to have every 
library and application that uses the java.util.zip APIs to read it? If 
it includes the latter, and the patch in the PR suggests you are, then 
it will require analysis to work through the API spec changes.


One suggestion is to look at JDK-8231640 and PR 5372 [1]. The original 
proposal was to use SOURCE_DATE_EPOCH. After a lengthy discussion we 
converged on the system property java.properties.date rather than the 
env variable. I suspect that much of that discussion applies here.


-Alan

[1] https://github.com/openjdk/jdk/pull/5372

Re: Running jdk's tests to produce coverage report

[Alan Bateman]

On 03/11/2021 16:34, Chaliasos, Stefanos wrote:

Thanks Alan,

I used the one that jtreg uses. The complete configuration of JCOV for 
jtreg is:


```
DEFAULT_JCOV_SRC_TAG=jcov3.0-b07
DEFAULT_JCOV_SRC_ARCHIVE_CHECKSUM=c5c26085750628d58de275b3f50a7409300c0497 


DEFAULT_ANT_VERSION=1.10.8
DEFAULT_ANT_ARCHIVE_CHECKSUM=dbe187ce2963f9df8a67de8aaff3b0a437d06978
DEFAULT_ASM_VERSION=8.0
DEFAULT_ASM_JAR_CHECKSUM=d1a17d07c60e9e82c8b31b1d8f9ca98726418db4
DEFAULT_ASM_TREE_JAR_CHECKSUM=7b31ca94da9f57334a5aed79b40f2b88c5ee9f4f
DEFAULT_ASM_UTIL_JAR_CHECKSUM=b21996293fd49851ed9017cfde3191e49f77fbd0
DEFAULT_JTHARNESS_SRC_TAG=jt6.0-b13
DEFAULT_JTHARNESS_SRC_ARCHIVE_CHECKSUM=43936b2616476fcac8ee4bd0132e73c015119337 


```

Could you please share from where I can download the JCOV version that 
you are referring to?


Here's the JBS issue that upgraded jcov to support v62 class files. So I 
think you need jcov 3.0.9


https://bugs.openjdk.java.net/browse/CODETOOLS-7902988

-Alan

Re: Running jdk's tests to produce coverage report

[Alan Bateman]

On 03/11/2021 15:12, Chaliasos, Stefanos wrote:

Hello,

I'm trying to compute code coverage for langtools in the JDK repo on a Ubuntu 
18.04 machine using JDK 18 for the compilation. I have run the following 
commands:

```
cd /home/user
git clone https://github.com/openjdk/jdk.git
cd jdk && bash configure && make jdk && cd ../
git clone https://github.com/openjdk/jtreg.git
cd jtreg
bash make/build.sh --jdk /home/user/jdk/build/linux-x86_64-server-release/jdk/
cd ../jdk
bash configure --with-jtreg=/home/user/jtreg/build/images/jtreg 
--with-jcov=/home/user/jtreg/build/deps/jcov/
make jcov-image
```

In the last command, I get errors with the following message:

```
Exception details: Unsupported class file major version 62
...

Which version of jcov is this? I did a jcov test on my local build today 
using what claims to be "3.0-9-jdk-asm+1.0" and it worked okay.


-Alan

Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v13]

[Alan Bateman]

On Tue, 2 Nov 2021 19:35:29 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-419 [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/419
>
> Maurizio Cimadamore has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Address impl review comments
>  - Address API review comments

src/java.base/share/classes/java/lang/Module.java line 114:

> 112: 
> 113: // true, if this module allows restricted native access; @Stable 
> makes sure that modules that allow native
> 114: // access capture this property as a constant.

Do you mind fixing this comment to avoid the really long line, it sticks out 
compare to everything else around it.

src/java.base/share/classes/sun/nio/ch/IOUtil.java line 478:

> 476: private static final JavaNioAccess NIO_ACCESS = 
> SharedSecrets.getJavaNioAccess();
> 477: 
> 478: static Runnable acquireScope(ByteBuffer bb, boolean async) {

At some point (not this PR) we should move the "async" out of this file, IOUtil 
was for synchronous I/O.

-

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

Re: Does CDS archive generation work for crossbuilds?

[Alan Bateman]

On 23/10/2021 07:57, Thomas Stüfe wrote:

Hi,

when I crossbuild (for linux aarch64, using a devkit, building on linux
x64), for some reason I don't
get the classes.jsa generated inside the images directory.

My configure options:

--with-devkit=/shared/projects/openjdk/devkits/x86_64-linux-gnu-to-aarch64-linux-gnu
--openjdk-target=aarch64-linux-gnu
--with-boot-jdk=/shared/projects/openjdk/jdks/sapmachine17
--with-build-jdk=/shared/projects/openjdk/jdk-jdk/output-release/images/jdk
--with-gtest=/shared/projects/openjdk/gtest/googletest
--with-debug-level=fastdebug

The build jdk is a freshly build x64 release VM from the same source tree.

Am I missing something obvious? Is CDS archive generation even supported
for crossbuilds?
It needs the generate run-time to execute "java -Xshare:dump" so I don't 
expect so. hotspot-runtime-dev is probably the place to discuss the 
details. BTW: this came up recently in the context of the jlink plugin 
that generates the CDS archive. The plugin needed a check to ensure that 
the target platform matched the current platform as it could launch the 
target VM to create the dump.


-Alan

Re: RFR: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume.

[Alan Bateman]

On Mon, 4 Oct 2021 13:44:44 GMT, Richard Reingruber  wrote:

> The following sentence in the JDWP Specification describing the Dispose 
> command confuses resume with suspend [1]:
> 
>   All threads suspended by the thread-level **resume** command or the VM-level
>   **resume** command are resumed as many times as necessary for them to run.
> 
> It should be changed to
> 
>   All threads suspended by the thread-level **suspend** command or the 
> VM-level
>   **suspend** command are resumed as many times as necessary for them to run.
> 
> [1] [JDWP Spec, Dispose Command 
> (JDK17).](https://docs.oracle.com/en/java/javase/17/docs/specs/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Dispose)

It looks like this typo goes all the way back to JDK 1.2 but was not noticed.

-

Marked as reviewed by alanb (Reviewer).

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

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

[Alan Bateman]

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.

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8274083: Update testing docs to mention tiered testing [v6]

[Alan Bateman]

On Fri, 24 Sep 2021 06:12:20 GMT, Aleksey Shipilev  wrote:

>> Now that OpenJDK has more or less complete `tier{1,2,3,4}` definitions, 
>> let's mention them in `testing.md`. 
>> 
>> Current patch is my braindump, I am open for suggestions :)
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Another minor touchup

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8274083: Update testing docs to mention tiered testing [v5]

[Alan Bateman]

On Thu, 23 Sep 2021 12:53:23 GMT, Aleksey Shipilev  wrote:

>> Now that OpenJDK has more or less complete `tier{1,2,3,4}` definitions, 
>> let's mention them in `testing.md`. 
>> 
>> Current patch is my braindump, I am open for suggestions :)
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More fixes

Marked as reviewed by alanb (Reviewer).

doc/testing.html line 80:

> 78: 
> 79: tier1: This is the lowest test tier. Multiple 
> developers run these tests every day. Because of the widespread use, the 
> tests in tier1 are carefully selected and optimized to run fast, 
> and to run in the most stable manner. The test failures in tier1 
> are usually followed up on quickly, either with fixes, or adding relevant 
> tests to problem list. GitHub Actions workflows, if enabled, run 
> tier1 tests.
> 80: tier2: This test group covers even more ground. These 
> contain, among other things, tests that either run for too long to be at 
> tier1, or may require special configuration, or tests that are 
> less stable, or cover the broader range of less critical JVM and JDK 
> features/components (for example, jaxp).

Thanks for the updates, I think it reads much better now.
A small suggestion is to change "less critical" to "non-core", and "jaxp" to 
"XML" as I don't expect too many people know what JAXP is.

-

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

Re: RFR: 8274083: Update testing docs to mention tiered testing [v4]

[Alan Bateman]

On Thu, 23 Sep 2021 11:30:20 GMT, Aleksey Shipilev  wrote:

>> Now that OpenJDK has more or less complete `tier{1,2,3,4}` definitions, 
>> let's mention them in `testing.md`. 
>> 
>> Current patch is my braindump, I am open for suggestions :)
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Drop GH link

doc/testing.html line 72:

> 70: The test specifications given in TEST is parsed into 
> fully qualified test descriptors, which clearly and unambigously show which 
> tests will be run. As an example, :tier1 will expand to 
> jtreg:$(TOPDIR)/test/hotspot/jtreg:tier1 jtreg:$(TOPDIR)/test/jdk:tier1 
> jtreg:$(TOPDIR)/test/langtools:tier1 jtreg:$(TOPDIR)/test/nashorn:tier1 
> jtreg:$(TOPDIR)/test/jaxp:tier1. You can always submit a list of fully 
> qualified test descriptors in the TEST variable if you want to 
> shortcut the parser.
> 71: Common Test Groups
> 72: In the ideal world, contributors would be advised to run all the tests 
> for every change. But in real world, one could only be expected to run as 
> many tests as practical, while being mindful of the scope for the change, the 
> testing resources available, etc.

A suggestion here is to start with something like "Ideally, all tests are run 
for every change but this may not be practical. Contributors are expected to at 
least run tier1 (see below) and the tests for the areas that are changed."

doc/testing.html line 79:

> 77: 
> 78: tier1: This is the lowest test tier. Multiple 
> developers run these tests every day. Normally, at least this tier should be 
> clean before integration. Because of the widespread use, the tests in 
> tier1 are carefully selected and optimized to run fast, and to 
> run in the most stable manner. The test failures in tier1 are 
> usually followed up on quickly, either with fixes, or adding relevant tests 
> to problem list. GitHub Actions workflows, if enabled, run tier1 
> tests.
> 79: tier2: This test group covers even more ground. These 
> contain, among other things, tests that either run for too long to be at 
> tier1, tests for less stable and/or experimental features, tests 
> for less essential JDK components (for example, jaxp).

I think this a bit unfair on tier2. Tests in tier2 need be run by anyone making 
changes in the security area, I/O, networking and many other core APIs. In 
general I think anyone changing code in the libraries area needs to look at 
TEST.groups to get some idea which tier or finer test group to run.

I'm not sure about words like "less stable" and "experimental features".  Do we 
have tests for hotspot experimental features in hotspot_tier2?

-

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

Re: RFR: 8274083: Update testing docs to mention tiered testing

[Alan Bateman]

On Tue, 21 Sep 2021 14:52:26 GMT, Aleksey Shipilev  wrote:

> Now that OpenJDK has more or less complete `tier{1,2,3,4}` definitions, let's 
> mention them in `testing.md`. 
> 
> Current patch is my braindump, I am open for suggestions :)

doc/testing.html line 77:

> 75: tier1: This test group is the first line of defense 
> against bugs. Multiple developers run these tests every day. Normally, at 
> least this tier is ran before integration. Because of the widespread, the 
> tests in tier1 are carefully selected and optimized to run fast, 
> and to run in the most stable manner. The test failures in tier1 
> are usually followed up on quickly, either with fixes, or adding relevant 
> tests to problem list. GitHub Actions 
> workflows, if enabled, run tier1 tests.
> 76: tier2: This test group covers even more ground. These 
> contain, among other things, tests that either run for too long to be at 
> tier1, test unstable and/or experimental features, test less 
> essential JDK components.
> 77: tier3: This test group covers more stressful tests, 
> the tests for corner cases not covered by previous tiers, plus the tests that 
> require GUIs. As such, this suite should either be run with low concurrency 
> (TEST_JOBS=1), or without headful tests 
> (JTREG_KEYWORDS=\!headful), or both.

It's not clear to me that these descriptions are useful. Instead of "First line 
of defense against bugs" then it might be better to say something about the 
hotspot and core library tests that are typically run in tier1. Folks 
contributing to some areas of the core, I/O, and networking libraries will need 
to learn about tier2 as that is where many of the tests for these APIs and 
implementations are. So I wouldn't use words like "unstable", "experimental", 
"less essential" but maybe say that it includes tests that may require some 
configuration, like changing firewall rules to allow multicast or other traffic.

-

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

Re: RFR: 8272805: Avoid looking up standard charsets

[Alan Bateman]

On Sun, 22 Aug 2021 02:53:44 GMT, Sergey Bylokhov  wrote:

> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
> 
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
> 
> This could be done more efficiently(up to x20 time faster) with use of 
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
> 
> The later variant also makes the code cleaner, as it is known not to throw 
> UnsupportedEncodingException in contrary to the former variant.
> 
> This change includes:
>  * demo/utils
>  * jdk.xx packages
>  * Some places were missed in the previous changes. I have found it by 
> tracing the calls to the Charset.forName() by executing tier1,2,3 and desktop 
> tests.
> 
> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
> 
> Code excluded in this fix: the Xerces library(should be fixed upstream), 
> J2DBench(should be compatible to 1.4), some code in the network(the change 
> there are not straightforward, will do it later).
> 
> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.

src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java line 342:

> 340: 
> 341: try {
> 342: for (String line : Files.readAllLines(statusPath, UTF_8)) {

The 1-arg readAllLines is specified to use UTF-8 so you can drop the second 
parameter here if you want.

-

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

Re: RFR: 8266035: class file for sun.misc.Contended not found

[Alan Bateman]

On Wed, 9 Jun 2021 11:05:37 GMT, Jan Lahoda  wrote:

> The ct.sym may contain classfiles referring to annotations that are not 
> present in ct.sym (liek JDK's internal annotation `sun.misc.Contended`). If 
> javac will try to load them (while discovering annotations for the purpose of 
> detecting which annotation processors should be run), an error will be 
> produced (please see the issue). The proposal is to strip annotations that 
> are not present in ct.sym when generating ct.sym.

Seems to be linked to the wrong issue, the issue you want is JDK-8266036.

-

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

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

[Alan Bateman]

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`.

Looks good, I had expected we would have more tests depending on the automatic 
module.

-

Marked as reviewed by alanb (Reviewer).

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

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

[Alan Bateman]

On Tue, 1 Jun 2021 15:21:33 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.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 11 commits:
> 
>  - merge from master
>  - rename setSecurityManagerDirect to implSetSecurityManager
>  - default behavior reverted to allow
>  - move one annotation to new method
>  - merge from master, two files removed, one needs merge
>  - keep only one systemProperty tag
>  - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>  - feedback from Sean, Phil and Alan
>  - add supresswarnings annotations automatically
>  - manual change before automatic annotating
>  - ... and 1 more: 
> https://git.openjdk.java.net/jdk/compare/74b70a56...ea2c4b48

Marked as reviewed by alanb (Reviewer).

-

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

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

[Alan Bateman]

On Mon, 31 May 2021 15:02:57 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.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   default behavior reverted to allow

System.setSecurityManagerDirect looks a bit ugly now. Can this be renamed to 
implSetSecurityManager and avoid the line break in the  middle of the 
declaration?

The usage of System.err usage in setSecurityManager also needs to be 
re-examined as this will run arbitrary code when System.err can be changed. To 
fix this will require capturing the stream at startup (as was done with the 
illegal access logger). It's okay to integrate with what you have for the first 
push and we can fix this issue with System.err when the warning message is 
changed to the intended message.

-

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

Re: RFR: 8267123: Remove RMI Activation

[Alan Bateman]

On Tue, 25 May 2021 18:04:45 GMT, Stuart Marks  wrote:

> This is the implementation of [JEP 407](https://openjdk.java.net/jeps/407).
> 
> This is a fairly straightforward removal of this component.
>  - Activation implementation classes removed
>  - Activation tests removed
>  - adjustments to various comments to remove references to Activation
>  - adjustments to some code to remove special-case support for Activation 
> from core RMI
>  - adjustments to several tests to remove testing and support for, and 
> mentions of Activation
>  - remove `rmid` tool
> 
> (Personally, I found that reviewing using the webrev was easier than 
> navigating github's diff viewer.)

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8267123: Remove RMI Activation

[Alan Bateman]

On Tue, 25 May 2021 18:04:45 GMT, Stuart Marks  wrote:

> This is the implementation of [JEP 407](https://openjdk.java.net/jeps/407).
> 
> This is a fairly straightforward removal of this component.
>  - Activation implementation classes removed
>  - Activation tests removed
>  - adjustments to various comments to remove references to Activation
>  - adjustments to some code to remove special-case support for Activation 
> from core RMI
>  - adjustments to several tests to remove testing and support for, and 
> mentions of Activation
>  - remove `rmid` tool
> 
> (Personally, I found that reviewing using the webrev was easier than 
> navigating github's diff viewer.)

One other test that might need an update is 
test/jdk/sun/rmi/log/ReliableLog/LogAlignmentTest.java, it might be just the 
summary.

-

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

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

[Alan Bateman]

On Fri, 21 May 2021 18:00:13 GMT, Phil Race  wrote:

> Are you suggesting that the patch doesn't need testing as it is ? It should 
> be the same either way.
> It is very straight forward to run the automated tests across all platforms 
> these days.
> I get the impression that no one is guaranteeing to do this straight after 
> integration.
> It sounds like it is up for deferral if time runs out.
> 
> The amount of follow-on work that I am hearing about here, and may be for 
> tests, does not make it sound
> like this JEP is nearly as done as first presented.
> 
> If there was some expectation that groups responsible for an area would get 
> involved with this
> issue which I am assured was already known about, then why was it not raised 
> before and made
> part of the plan ?

Sprinkling SuppressWarnings should be very low risk. Refactoring code to have 
the scope of the SW be as small as possible may be a bit more risky, esp. in 
areas where one doesn't know the code or the tests that exercise it. The tests 
may be good but it's not clear to me that we want to force Max to do 
significant refactoring in this PR.  PR 4138 has been created as the follow-on 
PR so I think we should help get that reviewed so that it can be integrated 
soon after this PR.

-

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

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

[Alan Bateman]

On Thu, 20 May 2021 04:22:32 GMT, Phil Race  wrote:

>> That is unfortunate, but nonetheless I think required to be done.
>> We have acknowledeged this can't reasonably be called an RFE, so the JEP is 
>> introducing bugs/technical debt/call it what you will. This should generally 
>> be part of a sandbox for the JEP and fixed before integration of the JEP.
>> From my point of view it is a blocker.
>
> The JEP isn't PTT for 17 so there's plenty of time isn't there ?

There are 827 files in this patch. Phil is right that adding SW at the class 
level is introducing technical debt but if addressing that requires refactoring 
and re-testing of AWT or font code then it would be better to have someone from 
that area working on. Is there any reason why this can't be going on now on 
awt-dev/2d-dev and integrated immediately after JEP 411? I don't think we 
should put Max through the wringer here as there are too many areas to cover.

-

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

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

[Alan Bateman]

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 alanb (Reviewer).

src/java.base/share/classes/java/security/AccessController.java line 877:

> 875: @CallerSensitive
> 876: public static  T doPrivileged(PrivilegedExceptionAction action,
> 877:  @SuppressWarnings("removal") 
> AccessControlContext context, Permission... perms)

you might find it easier if you put the Permissions parameter on a new line. 
There are several places in AccessController where the same thing happens.

-

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

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

[Alan Bateman]

On Tue, 18 May 2021 12:42:08 GMT, Sean Mullan  wrote:

>> src/java.base/share/classes/java/lang/SecurityManager.java line 315:
>> 
>>> 313:  *
>>> 314:  * @since   1.0
>>> 315:  * @deprecated The Security Manager is deprecated and subject to 
>>> removal in a
>> 
>> Javadoc will prefix, in bold, "Deprecated, for removal: This API element is 
>> subject to removal in a future version". I'm just wondering if the sentence 
>> will be repeated here, can you check?
>
> It includes both:
> ![Screen Shot 2021-05-18 at 8 41 11 
> AM](https://user-images.githubusercontent.com/35072269/118652730-dfb35400-b7b4-11eb-83ee-92be9136fea2.jpg)

Thanks for checking, I assumed that was the case so wondering if it should be 
dropped from the deprecation text to avoid the repeated sentence.

-

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

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

[Alan Bateman]

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.

src/java.base/share/classes/java/lang/SecurityManager.java line 315:

> 313:  *
> 314:  * @since   1.0
> 315:  * @deprecated The Security Manager is deprecated and subject to removal 
> in a

Javadoc will prefix, in bold, "Deprecated, for removal: This API element is 
subject to removal in a future version". I'm just wondering if the sentence 
will be repeated here, can you check?

-

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

Re: RFR: 8252600: [JVMCI] update JVMCI code style and mx configuration

[Alan Bateman]

On Sat, 17 Apr 2021 20:37:08 GMT, Doug Simon  wrote:

> This PR updates the configuration files used to develop the JVMCI Java and 
> C++ sources with mx and Eclipse.

Are you sure it make sense to have this dev config in the openjdk/jdk repo? I 
would think this is something for the downstream Graal repos.

-

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

Re: RFR: 8264779: Fix doclint warnings in java/nio [v2]

[Alan Bateman]

On Wed, 7 Apr 2021 14:40:48 GMT, Conor Cleary  wrote:

>> This fix addresses the following warnings which were generated by building 
>> JDK API documentation with the `-Xdoclint:all` option enabled:
>> 
>> ./build/linux-x64/support/gensrc/java.base/java/nio/charset/IllegalCharsetNameException.java:47:
>>  warning: no comment
>> private String charsetName;
>>^
>> ./open/src/java.base/share/classes/java/nio/charset/MalformedInputException.java:44:
>>  warning: no comment
>> private int inputLength;
>> ^
>> ./open/src/java.base/share/classes/java/nio/charset/UnmappableCharacterException.java:44:
>>  warning: no comment
>> private int inputLength;
>> ^
>> ./build/linux-x64/support/gensrc/java.base/java/nio/charset/UnsupportedCharsetException.java:47:
>>  warning: no comment
>> private String charsetName;
>>^
>> ./open/src/java.base/share/classes/java/nio/file/DirectoryIteratorException.java:81:
>>  warning: no @param for s
>> private void readObject(ObjectInputStream s)
>>  ^
>> ./open/src/java.base/share/classes/java/nio/file/DirectoryIteratorException.java:81:
>>  warning: no @throws for java.lang.ClassNotFoundException
>> private void readObject(ObjectInputStream s)
>>  ^
>> ./open/src/java.base/share/classes/java/nio/file/FileSystemException.java:43:
>>  warning: no comment
>> private final String file;
>>  ^
>> ./open/src/java.base/share/classes/java/nio/file/FileSystemException.java:44:
>>  warning: no comment
>> private final String other;
>>  ^
>> ./open/src/java.base/share/classes/java/nio/file/InvalidPathException.java:43:
>>  warning: no comment
>> private int index;
>> ^
>> ./open/src/java.base/share/classes/java/nio/file/InvalidPathException.java:42:
>>  warning: no comment
>> private String input;
>>^
>> ./open/src/java.base/share/classes/java/nio/file/attribute/UserPrincipalNotFoundException.java:43:
>>  warning: no comment
>> private final String name;
>>  ^
>> Changes to 
>> [`genExceptions.sh`](https://github.com/openjdk/jdk/commit/b729d8ed7970737a8a2d4e8aa788df33789faea2)
>>  and the two 
>> [`exceptions`](https://github.com/openjdk/jdk/commit/b729d8ed7970737a8a2d4e8aa788df33789faea2)
>>  templates are to address the warnings concerned with 
>> `UnsupportedCharsetException.java` and `IllegalCharsetNameException.java` 
>> which are generated when `make jdk-image` is run. A CSR will be filed in due 
>> course with respect to these changes.
>
> Conor Cleary has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8264779: Simplified comments

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8264779: Fix doclint warnings in java/nio [v2]

[Alan Bateman]

On Wed, 7 Apr 2021 18:01:25 GMT, Conor Cleary  wrote:

>> src/java.base/share/classes/java/nio/charset/MalformedInputException.java 
>> line 45:
>> 
>>> 43: 
>>> 44: /**
>>> 45:  * The length of the input byte sequence.
>> 
>> Should this comment also refer to the character sequence?
>
> It could, maybe something like "The length of the input byte (or character) 
> sequence." would work?

or just "The length of the input" so that it is consistent with the description 
of the getInputLength method.

-

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

Re: RFR: 8264779: Fix doclint warnings in java/nio

[Alan Bateman]

On Wed, 7 Apr 2021 13:22:44 GMT, Conor Cleary  wrote:

> This fix addresses the following warnings which were generated by building 
> JDK API documentation with the `-Xdoclint:all` option enabled:
> 
> ./build/linux-x64/support/gensrc/java.base/java/nio/charset/IllegalCharsetNameException.java:47:
>  warning: no comment
> private String charsetName;
>^
> ./open/src/java.base/share/classes/java/nio/charset/MalformedInputException.java:44:
>  warning: no comment
> private int inputLength;
> ^
> ./open/src/java.base/share/classes/java/nio/charset/UnmappableCharacterException.java:44:
>  warning: no comment
> private int inputLength;
> ^
> ./build/linux-x64/support/gensrc/java.base/java/nio/charset/UnsupportedCharsetException.java:47:
>  warning: no comment
> private String charsetName;
>^
> ./open/src/java.base/share/classes/java/nio/file/DirectoryIteratorException.java:81:
>  warning: no @param for s
> private void readObject(ObjectInputStream s)
>  ^
> ./open/src/java.base/share/classes/java/nio/file/DirectoryIteratorException.java:81:
>  warning: no @throws for java.lang.ClassNotFoundException
> private void readObject(ObjectInputStream s)
>  ^
> ./open/src/java.base/share/classes/java/nio/file/FileSystemException.java:43: 
> warning: no comment
> private final String file;
>  ^
> ./open/src/java.base/share/classes/java/nio/file/FileSystemException.java:44: 
> warning: no comment
> private final String other;
>  ^
> ./open/src/java.base/share/classes/java/nio/file/InvalidPathException.java:43:
>  warning: no comment
> private int index;
> ^
> ./open/src/java.base/share/classes/java/nio/file/InvalidPathException.java:42:
>  warning: no comment
> private String input;
>^
> ./open/src/java.base/share/classes/java/nio/file/attribute/UserPrincipalNotFoundException.java:43:
>  warning: no comment
> private final String name;
>  ^
> Changes to 
> [`genExceptions.sh`](https://github.com/openjdk/jdk/commit/b729d8ed7970737a8a2d4e8aa788df33789faea2)
>  and the two 
> [`exceptions`](https://github.com/openjdk/jdk/commit/b729d8ed7970737a8a2d4e8aa788df33789faea2)
>  templates are to address the warnings concerned with 
> `UnsupportedCharsetException.java` and `IllegalCharsetNameException.java` 
> which are generated when `make jdk-image` is run. A CSR will be filed in due 
> course with respect to these changes.

src/java.base/share/classes/java/nio/file/InvalidPathException.java line 44:

> 42: /**
> 43:  * The invalid input path string.
> 44:  */

This should probably be "The input string" to be consistent with the getInput 
method.

src/java.base/share/classes/java/nio/file/attribute/UserPrincipalNotFoundException.java
 line 44:

> 42: 
> 43: /**
> 44:  * The name of the {@code UserPrincipal} that does not exist.

Probably best to use "user principal name" so that it is consistent with 
getName.

-

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

Re: [Draft] RFR: 8241463: Move build tools to respective modules

[Alan Bateman]

On 09/02/2021 11:18, Magnus Ihse Bursie wrote:


This is a re-post of a change that was posted as webrev prior to the 
Github migration. It is not ready for integration as-is, since it 
needs to be rebased to the current HEAD, and that is bound to be a 
non-trivial operation after this much time.


However, I think it is relevant to have this available as a Github 
review, in light of the closely relatedJDK-8257733. The idea of 
posting this draft review is to make it easier for reviewers to 
visualize the end result of both JDK-8241463 and 
JDK-8257733. 



This patch will move the source code for the build tools into their 
respective modules, in a separate|tools|directory.


Draft review: https://github.com/openjdk/jdk/pull/2475


It might be worth summarizing in a few lines what the proposal is.

My reading of it of pull/2475 is that the build tools used to generate 
source files, resources, or other files for the lib directory are moved 
into src/$MODULE/share/tools. I assume this is compatible with the 
source paths that the idea.sh script emits so it won't get confused with 
the source code for the module.


There are a couple of tools that digest data files imported from 
upstream to create source files and resources for more than one module 
and they aren't moving at this time, right? There is also the checked in 
jdwp.spec that is processed by a tool at build time to generate sources 
for 2 modules, that one might be a bit awkward too.


Are there any other high-level points that we should know about?

-Alan.

Re: RFR: 8260406: Do not copy pure java source code to gensrc

[Alan Bateman]

On Tue, 26 Jan 2021 10:35:03 GMT, Magnus Ihse Bursie  wrote:

> For java.base gensrc we do the following:
> 
> # Copy two Java files that need no preprocessing.
> $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/%.java: 
> $(CHARACTERDATA_TEMPLATES)/%.java.template
> $(call LogInfo, Generating $(@F))
> $(call install-file)
> 
> GENSRC_CHARACTERDATA += 
> $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataUndefined.java \
> 
> $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataPrivateUse.java
> 
> What this means is that we have two "template" files that are just compiled 
> as java files, but only after being copied to gensrc. :-)
> 
> We should just rename these files to java and move them to the normal source 
> directory.

Good. I notice the comment in both source files says "Java.lang.Character" 
rather than "java.lang.Character", probably should fix that at some point.

-

Marked as reviewed by alanb (Reviewer).

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

Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]

[Alan Bateman]

On Mon, 25 Jan 2021 14:20:01 GMT, Andrew Leonard  wrote:

>> A problem was found downstream with Eclipse OpenJ9 builds whereby since 
>> JDK-8258411 they were unable to extend the module lists.
>> This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to 
>> enable the module list extension again.
>> 
>> Signed-off-by: Andrew Leonard 
>
> Andrew Leonard has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   8260289: Unable to customize module lists after change JDK-8258411
>   
>   Signed-off-by: Andrew Leonard 

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411

[Alan Bateman]

On Mon, 25 Jan 2021 13:08:53 GMT, Magnus Ihse Bursie  wrote:

>> A problem was found downstream with Eclipse OpenJ9 builds whereby since 
>> JDK-8258411 they were unable to extend the module lists.
>> This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to 
>> enable the module list extension again.
>> 
>> Signed-off-by: Andrew Leonard 
>
> make/conf/module-loader-map.conf line 100:
> 
>> 98: # Hook to include the corresponding custom file, if present.
>> 99: $(eval $(call IncludeCustomExtension, conf/module-loader-map.conf)) 
>> 100: 
> 
> Using IncludeCustomExtension is a good idea, but you should to this where 
> module-loader-map.conf is included in common/Modules.gmk, not here. The 
> `*.conf` files are supposed to be formatted as simple configuration files, 
> and not include special make commands.

@magicus Is there an ordering issue in common/Modules.gmk? It also invokes 
IncludeCustomExtension but this is done before it includes the conf file.

-

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

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

[Alan Bateman]

On Fri, 15 Jan 2021 14:58:14 GMT, Alan Bateman  wrote:

>> This PR is not stale; it's just still waiting for input from @AlanBateman.
>
> @magicus Can the CharacterDataXXX.template files move to 
> src/java.base/share/classes/java/lang?

> @AlanBateman When I moved the charset templates, I found this gold nugget:
> 
> ```
> # Copy two Java files that need no preprocessing.
> $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/%.java: 
> $(CHARACTERDATA_TEMPLATES)/%.java.template
>   $(call LogInfo, Generating $(@F))
>   $(call install-file)
> 
> GENSRC_CHARACTERDATA += 
> $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataUndefined.java \
> 
> $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataPrivateUse.java
> ```
> 
> What this means is that we have two "template" files that are just compiled 
> as java files, but only after being copied to gensrc. :-) That definitely 
> needs cleaning up, but this PR is large enough as it is, so I will not do it 
> now.

Good find, surprised this wasn't spotted before now. We should create a 
separate issue to rename them and get rid of the copying in the make file.

-

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

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

[Alan Bateman]

On Mon, 11 Jan 2021 09:20:07 GMT, Magnus Ihse Bursie  wrote:

>> Marked as reviewed by prr (Reviewer).
>
> This PR is not stale; it's just still waiting for input from @AlanBateman.

@magicus Can the CharacterDataXXX.template files move to 
src/java.base/share/classes/java/lang?

-

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

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

[Alan Bateman]

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

Thanks for the updates.

-

Marked as reviewed by alanb (Reviewer).

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

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

[Alan Bateman]

On Wed, 16 Dec 2020 13:51:50 GMT, Magnus Ihse Bursie  wrote:

>> The update to JRE_MODULES in Images.gmk resolves my comment above. However, 
>> the naming for the configuration is still a bit odd,  e.g. 
>> module-sets-classloaders.conf should be something like 
>> module-loader-map.conf as used to generate ModuleLoaderMap.java in the build.
>
> @AlanBateman I don't have a problem with renaming the conf files, I just did 
> not know what you wanted them to be called. :-) I renamed 
> `module-sets-classloaders.conf` to `module-loader-map.conf`. Based on this, I 
> rename the other two files `javadoc-modules.conf` and 
> `build-module-sets.conf`, respectively. I hope this is okay. Otherwise, 
> please just let me know what you think they should be called.

Thanks for the update.

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.

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).

We should probably look at UPGRADEABLE_MODULES while we are here. This is the 
modules that are overriddable by way of excluding from the hashes stored in 
java.base (CreateJmods.gmk). I think it's okay to leave it in 
module-loader-map.conf because these modules are mapped to the platform class 
loader. Could we just rename it to UPGRADEABLE_PLATFORM_MODULES so that its a 
bit clearer (in Modules.gmk) as to why they are append to PLATFORM_MODULES?

-

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

Re: RFR: 8258420: Move URL configuration from Docs.gmk to conf dir

[Alan Bateman]

On Tue, 15 Dec 2020 17:50:46 GMT, Magnus Ihse Bursie  wrote:

> In `Docs.gmk` there are some hard-coded links to online URL documentation and 
> bug reporting locations. These should not reside in the make file per se, but 
> instead move to the `make/conf` directory.

Okay.

-

Marked as reviewed by alanb (Reviewer).

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

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

[Alan Bateman]

On Wed, 16 Dec 2020 00:14:02 GMT, Magnus Ihse Bursie  wrote:

>> 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?
>
> @mlchung The entire point of this exercise is to *not* have hard-coded lists 
> of modules in make files... 
> 
> Having hard-coded lists have come back to bite us, time after time again. We 
> try to auto-discover everything that is possible. For these sets of modules, 
> however, auto-discovery is not possible since these lists *define* what we 
> mean by e.g. platform modules or an interim image.

The update to JRE_MODULES in Images.gmk resolves my comment above. However, the 
naming for the configuration is still a bit odd,  e.g. 
module-sets-classloaders.conf should be something like module-loader-map.conf 
as used to generate ModuleLoaderMap.java in the build.

-

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

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

[Alan Bateman]

On Tue, 15 Dec 2020 22:52:30 GMT, Magnus Ihse Bursie  wrote:

>> Reviewed changes to `characterdata`, `charsetmapping`, `cldr`, `currency`, 
>> `lsrdata`, `tzdata`, and `unicodedata` with minor comment. Looks good 
>> overall.
>
> I think this is almost ready to be pushed, but I'd like to have someone from 
> the client team review the changes for java.desktop as well. @prrace Any 
> change you can have a look at this?

I think it will be annoying to have the charset mapping tables and other data 
in the src tree, have you looked at other alternatives?

-

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

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

[Alan Bateman]

On Tue, 15 Dec 2020 18:15:28 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:
> 
>   Split up module-sets.conf

This looks better but I think we need to find better names for the conf files. 
Prefixing them with "module-sets" looks really strange.
JRE_TOOL_MODULES in module-sets-classloaders.conf might also need to be 
re-examined because it is not used to generate ModuleLoaderMap. Instead it was 
defined in Modules.gmk for the legacy-jre-image build target.

-

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

Re: RFR: 8258420: Move URL configuration from Docs.gmk to conf dir

[Alan Bateman]

On Tue, 15 Dec 2020 17:50:46 GMT, Magnus Ihse Bursie  wrote:

> In `Docs.gmk` there are some hard-coded links to online URL documentation and 
> bug reporting locations. These should not reside in the make file per se, but 
> instead move to the `make/conf` directory.

This looks okay me to but I can't help thinking that javadoc.conf should also 
be the place to list the root modules for javadoc (PR 1781 puts them in oddly 
named module-sets-docs.conf).

-

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

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

[Alan Bateman]

On Tue, 15 Dec 2020 16:11:45 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.

I really dislike patch as it mixes up several things in module-sets.conf. If 
you really need to move configuration out of Modules.gmk (and I see no reason 
to do this) then please look at separating out the static mapping of modules to 
class loaders, the modules used for the interim builds, and the modules used to 
create API docs.

-

Changes requested by alanb (Reviewer).

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

Re: RFR: 8253497: Core Libs Terminology Refresh [v2]

[Alan Bateman]

On Tue, 15 Dec 2020 01:46:08 GMT, Brent Christian  wrote:

>> This is part of an effort in the JDK to replace archaic/non-inclusive words 
>> with more neutral terms (see JDK-8253315 for details).
>> 
>> Here are the changes covering core libraries code and tests.  Terms were 
>> changed as follows:
>> 1. grandfathered -> legacy
>> 2. blacklist -> filter or reject
>> 3. whitelist -> allow or accept
>> 4. master -> coordinator
>> 5. slave -> worker
>> 
>> Addressing similar issues in upstream 3rd party code is out of scope of this 
>> PR.  Such changes will be picked up from their upstream sources.
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   updates, per code review

test/jdk/java/nio/channels/SocketChannel/CloseRegisteredChannel.java line 45:

> 43: SocketChannel client = SocketChannel.open ();
> 44: client.connect (new InetSocketAddress 
> (InetAddress.getLoopbackAddress(), port));
> 45: SocketChannel channel = server.accept ();

Can you rename this to "peer" instead? (we can remove the spurious space after 
accept while we are there).

-

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

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

[Alan Bateman]

On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman  wrote:

>> Also, to clarify, for me there is a fundamental difference between 
>> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files 
>> that are part of the module, owned by the content team, and the `$MODULE` 
>> part is essential to delineate this content. The latter is owned by the 
>> build team, and is just a convenient way to organize the build system within 
>> the `make` directory. So it's clearly a no-no to put anything but `.gmk` 
>> files in `make/modules/$MODULE`.
>
> The mapping and nr tables, and the *-X.java.template files in 
> make/data/charsetmapping are used to generate source code for the java.base 
> and jdk.charsets modules. The stdcs-$OS files configure the package and 
> module that each charset go into. If the tables used to generate the source 
> files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk 
> will probably need a new home too.

> @AlanBateman The process of modularization was not fully completed with 
> Project Jigsaw, and a few ugly warts remained. I was under the impression 
> that these should be addressed in follow-up fixes, but this was unfortunately 
> never done. Charsets and cldrconverter were both split between a core portion 
> in java.base and the rest in jdk.charsets and jdk.localedata, respectively, 
> but the split was never handled properly, but just "duct taped" in place.

This is a complicated area of the build, not really a Project Jigsaw issue. 
It's complicated because the source code for the charsets is generated at build 
time and the set of non-standard charsets included in java.base varies by 
platform, e.g. there's are several IBMxxx charsets in java.base when building 
on AIX that are not interesting to include in java.base on other platforms. 
This means we can't split up the mapping tables in make/data/charsetmapping and 
put them in different directories. If you are moving them into the src tree 
then src/java.base (as you have it) is best but will still have the ugly wart 
that some of these mapping tables will be used to generate code for the 
jdk.charsets module.

-

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

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

[Alan Bateman]

On Tue, 8 Dec 2020 08:32:28 GMT, Magnus Ihse Bursie  wrote:

>> @mlchung If you don't have any strong preference, I implore you to accept 
>> this change. I **vastly** prefer to move the data out of `make`!
>> 
>> This is not just about Skara tooling. When working with make files, autoconf 
>> and shell scripts, there is no fancy IDE support, so you are stuck with 
>> simple text editors and tools like `grep`. I've lost count on how many times 
>> I've had my grep searches blow up, since I happened to find e.g. a string in 
>> `tzdata` and get hundreds or more of hits. :-( And I do believe we will get 
>> a better code structure if the build team "owns" `make`;  or at least has a 
>> vested interest in what's in that directory. We still suffer a lot of the 
>> old "I don't know where to put this file, so I'll just put it in make cause 
>> nobody cares about it anyway" mentality, but I've been working for quite 
>> some time to make that list of misplaced files shorter and shorter.
>
> Also, to clarify, for me there is a fundamental difference between 
> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files 
> that are part of the module, owned by the content team, and the `$MODULE` 
> part is essential to delineate this content. The latter is owned by the build 
> team, and is just a convenient way to organize the build system within the 
> `make` directory. So it's clearly a no-no to put anything but `.gmk` files in 
> `make/modules/$MODULE`.

The mapping and nr tables, and the *-X.java.template files in 
make/data/charsetmapping are used to generate source code for the java.base and 
jdk.charsets modules. The stdcs-$OS files configure the package and module that 
each charset go into. If the tables used to generate the source files are moved 
to src/java.base then make/modules/jdk.charsets/Gensrc.gmk will probably need a 
new home too.

-

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

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

[Alan Bateman]

On Mon, 7 Dec 2020 21:14:55 GMT, Joe Darcy  wrote:

>> test/jdk/java/lang/module/ClassFileVersionsTest.java line 107:
>> 
>>> 105: { 61,   0,  Set.of(STATIC, TRANSITIVE) },
>>> 106: 
>>> 107: { 62,   0,  Set.of()},   // JDK 18
>> 
>> This seems unduly repetitive. Could this be dynamically generated, perhaps 
>> in a future release?
>
> I've had similar thoughts; that strikes me as a fine RFE for after this fork. 
> I see what the code is doing, but haven't delved into the module system 
> details to understand exactly the rationale for these tests. In any case, 
> filed the RFE JDK-8257856: "Make ClassFileVersionsTest.java robust to JDK 
> version updates."

There was a change to JVMS 4.7.25 in Java 10 to add a rule for the 
requires_flags that are allowed. This is why this test started was created to 
test 53.0 vs. 54.0 class files. It wasn't intended to be a burden to update at 
each release so I'll re-implement it.

-

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

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

[Alan Bateman]

On Fri, 4 Dec 2020 11:38:51 GMT, Magnus Ihse Bursie  wrote:

> And I can certainly move jdwp.spec to java.base instead. 

If jdwp.spec has to move to the src tree then src/java.se is probably the most 
suitable home because Java SE specifies JDWP as an optional interface. So 
nothing to do with java.base and the build will need to continue to generate 
the sources for the front-end (jdk.jdi) and back-end (jdk.jdwp.agent) as they 
implement the protocol.

-

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

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

[Alan Bateman]

On Fri, 4 Dec 2020 10:29:48 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.
>
> To facilitate review, here is a list on how the different directories under 
> make/data has moved.
> 
> **To java.base:**
> * blacklistedcertsconverter
> * cacerts
> * characterdata
> * charsetmapping
> * cldr
> * currency
> * lsrdata
> * publicsuffixlist
> * tzdata
> * unicodedata
> 
> **To java.desktop:**
> * dtdbuilder
> * fontconfig
> * macosxicons
> * x11wrappergen
> 
> **To jdk.compiler:**
> * symbols
> 
> **To jdk.jdi:**
> * jdwp
> 
> **Remaining in make:**
> * bundle
> * docs-resources
> * macosxsigning
> * mainmanifest

Are you proposing any text or guidelines to be added to JEP 201 as part of this?

I think the location of jdwp.spec and its location in the build tree may need 
to be looked at again. It was convenient to have it in the make tree, from 
which the protocol spec, and source code for the front end (module jdk.jdi) and 
a header file for the back end (module jdk.jdwp.agent) are created. Given that 
the JDWP protocol is standard (not JDK-specific) then there may be an argument 
to put it in src/java.se instead of a JDK-specific module.

-

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

Re: RFR: 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal

[Alan Bateman]

On Thu, 3 Dec 2020 04:34:52 GMT, David Holmes  wrote:

> The signal-chaining facility was introduced in JDK 1.4 nearly 20 years ago 
> and supported three different Linux signal API's: sigset, signal and 
> sigaction:
> 
> https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html
> 
> Only sigaction is a Posix supported API for multi-threaded processes, that we 
> can use cross-platform. Both signal and sigset are obsolete and have 
> undefined behaviour in a multi-threaded process. From the Linux man pages:
> 
> sigset: This API is obsolete: new applications should use the POSIX signal 
> API (sigaction(2), sigprocmask(2), etc.)
> 
> signal: The behavior of signal() varies across UNIX versions, and has also 
> varied historically across different versions of Linux. Avoid its use: use 
> sigaction(2) instead.
> 
> We should deprecate the use of signal and sigset in JDK 16 with a view to 
> their removal in JDK 17.
> 
> A CSR request has been filed.
> 
> Testing: hotspot/jtreg/runtime/signal tests
> 
> Thanks,
> David

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8257533: legacy-jre-image includes jpackage and jlink tools

[Alan Bateman]

On Wed, 2 Dec 2020 10:16:13 GMT, Magnus Ihse Bursie  wrote:

> JEP 343 added jdk.jpackage to Module.gmk/JRE_TOOL_MODULES with the result 
> that the legacy-jre-image builds a run-time image that contains the package 
> and jlink tools. This seems to be an oversight as these tools should not be 
> in the equivalent of what we knew in the past as the "JRE".

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v26]

[Alan Bateman]

On Mon, 9 Nov 2020 16:07:13 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the third incubation round 
>> of the foreign memory access API incubation  (see JEP 393 [1]). This 
>> iteration focus on improving the usability of the API in 3 main ways:
>> 
>> * first, by providing a way to obtain truly *shared* segments, which can be 
>> accessed and closed concurrently from multiple threads
>> * second, by providing a way to register a memory segment against a 
>> `Cleaner`, so as to have some (optional) guarantee that the memory will be 
>> deallocated, eventually
>> * third, by not requiring users to dive deep into var handles when they 
>> first pick up the API; a new `MemoryAccess` class has been added, which 
>> defines several useful dereference routines; these are really just thin 
>> wrappers around memory access var handles, but they make the barrier of 
>> entry for using this API somewhat lower.
>> 
>> A big conceptual shift that comes with this API refresh is that the role of 
>> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it 
>> used to be the case that a memory address could (sometimes, not always) have 
>> a back link to the memory segment which originated it; additionally, memory 
>> access var handles used `MemoryAddress` as a basic unit of dereference.
>> 
>> This has all changed as per this API refresh;  now a `MemoryAddress` is just 
>> a dumb carrier which wraps a pair of object/long addressing coordinates; 
>> `MemorySegment` has become the star of the show, as far as dereferencing 
>> memory is concerned. You cannot dereference memory if you don't have a 
>> segment. This improves usability in a number of ways - first, it is a lot 
>> easier to wrap native addresses (`long`, essentially) into a 
>> `MemoryAddress`; secondly, it is crystal clear what a client has to do in 
>> order to dereference memory: if a client has a segment, it can use that; 
>> otherwise, if the client only has an address, it will have to create a 
>> segment *unsafely* (this can be done by calling 
>> `MemoryAddress::asSegmentRestricted`).
>> 
>> A list of the API, implementation and test changes is provided below. If  
>> you have any questions, or need more detailed explanations, I (and the  rest 
>> of the Panama team) will be happy to point at existing discussions,  and/or 
>> to provide the feedback required. 
>> 
>> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without 
>> whom the work on shared memory segment would not have been possible; also 
>> I'd like to thank Paul Sandoz, whose insights on API design have been very 
>> helpful in this journey.
>> 
>> Thanks 
>> Maurizio 
>> 
>> Javadoc: 
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff: 
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html
>> 
>> CSR: 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254163
>> 
>> 
>> 
>> ### API Changes
>> 
>> * `MemorySegment`
>>   * drop factory for restricted segment (this has been moved to 
>> `MemoryAddress`, see below)
>>   * added a no-arg factory for a native restricted segment representing 
>> entire native heap
>>   * rename `withOwnerThread` to `handoff`
>>   * add new `share` method, to create shared segments
>>   * add new `registerCleaner` method, to register a segment against a cleaner
>>   * add more helpers to create arrays from a segment e.g. `toIntArray`
>>   * add some `asSlice` overloads (to make up for the fact that now segments 
>> are more frequently used as cursors)
>>   * rename `baseAddress` to `address` (so that `MemorySegment` can implement 
>> `Addressable`)
>> * `MemoryAddress`
>>   * drop `segment` accessor
>>   * drop `rebase` method and replace it with `segmentOffset` which returns 
>> the offset (a `long`) of this address relative to a given segment
>> * `MemoryAccess`
>>   * New class supporting several static dereference helpers; the helpers are 
>> organized by carrier and access mode, where a carrier is one of the usual 
>> suspect (a Java primitive, minus `boolean`); the access mode can be simple 
>> (e.g. access base address of given segment), or indexed, in which case the 
>> accessor takes a segment and either a low-level byte offset,or a high level 
>> logical index. The classification is reflected in the naming scheme (e.g. 
>> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`).
>> * `MemoryHandles`
>>   * drop `withOffset` combinator
>>   * drop `withStride` combinator
>>   * the basic memory access handle factory now returns a var handle which 
>> takes a `MemorySegment` and a `long` - from which it is easy to derive all 
>> the other handles using plain var handle combinators.
>> * `Addressable`
>>   * This is a new interface which is attached to entities which can be 
>> projected to a `MemoryAddress`. For now, both `MemoryAddress` and 
>> 

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v22]

[Alan Bateman]

On Thu, 5 Nov 2020 17:14:16 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the third incubation round 
>> of the foreign memory access API incubation  (see JEP 393 [1]). This 
>> iteration focus on improving the usability of the API in 3 main ways:
>> 
>> * first, by providing a way to obtain truly *shared* segments, which can be 
>> accessed and closed concurrently from multiple threads
>> * second, by providing a way to register a memory segment against a 
>> `Cleaner`, so as to have some (optional) guarantee that the memory will be 
>> deallocated, eventually
>> * third, by not requiring users to dive deep into var handles when they 
>> first pick up the API; a new `MemoryAccess` class has been added, which 
>> defines several useful dereference routines; these are really just thin 
>> wrappers around memory access var handles, but they make the barrier of 
>> entry for using this API somewhat lower.
>> 
>> A big conceptual shift that comes with this API refresh is that the role of 
>> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it 
>> used to be the case that a memory address could (sometimes, not always) have 
>> a back link to the memory segment which originated it; additionally, memory 
>> access var handles used `MemoryAddress` as a basic unit of dereference.
>> 
>> This has all changed as per this API refresh;  now a `MemoryAddress` is just 
>> a dumb carrier which wraps a pair of object/long addressing coordinates; 
>> `MemorySegment` has become the star of the show, as far as dereferencing 
>> memory is concerned. You cannot dereference memory if you don't have a 
>> segment. This improves usability in a number of ways - first, it is a lot 
>> easier to wrap native addresses (`long`, essentially) into a 
>> `MemoryAddress`; secondly, it is crystal clear what a client has to do in 
>> order to dereference memory: if a client has a segment, it can use that; 
>> otherwise, if the client only has an address, it will have to create a 
>> segment *unsafely* (this can be done by calling 
>> `MemoryAddress::asSegmentRestricted`).
>> 
>> A list of the API, implementation and test changes is provided below. If  
>> you have any questions, or need more detailed explanations, I (and the  rest 
>> of the Panama team) will be happy to point at existing discussions,  and/or 
>> to provide the feedback required. 
>> 
>> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without 
>> whom the work on shared memory segment would not have been possible; also 
>> I'd like to thank Paul Sandoz, whose insights on API design have been very 
>> helpful in this journey.
>> 
>> Thanks 
>> Maurizio 
>> 
>> Javadoc: 
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff: 
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html
>> 
>> CSR: 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254163
>> 
>> 
>> 
>> ### API Changes
>> 
>> * `MemorySegment`
>>   * drop factory for restricted segment (this has been moved to 
>> `MemoryAddress`, see below)
>>   * added a no-arg factory for a native restricted segment representing 
>> entire native heap
>>   * rename `withOwnerThread` to `handoff`
>>   * add new `share` method, to create shared segments
>>   * add new `registerCleaner` method, to register a segment against a cleaner
>>   * add more helpers to create arrays from a segment e.g. `toIntArray`
>>   * add some `asSlice` overloads (to make up for the fact that now segments 
>> are more frequently used as cursors)
>>   * rename `baseAddress` to `address` (so that `MemorySegment` can implement 
>> `Addressable`)
>> * `MemoryAddress`
>>   * drop `segment` accessor
>>   * drop `rebase` method and replace it with `segmentOffset` which returns 
>> the offset (a `long`) of this address relative to a given segment
>> * `MemoryAccess`
>>   * New class supporting several static dereference helpers; the helpers are 
>> organized by carrier and access mode, where a carrier is one of the usual 
>> suspect (a Java primitive, minus `boolean`); the access mode can be simple 
>> (e.g. access base address of given segment), or indexed, in which case the 
>> accessor takes a segment and either a low-level byte offset,or a high level 
>> logical index. The classification is reflected in the naming scheme (e.g. 
>> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`).
>> * `MemoryHandles`
>>   * drop `withOffset` combinator
>>   * drop `withStride` combinator
>>   * the basic memory access handle factory now returns a var handle which 
>> takes a `MemorySegment` and a `long` - from which it is easy to derive all 
>> the other handles using plain var handle combinators.
>> * `Addressable`
>>   * This is a new interface which is attached to entities which can be 
>> projected to a `MemoryAddress`. For now, both `MemoryAddress` and 
>> 

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)

[Alan Bateman]

On Wed, 4 Nov 2020 07:45:09 GMT, Alan Bateman  wrote:

>>> The javadoc for copyFrom isn't changed in this update but I notice it 
>>> specifies IndexOutOfBoundException when the source segment is larger than 
>>> the receiver, have other exceptions been examined?
>> 
>> This exception is consistent with other uses of this exception throughout 
>> this API (e.g. when writing a segment out of bounds).
>
> I assume the CSR needs to be updated so that it's in sync with the API 
> changes in the latest round.

I see the xxxByteAtIndex methods that took a ByteOrder have been removed from 
MemoryAccess. Should the xxxByte and xxxByteAtOffset that take a ByteOrder be 
removed too?

-

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

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)

[Alan Bateman]

On Mon, 2 Nov 2020 11:26:51 GMT, Maurizio Cimadamore  
wrote:

>> I looked through the changes in this update.
>> 
>> The shared memory segment support looks sound and the mechanism to close a 
>> shared memory segment is clever (albeit a bit surprising at first that it 
>> does global handshake to look for a frame in a scoped region. Also 
>> surprising that close can cause failure at both ends - it took me a while to 
>> see that this is pragmatic approach).
>> 
>> The share method specifies NPE if thread == null but there is no thread 
>> parameter, is this a cut 'n paste error? Another one in registerCleaner 
>> where it should be NPE if the cleaner is null.
>> 
>> I think the javadoc for the close method needs to be a bit clearer on the 
>> state of the memory segment when IllegalStateException is thrown. Will it be 
>> marked "not alive" when it fails? Does this mean there is a resource leak? I 
>> think an apiNote to explain the rational for why close is not idempotent is 
>> also needed, or maybe it should be re-visited so that close is a no-op when 
>> the memory segment is not alive.
>> 
>> Now that MemorySegment is AutoCloseable then maybe the term "alive" should 
>> be replaced with "open" or  "closed" and isAlive replaced with isOpen is 
>> isClosed.
>> 
>> FileDescriptor can be attraction nuisance and forced reference counting 
>> everywhere that it is used. Is it needed? Could an isMapped method work 
>> instead?
>> 
>> mapFromPath was in the second preview but I think the method name should be 
>> re-examined as it maps a file, the path just locates the file.  Naming is 
>> subjectives but in this case using "map" or "mapFile" would fit beside the 
>> allocateNative methods.
>> 
>> MappedMemorySegments. The force method specifies a write back guarantee but 
>> at the same time, the implNote in the class description suggests that the 
>> methods might be a no-op. You might want to adjust the wording to avoid any 
>> suggestion that force might be a no-op.
>> 
>> The javadoc for copyFrom isn't changed in this update but I notice it 
>> specifies IndexOutOfBoundException when the source segment is larger than 
>> the receiver, have other exceptions been examined?
>> 
>> I don't have any any comments on MemoryAccess except that it's not 
>> immediately clear why there are "Byte" methods that take a ByteOrder. Make 
>> sense for the multi-byte types of course.
>> 
>> The updates the java/nio sources look okay but it would be helpful if the 
>> really long lines could be chopped down as it's just too hard to do 
>> side-by-side reviews when the lines are so long. A minor nit but the changes 
>> X-Buffer.java.template mess up the alignment of the parameters to 
>> copyMemory/copySwapMemory methods.
>
>> The javadoc for copyFrom isn't changed in this update but I notice it 
>> specifies IndexOutOfBoundException when the source segment is larger than 
>> the receiver, have other exceptions been examined?
> 
> This exception is consistent with other uses of this exception throughout 
> this API (e.g. when writing a segment out of bounds).

I assume the CSR needs to be updated so that it's in sync with the API changes 
in the latest round.

-

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

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)

[Alan Bateman]

On Thu, 29 Oct 2020 14:13:40 GMT, Maurizio Cimadamore  
wrote:

>>> @mcimadamore, if you pull from current master, you would get the Linux 
>>> x86_32 tier1 run "for free".
>> 
>> Just did that - I also removed TestMismatch from the problem list in the 
>> latest iteration, and fixed the alignment for long/double layouts, after 
>> chatting with the team (https://bugs.openjdk.java.net/browse/JDK-8255350)
>
> I've just uploaded another iteration which addresses some comments from 
> @AlanBateman. Basically, there are some operations on Channel and Socket 
> which take ByteBuffer as arguments, and then, if such buffers are *direct*, 
> they get the address and pass it down to some native function. This idiom is 
> problematic because there's no way to guarantee that the buffer won't be 
> closed (if obtained from a memory segment) after the address has been 
> obtained. As a stop gap solution, I've introduced checks in 
> `DirectBuffer::address` method, which is used in around 30 places in the JDK. 
> This method will now throw if (a) the buffer has a shared scope, or (b) if 
> the scope is confined, but already closed. With this extra check, I believe 
> there's no way to misuse the buffer obtained from a segment. We have 
> discussed plans to remove this limitations (which we think will be possible) 
> - but for the time being, it's better to play the conservative card.

I looked through the changes in this update.

The shared memory segment support looks sound and the mechanism to close a 
shared memory segment is clever (albeit a bit surprising at first that it does 
global handshake to look for a frame in a scoped region. Also surprising that 
close can cause failure at both ends - it took me a while to see that this is 
pragmatic approach).

The share method specifies NPE if thread == null but there is no thread 
parameter, is this a cut 'n paste error? Another one in registerCleaner where 
it should be NPE if the cleaner is null.

I think the javadoc for the close method needs to be a bit clearer on the state 
of the memory segment when IllegalStateException is thrown. Will it be marked 
"not alive" when it fails? Does this mean there is a resource leak? I think an 
apiNote to explain the rational for why close is not idempotent is also needed, 
or maybe it should be re-visited so that close is a no-op when the memory 
segment is not alive.

Now that MemorySegment is AutoCloseable then maybe the term "alive" should be 
replaced with "open" or  "closed" and isAlive replaced with isOpen is isClosed.

FileDescriptor can be attraction nuisance and forced reference counting 
everywhere that it is used. Is it needed? Could an isMapped method work instead?

mapFromPath was in the second preview but I think the method name should be 
re-examined as it maps a file, the path just locates the file.  Naming is 
subjectives but in this case using "map" or "mapFile" would fit beside the 
allocateNative methods.

MappedMemorySegments. The force method specifies a write back guarantee but at 
the same time, the implNote in the class description suggests that the methods 
might be a no-op. You might want to adjust the wording to avoid any suggestion 
that force might be a no-op.

The javadoc for copyFrom isn't changed in this update but I notice it specifies 
IndexOutOfBoundException when the source segment is larger than the receiver, 
have other exceptions been examined?

I don't have any any comments on MemoryAccess except that it's not immediately 
clear why there are "Byte" methods that take a ByteOrder. Make sense for the 
multi-byte types of course.

The updates the java/nio sources look okay but it would be helpful if the 
really long lines could be chopped down as it's just too hard to do 
side-by-side reviews when the lines are so long. A minor nit but the changes 
X-Buffer.java.template mess up the alignment of the parameters to 
copyMemory/copySwapMemory methods.

-

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

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

[Alan Bateman]

On Wed, 21 Oct 2020 02:28:30 GMT, Kim Barrett  wrote:

>> Finally returning to this review that was started in April 2020.  I've
>> recast it as a github PR.  I think the security concern raised by Gil
>> has been adequately answered.
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
>> 
>> Please review a new function: java.lang.ref.Reference.refersTo.
>> 
>> This function is needed to test the referent of a Reference object without
>> artificially extending the lifetime of the referent object, as may happen
>> when calling Reference.get.  Some garbage collectors require extending the
>> lifetime of a weak referent when accessed, in order to maintain collector
>> invariants.  Lifetime extension may occur with any collector when the
>> Reference is a SoftReference, as calling get indicates recent access.  This
>> new function also allows testing the referent of a PhantomReference, which
>> can't be accessed by calling get.
>> 
>> The new function uses native methods whose implementations are in the VM so
>> they can use the Access API.  It is the intent that these methods will be
>> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
>> implemented yet.  Bear that in mind before rushing off to change existing
>> uses of Reference.get.
>> 
>> There are two native methods involved, one in Reference and an override in
>> PhantomReference, both package private in java.lang.ref. The reason for this
>> split is to simplify the intrinsification. This is a change from the version
>> from April 2020; that version had a single native method in Reference,
>> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
>> However, adding support for that category in the compilers adds significant
>> implementation effort and complexity. Splitting avoids that complexity.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) verified the new test passes with various garbage 
>> collectors.
>
> Kim Barrett has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   improve wording in refersTo javadoc

The API looks good, thanks for getting this in.

-

Marked as reviewed by alanb (Reviewer).

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

Re: RFR: 8245194: Unix domain socket channel implementation [v13]

[Alan Bateman]

On Fri, 2 Oct 2020 13:17:04 GMT, Michael McMahon  wrote:

>> make/modules/java.base/Copy.gmk line 195:
>> 
>>> 193:$(call MakeTargetDir)
>>> 194:$(RM) $@ $@.tmp
>>> 195:$(foreach f,$(NET_PROPERTIES_SRC_LIST),$(CAT) $(f) >> $@.tmp;)
>> 
>> This can be simplified. Cat takes multiple files as input, so no need for 
>> 'foreach'. Also no need to go via a temp
>> file. We have make configured to delete targets if a recipe fails, so the 
>> tmp dance isn't needed. (I know we still have
>> this pattern all over the place, but we are trying to quit the practice)
>
> Good points. I will update as suggested. Thanks.

I would prefer if we didn't rename net.properties. Can we use the same approach 
as lib/security/default.policy where
the share and platform specific are concatenated?

-

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

Re: How can I know which vulnerabilities (CVEs) are fixed in specific tag of open JDK?

[Alan Bateman]

On 23/09/2020 11:29, Moshe Zuisman wrote:

Hi.
I have the following problem. We provide OpenJDK binary distro with our
product.
With the current version we provided JDK8u-b222
Customer comes with a list of CVEs and asks if they are fixed in distro, we
provided with our product.
For example he asks about cve-2014-3566, jre-vuln-cve-2017-3241(it is only
a part of the full list he asks about).
In the release note of b222 (
https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-July/009840.html) I
do not see any info about fixed CVEs.
Is there any way I figure out what is a full list of CVEs - fixed in
specific, or opposite - can I somehow know if some specific CVE fixed in
some build?
Advisories are posted to the vuln-announce mailing list and also linked 
from the advisories page [1].


-Alan

[1] https://openjdk.java.net/groups/vulnerability/advisories/

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[Alan Bateman]

On Thu, 10 Sep 2020 13:53:10 GMT, David Holmes  wrote:

>> @dholmes-ora raises a good point. Hopefully there is a balance point between 
>> a dozen different bugs / pull requests
>> each targeting one area and one bug / PR targeting a dozen separate areas. I 
>> don't have a good general rule to suggest.
>> Maybe @AlanBateman or @jddarcy can offer some advice?
>
> 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to 
> post, but even if you are a reply-all will
> be delayed due to all of the mails being held up for moderator approval due 
> to:
> " Too many recipients to the message"
> 
> I have a longer email coming once it gets through the moderator approval 
> delay. :(

Patches that do global replace are always awkward. In this case, there are 150 
files changed and most seem to be tests
or changes to tools used in the build (includes 
src/hotspot/share/prims/jvmtiEnvFill.java). If these are dropped from
the patch then it leaves just 43 files, many of which are from 3rd party code 
that can also be dropped. This should
reduce the patch down to a manageable 24 or so files that should be trivial to 
review.

-

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

Re: JDK-8252803: Add the /LARGEADDRESSAWARE flag when linking executables for Windows 32-bit.

[Alan Bateman]

On 10/09/2020 10:44, Archana Nogriya wrote:

Hi,

I am requesting a proposal for the contribution to OpenJDK.
I have created webrev with my changes.

Moving to build-dev as that is normally where the linker options are 
discussed.


I don't know if there is a significant need for Windows 32-bit build 
these days but one thing to mention is that adding /LARGEADDRESSAWARE 
created compatibility concerns with JNI code in the past. I see Joe 
Darcy has linked to JDK-5061380 [1], there may be others.


-Alan

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

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[Alan Bateman]

On Thu, 10 Sep 2020 08:47:35 GMT, Dmitriy Dumanskiy 
 wrote:

>> Before this Enhancement can be formally reviewed, you will need a JBS bug 
>> ID. If you are already working with a
>> Committer or Reviewer in the `jdk` project who has agreed to sponsor your 
>> change, they can file the Enhancement
>> request. Otherwise, you can file it using the web interface at 
>> [bugreport.java.com](https://bugreport.java.com/). Once
>> you have a JBS bug ID, you need to edit the PR title to include that bug ID 
>> (without the `JDK-`) replacing
>> "Improvement".   Since this PR cuts across many functional areas (each gray 
>> label represents a functional area), you
>> should expect a longer review process, since someone from each functional 
>> area will need to look at the changes in
>> their area (like @mrserb started to do for the `2d` area).
>
> @kevinrushforth thanks. Done.
> 
> Similar issues:
> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014
> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246
> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237
> 
> could be joined somehow?

The code in java.base was updated to use String::isEmpty in JDK 12 
(JDK-8215281). There was follow-up in JDK 13 to do
the same in the java.desktop module (JDK-8223237). Changing the remaining 
usages make sense although I see that more
more than half are in tests.

It would be good to hear from security-dev on the changes to the Apache 
Santuario code (in java.xml.crypto module) in
case it would be better to contribute those upstream instead. Ditto for the 
Apache Xalan code (in the java.xml module)
but it may be significantly forked already that it doesn't matter.

I assume you can use JDK-8215014 rather than creating a new issue.

-

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

Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v2]

[Alan Bateman]

On Wed, 9 Sep 2020 09:49:44 GMT, Philippe Marschall 
 wrote:

>> Hello, newbie here
>> 
>> I picked JDK-8138732 to work on because it has a "starter" label and I 
>> believe I understand what to do.
>> 
>> - I tried to update the copyright year to 2020 in every file.
>> - I decided to change `@since` from 9 to 16 since it is a new annotation 
>> name in a new package.
>> - I tried to keep code changes to a minimum, eg not change to imports if 
>> fully qualified class names are used instead of
>>   imports. In some cases I did minor reordering of imports to keep them 
>> sorted alphabetically.
>> - All tier1 tests pass.
>> - One jpackage/jlink tier2 test fails but I don't believe it is related.
>> - Some tier3 Swing tests fail but I don't think they are related.
>
> Philippe Marschall has refreshed the contents of this pull request, and 
> previous commits have been removed. The
> incremental views will show differences compared to the previous content of 
> the PR. The pull request contains one new
> commit since the last revision:
>   8138732: Rename HotSpotIntrinsicCandidate to IntrinsicCandidate

Marked as reviewed by alanb (Reviewer).

-

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

Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package

[Alan Bateman]

On Mon, 7 Sep 2020 09:44:09 GMT, Philippe Marschall 
 wrote:

> Hello, newbie here
> 
> I picked JDK-8138732 to work on because it has a "starter" label and I 
> believe I understand what to do.
> 
> - I tried to update the copyright year to 2020 in every file.
> - I decided to change `@since` from 9 to 16 since it is a new annotation name 
> in a new package.
> - I tried to keep code changes to a minimum, eg not change to imports if 
> fully qualified class names are used instead of
>   imports. In some cases I did minor reordering of imports to keep them 
> sorted alphabetically.
> - All tier1 tests pass.
> - One jpackage/jlink tier2 test fails but I don't believe it is related.
> - Some tier3 Swing tests fail but I don't think they are related.

This one probably needs discussion on hotspot-dev to get agreement on the 
rename/move. It might need coordination with
Graal. If the change does go ahead then please check if java.base's 
module-info.java still needs to export jdk.internal
to jdk.jfr, I suspect that qualified export can be removed.

-

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

Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port

[Alan Bateman]

On Mon, 7 Sep 2020 11:23:28 GMT, Aleksei Voitylov  wrote:

> continuing the review thread from here 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html
> 
>> The download side of using JNI in these tests is that it complicates the
>> setup a bit for those that run jtreg directly and/or just build the JDK
>> and not the test libraries. You could reduce this burden a bit by
>> limiting the load library/isMusl check to Linux only, meaning isMusl
>> would not be called on other platforms.
>>
>> The alternative you suggest above might indeed be better. I assume you
>> don't mean splitting the tests but rather just adding a second @test
>> description so that the vm.musl case runs the test with a system
>> property that allows the test know the expected load library path behavior.
> 
> I have updated the PR to split the two tests in multiple @test s.
> 
>> The updated comment in java_md.c in this looks good. A minor comment on
>> Platform.isBusybox is Files.isSymbolicLink returning true implies that
>> the link exists so no need to check for exists too. Also the
>> if-then-else style for the new class in ProcessBuilder/Basic.java is
>> inconsistent with the rest of the test so it stands out.
> 
> Thank you, these changes are done in the updated PR.
> 
>> Given the repo transition this weekend then I assume you'll create a PR
>> for the final review at least. Also I see JEP 386 hasn't been targeted
>> yet but I assume Boris, as owner, will propose-to-target and wait for it
>> to be targeted before it is integrated.
> 
> Yes. How can this be best accomplished with the new git workflow?
> - we can continue the review process till the end and I will request the 
> integration to happen only after the JEP is
>   targeted. I guess this step is now done by typing "slash integrate" in a 
> comment.
> - we can pause the review process now until the JEP is targeted.
> 
> In the first case I'm kindly asking the Reviewers who already chimed in on 
> that to re-confirm the review here.

Marked as reviewed by alanb (Reviewer).

This change was in review on core-libs-dev and other mailing lists before the 
switch to skara/git. The issues that I
brought up have been added in the PR and I don't have any further comments.

-

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

Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port

[Alan Bateman]

On 04/09/2020 21:55, Aleksei Voitylov wrote:

Alan,

here is a simpler version which, for the two tests in question, refers
to a local helper class with a native method:

http://cr.openjdk.java.net/~avoitylov/webrev.8247589.07/

If there is a preference to avoid JNI, there is yet another alternative:
split the two launcher tests in question into tests  with @requires
vm.musl | os.family = "aix" and with @requires !vm.musl & os.family !=
"aix".

The download side of using JNI in these tests is that it complicates the 
setup a bit for those that run jtreg directly and/or just build the JDK 
and not the test libraries. You could reduce this burden a bit by 
limiting the load library/isMusl check to Linux only, meaning isMusl 
would not be called on other platforms.


The alternative you suggest above might indeed be better. I assume you 
don't mean splitting the tests but rather just adding a second @test 
description so that the vm.musl case runs the test with a system 
property that allows the test know the expected load library path behavior.


The updated comment in java_md.c in this looks good. A minor comment on 
Platform.isBusybox is Files.isSymbolicLink returning true implies that 
the link exists so no need to check for exists too. Also the 
if-then-else style for the new class in ProcessBuilder/Basic.java is 
inconsistent with the rest of the test so it stands out.


Given the repo transition this weekend then I assume you'll create a PR 
for the final review at least. Also I see JEP 386 hasn't been targeted 
yet but I assume Boris, as owner, will propose-to-target and wait for it 
to be targeted before it is integrated.


-Alan