Re: RFR: 8286176: Add JNI_VERSION_19 to jni.h and JNI spec

[David Holmes]

On Sat, 11 Jun 2022 15:42:34 GMT, Alan Bateman  wrote:

> JNI is updated in Java 19 so we need to define JNI_VERSION_19 and change 
> GetVersion to return this version.
> 
> test/hotspot/jtreg/native_sanity/JniVersion.java is updated to check that 
> JNI_VERSION_19 is returned. The native library in the JMX agent, and several 
> tests, define JNI_OnLoad that return JNI_VERSION_10 as the JNI version 
> needed. These are updated to return JNI_VERSION_19 although these updates 
> aren't strictly needed.

@AlanBateman  sorry I missed your statement "although these updates aren't 
strictly needed".

-

PR: https://git.openjdk.org/jdk19/pull/7

Re: RFR: 8288378: [BACKOUT] DST not applying properly with zone id offset set with TZ env variable

[David Holmes]

On Tue, 14 Jun 2022 00:56:47 GMT, Naoto Sato  wrote:

> Backing out a fix that is causing a T1 failure.
> 
> This reverts commit 9b6d0a7e94fd18d302c559bec6f785d71a919a88.

Backout looks accurate - thanks.

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9151

Integrated: 8279047: Remove expired flags in JDK 20

[David Holmes]

On Fri, 10 Jun 2022 13:23:51 GMT, David Holmes  wrote:

> Expired Flags in 20:
> 
> - FilterSpuriousWakeups
> - MinInliningThreshold
> - PrefetchFieldsAhead
> 
> No remaining usages in code or tests.
> 
> - UseHeavyMonitors  (expired in PRODUCT ONLY)
> 
> As this flag now only exists for debug builds it has to be a "develop" flag 
> rather than product. There are then changes to two tests that use this flag, 
> so that they only run on a debug VM.
> 
> - test/jdk/java/lang/Thread/virtual/HoldsLock.java
> 
> The second @run that uses UseHeavyMonitors is moved to a second test segment 
> that only applies to the debug VM.
> 
> - test/jdk/java/util/concurrent/ConcurrentHashMap/MapLoops.java
> 
> Change the test section that uses UseHeavyMonitors to only run on a debug VM 
> and remove the IgnoreUnrecognizedOptions flag. Note that the existing test 
> logic would run the same test twice on product builds.
> 
> No documentation in the manpage exists for any of the newly expired flags.
> 
> Obsoleted flags in 20:
> 
> - ExtendedDTraceProbes
> 
> Documented in manpage so moved from Deprecated section to Obsolete section. 
> There is special handling for messages about use of this flag so that won't 
> be updated until the flag is actually obsoleted (JDK-8279913).
> 
> - UseContainerCpuShares
> - PreferContainerQuotaForCPUCount
> - AliasLevel
> 
> Undocumented.
> 
> Java manpage updates:
> - Updates Java version to 20
> - Moved ExtendedDTraceProbe info as previously mentioned
> - Applied changes from the .1 file that had not been applied to the markdown 
> source so that they were not lost (and note the .1 file was also missing 
> changes from the markdown file that had not been propagated).
> - Removed an unrepresentable character (the section symbol) that was not 
> being generated into either the html or nroff file correctly

This pull request has now been integrated.

Changeset: d46f404b
Author:David Holmes 
URL:   
https://git.openjdk.org/jdk/commit/d46f404b3179c66e8e5775a9e2253c95238153c7
Stats: 69 lines in 5 files changed: 23 ins; 26 del; 20 mod

8279047: Remove expired flags in JDK 20

Reviewed-by: kvn, kbarrett, alanb

-

PR: https://git.openjdk.org/jdk/pull/9127

Re: RFR: 8279047: Remove expired flags in JDK 20

[David Holmes]

On Sat, 11 Jun 2022 05:48:14 GMT, Alan Bateman  wrote:

>> Expired Flags in 20:
>> 
>> - FilterSpuriousWakeups
>> - MinInliningThreshold
>> - PrefetchFieldsAhead
>> 
>> No remaining usages in code or tests.
>> 
>> - UseHeavyMonitors  (expired in PRODUCT ONLY)
>> 
>> As this flag now only exists for debug builds it has to be a "develop" flag 
>> rather than product. There are then changes to two tests that use this flag, 
>> so that they only run on a debug VM.
>> 
>> - test/jdk/java/lang/Thread/virtual/HoldsLock.java
>> 
>> The second @run that uses UseHeavyMonitors is moved to a second test segment 
>> that only applies to the debug VM.
>> 
>> - test/jdk/java/util/concurrent/ConcurrentHashMap/MapLoops.java
>> 
>> Change the test section that uses UseHeavyMonitors to only run on a debug VM 
>> and remove the IgnoreUnrecognizedOptions flag. Note that the existing test 
>> logic would run the same test twice on product builds.
>> 
>> No documentation in the manpage exists for any of the newly expired flags.
>> 
>> Obsoleted flags in 20:
>> 
>> - ExtendedDTraceProbes
>> 
>> Documented in manpage so moved from Deprecated section to Obsolete section. 
>> There is special handling for messages about use of this flag so that won't 
>> be updated until the flag is actually obsoleted (JDK-8279913).
>> 
>> - UseContainerCpuShares
>> - PreferContainerQuotaForCPUCount
>> - AliasLevel
>> 
>> Undocumented.
>> 
>> Java manpage updates:
>> - Updates Java version to 20
>> - Moved ExtendedDTraceProbe info as previously mentioned
>> - Applied changes from the .1 file that had not been applied to the markdown 
>> source so that they were not lost (and note the .1 file was also missing 
>> changes from the markdown file that had not been propagated).
>> - Removed an unrepresentable character (the section symbol) that was not 
>> being generated into either the html or nroff file correctly
>
> Marked as reviewed by alanb (Reviewer).

Thanks for the review @AlanBateman !

-

PR: https://git.openjdk.org/jdk/pull/9127

Re: RFR: 8279047: Remove expired flags in JDK 20

[David Holmes]

On Fri, 10 Jun 2022 15:57:40 GMT, Vladimir Kozlov  wrote:

>> Expired Flags in 20:
>> 
>> - FilterSpuriousWakeups
>> - MinInliningThreshold
>> - PrefetchFieldsAhead
>> 
>> No remaining usages in code or tests.
>> 
>> - UseHeavyMonitors  (expired in PRODUCT ONLY)
>> 
>> As this flag now only exists for debug builds it has to be a "develop" flag 
>> rather than product. There are then changes to two tests that use this flag, 
>> so that they only run on a debug VM.
>> 
>> - test/jdk/java/lang/Thread/virtual/HoldsLock.java
>> 
>> The second @run that uses UseHeavyMonitors is moved to a second test segment 
>> that only applies to the debug VM.
>> 
>> - test/jdk/java/util/concurrent/ConcurrentHashMap/MapLoops.java
>> 
>> Change the test section that uses UseHeavyMonitors to only run on a debug VM 
>> and remove the IgnoreUnrecognizedOptions flag. Note that the existing test 
>> logic would run the same test twice on product builds.
>> 
>> No documentation in the manpage exists for any of the newly expired flags.
>> 
>> Obsoleted flags in 20:
>> 
>> - ExtendedDTraceProbes
>> 
>> Documented in manpage so moved from Deprecated section to Obsolete section. 
>> There is special handling for messages about use of this flag so that won't 
>> be updated until the flag is actually obsoleted (JDK-8279913).
>> 
>> - UseContainerCpuShares
>> - PreferContainerQuotaForCPUCount
>> - AliasLevel
>> 
>> Undocumented.
>> 
>> Java manpage updates:
>> - Updates Java version to 20
>> - Moved ExtendedDTraceProbe info as previously mentioned
>> - Applied changes from the .1 file that had not been applied to the markdown 
>> source so that they were not lost (and note the .1 file was also missing 
>> changes from the markdown file that had not been propagated).
>> - Removed an unrepresentable character (the section symbol) that was not 
>> being generated into either the html or nroff file correctly
>
> Looks good.

Thanks for the reviews @vnkozlov  and @kimbarrett !

-

PR: https://git.openjdk.org/jdk/pull/9127

RFR: 8279047: Remove expired flags in JDK 20

[David Holmes]

Expired Flags in 20:

- FilterSpuriousWakeups
- MinInliningThreshold
- PrefetchFieldsAhead

No remaining usages in code or tests.

- UseHeavyMonitors  (expired in PRODUCT ONLY)

As this flag now only exists for debug builds it has to be a "develop" flag 
rather than product. There are then changes to two tests that use this flag, so 
that they only run on a debug VM.

- test/jdk/java/lang/Thread/virtual/HoldsLock.java

The second @run that uses UseHeavyMonitors is moved to a second test segment 
that only applies to the debug VM.

- test/jdk/java/util/concurrent/ConcurrentHashMap/MapLoops.java

Change the test section that uses UseHeavyMonitors to only run on a debug VM 
and remove the IgnoreUnrecognizedOptions flag. Note that the existing test 
logic would run the same test twice on product builds.

No documentation in the manpage exists for any of the newly expired flags.

Obsoleted flags in 20:

- ExtendedDTraceProbes

Documented in manpage so moved from Deprecated section to Obsolete section. 
There is special handling for messages about use of this flag so that won't be 
updated until the flag is actually obsoleted (JDK-8279913).

- UseContainerCpuShares
- PreferContainerQuotaForCPUCount
- AliasLevel

Undocumented.

Java manpage updates:
- Updates Java version to 20
- Moved ExtendedDTraceProbe info as previously mentioned
- Applied changes from the .1 file that had not been applied to the markdown 
source so that they were not lost (and note the .1 file was also missing 
changes from the markdown file that had not been propagated).
- Removed an unrepresentable character (the section symbol) that was not being 
generated into either the html or nroff file correctly

-

Commit messages:
 - Fix typo
 - 8279047: Remove expired flags in JDK 20

Changes: https://git.openjdk.org/jdk/pull/9127/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9127=00
  Issue: https://bugs.openjdk.org/browse/JDK-8279047
  Stats: 69 lines in 5 files changed: 23 ins; 26 del; 20 mod
  Patch: https://git.openjdk.org/jdk/pull/9127.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/9127/head:pull/9127

PR: https://git.openjdk.org/jdk/pull/9127

Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v3]

[David Holmes]

On Wed, 1 Jun 2022 06:04:14 GMT, Alan Bateman  wrote:

>> This patch adds an alternative virtual thread implementation where each 
>> virtual thread is backed by an OS thread. It doesn't scale but it can be 
>> used by ports that don't have continuations support in the VM. Aside from 
>> scalability, the lack of continuations support means:
>> 
>> 1. JVM TI is not supported when running with --enable-preview (the JVM TI 
>> spec allows for this) 
>> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs 
>> and so needs JVM TI)
>> 
>> The VM option "VMContinuations" is added as an experimental option so it can 
>> be used by tests. A number of tests are changed to re-run with 
>> -XX:-VMContinuations. A new jtreg property is added so that tests that need 
>> the underlying VM support to be present can use "@requires vm.continuations" 
>> in the test description. A follow-up change would be to add "@requires 
>> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with 
>> preview features enabled.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 11 commits:
> 
>  - Fixed another typo in comment
>  - Merge
>  - Fix typos in comments
>  - Allowing linking without intrinsic stub, contributed-by: rehn
>  - Continuation clinit should fail if no continuations support
>  - Fix merge issue with test
>  - Change VMContinuations to be experimental option, ensure JNI GetEnv 
> returns EVERSION
>  - Update
>  - Expend test coverage
>  - Merge
>  - ... and 1 more: 
> https://git.openjdk.java.net/jdk/compare/3deb58a8...a5c06cb6

src/java.base/share/classes/java/lang/Thread.java line 742:

> 740:  * @param name thread name, can be null
> 741:  * @param characteristics thread characteristics
> 742:  * @param bound true when bound to an OS thread

Nit: s/OS/VM/ ?

-

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

Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v4]

[David Holmes]

On Wed, 1 Jun 2022 03:18:44 GMT, Joe Darcy  wrote:

>> Time to start getting ready for JDK 20...
>
> Joe Darcy has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains 38 additional commits since 
> the last revision:
> 
>  - Adjust deprecated options test.
>  - Merge branch 'master' into JDK-8284858
>  - Update deprecated options test.
>  - Merge branch 'master' into JDK-8284858
>  - Respond to review feedback.
>  - Update symbol information for JDK 19 b24.
>  - Merge branch 'master' into JDK-8284858
>  - Merge branch 'master' into JDK-8284858
>  - Merge branch 'master' into JDK-8284858
>  - Merge branch 'master' into JDK-8284858
>  - ... and 28 more: 
> https://git.openjdk.java.net/jdk/compare/c1abb195...54e872b5

Updates look good. Thanks

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v2]

[David Holmes]

On Tue, 31 May 2022 15:10:38 GMT, Adam Sotona  wrote:

>> This is continuation of PR #4256 
>> 
>> The patch simply rounds up the specified stack size to multiple of the 
>> system page size, on systems where necessary.
>> The patch is based on the original PR/branch, with reflected remaining 
>> recommendations.
>> 
>> Please review.
>> 
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated Java mannpage

Thanks for the manpage update!

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS

[David Holmes]

On Tue, 31 May 2022 08:37:19 GMT, Adam Sotona  wrote:

> This is continuation of PR #4256 
> 
> The patch simply rounds up the specified stack size to multiple of the system 
> page size, on systems where necessary.
> The patch is based on the original PR/branch, with reflected remaining 
> recommendations.
> 
> Please review.
> 
> Thank you,
> Adam

Changes looks good. But the java manpage needs an update too.

Thanks,
David

src/java.base/share/classes/sun/launcher/resources/launcher.properties line 176:

> 174: \-Xssset java thread stack size\n\
> 175: \  The actual size may be rounded up to a multiple 
> of the\n\
> 176: \  system page size as required by the operating 
> system.\n\

The Java manpage will also need this update.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: Integrated: 8286562: GCC 12 reports some compiler warnings

[David Holmes]

The new assertion in src/hotspot/share/utilities/globalDefinitions.hpp

inline const char* type2name(BasicType t) {
  assert((uint)t < T_CONFLICT + 1, "invalid type");
  return type2name_tab[t];
}

is failing with test

compiler/jvmci/errors/TestInvalidDebugInfo.java

I have filed:

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

The test will probably need ProblemListing as it is causing major noise 
in our CI.


David

On 28/05/2022 12:12 pm, Yasumasa Suenaga wrote:

On Wed, 11 May 2022 05:58:31 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:


This pull request has now been integrated.

Changeset: 410a25d5
Author:Yasumasa Suenaga 
URL:   
https://git.openjdk.java.net/jdk/commit/410a25d59a11b6a627bbb0a2c405c2c2be19f464
Stats: 41 lines in 5 files changed: 26 ins; 1 del; 14 mod

8286562: GCC 12 reports some compiler warnings

Reviewed-by: ihse, kbarrett, prr

-

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

Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v2]

[David Holmes]

On Thu, 26 May 2022 23:05:32 GMT, Joe Darcy  wrote:

>> Time to start getting ready for JDK 20...
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by dholmes (Reviewer).

-

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

Re: RFR: JDK-8284858: Start of release updates for JDK 20

[David Holmes]

On Thu, 14 Apr 2022 05:09:14 GMT, Joe Darcy  wrote:

> Time to start getting ready for JDK 20...

One comment below.
I ignored the sym files.
Everything else appears okay.
Thanks.

src/java.base/share/classes/jdk/internal/org/objectweb/asm/Opcodes.java line 
312:

> 310: int V18 = 0 << 16 | 62;
> 311: int V19 = 0 << 16 | 63;
> 312: int V20 = 0 << 17 | 64;

17 ??

Though why do we even bother with this when the minor version is zero? Simple 
assignment works.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8287137: Problemlist failing x86_32 tests after Loom integration [v3]

[David Holmes]

On Mon, 23 May 2022 18:25:23 GMT, Aleksey Shipilev  wrote:

>> [JDK-8284161](https://bugs.openjdk.java.net/browse/JDK-8284161) broke a lot 
>> of x86_32 code. The x86_32 porting is done under 
>> [JDK-8286642](https://bugs.openjdk.java.net/browse/JDK-8286642). Meanwhile, 
>> we can problemlist the failing tests to get cleaner runs for other patches. 
>> This should also make GHA runs cleaner.
>> 
>> Additional testing:
>>  - [x] Linux x86_32 fastdebug `tier1` (some unrelated failures in hotspot)
>>  - [x] Linux x86_32 fastdebug `tier2` (some unrelated failures in hotspot)
>>  - [x] Linux x86_32 fastdebug `tier3` (clean now)
>>  - [x] GHA run
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Another release test

There is a mistake in this changeset.  The JDK ProblemList already contained:

`java/nio/channels/FileChannel/LargeMapTest.java 8286980 
windows-all`

and this change then added:

`java/nio/channels/FileChannel/LargeMapTest.java 8286642 
generic-i586`

which appears to have negated the first entry.

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher [v2]

[David Holmes]

On Wed, 18 May 2022 07:40:43 GMT, Thomas Stuefe  wrote:

>> src/java.base/share/native/libjli/java.c line 1631:
>> 
>>> 1629: const char *arg = jargv[i];
>>> 1630: if (arg[0] == '-' && arg[1] == 'J') {
>>> 1631: assert(arg[2] != '\0' && "Invalid JAVA_ARGS or 
>>> EXTRA_JAVA_ARGS defined by build");
>> 
>> Interesting trick.
>
> Since we pass NDEBUG, this assert is disabled in release builds. This is the 
> supposed behavior, right? That we don't do anything on release builds?

Right - that is what I would expect from an assert.

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[David Holmes]

On Tue, 17 May 2022 13:54:59 GMT, Yasumasa Suenaga  wrote:

>> You forced me to look at this in depth. The values are set via the build 
>> system. In the build system it is impossible to get just -J because the -J 
>> is only added as a prefix for an existing string. So basically this is 
>> impossible code to reach unless you manually override the build system 
>> definition. So as far as I can see this is a classic case where we want an 
>> assert.
>> 
>> 
>> if (arg[0] == '-' && arg[1] == 'J') {
>> assert(arg[2] != '\0', "Invalid JAVA_ARGS or EXTRA_JAVA_ARGS defined by 
>> build");
>> *nargv++ = JLI_StringDup(arg + 2);
>> }
>
> @dholmes-ora Thanks for your comment!
> 
> We cannot use `assert(cond, message)` at here because it is not HotSpot code. 
> In imageFile.cpp for libjimage. It uses `assert(cond && message)`, so I use 
> this style in new commit, and the warning has gone.
> 
> I can change to "normal" `assert` usage at here if it is strange.

@YaSuenag I know this is not hotspot code. Please see existing use of assert in 
related code i.e. src/java.base/share/native/libjli/args.c

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher [v2]

[David Holmes]

On Tue, 17 May 2022 13:55:43 GMT, Yasumasa Suenaga  wrote:

>> GCC 12 reports as following:
>
> Yasumasa Suenaga has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Use assert() to check jargv
>  - Merge remote-tracking branch 'upstream/master' into JDK-8286694
>  - 8286694: Incorrect argument processing in java launcher

Looks fine to me.

Thanks.

Sorry I see what you mean, it is just an `assert(cond)` not `assert(cond, 
message)`, but that is fine.

src/java.base/share/native/libjli/java.c line 1631:

> 1629: const char *arg = jargv[i];
> 1630: if (arg[0] == '-' && arg[1] == 'J') {
> 1631: assert(arg[2] != '\0' && "Invalid JAVA_ARGS or 
> EXTRA_JAVA_ARGS defined by build");

Interesting trick.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[David Holmes]

On Fri, 13 May 2022 08:56:59 GMT, Yasumasa Suenaga  wrote:

> GCC 12 reports as following:

You forced me to look at this in depth. The values are set via the build 
system. In the build system it is impossible to get just -J because the -J is 
only added as a prefix for an existing string. So basically this is impossible 
code to reach unless you manually override the build system definition. So as 
far as I can see this is a classic case where we want an assert.


if (arg[0] == '-' && arg[1] == 'J') {
assert(arg[2] != '\0', "Invalid JAVA_ARGS or EXTRA_JAVA_ARGS defined by 
build");
*nargv++ = JLI_StringDup(arg + 2);
}

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[David Holmes]

On Fri, 13 May 2022 08:56:59 GMT, Yasumasa Suenaga  wrote:

> GCC 12 reports as following:

src/java.base/share/native/libjli/java.c line 1632:

> 1630: if (JLI_IsTraceLauncher()) {
> 1631: printf("Empty option: jargv[%d]\n", i);
> 1632: }

I'd say this is an error as well - but a developer error rather than end-user.

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[David Holmes]

On Fri, 13 May 2022 08:56:59 GMT, Yasumasa Suenaga  wrote:

> GCC 12 reports as following:

I don't recall what "builtin arguments" means. Can you tell me how we would 
actually hit this conditions such that we string dup and  the null string? Or 
are you saying this is actually an unreachable case: the check for NULL is 
always false and the actual  string is never null anyway?

-

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

Re: RFR: 8286744: failure_handler: dmesg command on macos fails to collect data due to permission issues [v2]

[David Holmes]

On Mon, 16 May 2022 07:54:09 GMT, Jaikiran Pai  wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   copyright year
>
> Thank you Daniel and Lance for the reviews. 
> 
> Would anyone from the build team like to provide any additional reviews?

@jaikiran  this is more a SQE issue that an build issue. @lmesnik may have an 
opinion.

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[David Holmes]

On Fri, 13 May 2022 08:56:59 GMT, Yasumasa Suenaga  wrote:

> GCC 12 reports as following:

Simply considering the compiler warning shouldn't `(arg+2) == NULL` actually be 
`arg[2] = '\0'` ? as we are looking to see if this arg is only three 
characters: '-', 'J' and '\0'

-

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

Re: RFR: 8286368: Cleanup problem lists after loom integration [v2]

[David Holmes]

On Mon, 16 May 2022 02:25:26 GMT, Leonid Mesnik  wrote:

>> test/hotspot/jtreg/ProblemList-Xcomp.txt line 38:
>> 
>>> 36: serviceability/sa/TestJhsdbJstackMixed.java 8248675 linux-aarch64
>>> 37: 
>>> 38: compiler/c2/irTests/TestSkeletonPredicates.java 8286361 generic-all
>> 
>> @lmesnik This line should not have been removed!
>
> It might be a merge problem. My PR was submitted before 8286442. I remereged 
> changes, however my merge commit also doesn't have removal of this line:
> https://github.com/openjdk/jdk/pull/8604/commits/8255e760ff1a2ff616f00f559de29e66d7036b39
> 
> So I am really confused and can't understand how I managed to introduce this 
> problem.

That is really bizarre. No individual commit shows the line being deleted, yet 
in the final commit it is.

-

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

Re: RFR: 8286368: Cleanup problem lists after loom integration [v2]

[David Holmes]

On Tue, 10 May 2022 19:16:34 GMT, Leonid Mesnik  wrote:

>> 8286368: Cleanup problem lists after loom integration
>
> Leonid Mesnik has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains two commits:
> 
>  - Merge
>  - 8286368: Cleanup problem lists after loom integration

test/hotspot/jtreg/ProblemList-Xcomp.txt line 38:

> 36: serviceability/sa/TestJhsdbJstackMixed.java 8248675 linux-aarch64
> 37: 
> 38: compiler/c2/irTests/TestSkeletonPredicates.java 8286361 generic-all

@lmesnik This line should not have been removed!

-

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

Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold

[David Holmes]

On Fri, 13 May 2022 13:02:52 GMT, Jie Fu  wrote:

>> I assume you are running the tests with:
>>make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" 
>> in which case, all of the tests you select to run will be run with that GC. 
>> 
>> What you have is not wrong but wouldn't be a better to add a new test to 
>> test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign?
>
>> I think I agree with @AlanBateman - in the sense that this seems to go down 
>> a slippery slope where every test would need to be executed against all 
>> possible GCs. AFAIK, there are no other foreign tests doing this.
> 
> I think it's a waste of time to write a separate test for this bug.
> 
> I can't understand why you are against adding one more test config in the 
> foreign test.
> Yes, it caught the bug in ShenandoahGC but who knows it wouldn't find some 
> other bugs in the foreign api in the future.
> If you are still against the newly added test config, I can revert the test 
> change.
> Thanks.

My comment seems to have never made it through. The problem with what your are 
doing is two-fold:
1. When running the test suite specifically to test xGC for whatever x you are 
now forcing a test run for Shenandoah.
2.  When x is Shenandoah then you run this test twice.

You don't need a config just to run this with Shenandoah - you run the test 
suite with Shenandoah.

-

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

Re: RFR: 8283689: Update the foreign linker VM implementation [v10]

[David Holmes]

On Thu, 12 May 2022 08:23:11 GMT, Nick Gasson  wrote:

>> src/hotspot/cpu/aarch64/foreign_globals_aarch64.cpp line 3:
>> 
>>> 1: /*
>>> 2:  * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights 
>>> reserved.
>>> 3:  * Copyright (c) 2019, 2021, Arm Limited. All rights reserved.
>> 
>> Only update third-party copyrights under direction from that copyright 
>> holder. This may not be the correct format for example.
>> Also it is 2022. :)
>
> I think the Arm Ltd one was probably changed by me in one of the PRs in the 
> Panama repo.

That's fine, just needed to check. But as these are now being committed to 
mainline the year does need to change to 2022 - at least in Oracle copyright. 
@nick-arm will have to advise what he thinks should be done with the ARM 
copyright.

-

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

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

[David Holmes]

On Wed, 11 May 2022 16:00:32 GMT, Maxim Kartashev  
wrote:

>> 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
>
> This change seem to have made this group of declarations obsolete: 
> `src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h:157` (follow 
> the [link](
> https://github.com/openjdk/jdk/blob/89de756ffbefac452c7df559e2a4eb50bf71368b/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h#L157)).
>  Does anyone have plans to drop those? Have any bugs been filed for that? If 
> not, I'll attend to that myself.

@mkartashev  all references to WINVER in the AWT code seem obsolete. Possibly 
most of the IS_WINxxx usages could also be replaced.

-

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

Re: RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach()

[David Holmes]

On Wed, 11 May 2022 23:08:32 GMT, Ioi Lam  wrote:

> The API `jdk.internal.perf.Perf.::attach(String user, int lvmid)` is never 
> used. It should be removed, and all the handling of a specified user name 
> should be removed.

Nice cleanup! I checked back in JDK 7 and couldn't find any use of this 
particular API.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8283689: Update the foreign linker VM implementation [v7]

[David Holmes]

On Wed, 11 May 2022 20:27:42 GMT, Patricio Chilano Mateo 
 wrote:

>> I went ahead and implemented this suggestion. Now we block async exceptions 
>> in on_entry, and unblock in on_exit.
>
> Is it possible for these upcalls to be nested? If yes, we could add a boolean 
> to context to avoid unsetting the flag in those nested cases. And now that I 
> think we should probably add that check in NoAsyncExceptionDeliveryMark too 
> if we allow broader use of this flag. David added the 
> NoAsyncExceptionDeliveryMark code with that assert about nesting so maybe he 
> might have more insights about that.

NoAsyncExceptionDeliveryMark is not for general use! There is no provision for 
blocking async exceptions when running user-defined Java code. 
NoAsyncExceptionDeliveryMark was purely for protecting "system Java code".

-

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

Re: RFR: 8283689: Update the foreign linker VM implementation [v10]

[David Holmes]

On Wed, 11 May 2022 17:51:31 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This PR updates the VM implementation of the foreign linker, by bringing 
>> over commits from the panama-foreign repo.
>> 
>> This is split off from the main JEP integration for 19, since we have 
>> limited resources to handle this. As such, this PR might fall over to 20, 
>> but it would be nice if we could get it into 19.
>> 
>> I've written up an overview of the Linker architecture here: 
>> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful 
>> to read that first.
>> 
>> This patch moves from the "legacy" implementation, to what is currently 
>> implemented in the panama-foreign repo, except for replacing the use of 
>> method handle combinators with ASM. That will come in a later path. To 
>> recap. This PR contains the following changes:
>> 
>> 1. VM stubs for downcalls are now generated up front, instead of lazily by 
>> C2 [1].
>> 2. the VM support for upcalls/downcalls now support all possible call 
>> shapes. And VM stubs and Java code implementing the buffered invocation 
>> strategy has been removed  [2], [3], [4], [5].
>> 3. The existing C2 intrinsification support for the `linkToNative` method 
>> handle linker was no longer needed and has been removed [6] (support might 
>> be re-added in another form later).
>> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now 
>> implements RuntimeBlob directly. Binding to java classes has been rewritten 
>> to use javaClasses.h/cpp (this wasn't previously possible due to these java 
>> classes being in an incubator module) [7], [8], [9].
>> 
>> While the patch mostly consists of VM changes, there are also some Java 
>> changes to support (2).
>> 
>> The original commit structure has been mostly retained, so it might be 
>> useful to look at a specific commit, or the corresponding patch in the 
>> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) 
>> repo as well. I've also left some inline comments to explain some of the 
>> changes, which will hopefully make reviewing easier.
>> 
>> Testing: Tier1-4
>> 
>> Thanks,
>> Jorn
>> 
>> [1]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358
>> [2]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49
>> [3]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3
>> [4]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3
>> [5]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120
>> [6]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a
>> [7]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062
>> [8]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f
>> [9]: 
>> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9
>
> Jorn Vernee has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 26 commits:
> 
>  - Block async exceptions during upcalls
>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>  - Fix use of rt_call
>  - Migrate to GrowableArray
>  - Address some review comments
>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>  - Remove unneeded ComputeMoveOrder
>  - Remove comment about native calls in lcm.cpp
>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>
>Reviewed-by: jvernee, mcimadamore
>  - Update riscv and arm stubs
>  - ... and 16 more: 
> https://git.openjdk.java.net/jdk/compare/cdd006e7...b29ad8f4

src/hotspot/cpu/aarch64/foreign_globals_aarch64.cpp line 3:

> 1: /*
> 2:  * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights 
> reserved.
> 3:  * Copyright (c) 2019, 2021, Arm Limited. All rights reserved.

Only update third-party copyrights under direction from that copyright holder. 
This may not be the correct format for example.
Also it is 2022. :)

-

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

Re: RFR: 8284550: test failure_handler is not properly invoking jhsdb jstack, resulting in failure to produce a stack when a test times out

[David Holmes]

On Sun, 8 May 2022 21:57:20 GMT, Leonid Mesnik  wrote:

> …resulting in failure to produce a stack when a test times out

Looks good.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

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

[David Holmes]

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

This seems okay to me in this form.

I agree that explicitly setting this is better than unknowingly using API's 
that might not be available on supported platforms.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

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

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

[David Holmes]

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

I'm confused.  `src\jdk.crypto.mscapi\windows\native\libsunmscapi\security.cpp` 
doesn't set _WIN32_WINNT so how is that later API being enabled? Does this mean 
that not setting _WIN32_WINNT means :any API is allowed" ?

-

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

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

[David Holmes]

On Tue, 3 May 2022 07:10:58 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 Copyright year in wepoll.c

I agree with Erik - the source locations need to be modified to not define it. 
If we want to keep a record perhaps add an assertion that the value is what was 
expected?

I still feel we have a disconnect between this and an actual check for what the 
build and runtime platform version is ...

and it isn't at all clear how someone using an API only in a later Windows 
version, and so setting _WIN32_WINNT to a higher value, will know that this is 
defined down in the build files ?

-

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

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings

[David Holmes]

On Wed, 27 Apr 2022 14:57:41 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).

That only seems to be half of the issue though. If we are defining 
_WIN32_WINNT=0x0601 because the minimum required OS API support level is 
Windows 7, then don't we need a check that the build platform is also at least 
Windows 7?

-

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

Re: Improve `finally` block exception handling

[David Holmes]

On 21/04/2022 7:30 am, 
some-java-user-99206970363698485...@vodafonemail.de wrote:

Thanks a lot for your feedback! My original message was a bit vague in parts, 
sorry for that.

The proposal consists of the following:
1. Forbidding usage of `break`, `continue`, `yield` and `return` in `finally` 
blocks
2. Adding exceptions as suppressed exceptions:
If exception E1 led to execution of the `finally` block and the block is 
left due to
another exception E2, then either E1 or E2 should be thrown with the other 
one added
as suppressed exception. For consistency with try-with-resources and 
because E1 is
most likely more relevant ideally E1 would be thrown and E2 should be 
suppressed. But
if you think the impact on backward compatibility would be too large, then 
E2 should
be thrown (the current behavior), but E1 should at least be added as 
suppressed exception.

The following replies to your comments hopefully make the rationale for these 
proposed
changes clearer.



The behaviour of try/catch/finally is not "obvious at all", you have to read 
what it means
and when you do that you clearly see what happens regarding exceptions.


For try-catch you see that the code catches some specific exception and then 
handles it in a
certain way, whether that is by rethrowing, logging or itentionally ignoring it.
The issue with `finally` is that in its current form, exceptions which occurred 
in the `try`
block might just silently disappear. Consider this simple example:
```
try {
 throw new RuntimeException();
} finally {
 return true;
}
```
Here it is not clear at all, unless you have read the JLS in detail, that the 
thrown exception
just vanishes. There is no explicit indication that the `finally` has any 
effect on the
thrown exception. Of course this is a contrived example, but consider the same 
situation where
the `try` block calls a method which might throw an exception (or the general 
case that a
VirtualMachineError occurs), and that the `return` (or any of the other 
affected statements)
is not the only statement in the `finally` block.

Similar confusing code can be written where the `try` or `catch` block returns 
a value
(or continues or breaks loop iteration), but the `finally` block overwrites 
that action:
```
try {
 return 1;
} finally {
 return 2;
}
```
Again, contrived, but consider the same code with additional statements in the 
`try` and
`finally` blocks. This breaks fundamental assumptions one has about the 
behavior of the
statements, such as `return`.


I still maintain this is simply a matter of user education. You have no 
business using a finally block if you have never bothered to even learn 
how it works. Sorry no sympathy from me on that one.



Are there any plans to forbid usage of `break`, `continue`, `yield` and 
`return` in
`finally` blocks?


Why would we do that? What would that gain?


It would prevent code such as the one shown above, where code, most likely 
unintentionally,
discards exceptions. Now also consider that inside the `try` block a 
VirtualMachineError
(or a subclass of it) may occur which you really should not catch. Yet the code 
in the
`finally` block silently discards it without you ever noticing the error, 
before it occurs
later in another part of the application again and has possibly already 
corrupted the state
of the application.


Again this is primarily user education. And there are times when you 
definitely do want to use those statements and you would have to jump 
through hoops to get the same effect if they were banned.


But in terms of "are there any plans ..." the answer is a clear no as 
the compatibility issues would make this a non-starter.





Similarly for `throw` and any implicitly thrown exceptions, is there a plan to 
at least
add the original exception as suppressed one to the newly thrown?


That suggestion is not completely without merit, but nor is it a "slam-dunk" obvious thing to do. 
The semantic implications of the exceptions matter, and semantics come from the programmers intent. There's 
no reasonable way to automagically determine that when an exception is created that another exception (that 
led to the finally block) should be inserted as a "suppressed exception". That would actually be 
completely wrong to do in many circumstances you would instead need to act when the exception would terminate 
the finally block and then add the original exception as the "suppressed" exception.


To clarify my suggestion: If a `finally` block is entered due to an exception 
E1, and is
exited due to an exception E2 (regardless of whether explicitly thrown by a 
`throw` statement),
and E1 != E2, then both exceptions should be preserved, with one being added as 
suppressed
exception to the other one.


That is not unreasonable: E2 suppresses E1.




But really the programmer is in the best position to decide how exceptions need 
to be handled.


Except that in a `finally` block they don't have access 

Re: RFR: JDK-8285730: unify _WIN32_WINNT settings

[David Holmes]

On Wed, 27 Apr 2022 14:57:41 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).

There is obviously a lot of history here and different parts of the codebase 
had hit different minimum OS version requirements at different times. While at 
one time we would have had to cater for building on systems with and without a 
given API those days are long gone for the code being touched here (AFAICS). As 
Erik states we should be able to set a minimum _WIN32_WINNT_ value needed to 
build all the codebase and simply have that checked and set at configure time. 
The code itself would not need to define _WIN32_WINNT.

-

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

Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 [v3]

[David Holmes]

On Mon, 25 Apr 2022 14:26:17 GMT, Harold Seigel  wrote:

>> Please review this small fix for JDK-8284642.  The fix was tested by running 
>> Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux 
>> x64.  Additionally, the modified test and the test in the bug report were 
>> run locally.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix globals.hpp text

That all looks good to me.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8285366: Fix typos in serviceability

[David Holmes]

On Thu, 21 Apr 2022 18:08:05 GMT, Kevin Walls  wrote:

>> But on the other hand we have `javax.script.Invocable`. :-) 
>> 
>> Codespell suggested this change, and I based my decision to keep it based on 
>> [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not 
>> even listing "invokable" as an alternate spelling, and that "invocable" has 
>> about 3x the number of Google hits than "invokable". 
>> 
>> But sure, there is perhaps reason to consider "invokable" an acceptable 
>> alternative and keep it. I guess it depends on if you consider the word to 
>> be based on "invoke" with a suffix, or a latinized form, like "invocation". 
>> 
>> I'll wait a while for others to chime in, otherwise I'll revert the 
>> "invokable" changes.
>
> Sure, I just thought there was enough evidence that invokable is not 
> definitely wrong, even if invocable is more correct and popular, so it's not 
> obvious it should be changed.  Don't lose sleep over it. 8-)
> 
> More importantly, on the tests -- I saw the changes in exception messages, 
> searched for the wrong text in the test directories, and didn't find any 
> matches that looked like checks.

Invocable, Invokable and Invokeable are all used in practice. We have a mix of 
invocable and invokable throughout our codebase, with more of the former than 
the latter - and in prose "invocable" is probably best.

-

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

Re: RFR: 8283620: System.out does not use the encoding/charset specified in the Javadoc [v2]

[David Holmes]

On Fri, 22 Apr 2022 18:10:27 GMT, Naoto Sato  wrote:

>> src/java.base/share/classes/java/lang/System.java line 780:
>> 
>>> 778:  * The property may be set on the command line to the value
>>> 779:  * {@code UTF-8}. Setting the property to a value other than 
>>> {@code UTF-8}
>>> 780:  * leads to unspecified behavior.
>> 
>> I think the proposal to introduce two standard properties is good and is 
>> consistent with the recently introduced native.encoding. I'm not 100% sure 
>> that the sentence "The property may be set on the command line ..." is 
>> appropriate for the spec of standard properties. We got away with that for 
>> file.encoding in implNote but that isn't spec. I think we may have to 
>> replace this with something that says that the Java runtime can be started 
>> with the system property set to "UTF-8", starting it with the property set 
>> to another value clears to undefined behavior.
>
> Thanks, Alan. Modified them as suggested.

I think Alan has a typo: "clears" -> "leads"

-

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

Re: RFR: 8283620: System.out does not use the encoding/charset specified in the Javadoc [v2]

[David Holmes]

On Fri, 22 Apr 2022 18:14:18 GMT, Naoto Sato  wrote:

>> Promoting the internal system properties for `System.out` and `System.err` 
>> so that users can override the encoding used for those streams to `UTF-8`, 
>> aligning to the `Charset.defaultCharset()`. A CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Modified the spec for the new system properties.

src/java.base/share/classes/java/lang/System.java line 774:

> 772:  * Character encoding name for {@link System#out System.out}.
> 773:  * The Java runtime can be started with the system property set 
> to {@code UTF-8},
> 774:  * starting it with the property set to another value clears to 
> undefined behavior.

_leads_ to undefined behavior

-

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

Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 [v2]

[David Holmes]

On Fri, 22 Apr 2022 18:14:27 GMT, Harold Seigel  wrote:

>> Please review this small fix for JDK-8284642.  The fix was tested by running 
>> Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux 
>> x64.  Additionally, the modified test and the test in the bug report were 
>> run locally.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   restore source, fix man page

I didn't realize that text was from the manpage. It is fine but I think we also 
need something in globals.hpp:


 product(uint64_t, MaxDirectMemorySize, 0, \
  "Maximum total size of NIO direct-buffer allocations. "\
  "Ignored if not explicitly set.")
  range(0, max_jlong)   \

-

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

Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0

[David Holmes]

On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel  wrote:

> Please review this small fix for JDK-8284642.  The fix was tested by running 
> Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux 
> x64.  Additionally, the modified test and the test in the bug report were run 
> locally.
> 
> Thanks, Harold

I would delete the last sentence:

> By default, the size is set to 0, meaning that the JVM chooses the size for 
> NIO direct-buffer allocations automatically.

and replace with:

> If not set, the flag is ignored and the JVM chooses the size for NIO 
> direct-buffer allocations automatically.

-

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

Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0

[David Holmes]

On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel  wrote:

> Please review this small fix for JDK-8284642.  The fix was tested by running 
> Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux 
> x64.  Additionally, the modified test and the test in the bug report were run 
> locally.
> 
> Thanks, Harold

See main comment

-

Changes requested by dholmes (Reviewer).

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

Re: Improve `finally` block exception handling

[David Holmes]

Hello,

On 18/04/2022 5:43 am, 
some-java-user-99206970363698485...@vodafonemail.de wrote:


Hello,

are there any plans to improve exception handling in combination with `finally` 
blocks?


I'm not aware of anything, nor what that anything could realistically 
be. You make only one suggestion that has a little merit.



Currently, when a `finally` block does not complete normally, the original 
exception is
silently discarded (as described in the JLS). This behavior is error-prone, not 
obvious
at all, and has been criticized in the past a lot. An example for this is 
https://stackoverflow.com/q/48088
and the linked posts there.


The behaviour of try/catch/finally is not "obvious at all", you have to 
read what it means and when you do that you clearly see what happens 
regarding exceptions.



While javac warns you about this issue when using `-Xlint:finally`, it is only 
a warning
and not even enabled by default.

Are there any plans to forbid usage of `break`, `continue`, `yield` and 
`return` in
`finally` blocks?


Why would we do that? What would that gain?


Switch expressions have already set a precedent for this by not allowing to 
leave the
switch expression by something other than a `throw` or `yield` statement, 
trying to use
for example a `return` statement causes a compilation error.


That is because it is an _expression_ - expressions have to have 
different language rules to statements. There is no connection to a 
finally block.



Similarly for `throw` and any implicitly thrown exceptions, is there a plan to 
at least
add the original exception as suppressed one to the newly thrown?


That suggestion is not completely without merit, but nor is it a 
"slam-dunk" obvious thing to do. The semantic implications of the 
exceptions matter, and semantics come from the programmers intent. 
There's no reasonable way to automagically determine that when an 
exception is created that another exception (that led to the finally 
block) should be inserted as a "suppressed exception". That would 
actually be completely wrong to do in many circumstances you would 
instead need to act when the exception would terminate the finally block 
and then add the original exception as the "suppressed" exception. But 
really the programmer is in the best position to decide how exceptions 
need to be handled.



Of course one could argue that the same applies to `catch` clauses whose body 
accidentally
causes an exception which discards the caught one. However the main difference 
is that


Yes exactly the same.


there, only a specific exception type is caught (and discarded), whereas for 
`finally`
exceptions of _any_ type are discarded. It could also be argued that adding 
suppressed


We have multi-catch now so that argument is somewhat weaker.


exceptions decreases performance or causes memory leaks, but one the other hand
this behavior was explicitly included try-with-resources statements, most 
likely because the
value this adds was considered more important than any performance issues this
might cause.


try-with-resources added support for suppressed exceptions because the 
automatic closing of the resource could throw an exception, and that had 
to be factored in to the whole mechanism.



Also, it is important to keep in mind that this affects _all_ Throwable types, 
including
VM errors which you really should not catch, and which due to a `finally` block 
might
silently be discarded. Most, if not all, code in which a `finally` does not 
complete
normally does not seem to consider this.


That is true. But for me a finally block is intended to be quite small 
and succint and should be written from the perspective of "an unexpected 
exception has occurred, what it is it critical for me to do before 
leaving the current scope".



This is also not a theoretical problem; this issue exists in multiple open 
source projects,
potentially even in the JDK itself.
Often the code can be rewritten by either moving the complete code or parts of 
it
outside of the `finally` block, or by introducing a local variable to hold the 
result and
to return that after the `finally` block.

What do you think about this topic?


I think there is no clear and obvious solution that the language can put 
in place here.


Just my personal 2c.

Cheers,
David


Is backward compatibility a concern? If so, can you provide an example where 
using such
a pattern provides any notable advantages which justify the potential loss of 
thrown
VM errors or exceptions.

Kind regards

Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0

[David Holmes]

On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel  wrote:

> Please review this small fix for JDK-8284642.  The fix was tested by running 
> Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux 
> x64.  Additionally, the modified test and the test in the bug report were run 
> locally.
> 
> Thanks, Harold

I don't this is the right "fix". There is a bit of a disconnect between the VM 
option and the system property. Although the default value of the VM flag is 
zero, that is ignored and the JDK side, not being directed otherwise, 
determines the max size. If you set the flag explicitly to zero that is passed 
through to the JDK and the max memory is actually set to zero.

I think this is more a documentation issue - MaxDirectMemory has no affect 
unless explicitly set on the command-line. The default value of the flag is 
actually irrelevant.

-

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

Re: RFR: 8284579: Improve VarHandle checks for interpreter [v3]

[David Holmes]

On Mon, 11 Apr 2022 10:13:40 GMT, Claes Redestad  wrote:

> checkExactAccessMode -> checkAccessModeThenIsDirect

Don't you still want "Exact" in there? That "access" check seems odd anyway as 
it only checks for one form of mismatch - should it not also check for `!exact 
&& accessModeType(ad.type) == ad.symbolicMethodTypeExact`?

-

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

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

[David Holmes]

On Mon, 11 Apr 2022 12:04:41 GMT, Maurizio Cimadamore  
wrote:

>> src/hotspot/share/prims/scopedMemoryAccess.cpp line 141:
>> 
>>> 139: 
>>> 140: /*
>>> 141:  * This function performs a thread-local handshake against all threads 
>>> running at the time
>> 
>> Nit: thread-local??
>
> I was assuming the term "thread-local handshake" was a thing in the VM, as 
> per:
> https://openjdk.java.net/jeps/312

Ha! Mea culpa. I've been too buried in the other kind of thread-local lately. I 
completely forgot we actually described handshakes that way. :)

-

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

Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]

[David Holmes]

On Mon, 11 Apr 2022 14:55:32 GMT, Thomas Schatzl  wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that adds dedicated filler objects to 
>> the VM?
>> 
>> Currently, when formatting areas of dead objects all gcs use instances of 
>> j.l.Object and int-arrays.
>> 
>> This has the drawback of not being easily able to discern whether a given 
>> object is actually dead (and should never be referenced) or just a regular 
>> j.l.Object/int array.
>> 
>> This also makes enhanced error detection (any reference to such an object is 
>> an error - i.e. detecting references to such objects) and to skip 
>> potentially already unloaded classes when scanning areas of the heap below 
>> TAMS, G1 uses its prev bitmap.
>> Other collectors do not have this extra information at the moment, so they 
>> can't (and don't) do this kind of verification.
>> 
>> With [JDK-8210708](https://bugs.openjdk.java.net/browse/JDK-8210708) the 
>> prev bitmap will effectively be removed in G1; G1 will format the dead areas 
>> with these filler objects to avoid coming across unloaded classes. This is 
>> fine wrt to normal operation, however, this looses the existing enhanced 
>> verification mentioned above.
>> 
>> This change proposes to add dedicated VM-internal filler objects, i.e. 
>> equivalents of j.l.Object and int-arrays.
>> 
>> This has the following benefits:
>> 
>> - keep this error detection (actually making it much simpler) and allowing 
>> similar verification for other collectors. (This change does not add this)
>> 
>> - this also makes it "easy" to detect references to filler objects in 
>> debugging tools - you only need to know the two klasses (or just get their 
>> friendly name) to see whether that reference may actually be valid (or 
>> refers to the inside such an object). References to these classes in the 
>> crash file may also allow the issue to be more clear.
>> 
>> This causes some minor changes to external behavior:
>> 
>> - logs/heap dumps now contain instances of these objects - which seems fine 
>> as previously they have just been reported as part of j.l.Object/int-arrays 
>> statistics. The VM spec also does not guarantee whether a particular kind of 
>> object should/should not show there anyway afaik.
>> 
>> - if the application ever gets to instantiate a reference to such an object 
>> somehow, any enabled verification will crash the VM. That's bad luck for 
>> messing with internal classes, but that's the purpose of these objects.
>> 
>> The change takes care that getting a reference will not be possible by 
>> normal means (i.e. via Class.forName() etc) - which should be sufficient to 
>> avoid the issue. Actually, existing mechanisms seem to be sufficient.
>> 
>> 
>> Testing: tier1-8
>> 
>> There is one question I would like the reviewers to specially think about, 
>> the name of the filler array klass. I just used 
>> `Ljava/internal/vm/FillerArray;` for that, looking at other internal 
>> symbols/klasses, but I'm not sure this adheres to naming guidelines.
>> 
>> Thanks go to @iklam for helping out with the change.
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix test

Do you really need to define a real `FillerObject.java` class? Can't you use an 
internal-only variant of a hidden class to represent this?

-

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

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

[David Holmes]

On Mon, 4 Apr 2022 14:57:30 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:
> 
>   Fix TestLinkToNativeRBP

VM changes look good.

Thanks,
David

src/hotspot/share/prims/scopedMemoryAccess.cpp line 141:

> 139: 
> 140: /*
> 141:  * This function performs a thread-local handshake against all threads 
> running at the time

Nit: thread-local??

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8284579: Improve VarHandle checks for interpreter [v2]

[David Holmes]

On Fri, 8 Apr 2022 12:20:32 GMT, Claes Redestad  wrote:

>> A few additional enhancements aiming to improve VH performance in the 
>> interpreter:
>> 
>> - Flatten `TypeAndInvokers`: adds a pointer to `VarHandle` (a small increase 
>> 40->48) but removes an object and an indirection on any instance actually 
>> used - and might avoid allocating the `MethodHandle[]` unnecessarily on some 
>> instances
>> - Have `checkExactAccessMode` return the directness of the `VarHandle` so 
>> that we can avoid some `isDirect` method calls.
>> 
>> Baseline, `-Xint`
>> 
>> Benchmark Mode  CntScore   Error  Units
>> VarHandleExact.exact_exactInvocation  avgt   30  478.324 ? 5.762  ns/op
>> VarHandleExact.generic_exactInvocationavgt   30  392.114 ? 1.644  ns/op
>> VarHandleExact.generic_genericInvocation  avgt   30  822.484 ? 1.865  ns/op
>> 
>> 
>> Patched, `-Xint`
>> 
>> Benchmark Mode  CntScore   Error  Units
>> VarHandleExact.exact_exactInvocation  avgt   30  437.704 ? 5.320  ns/op
>> VarHandleExact.generic_exactInvocationavgt   30  374.512 ? 3.154  ns/op
>> VarHandleExact.generic_genericInvocation  avgt   30  757.054 ? 1.237  ns/op
>> 
>> 
>> No significant performance difference in normal mode.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Simplified as suggested by @ExE-Boss

`checkExactAccessModeThenIsDirect`?

At a minimum the return value needs documenting via a comment!

-

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

Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v13]

[David Holmes]

On Fri, 8 Apr 2022 22:17:23 GMT, Srinivas Vamsi Parasa  
wrote:

>> Optimizes the divideUnsigned() and remainderUnsigned() methods in 
>> java.lang.Integer and java.lang.Long classes using x86 intrinsics. This 
>> change shows 3x improvement for Integer methods and upto 25% improvement for 
>> Long. This change also implements the DivMod optimization which fuses 
>> division and modulus operations if needed. The DivMod optimization shows 3x 
>> improvement for Integer and ~65% improvement for Long.
>
> Srinivas Vamsi Parasa has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix the divmod crash due to lack of control node

This change appears to be causing crashes in tier4 - possibly Xcomp related:

#  assert(ctrl == kit.control()) failed: Control flow was added although the 
intrinsic bailed out

I will file a new bug.

-

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

Re: [External] : Re: jpackage Windows support for 4 digits version

[David Holmes]

On 25/03/2022 5:14 am, Alexey Semenyuk wrote:

Hi Sverre,

Oh, I misunderstood the problem. I though the issue was with parsing WiX 
version string, but the problem is that jpackage doesn't like the value 
of --version cli parameter.
On windows this value should satisfy constrains of MSI ProductVersion 
property [1]:

The format of the string is as follows:
major.minor.build
The first field is the major version and has a maximum value of 255. The 
second field is the minor version and has a maximum value of 255. The 
third field is called the build version or the update version and has a 
maximum value of 65,535.


It also states:

"Note that Windows Installer uses only the first three fields of the 
product version. If you include a fourth field in your product version, 
the installer ignores the fourth field."


So I think ignoring that fourth field is what is being requested.

Cheers,
David

If not doing validation early, wix candle will fail later. This is 
platform limitation, not the limitation of jpackage.


[1] https://msdn.microsoft.com/en-us/library/aa370859%28v=VS.85%29.aspx

- Alexey

On 3/24/2022 2:04 PM, Sverre Moe wrote:

Hi,

This has been attempted on Java 15, Java 16 and the latest Java 17.

This is the output from Java 17

14:46:49 
 C:\cygwin64\home\build\jenkins-test\workspace\our-awesome-project_sverre_SF-99>gradlew.bat 
   --no-daemon --version

14:46:51
14:46:51  
14:46:51  Gradle 7.3.3
14:46:51  
14:46:51
14:46:51  Build time:   2021-12-22 12:37:54 UTC
14:46:51  Revision: 6f556c80f945dc54b50e0be633da6c62dbe8dc71
14:46:51
14:46:51  Kotlin:       1.5.31
14:46:51  Groovy:       3.0.9
14:46:51  Ant:          Apache Ant(TM) version 1.10.11 compiled on 
July 10 2021

14:46:51  JVM:          17.0.2 (Eclipse Adoptium 17.0.2+8)
14:46:51  OS:           Windows 10 10.0 amd64


14:46:53 
 C:\cygwin64\home\build\jenkins-test\workspace\our-awesome-project_sverre_SF-99>gradlew.bat 
   --no-daemon --stacktrace --refresh-dependencies -Pheadless 
createPackage

14:48:51  > Task :jpackage
14:48:51  [14:49:12.348] Running candle.exe
14:48:51  [14:49:12.386] Running C:\Program Files (x86)\WiX Toolset 
v3.11\bin\candle.exe

14:48:51  [14:49:12.762] Running light.exe
14:48:51  [14:49:12.773] Running C:\Program Files (x86)\WiX Toolset 
v3.11\bin\light.exe

14:48:51  [14:49:13.207] Detected [candle.exe] version [3.11.2.4516].
14:48:51  [14:49:13.208] Detected [light.exe] version [3.11.2.4516].
14:48:51  [14:49:13.210] WiX 3.11.2.4516 detected. Enabling advanced 
cleanup action.
14:48:51  [14:49:13.211] jdk.jpackage.internal.ConfigException: 
Version sting may have between 1 and 3 components: 1, 1.2, 1.2.3.
14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.validate(WinMsiBundler.java:319) 

14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.WinExeBundler.validate(WinExeBundler.java:93) 

14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.Arguments.generateBundle(Arguments.java:675) 

14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.Arguments.processArguments(Arguments.java:550) 


14:48:51   at jdk.jpackage/jdk.jpackage.main.Main.execute(Main.java:91)
14:48:51   at jdk.jpackage/jdk.jpackage.main.Main.main(Main.java:52)
14:48:51  Caused by: java.lang.IllegalArgumentException: Version sting 
may have between 1 and 3 components: 1, 1.2, 1.2.3.
14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.MsiVersion.of(MsiVersion.java:46)
14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.validate(WinMsiBundler.java:317) 


14:48:51   ... 5 more
14:48:51  [14:49:13.218] jdk.jpackage.internal.PackagerException: 
Bundler EXE Installer Package skipped because of a configuration 
problem: Version sting may have between 1 and 3 components: 1, 1.2, 
1.2.3.
14:48:51  Advice to fix: Set value of --app-version parameter 
according to these rules: 
https://msdn.microsoft.com/en-us/library/aa370859%28v=VS.85%29.aspx 
 

14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.Arguments.generateBundle(Arguments.java:688) 

14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.Arguments.processArguments(Arguments.java:550) 


14:48:51   at jdk.jpackage/jdk.jpackage.main.Main.execute(Main.java:91)
14:48:51   at jdk.jpackage/jdk.jpackage.main.Main.main(Main.java:52)
14:48:51  Caused by: jdk.jpackage.internal.ConfigException: Version 
sting may have between 1 and 3 components: 1, 1.2, 1.2.3.
14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.validate(WinMsiBundler.java:319) 

14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.WinExeBundler.validate(WinExeBundler.java:93) 

14:48:51   at 
jdk.jpackage/jdk.jpackage.internal.Arguments.generateBundle(Arguments.java:675) 


14:48:51   ... 3 more
14:48:51  

Re: RFR: 8186958: Need method to create pre-sized HashMap

[David Holmes]

On Wed, 23 Mar 2022 18:41:59 GMT, XenoAmess  wrote:

> 8186958: Need method to create pre-sized HashMap

src/java.base/share/classes/java/util/Collections.java line 5836:

> 5834:  * @param   the type of keys maintained by this map
> 5835:  * @param   the type of mapped values
> 5836:  * @return initial capacity for HashMap based classes.

Again this method returns a Map

-

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

Re: RFR: 8186958: Need method to create pre-sized HashMap

[David Holmes]

On Thu, 24 Mar 2022 07:01:43 GMT, XenoAmess  wrote:

>> src/java.base/share/classes/java/util/Collections.java line 5822:
>> 
>>> 5820:  * @param   the type of keys maintained by this map
>>> 5821:  * @param   the type of mapped values
>>> 5822:  * @return initial capacity for HashMap based classes.
>> 
>> That isn't what is returned.
>
>> That isn't what is returned.
> 
> @dholmes-ora Yes it isn't actually. but I cannot find a better description 
> string for it.
> 
> "the pre-processed raw initial capacity for HashMap based classes." is more 
> exact but sounds weird.
> 
> Have you any suggestions to replace this? Thanks!

This method doesn't return the capacity it returns the Map!

-

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

Re: RFR: 8186958: Need method to create pre-sized HashMap

[David Holmes]

On Wed, 23 Mar 2022 18:41:59 GMT, XenoAmess  wrote:

> 8186958: Need method to create pre-sized HashMap

src/java.base/share/classes/java/util/Collections.java line 5822:

> 5820:  * @param   the type of keys maintained by this map
> 5821:  * @param   the type of mapped values
> 5822:  * @return initial capacity for HashMap based classes.

That isn't what is returned.

-

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

Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes

[David Holmes]

On 22/03/2022 4:56 pm, Rémi Forax wrote:

On Tue, 22 Mar 2022 04:38:15 GMT, ExE Boss  wrote:


javac should be changed to allow fully qualified access to private inner 
classes in the permits clause of an enclosing class.


This is not how private works, private means accessible between the '{' and the 
'}' of the class.
The permits clause is declared outside of the '{' and the '}' of the class, 
thus a private member class is not visible from the permits clause.


This seems like an oversight in the Sealed classes specification. It 
makes perfect sense to have a sealed class that can only be extended by 
its own nested classes, so you should be able to clearly state that.


David
-


-

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

Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v2]

[David Holmes]

On Tue, 22 Mar 2022 12:08:01 GMT, Fei Yang  wrote:

>> make/autoconf/libraries.m4 line 152:
>> 
>>> 150:   fi
>>> 151: 
>>> 152:   # Programs which use C11 or C++11 atomics, like #include ,
>> 
>> Use of C++ atomics is not allowed in hotspot code base. See the style guide:
>> https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
>> 
>> That said, I don't see any actual use of C++ atomics. ??
>
> I think the old code comment here is a bit too general. It does not mean we 
> introduce any use of C++ atomics here.
> The fact is that RISC-V only has word-sized atomics, it requries libatomic 
> where other common architectures do not [1].
> So atomic support would require explicit linking against -latomic on RISC-V. 
> Otherwise we got build errors like:

New comment looks good - thanks for clarifying.

-

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

Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]

[David Holmes]

On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang  wrote:

>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the 
>> https://openjdk.java.net/projects/riscv-port branch.
>> 
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive 
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are 
>> also carried out regularly. So it should be good enough to run most Java 
>> programs.
>> 
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments

Marked as reviewed by dholmes (Reviewer).

-

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

Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v2]

[David Holmes]

On Tue, 22 Mar 2022 03:31:16 GMT, Fei Yang  wrote:

>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the 
>> https://openjdk.java.net/projects/riscv-port branch.
>> 
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive 
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are 
>> also carried out regularly. So it should be good enough to run most Java 
>> programs.
>> 
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains two additional commits since 
> the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into JDK-8276799
>  - 8276799: Implementation of JEP 422: Linux/RISC-V Port

Hi,

I've looked at everything that is not a RISC-V specific file, except for the C1 
changes as the compiler folk will need to approve those.

Some copyrights will need updating to 2022 on the Oracle copyright line please.

I have flagged one issue in regard to C++ atomics - see below.

Thanks,
David

make/autoconf/libraries.m4 line 152:

> 150:   fi
> 151: 
> 152:   # Programs which use C11 or C++11 atomics, like #include ,

Use of C++ atomics is not allowed in hotspot code base. See the style guide:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md

That said, I don't see any actual use of C++ atomics. ??

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8283426: Fix 'exeption' typo

[David Holmes]

On Sun, 20 Mar 2022 13:30:01 GMT, Andrey Turbanov  wrote:

> Fix repeated typo `exeption`

Looks good.

Thanks for cleaning this up.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2 [v2]

[David Holmes]

On Fri, 18 Mar 2022 02:39:23 GMT, Mikael Vidstedt  wrote:

>> Note: this PR replaces the one I messed up earlier.
>> 
>> Background, from JBS:
>> 
>> src/java.base/share/native/libverify/check_code.c: In function 
>> 'read_all_code':
>> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' 
>> may be used uninitialized [-Werror=maybe-uninitialized]
>> 942 | check_and_push(context, lengths, VM_MALLOC_BLK);
>> | ^~~
>> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument 
>> 2 of type 'const void *' to 'check_and_push' declared here
>> 4145 | static void check_and_push(context_type *context, const void *ptr, 
>> int kind)
>> | ^~
>> 
>> Because the second argument of check_and_push is "const void*" GCC assumes 
>> that the malloc:ed data, which has not yet been initialized, will not be/can 
>> not be modified later which in turn suggests it may be used without ever 
>> being initialized.
>> 
>> The same general issue was addressed in JDK-8266168, presumably for GCC 11.1.
>> 
>> Details:
>> 
>> Instead of sprinkling more calloc calls around or using pragmas/gcc 
>> attributes I chose to change the check_and_push function to take a 
>> (non-const) void* argument, and provide a new wrapper function 
>> check_and_push_const which handles the const argument case. For the 
>> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a 
>> const conversion.
>> 
>> To avoid having multiple ways of solving the same problem I also chose to 
>> revert the change made in JDK-8266168, reverting the calloc back to a malloc 
>> call.
>> 
>> Testing:
>> 
>> tier1 + builds-tier{2,3,4,5}
>
> Mikael Vidstedt has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Use const char for check_and_push_string_utf

Looks good!

-

Marked as reviewed by dholmes (Reviewer).

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

Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?

[David Holmes]

On 18/03/2022 10:51 am, Cheng Jin wrote:

Hi Raffaello,

My concern is why the verification happens even without initialization in the 
test without mh.invoke() in the main(), which I don't think is covered or 
explained in the JVM Spec.
Put it in another way, my understanding is, when the class gets loaded, it is 
verified which doesn't necessarily lead to initialization, am I correct?


From Section 5.4 of the JVMS:

This specification allows an implementation flexibility as to when 
linking activities (and, because of recursion, loading) take place, 
provided that all of the following properties are maintained:


• A class or interface is completely loaded before it is linked.
• A class or interface is completely verified and prepared before it is 
initialized.


---

Hotspot does loading, verification and preparing upfront/eagerly.

HTH.

David
-


Best Regards
Cheng

-Original Message-
From: core-libs-dev  On Behalf Of 
Raffaello Giulietti
Sent: March 17, 2022 8:35 PM
To: core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: When to initialize the method's class for 
MethodHandles.Lookup.findStatic()?

Cheng,

initialization is the last thing that happens because it's where user provided 
code gets executed.

This has always been this way, as long as I can remember. See the JVMS for the 
gory details.


Greetings
Raffaello


On 2022-03-18 01:21, Cheng Jin wrote:

Hi David,

1) for the test with mh.invoke() in main(),  the log shows:
[0.262s][info][class,init] Start class verification for: Test_1
[0.262s][info][class,init] End class verification for: Test_1
[0.263s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800)
[0.263s][info][class,init] Start class verification for: Test_2
[0.263s][info][class,init] End class verification for: Test_2
[0.272s][info][class,init] 366 Initializing 'Test_2' (0x000800c00a08) 
<--

2) for the test without  mh.invoke() in main(),  the log shows:
[0.296s][info][class,init] Start class verification for: Test_1
[0.296s][info][class,init] End class verification for: Test_1
[0.297s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800)
[0.297s][info][class,init] Start class verification for: Test_2
[0.297s][info][class,init] End class verification for: Test_2
(Test_2 was verified but didn't get initialized)

The comparison above literally surprised me that the bytecode verification 
happened prior to the class initialization, which means
the class got verified at first even without initialization coz I previously 
thought the initialization should trigger the verification rather than in the 
reversed order.

Could you explain a little more about why it goes in this way?

Best Regards
Cheng


-Original Message-
From: core-libs-dev  On Behalf Of David 
Holmes
Sent: March 17, 2022 7:46 PM
To: Raffaello Giulietti ; 
core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: When to initialize the method's class for 
MethodHandles.Lookup.findStatic()?

Run with -Xlog:class+init=info to see the classes that get initialized and in 
what order.

David

On 18/03/2022 5:53 am, Raffaello Giulietti wrote:

Hi again,

here's code that shows that initialization doesn't happen during
lookup but only upon invoking the method handle. (I'm on Java 17.)

As long as the 2nd line in main() is commented, you don't see the
message "Test_2 initialized", which shows that the lookup doesn't
initialize Test_2.
When you uncomment the line in main(), the message will appear.

So, as advertised, it's the invocation of the method handle that can
trigger initialization, not the lookup.


HTH
Raffaello



import java.lang.invoke.*;

public class Test_1 {

       static MethodHandle mh;

       static {
       try {
       mh = MethodHandles.lookup().findStatic(Test_2.class,
"testMethod", MethodType.methodType(int.class, int.class));
       } catch (NoSuchMethodException | IllegalAccessException e) {
       e.printStackTrace();
       }
       }

       public static void main(String[] args) throws Throwable {
       System.out.println(mh);
       // System.out.println(Test_1.mh.invoke(0));
       }

}




public class Test_2 {

       static {
       System.out.println("Test_2 initialized");
       }

       static int testMethod(int value) { return (value + 1); }

}




On 2022-03-17 20:38, Raffaello Giulietti wrote:

Hi,

as far as I can see, the code should not even compile, as there's a
static field in Test_1 which is initialized with an expression that
throws checked exceptions (findStatic(..)).

In addition, it seems to me that there's nothing in your code that
reveals whether Test_2 has been initialized during the lookup. How
can you tell?

Finally, the method handle invocation in Test_1 will throw, as you
don't pass any argument to a handle that expects one.

Can you perhaps add more details?


Greetings
Raffaello



On 2022-03-17 17:42, Cheng Jin wrote:

Hi there,

Th

Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]

[David Holmes]

On Thu, 17 Mar 2022 16:23:14 GMT, Tyler Steele  wrote:

>> As described in the linked issue, NullClassBytesTest fails due an 
>> OutOfMemoryError produced on AIX when the test calls defineClass with a byte 
>> array of size of 0. The native implementation of defineClass then calls  
>> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other 
>> platforms it return a valid address. When NULL is produced by malloc for 
>> this reason, ClassLoader.c incorrectly interprets this as a failure due to a 
>> lack of memory.
>> 
>> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when 
>> `errno == ENOMEM` and to produce a ClassFormatError with the message 
>> "ClassLoader internal allocation failure" in all other cases (in which 
>> malloc returns NULL).~~ [edit: The above no longer describes the PR's 
>> proposed fix. See discussion below]
>> 
>> In addition, I performed some minor tidy-up work in ClassLoader.c by 
>> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` 
>> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code 
>> in ClassLoader.c, but didn't feel worthy of opening a separate issue.
>> 
>> ### Alternatives
>> 
>> It would be possible to address this failure by modifying the test to accept 
>> the OutOfMemoryError on AIX. I thought it was a better solution to modify 
>> ClassLoader.c to produce an OutOfMemoryError only when the system is 
>> actually out of memory.
>> 
>> ### Testing
>> 
>> This change has been tested on AIX and Linux/x86.
>
> Tyler Steele has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Improve comment
>   
>   - Reword to avoid double use of malloc(X)
>   - Remove bug id

Update looks good. Sorry for the AIX_ONLY misdirect.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2

[David Holmes]

On Thu, 17 Mar 2022 20:56:34 GMT, Mikael Vidstedt  wrote:

> Note: this PR replaces the one I messed up earlier.
> 
> Background, from JBS:
> 
> src/java.base/share/native/libverify/check_code.c: In function 
> 'read_all_code':
> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may 
> be used uninitialized [-Werror=maybe-uninitialized]
> 942 | check_and_push(context, lengths, VM_MALLOC_BLK);
> | ^~~
> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument 
> 2 of type 'const void *' to 'check_and_push' declared here
> 4145 | static void check_and_push(context_type *context, const void *ptr, int 
> kind)
> | ^~
> 
> Because the second argument of check_and_push is "const void*" GCC assumes 
> that the malloc:ed data, which has not yet been initialized, will not be/can 
> not be modified later which in turn suggests it may be used without ever 
> being initialized.
> 
> The same general issue was addressed in JDK-8266168, presumably for GCC 11.1.
> 
> Details:
> 
> Instead of sprinkling more calloc calls around or using pragmas/gcc 
> attributes I chose to change the check_and_push function to take a 
> (non-const) void* argument, and provide a new wrapper function 
> check_and_push_const which handles the const argument case. For the 
> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a 
> const conversion.
> 
> To avoid having multiple ways of solving the same problem I also chose to 
> revert the change made in JDK-8266168, reverting the calloc back to a malloc 
> call.
> 
> Testing:
> 
> tier1 + builds-tier{2,3,4,5}

Changes requested by dholmes (Reviewer).

src/java.base/share/native/libverify/check_code.c line 472:

> 470: 
> 471: static void check_and_push_malloc_block(context_type *context, void 
> *ptr);
> 472: static void check_and_push_string_utf(context_type *context, const void 
> *ptr);

Can't this be:

`static void check_and_push_string_utf(context_type *context, const char* str);`

-

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

Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?

[David Holmes]

Run with -Xlog:class+init=info to see the classes that get initialized 
and in what order.


David

On 18/03/2022 5:53 am, Raffaello Giulietti wrote:

Hi again,

here's code that shows that initialization doesn't happen during lookup 
but only upon invoking the method handle. (I'm on Java 17.)


As long as the 2nd line in main() is commented, you don't see the 
message "Test_2 initialized", which shows that the lookup doesn't 
initialize Test_2.

When you uncomment the line in main(), the message will appear.

So, as advertised, it's the invocation of the method handle that can 
trigger initialization, not the lookup.



HTH
Raffaello



import java.lang.invoke.*;

public class Test_1 {

     static MethodHandle mh;

     static {
     try {
     mh = MethodHandles.lookup().findStatic(Test_2.class, 
"testMethod", MethodType.methodType(int.class, int.class));

     } catch (NoSuchMethodException | IllegalAccessException e) {
     e.printStackTrace();
     }
     }

     public static void main(String[] args) throws Throwable {
     System.out.println(mh);
     // System.out.println(Test_1.mh.invoke(0));
     }

}




public class Test_2 {

     static {
     System.out.println("Test_2 initialized");
     }

     static int testMethod(int value) { return (value + 1); }

}




On 2022-03-17 20:38, Raffaello Giulietti wrote:

Hi,

as far as I can see, the code should not even compile, as there's a 
static field in Test_1 which is initialized with an expression that 
throws checked exceptions (findStatic(..)).


In addition, it seems to me that there's nothing in your code that 
reveals whether Test_2 has been initialized during the lookup. How can 
you tell?


Finally, the method handle invocation in Test_1 will throw, as you 
don't pass any argument to a handle that expects one.


Can you perhaps add more details?


Greetings
Raffaello



On 2022-03-17 17:42, Cheng Jin wrote:

Hi there,

The document of 
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType) 
in the Java API is ambiguous in terms of when to initialize the 
method's class as follows (the same description as in other OpenJDK 
versions)


If the returned method handle is invoked, the method's class will be 
initialized, if it has not already been initialized.



It occurs to me that the method's class should be initialized when 
invoking the method handle but OpenJDK actually chooses to do the 
initialization in lookup.findStatic() rather than in mh.invoke()

e.g.
import java.lang.invoke.*;

public class Test_1 {
 static MethodHandle mh = 
MethodHandles.lookup().findStatic(Test_2.class, "testMethod", 
MethodType.methodType(int.class, int.class)); <--- 
Test_2.class gets initialized and verified.


 public static void main(String[] args) throws Throwable {
 Test_1.mh.invoke();
 }
}

public class Test_2 {
 static int testMethod(int value) { return (value + 1); }
}

So there should be more clear explanation what is the correct or 
expected behaviour at this point and why OpenJDK doesn't comply with 
the document to delay the initialization of the method's class to 
mh.invoke().


Best Regards
Cheng Jin

Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]

[David Holmes]

On Wed, 16 Mar 2022 21:10:14 GMT, Tyler Steele  wrote:

>> As described in the linked issue, NullClassBytesTest fails due an 
>> OutOfMemoryError produced on AIX when the test calls defineClass with a byte 
>> array of size of 0. The native implementation of defineClass then calls  
>> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other 
>> platforms it return a valid address. When NULL is produced by malloc for 
>> this reason, ClassLoader.c incorrectly interprets this as a failure due to a 
>> lack of memory.
>> 
>> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when 
>> `errno == ENOMEM` and to produce a ClassFormatError with the message 
>> "ClassLoader internal allocation failure" in all other cases (in which 
>> malloc returns NULL).~~ [edit: The above no longer describes the PR's 
>> proposed fix. See discussion below]
>> 
>> In addition, I performed some minor tidy-up work in ClassLoader.c by 
>> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` 
>> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code 
>> in ClassLoader.c, but didn't feel worthy of opening a separate issue.
>> 
>> ### Alternatives
>> 
>> It would be possible to address this failure by modifying the test to accept 
>> the OutOfMemoryError on AIX. I thought it was a better solution to modify 
>> ClassLoader.c to produce an OutOfMemoryError only when the system is 
>> actually out of memory.
>> 
>> ### Testing
>> 
>> This change has been tested on AIX and Linux/x86.
>
> Tyler Steele 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:
> 
>   Addresses failure in NullClassTest on AIX.
>   
>   - Changes malloc(0) call to malloc(1) on AIX.

src/java.base/share/native/libjava/ClassLoader.c line 102:

> 100: }
> 101: 
> 102: // On malloc(0), implementators of malloc(3) have the choice to 
> return either

It is confusing to mix `malloc(0)`, where you are passing an argument zero to 
malloc, with `malloc(3)` which actually means the definition of malloc as per 
the man page in section 3.

Given this is only an issue on AIX the comment can simply say:

`// On AIX malloc(0) returns NULL which looks like an out-of-memory condition; 
so adjust it to malloc(1)`.

src/java.base/share/native/libjava/ClassLoader.c line 106:

> 104: // we chose the latter. (see 8283225)
> 105: #ifdef _AIX
> 106: body = (jbyte *)malloc(length == 0 ? 1 : length);

Using AIX_ONLY this can be simplified:

`body = (jbyte *)malloc(length AIX_ONLY( == 0 ? 1 : length));`

-

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

Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2

[David Holmes]

On Fri, 11 Mar 2022 23:38:10 GMT, Mikael Vidstedt  wrote:

> Background, from JBS:
> 
> src/java.base/share/native/libverify/check_code.c: In function 
> 'read_all_code': 
> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may 
> be used uninitialized [-Werror=maybe-uninitialized] 
>   942 | check_and_push(context, lengths, VM_MALLOC_BLK); 
>   | ^~~ 
> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument 
> 2 of type 'const void *' to 'check_and_push' declared here 
>  4145 | static void check_and_push(context_type *context, const void *ptr, 
> int kind) 
>   | ^~ 
> 
> 
> Because the second argument of check_and_push is "const void*" GCC assumes 
> that the malloc:ed data, which has not yet been initialized, will not be/can 
> not be modified later which in turn suggests it may be used without ever 
> being initialized. 
> 
> The same general issue was addressed in 
> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably 
> for GCC 11.1.
> 
> 
> Details:
> 
> Instead of sprinkling more calloc calls around or using pragmas/gcc 
> attributes I chose to change the check_and_push function to take a 
> (non-const) void* argument, and provide a new wrapper function 
> `check_and_push_const` which handles the const argument case. For the 
> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a 
> const conversion.
> 
> To avoid having multiple ways of solving the same problem I also chose to 
> revert the change made in JDK-8266168, reverting the calloc back to a malloc 
> call.
> 
> Testing:
> 
> tier1 + builds-tier{2,3,4,5}

src/java.base/share/native/libverify/check_code.c line 4168:

> 4166: }
> 4167: 
> 4168: static void check_and_push_const(context_type *context, const void 
> *ptr, int kind) {

IIUC the only `const` cases all pass a `const char*` so perhaps we can make 
that explicit in the signature? I confess I can't figure out how the passed in 
memory eventually gets used and it seems bizarre that it could be a freshly 
malloc'd block or an existing string.

-

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

Re: RFR: 8253495: CDS generates non-deterministic output

[David Holmes]

On 11/03/2022 4:50 pm, Thomas Stuefe wrote:

On Fri, 11 Mar 2022 05:59:00 GMT, David Holmes  wrote:


Thanks for pointing this out. I ran more tests and found that on certain 
platforms, there are other structures that have problems with uninitialized 
gaps. I ended up changing `os::malloc()` to zero the buffer when running with 
-Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't 
matter too much for regular VM executions because `os::malloc()` already has a 
high overhead.


This is raising red flags for me sorry. Every user of the JDK is now paying a penalty 
because of something only needed when dumping the shared archive. It might not be much 
but it is the old "death by a thousand cuts".


Well, he does it for `DumpSharedSpaces` only. Are you really worried about that 
one load+conditional jump?


As I said (and I'm not the only one who says this :) ) "death by a 
thousand cuts".



@iklam: I dislike the fact that CDS terminology is now in os::malloc. I would give this 
another flag, "ZapMalloc" or similar, and maybe merge it with the 
`DEBUG_ONLY(memset(..uninitBlockPad))` above.


Is there any way to tell the OS to pre-zero all memory provided to the current 
process, such that we could set that when dumping and not have to check on each 
allocation?


No. Malloced memory is not provided by the OS but by the glibc, and it may be 
polluted with whatever the allocator did with it (eg pointers to chain free 
blocks), or by prior user payload.

Glibc has a specific tunable, `glibc.malloc.perturb`, that initializes malloc 
memory to a given value, but you cannot directly set the value. It is very 
handy to check for uninitialized memory. Always wanted to add some tests that 
used it, but never got around. But obviously its nothing you could do in 
production.



And I have to wonder how easy it would be to re-introduce non-deterministic 
values in these data structures that are being dumped. Does malloc itself even 
guarantee to return the same set of addresses for the same sequence of requests 
in different executions of a program?


No, of course not. Non-Java threads may still run concurrently, no? System 
libraries do malloc too. But are pointer *values* even the problem?


I assumed that pointer values could be the problem, even if not 
presently, but apparently all the pointers get rewritten to use offsets 
from the known base of the shared archive - so not an issue.


Cheers,
David


-

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

Re: RFR: 8253495: CDS generates non-deterministic output

[David Holmes]

On 11/03/2022 4:40 pm, Ioi Lam wrote:

On Fri, 11 Mar 2022 05:59:00 GMT, David Holmes  wrote:


I ended up changing `os::malloc()` to zero the buffer when running with 
-Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't 
matter too much for regular VM executions because `os::malloc()` already has a 
high overhead.


This is raising red flags for me sorry. Every user of the JDK is now paying a penalty 
because of something only needed when dumping the shared archive. It might not be much 
but it is the old "death by a thousand cuts". Is there any way to tell the OS 
to pre-zero all memory provided to the current process, such that we could set that when 
dumping and not have to check on each allocation?


I don't know how to tell the OS (or C library) to zero out the buffer returned 
by malloc. However, in the current code path, we already have a test for an 
uncommon condition when `os::malloc()` calls `MemTracker::record_malloc()` 
which calls `MallocTracker::record_malloc()`


void* MallocTracker::record_malloc(void* malloc_base, size_t size, MEMFLAGS 
flags,
   const NativeCallStack& stack)
{
   if (MemTracker::tracking_level() == NMT_detail) {
 MallocSiteTable::allocation_at(stack, size, _marker, flags);
   }


I can combine the tests for `MemTracker::tracking_level()` and 
`DumpSharedSpaces` into a single test and do more work only when the uncommon 
path is taken. This would require some refactoring of the 
MemTracker/MallocTracker code. I'd rather do that in a separate RFE.

In fact, `MemTracker::_tracking_level` is tested twice in the current 
implementation. We can change it to do a single test in the most common case 
(NMT_summary) if we really want to cut down the number of tests. But honestly I 
don't think this makes any difference.


And I have to wonder how easy it would be to re-introduce non-deterministic 
values in these data structures that are being dumped. Does malloc itself even 
guarantee to return the same set of addresses for the same sequence of requests 
in different executions of a program?


The malloc'ed objects are copied into the CDS archive at deterministic 
addresses. Any pointers inside such objects will be relocated.



Okay. I won't object further but I really don't like it - c'est la vie! 
I'll let others review the actual code changes in detail.


Cheers,
David


-

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

Re: RFR: 8253495: CDS generates non-deterministic output [v2]

[David Holmes]

I can't find this comment in the PR so replying via email ...

On 11/03/2022 9:24 am, Ioi Lam wrote:

On Wed, 9 Mar 2022 07:47:19 GMT, Thomas Stuefe  wrote:


Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

   Fixed zero build


src/hotspot/share/utilities/hashtable.hpp line 42:


40:
41:   LP64_ONLY(unsigned int _gap;)
42:


For 64-bit, you now lose packing potential in the theoretical case the 
following payload does not have to be aligned to 64 bit. E.g. for T=char, where 
the whole entry would fit into 8 bytes. Probably does not matter as long as 
entries are allocated individually from C-heap which is a lot more wasteful 
anyway.

For 32-bit, I think you may have the same problem if the payload starts with a 
uint64_t. Would that not be aligned to a 64-bit boundary too? Whether or not 
you build on 64-bit?

I think setting the memory, or at least the first 8..16 bytes, of the entry to zero 
in BasicHashtable::new_entry could be more robust:

(16 bytes in case the payload starts with a long double but that may be 
overthinking it :)


template  BasicHashtableEntry* 
BasicHashtable::new_entry(unsigned int hashValue) {
   char* p = :new (NEW_C_HEAP_ARRAY(char, this->entry_size(), F);
   ::memset(p, 0, MIN2(this->entry_size(), 16)); // needs reproducable
   BasicHashtableEntry* entry = ::new (p) BasicHashtableEntry(hashValue);
   return entry;
}

If you are worried about performance, this may also be controlled by a template 
parameter, and then you do it just for the system dictionary.


Thanks for pointing this out. I ran more tests and found that on certain 
platforms, there are other structures that have problems with uninitialized 
gaps. I ended up changing `os::malloc()` to zero the buffer when running with 
-Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't 
matter too much for regular VM executions because `os::malloc()` already has a 
high overhead.


This is raising red flags for me sorry. Every user of the JDK is now 
paying a penalty because of something only needed when dumping the 
shared archive. It might not be much but it is the old "death by a 
thousand cuts". Is there any way to tell the OS to pre-zero all memory 
provided to the current process, such that we could set that when 
dumping and not have to check on each allocation?


And I have to wonder how easy it would be to re-introduce 
non-deterministic values in these data structures that are being dumped. 
Does malloc itself even guarantee to return the same set of addresses 
for the same sequence of requests in different executions of a program?


Cheers,
David
-


-

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

Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver

[David Holmes]

On Wed, 9 Mar 2022 22:52:41 GMT, Mandy Chung  wrote:

> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null 
> receiver check rather than NPE thrown by `Object::getClass`.  The message of 
> NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be 
> helpful but not in this case.

I'd like to know if the explicit null check will lead to removal of the 
existing implicit null checks? Otherwise this is just "death by a thousand 
cuts" whether requireNonNull is inlined or not.

-

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

Re: RFR: 8253495: CDS generates non-deterministic output [v2]

[David Holmes]

On Thu, 10 Mar 2022 19:41:03 GMT, Ioi Lam  wrote:

>> I think he already did. I'm quoting:
>> 
>>> However, the CDS archive also contains a heap dump, which includes Java 
>>> HashMaps. If I allow those 3 Java threads to start, some HashMaps in the 
>>> module graph will have unstable ordering. I think the reason is concurrent 
>>> thread execution causes unstable assignment of the identity_hash for 
>>> objects in the heap dump.
>
>> @magicus the issue is not the list of classes dumped, or their format in the 
>> dump. As Ioi indicated that list is fixed. The issue is with the heap dump 
>> part of the archive. Running these other threads affects the heap so by not 
>> running them with end up with a different heap. So the question is whether 
>> there is anything about having a different heap dumped that we need to be 
>> concerned about. We dump the heap into the archive for a reason and this 
>> changes what we dump,
> 
> To be clear, if multiple threads are running, classes could be loaded in a 
> different order and the symbols will have different orders. This would cause 
> the vtables in Klass objects to be laid out differently. Trying to fix this 
> is very difficult. I have a different patch that makes it work but it's just 
> too complicated.
> 
> So, disabling the threads is also necessary for (easily) sorting the 
> metaspace objects.

@iklam  thanks for clarifying.

-

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

Re: RFR: 8253495: CDS generates non-deterministic output [v2]

[David Holmes]

On Thu, 10 Mar 2022 12:50:58 GMT, Magnus Ihse Bursie  wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed zero build
>
> Well, previously we'd get different dumps on different runs. If that was an 
> issue, surely it would have manifested itself by now? With this change, we'll 
> just get the same dump each run. I fail to see how that could be a risk.

@magicus  I think we need @iklam to weigh in here and explain exactly what the 
"heap dump" consists of and how not running those threads affects its contents. 
Presently the heap dump is potentially different on each run, IIUC, only due to 
the order of its contents, not the contents themselves.

-

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

Re: RFR: 8253495: CDS generates non-deterministic output [v2]

[David Holmes]

On Thu, 10 Mar 2022 12:11:06 GMT, Magnus Ihse Bursie  wrote:

>> The "heap dump" aspect of this is not something I'm familiar with, but if 
>> the threads don't affect the list of classes dumped, they surely must affect 
>> what is in the heap dump otherwise their execution would not be an issue. So 
>> you must be sacrificing something by not having these threads start.
>
> @dholmes-ora That something is "sacrificed" does not follow from that 
> something is "different". The list of classes to dump is specified in the 
> lib/classlist file, which is generated during the build. 
> 
> The process of creating this involves running a suitable "exercise most 
> important parts" java program, and logging the classes loaded. This class 
> file is then post-processed (sorted) to make sure it is reproducible for the 
> same JDK code base.
> 
> As Ioi say, in this case threads are started freely, and may run in any 
> non-deterministic order.
> 
> At the next stage, we take this file (which is just done implicitly by 
> -Xshare:dump), and generate the actual CDS archive, classes.jsa. Now it turns 
> out this generation is non-deterministic. And Ioi's analysis is that this is 
> due to thread non-determinism. So if we just disable threads during the dump 
> process (where we are not really running the JVM "actually" -- it's a special 
> mode, where we don't even have a Java program to run!), there's no harm in 
> that.

@magicus the issue is not the list of classes dumped, or their format in the 
dump. As Ioi indicated that list is fixed. The issue is with the heap dump part 
of the archive. Running these other threads affects the heap so by not running 
them with end up with a different heap. So the question is whether there is 
anything about having a different heap dumped that we need to be concerned 
about. We dump the heap into the archive for a reason and this changes what we 
dump,

-

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

Re: RFR: JDK-8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver

[David Holmes]

On Wed, 9 Mar 2022 22:52:41 GMT, Mandy Chung  wrote:

> A simple patch to call `Objects.requireNonNull(recv)` for an explicit null 
> receiver check rather than NPE thrown by `Object::getClass`.  The message of 
> NPE generated by JEP 358 (Helpful NullPointerExceptions) is supposed to be 
> helpful but not in this case.

Hi Mandy,

This is an enhancement request not a bug and I don't think adding a performance 
penalty to all correct code just to produce a slightly clearer NPE message for 
broken code, is the right trade-off here. 

Cheers,
David

-

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

Re: RFR: 8253495: CDS generates non-deterministic output [v2]

[David Holmes]

On Wed, 9 Mar 2022 05:10:44 GMT, Ioi Lam  wrote:

>> This patch makes the result of "java -Xshare:dump" deterministic:
>> - Disabled new Java threads from launching. This is harmless. See comments 
>> in jvm.cpp
>> - Fixed a problem in hashtable ordering in heapShared.cpp
>> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random 
>> bits. Added code to zero it.
>> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in 
>> make/scripts/compare.sh
>> 
>> Note:  $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. 
>> This will be fixed in 
>> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
>> 
>> Testing under way:
>> - tier1~tier5
>> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, 
>> windows-x86-cmp-baseline,  etc).
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Fixed zero build

The "heap dump" aspect of this is not something I'm familiar with, but if the 
threads don't affect the list of classes dumped, they surely must affect what 
is in the heap dump otherwise their execution would not be an issue. So you 
must be sacrificing something by not having these threads start.

-

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

Re: RFR: 8253495: CDS generates non-deterministic output [v2]

[David Holmes]

On Wed, 9 Mar 2022 05:10:44 GMT, Ioi Lam  wrote:

>> This patch makes the result of "java -Xshare:dump" deterministic:
>> - Disabled new Java threads from launching. This is harmless. See comments 
>> in jvm.cpp
>> - Fixed a problem in hashtable ordering in heapShared.cpp
>> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random 
>> bits. Added code to zero it.
>> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in 
>> make/scripts/compare.sh
>> 
>> Note:  $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. 
>> This will be fixed in 
>> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
>> 
>> Testing under way:
>> - tier1~tier5
>> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, 
>> windows-x86-cmp-baseline,  etc).
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Fixed zero build

I have reservations about contorting things this way just to get "deterministic 
output".

The VM needs to fully initialize and then become quiescent before the dump 
occurs, and as I say below if you don't start other threads then you 
potentially remove part of the archive because classes won't be loaded by those 
threads.

I think if you care about the order of dumping classes then you should control 
that order, you don't try to force the order of loading. Can't you sort things 
before dumping? ie rehash/rebuild the hashtables etc so it has a canonical 
ordering? I see this was mentioned in the bug report and is considered a 
largish/complex fix, but it would be the proper fix IMO.

Thanks,
David

src/hotspot/share/prims/jvm.cpp line 2873:

> 2871: // execute in parallel, symbols and classes may be loaded in
> 2872: // random orders which will make the resulting CDS archive
> 2873: // non-deterministic.

Yes but by not starting these threads you are potentially excluding a range of 
classes from the shared archive!

-

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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[David Holmes]

On Tue, 8 Mar 2022 09:18:01 GMT, Sergey Bylokhov  wrote:

>> Please review this small patch that fixes a potential memory leak that 
>> exception return fails to release allocated `cacheDirs`
>> 
>> Test:
>> 
>> - [x] jdk_desktop on Linux x86_64
>
> src/java.desktop/unix/native/common/awt/fontpath.c line 938:
> 
>> 936: while ((cnt < max) && (cacheDir = 
>> (*FcStrListNext)(cacheDirs))) {
>> 937: jstr = (*env)->NewStringUTF(env, (const char*)cacheDir);
>> 938: JNU_CHECK_EXCEPTION_AND_CLEANUP(env, 
>> (*FcStrListDone)(cacheDirs));
> 
> I think you do not need to create an additional macro here, just inline it 
> and call "(*FcStrListDone)(cacheDirs);" directly. Something like:
> 
> if (IS_NULL(jstr) {
> (*FcStrListDone)(cacheDirs);
> return;
> }
> 
> Note that the "IS_NULL" is used in this file after NewStringUTF. Any 
> objections?

Good catch! Elsewhere in this file IS_NULL is always used to check 
NewStringUTF, so the existing code was inconsistent in checking for a pending 
exception.

Checking for NULL and checking for a pending exception are logically equivalent 
as long as the passed in pointer is not NULL.

-

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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[David Holmes]

On Tue, 8 Mar 2022 09:20:19 GMT, Sergey Bylokhov  wrote:

>> Please review this small patch that fixes a potential memory leak that 
>> exception return fails to release allocated `cacheDirs`
>> 
>> Test:
>> 
>> - [x] jdk_desktop on Linux x86_64
>
> src/java.desktop/unix/native/common/awt/fontpath.c line 940:
> 
>> 938: JNU_CHECK_EXCEPTION_AND_CLEANUP(env, 
>> (*FcStrListDone)(cacheDirs));
>> 939: 
>> 940: (*env)->SetObjectArrayElement(env, cacheDirArray, 
>> cnt++, jstr);
> 
> Probably we should add the check+cleanup after the SetObjectArrayElement? 
> Otherwise, we may call NewStringUTF while an exception is raised by the 
> SetObjectArrayElement.

??? You want to check and cleanup if NewStringUTF fails. You only want to call 
SetObjectElementArray if NewStringUTF succeeds.

-

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

Re: Wrong behavior of standard IO library when interacting with Samba (very serious)

[David Holmes]

On 7/03/2022 6:28 pm, Alan Bateman wrote:

On 07/03/2022 05:33, Glavo wrote:

I am a Java application developer. I noticed that when my program runs on
Windows in a samba shared folder (mounted as a drive, or accessed via 
a UNC

path), the Java standard IO library has some unusual behavior.
Note that these issues only occur when accessing a folder shared by
*Samba*, but not for the folder shared via SMB by another Windows host.

One of the bugs was reported years ago (JDK-8154915): `Files.isWritable`
always returns false for files shared by samba. It's worth noting for 
this

question that `File::canWrite()`'s behavior is normal.
(So in my program I pass `!Files.isWritable(p) && 
p.toFile().canWrite()` to

detect if it's shared by samba and give the user a warning)
This problem keeps showing up on several of my devices, so it should be
fine to reproduce. The reason it wasn't resolved seems to be that the
OpenJDK maintainers didn't understand that it came up when interacting 
with

Samba (not just SMB).
Testing if a file is writable, without side effects, can be complicated 
on Windows. File.canXXX only looks at the DOS attribute so can't give an 
accurate result. Files.isWritable examines the DACL, the legacy DOS 
attribute, and whether the volume is read-only, so there is more to go 
wrong. We've looked at many Windows <--> SAMBA interop issues over the 
years and it's always been that the Windows calls were failing or 
returning results that suggested the file had a DACL with 0 entries. 


That sounds similar to this old (but still open) SAMBA bug report:

https://bugzilla.samba.org/show_bug.cgi?id=7973

David
-

I 
can't tell from your mail where the issue is but some of the behavior 
you report in your mail is very unusual. The JDK does not cache results 
so if you are saying that it requires restarting the JDK then it 
suggests an issue at a lower level.


-Alan

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[David Holmes]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

I eyeballed the diff file and all seems okay.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[David Holmes]

On Fri, 4 Mar 2022 21:54:57 GMT, Zhengyu Gu  wrote:

> The macros are used to hide the different syntax of c and c++ to avoid 
> polluting code here, it seems to be a common pattern in client code.

That's to allow for general use by C or C++ source files. But you are only 
needing this in a C source file so you would only need the C variant of the 
calling syntax.

-

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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

[David Holmes]

On Fri, 4 Mar 2022 13:25:12 GMT, Zhengyu Gu  wrote:

> Please review this small patch that fixes a potential memory leak that 
> exception return fails to release allocated `cacheDirs`
> 
> Test:
> 
> - [x] jdk_desktop on Linux x86_64

I would have just inlined JNU_CHECK_EXCEPTION and added the cleanup code 
directly. The additional macro wasn't really necessary and would not really be 
usable for any kind of general cleanup actions beyond a one-liner.

But it is okay.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8282608: RawNativeLibraryImpl can't be passed to NativeLibraries::findEntry0 [v2]

[David Holmes]

On Thu, 3 Mar 2022 23:29:31 GMT, Mandy Chung  wrote:

>> This fixes a bug in JDK-8282515 that `NativeLibraries::findEntry0` reads the 
>> handle from the given native library instance with the field ID for 
>> `NativeLibraryImpl::handle` which is wrong for `RawNativeLibraryImpl`.  The 
>> `NativeLibraries::findEntry0` now takes the handle of the library instead 
>> and that can be used to look up symbols in the native library loaded by 
>> System::loadLibrary and also RawNativeLibraries.
>> 
>> Tier 1-4 testing is in progress.
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move findEntry0 to NativeLibrary

HI Mandy,

This looks reasonable to me.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

Re: PriorityQueue PR requiring review

[David Holmes]

On 3/03/2022 11:02 pm, Julian Waters wrote:
I understand, I'll notify the author about this. I'm not sure if they'll 
be alright with discussing on the mailing lists though, since they have 
expressed that they prefer discussing it on the PR itself


That's not the way OpenJDK works, and anything in the PR (once properly 
setup) goes to the mailing list anyway.


https://openjdk.java.net/contribute/

Cheers,
David


best regards,
Julian Waters

On Thu, Mar 3, 2022 at 8:57 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


On 3/03/2022 10:47 pm, Julian Waters wrote:
 > Hi David,
 >
 > I did not create the PR, I'm instead asking for others to review it
 > before I help the author create the issue on the JBS. Should I
just go
 > ahead and create the issue for them?

The best thing would be for the PR creator to discuss the proposed API
addition on the mailing list first. I have a fairly good idea what the
outcome of that discussion will be but ... :)

Cheers,
David

 > best regards,
 > Julian
 >
 > On Thu, Mar 3, 2022 at 8:45 PM David Holmes
mailto:david.hol...@oracle.com>
 > <mailto:david.hol...@oracle.com
<mailto:david.hol...@oracle.com>>> wrote:
 >
 >     Hi Julian,
 >
 >     On 3/03/2022 10:33 pm, Jules W. wrote:
 >      > Hi all,
 >      >
 >      > A new PR that adds methods to PriorityQueue was created
some time
 >     ago at
 >      > https://github.com/openjdk/jdk/pull/6938
<https://github.com/openjdk/jdk/pull/6938>
 >     <https://github.com/openjdk/jdk/pull/6938
<https://github.com/openjdk/jdk/pull/6938>> but has no corresponding
 >     issue. As
 >      > I'm not too familiar with this part of the JDK I'm
querying this
 >     mailing
 >      > list for anyone to properly review the PR before I create an
 >     issue for it
 >      > in the JBS
 >
 >     First you need an issue, then you review a PR. Your PR would
not be
 >     seen
 >     by anyone unless they go looking for it, as without the
association
 >     with
 >     an issue, the system will not send the email to the mailing
lists.
 >
 >     I see someone is helping you create the issue so things
should progress
 >     in that sense. But note the bar for adding to a public API is set
 >     very high.
 >
 >     Cheers,
 >     David
 >
 >      > best regards,
 >      > Julian Waters
 >

Re: PriorityQueue PR requiring review

[David Holmes]

On 3/03/2022 10:47 pm, Julian Waters wrote:

Hi David,

I did not create the PR, I'm instead asking for others to review it 
before I help the author create the issue on the JBS. Should I just go 
ahead and create the issue for them?


The best thing would be for the PR creator to discuss the proposed API 
addition on the mailing list first. I have a fairly good idea what the 
outcome of that discussion will be but ... :)


Cheers,
David


best regards,
Julian

On Thu, Mar 3, 2022 at 8:45 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


Hi Julian,

On 3/03/2022 10:33 pm, Jules W. wrote:
 > Hi all,
 >
 > A new PR that adds methods to PriorityQueue was created some time
ago at
 > https://github.com/openjdk/jdk/pull/6938
<https://github.com/openjdk/jdk/pull/6938> but has no corresponding
issue. As
 > I'm not too familiar with this part of the JDK I'm querying this
mailing
 > list for anyone to properly review the PR before I create an
issue for it
 > in the JBS

First you need an issue, then you review a PR. Your PR would not be
seen
by anyone unless they go looking for it, as without the association
with
an issue, the system will not send the email to the mailing lists.

I see someone is helping you create the issue so things should progress
in that sense. But note the bar for adding to a public API is set
very high.

Cheers,
David

 > best regards,
 > Julian Waters

Re: PriorityQueue PR requiring review

[David Holmes]

Sorry Julian, I see now you were the person aiding the person who 
created the PR.


David

On 3/03/2022 10:45 pm, David Holmes wrote:

Hi Julian,

On 3/03/2022 10:33 pm, Jules W. wrote:

Hi all,

A new PR that adds methods to PriorityQueue was created some time ago at
https://github.com/openjdk/jdk/pull/6938 but has no corresponding 
issue. As

I'm not too familiar with this part of the JDK I'm querying this mailing
list for anyone to properly review the PR before I create an issue for it
in the JBS


First you need an issue, then you review a PR. Your PR would not be seen 
by anyone unless they go looking for it, as without the association with 
an issue, the system will not send the email to the mailing lists.


I see someone is helping you create the issue so things should progress 
in that sense. But note the bar for adding to a public API is set very 
high.


Cheers,
David


best regards,
Julian Waters

Re: PriorityQueue PR requiring review

[David Holmes]

Hi Julian,

On 3/03/2022 10:33 pm, Jules W. wrote:

Hi all,

A new PR that adds methods to PriorityQueue was created some time ago at
https://github.com/openjdk/jdk/pull/6938 but has no corresponding issue. As
I'm not too familiar with this part of the JDK I'm querying this mailing
list for anyone to properly review the PR before I create an issue for it
in the JBS


First you need an issue, then you review a PR. Your PR would not be seen 
by anyone unless they go looking for it, as without the association with 
an issue, the system will not send the email to the mailing lists.


I see someone is helping you create the issue so things should progress 
in that sense. But note the bar for adding to a public API is set very high.


Cheers,
David


best regards,
Julian Waters

Re: Future.isCancelled and thread interruption

[David Holmes]

Hi,

On 1/03/2022 4:17 am, Pushkar N Kulkarni wrote:

"Future.cancel(true)" cancels the receiver Future and also attempts to interrupt the 
executor thread that is running this task. However, not all threads may be interrupted. An 
exception are threads that executing one of the "restart-able blocking system calls" from 
libnet.
Such threads will ignore the thread interrupt(EINTR) and restart the blocking 
system call that was interrupted. See the system calls wrapped in the 
BLOCKING_IO_RETURN_INT macro: 
https://github.com/openjdk/jdk/blob/master/src/java.base/linux/native/libnet/linux_close.c#L352


You are mixing up two completely different notions of "interrupt". 
Thread.interrupt does not "interrupt" system calls - that only happens 
with signals. Thread.interrupt will not unblock low-level blocking I/O 
calls - like a socket connect. Only InterruptibleChannels provide some 
support for Thread.interrupt to break out of the I/O operation.


David
-



The javadoc for "java.util.concurrent.Future.cancel(boolean mayInterruptIfRunning)" DOES clarify that this 
method is an "attempt" to cancel the Future, and that it has no effect if the "task is already completed 
or cancelled, or could not be cancelled for *some other reason*". It is also made clear that "the return 
value from this method does not necessarily indicate whether the task is now cancelled". This is good, in the 
context of this note.

The javadoc also presents "Future.isCancelled()" as a definitive way to test if a Future 
was cancelled. However, it does not comment on the thread interruption attempted by 
Future.cancel(true). This might lead users to assume that if "Future.isCancelled()" 
returns true, the related executor thread was also successfully interrupted. This assumption would 
be invalid if the related executor thread was blocked in one of libnet's restart-able system calls 
(connect() could block for a couple of minutes).

I am attaching a test program that reproduces the mentioned behaviour. The executor 
thread held a lock and it was assumed that when "Future.isCancelled()" returned 
true, the executor had been interrupted and the lock released. In reality, the lock was 
held for a longer time and it blocked the main thread where the invalid assumption was 
made.

I am curious to know what others think of this matter! Any 
help/corrections/opinions will be appreciated. Thank you!

--
import java.net.*;
import java.util.concurrent.*;

public class ConnectionTest {
 private  synchronized Socket connect(String host, int port) throws 
Exception {
 InetSocketAddress address = new InetSocketAddress(host, port);
 Socket s = new Socket();
 s.connect(address); // HERE: s.connect(address, T), with any T>0, 
would resolve the hang!
 return s;
 }

 private Socket connectToMain() throws Exception {
 System.out.println("Connecting to main...");
 return connect("www.google.com", 81);
 }

 private Socket connectToAlternate() throws Exception {
 System.out.println("Connecting to alternate...");
 return connect("www.example.com", 80);
 }

 public void test() throws Exception {
 ExecutorService es = Executors.newFixedThreadPool(1);
 Future f = es.submit(new Callable() {
 public Socket call() throws Exception {
 return connectToMain();
 }
 });
 try {
 f.get(2000, TimeUnit.MILLISECONDS);
 System.out.println("Connected to main!");
 return;
 } catch (TimeoutException e) {
 System.out.println("Connection to main timed out, cancelling the Future 
with mayInterruptIfRunning = true");
 boolean ret = f.cancel(true);
 System.out.println("Future.cancel(true) returned " + ret);
 }
 System.out.println("Is Future canceled? ..." + f.isCancelled());
 if (f.isCancelled()) {
 connectToAlternate();
 System.out.println("Connected to alternate!");
 }
 }

 public static void main(String [] args) throws Exception {
 new ConnectionTest().test();
 }
}
--

-Pushkar

Re: Should System.exit be controlled by a Scope Local?

[David Holmes]

On 28/02/2022 8:20 am, Ethan McCue wrote:
My understanding is that when you System.exit all threads associated 
with the JVM process are killed. That's what I meant by "nuclear 
Thread.interrupt".


The process is terminated, the threads are not individually "killed". 
All Thread.interrupt does is set a flag and unpark blocked threads (in 
some specific cases). There's really no comparison at all.


David
-

It's the same issue as was raised about System.exit implicitly ending 
control flow or implicitly closing open file handles - a process could 
be relying on the behavior of implicitly killing all threads and not 
have another cleanup mechanism


On Sun, Feb 27, 2022, 5:16 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


On 28/02/2022 3:20 am, Ethan McCue wrote:
 > I think continuations could work for the single threaded case,
depending on
 > their behavior with "finally" blocks. I'm sure there are more
caveats once
 > we add another thread to the mix though. System.exit is a nuclear
 > Thread.interrupt, so replicating that behavior might be a bit
daunting

What has Thread.interrupt got to do with System.exit ?

David

 >      private static final class ExitCode {
 >          volatile Integer code = null;
 >      }
 >
 >      private final ScopeLocal EXIT_CODE =
ScopeLocal.newInstance();
 >
 >      public void overridingExitBehavior(IntConsumer exit,
Runnable run) {
 >          var exitCode = new ExitCode();
 >          ScopeLocal.with(EXIT_CODE, exitCode).run(() -> {
 >              // by whatever syntax
 >              var _ = inContinuation(run);
 >              if (exitCode.code != null) {
 >                  exit.accept(code.exitCode)
 >              }
 >          });
 >      }
 >
 >      public void exit(int status) {
 >          if (EXIT_CODE.isBound()) {
 >               EXIT_CODE.get().code = status;
 >               Continuation.yield();
 >          }
 >          else {
 >              Shutdown.exit(status);
 >          }
 >      }
 >
 >
 >
 > On Sun, Feb 27, 2022 at 10:41 AM Glavo mailto:zjx001...@gmail.com>> wrote:
 >
 >> I think there is a problem with this: `System.exit` contains
semantics to
 >> interrupt the flow of control and exit, and if you implement it
that way,
 >> you might have some program abnormally execute parts of it that
should
 >> never be executed.
 >>
 >> Of course, using exceptions like this should solve part of the
problem:
 >>
 >> class Exit extends Error {
 >>      final int exitCode;
 >>      public Exit(int exitCode) {
 >>          this.exitCode = exitCode;
 >>      }
 >>
 >>      @Override
 >>      public synchronized Throwable fillInStackTrace() {
 >>          return this;
 >>      }
 >> }
 >>
 >> try {
 >>    Runtime.getRuntime().overridingExitBehavior(exitCode ->
{throw new
 >> Exit(exitCode);}, ...);
 >> } catch (Exit exit) {
 >>    ...
 >> }
 >>
 >> However, the calling method may catch this exception
unexpectedly, and
 >> there may be unexpected behavior under multithreading.
 >> Of course, this part of the problem also exists for the security
manager.
 >> But, if possible, it would be better to have a solution for these
 >> situations.
 >> (`Continuation` might help us?)
 >>
 >>
 >>
 >> On Sun, Feb 27, 2022 at 11:07 PM Ethan McCue mailto:et...@mccue.dev>> wrote:
 >>
 >>> That undermines my point some, but I think the overall shape of
the use
 >>> case still makes sense
 >>>
 >>> On Sun, Feb 27, 2022 at 8:01 AM Remi Forax mailto:fo...@univ-mlv.fr>> wrote:
 >>>
 >>>> Hi Ethan,
 >>>> there is a far simpler solution, call org.apache.ivy.run(args,
true)
 >>>> instead of org.apache.ivy.main(args) in your tool provider.
 >>>>
 >>>> regards,
 >>>> Rémi
 >>>>
 >>>> - Original Message -
 >>>>> From: "Ethan McCue" mailto:et...@mccue.dev>>
 >>>>> To: "core-libs-dev" mailto:core-libs-dev@openjdk.java.net>>
 >>>>> Sent: Saturday, February 26, 2022 11:14:19 PM
 >>

Re: Should System.exit be controlled by a Scope Local?

[David Holmes]

On 28/02/2022 3:20 am, Ethan McCue wrote:

I think continuations could work for the single threaded case, depending on
their behavior with "finally" blocks. I'm sure there are more caveats once
we add another thread to the mix though. System.exit is a nuclear
Thread.interrupt, so replicating that behavior might be a bit daunting


What has Thread.interrupt got to do with System.exit ?

David


 private static final class ExitCode {
 volatile Integer code = null;
 }

 private final ScopeLocal EXIT_CODE = ScopeLocal.newInstance();

 public void overridingExitBehavior(IntConsumer exit, Runnable run) {
 var exitCode = new ExitCode();
 ScopeLocal.with(EXIT_CODE, exitCode).run(() -> {
 // by whatever syntax
 var _ = inContinuation(run);
 if (exitCode.code != null) {
 exit.accept(code.exitCode)
 }
 });
 }

 public void exit(int status) {
 if (EXIT_CODE.isBound()) {
  EXIT_CODE.get().code = status;
  Continuation.yield();
 }
 else {
 Shutdown.exit(status);
 }
 }



On Sun, Feb 27, 2022 at 10:41 AM Glavo  wrote:


I think there is a problem with this: `System.exit` contains semantics to
interrupt the flow of control and exit, and if you implement it that way,
you might have some program abnormally execute parts of it that should
never be executed.

Of course, using exceptions like this should solve part of the problem:

class Exit extends Error {
 final int exitCode;
 public Exit(int exitCode) {
 this.exitCode = exitCode;
 }

 @Override
 public synchronized Throwable fillInStackTrace() {
 return this;
 }
}

try {
   Runtime.getRuntime().overridingExitBehavior(exitCode -> {throw new
Exit(exitCode);}, ...);
} catch (Exit exit) {
   ...
}

However, the calling method may catch this exception unexpectedly, and
there may be unexpected behavior under multithreading.
Of course, this part of the problem also exists for the security manager.
But, if possible, it would be better to have a solution for these
situations.
(`Continuation` might help us?)



On Sun, Feb 27, 2022 at 11:07 PM Ethan McCue  wrote:


That undermines my point some, but I think the overall shape of the use
case still makes sense

On Sun, Feb 27, 2022 at 8:01 AM Remi Forax  wrote:


Hi Ethan,
there is a far simpler solution, call org.apache.ivy.run(args, true)
instead of org.apache.ivy.main(args) in your tool provider.

regards,
Rémi

- Original Message -

From: "Ethan McCue" 
To: "core-libs-dev" 
Sent: Saturday, February 26, 2022 11:14:19 PM
Subject: Should System.exit be controlled by a Scope Local?



I have a feeling this has been considered and I might just be

articulating

the obvious - but:

As called out in JEP 411, one of the remaining legitimate uses of the
Security Manager is to intercept calls to System.exit. This seems

like a

decent use case for the Scope Local mechanism.


public class Runtime {
...
private final ScopeLocal EXIT =
ScopeLocal.newInstance();

...

public void overridingExitBehavior(IntConsumer exit, Runnable

run) {

ScopeLocal.with(EXIT, exit).run(run);
}

...

public void exit(int status) {
if (EXIT.isBound()) {
EXIT.get().accept(status);
}
else {
Shutdown.exit(status);
}
}
}


One of the likely minor benefits in the scope of things, but related

to

the

parts of the ecosystem I am doodling with so I'll mention it, is that

it

would become possible to wrap "naive" cli programs with the

ToolProvider

SPI without rewriting their code if this System.out, and System.err

all

became reliably configurable.

For instance, Apache Ivy's CLI has a main class that looks like this





https://github.com/apache/ant-ivy/blob/424fa89419147f50a41b4bdc665d8ea92b5da516/src/java/org/apache/ivy/Main.java


package org.apache.ivy;

public final class Main {
...

public static void main(String[] args) throws Exception {
try {
run(args, true);
System.exit(0);
} catch (ParseException ex) {
System.err.println(ex.getMessage());
System.exit(1);
}
}
 }

Making these otherwise static parts of the system configurable would

enable

a third party library to write

public final class IvyToolProvider implements ToolProvider {
@Override
public String name() {
return "ivy";
}

@Override
public int run(PrintWriter out, PrintWriter err, String...

args) {

var exit = new AtomicInteger(0);
Runtime.getRuntime().overridingExitBehavior(exit::set, ()

-> {

System.overridingOut(out, () -> {
 

Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build

[David Holmes]

On Mon, 21 Feb 2022 19:55:14 GMT, Daniel Jeliński  wrote:

> Please review this PR that enables 
> [Zc:strictStrings](https://docs.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170)
>  compiler flag, which makes assigning a string literal to a non-const pointer 
> a compile-time error.
> 
> This type of assignment is [disallowed by C++ standard since 
> C++11](https://en.cppreference.com/w/cpp/language/string_literal). Writing to 
> a string literal through a non-const pointer [produces a run-time 
> error](https://docs.microsoft.com/en-us/cpp/cpp/string-and-character-literals-cpp?view=msvc-170#microsoft-specific-1).
> 
> The included code changes are trivial; I added `const` keyword to variable 
> and parameter declarations where needed, and added explicit casts to 
> non-const pointers where adding `const` was not feasible.
> 
> I verified that the build passes both with and without `--enable-debug`, both 
> with VS2017 and VS2019.

Hi Daniel,

Hotspot changes look fine.

Some of the casts in other code look odd to me, but I'll leave that for the 
security-libs folk to comment on.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

Re: Unused static constant MathContext.DEFAULT_DIGITS

[David Holmes]

On 18/02/2022 7:55 pm, Andrey Turbanov wrote:

Hello.
I noticed unused field java.math.MathContext#DEFAULT_DIGITS
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/math/MathContext.java#L61

As I can see, it was already unused in the initial OpenJDK source.
Does VM use this field somehow? Or used to use?


The VM doesn't use it. Looks like dead code.

Cheers,
David


Andrey Turbanov

Re: RFR: JDK-8281766: Update java.lang.reflect.Parameter to implement Member

[David Holmes]

On Tue, 15 Feb 2022 01:13:43 GMT, Joe Darcy  wrote:

> Retrofitting of Parameter to implement Member, an interface already 
> implemented by similar reflective modeling classes.
> 
> Since Parameter is final, there is little compatibility impact from adding a 
> new method.
> 
> Please also review the corresponding CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8281767
> 
> I'll update the copyright year before pushing; I judged this as trivial 
> enough to not require a regression test.

Technically a constructor is not a Member either, but the reflection API can 
get away with lumping constructors in with methods.

The JLS definition of Member comes from 8.1.6 and 8.2 - the key definition 
being in 8.1.6:

"A class body may contain declarations of members of the class, that is, fields 
(§8.3),
methods (§8.4), classes (§8.5), and interfaces (§8.5)."

-

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

Re: RFR: JDK-8281766: Update java.lang.reflect.Parameter to implement Member

[David Holmes]

Hi Joe,

On 15/02/2022 11:22 am, Joe Darcy wrote:

Retrofitting of Parameter to implement Member, an interface already implemented 
by similar reflective modeling classes.


But a parameter is not a Member!

David


Since Parameter is final, there is little compatibility impact from adding a 
new method.

Please also review the corresponding CSR:

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

I'll update the copyright year before pushing; I judged this as trivial enough 
to not require a regression test.

-

Commit messages:
  - JDK-8281767: Update java.lang.reflect.Parameter to implement Member

Changes: https://git.openjdk.java.net/jdk/pull/7468/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7468=00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8281766
   Stats: 15 lines in 1 file changed: 14 ins; 0 del; 1 mod
   Patch: https://git.openjdk.java.net/jdk/pull/7468.diff
   Fetch: git fetch https://git.openjdk.java.net/jdk pull/7468/head:pull/7468

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

Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc [v2]

[David Holmes]

On Fri, 11 Feb 2022 08:54:51 GMT, Leo Korinth  wrote:

>> Remove unused imports under test/lib and jtreg/gc. They create lots of 
>> warnings if editing using an IDE. Tests in hotspot_gc passed.
>
> Leo Korinth has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   updating copyright

Looks good.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc

[David Holmes]

On Thu, 10 Feb 2022 15:39:53 GMT, Leo Korinth  wrote:

> Remove unused imports under test/lib and jtreg/gc. They create lots of 
> warnings if editing using an IDE. Tests in hotspot_gc passed.

Forgot to mention copyright years need updating before integrating! Thanks.

-

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

Re: RFR: 8281585: Remove unused imports under test/lib and jtreg/gc

[David Holmes]

On Thu, 10 Feb 2022 15:39:53 GMT, Leo Korinth  wrote:

> Remove unused imports under test/lib and jtreg/gc. They create lots of 
> warnings if editing using an IDE. Tests in hotspot_gc passed.

Looks fine. The proof of these changes is in compiling the files - how did you 
test the non-gc-test changes?

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: JDK-8221642: AccessibleObject::setAccessible throws NPE when invoked by JNI code with no java frame on stack

[David Holmes]

On Fri, 28 Jan 2022 17:50:19 GMT, Mandy Chung  wrote:

> `AccessibleObject::setAccessible` and `trySetAccessible` methods should only 
> allow access to public member of a public type that is unconditionally 
> exported consistent with the access check as described in the class 
> specification, when invoked by JNI code with no Java class on the stack.   
> The current implementation throws NPE when finding the module of the caller 
> class as the caller class is null.
> 
> The specification of `canAccess`, `setAccessible` and `trySetAccessible` are 
> updated to specify the behavior when the caller class is null.   I consider 
> this spec update as a clarification as the class specification covers this 
> case.

Apologies as I somehow missed the CSR info above.

-

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