Re: RFR: 8332400: isspace argument should be a valid unsigned char

[David Holmes]

On Mon, 10 Jun 2024 13:36:06 GMT, Robert Toyonaga  wrote:

> But what about when an int is passed to isspace?

The current casts to int are incorrect as a negative value would be 
sign-extended and then fail the range check. I think we have to cast to 
unsigned char in all cases in the caller, which renders the wrapper and 
range-check obsolete AFAICS.

-

PR Comment: https://git.openjdk.org/jdk/pull/19567#issuecomment-2159586765

Integrated: 8330205: Initial troff manpage generation for JDK 24

[David Holmes]

On Mon, 10 Jun 2024 02:31:00 GMT, David Holmes  wrote:

> Sets the version to 24/24-ea and the copyright year to 2025.
> 
> Content changes related to the start of release (e.g. for removed options in 
> java.1) are handled separately.
> 
> This initial generation also picks up the unpublished changes from: 
> 
> - ~~JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC~~ 
> (covered by merge)
> - JDK-8284500: Typo in Supported Named Extensions - BasicContraints
> - JDK-8324342: Doclet should default `@since` for a nested class to that of 
> its enclosing class
> 
> Thanks

This pull request has now been integrated.

Changeset: 3a01b47a
Author:David Holmes 
URL:   
https://git.openjdk.org/jdk/commit/3a01b47ac97714608356ce3faf797c37dc63e9af
Stats: 43 lines in 28 files changed: 2 ins; 3 del; 38 mod

8330205: Initial troff manpage generation for JDK 24

Reviewed-by: alanb, iris

-

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

Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v3]

[David Holmes]

On Mon, 10 Jun 2024 17:27:18 GMT, Iris Clark  wrote:

>> David Holmes has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Regenerated after merge
>
> Marked as reviewed by iris (Reviewer).

Thanks for the review @irisclark .

I've merged in the latest updates that also brought in part of the missing 
changes. If anything goes wrong I have another PR in preparation that also 
updates java.1

-

PR Comment: https://git.openjdk.org/jdk/pull/19617#issuecomment-2159577044

Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v3]

[David Holmes]

> Sets the version to 24/24-ea and the copyright year to 2025.
> 
> Content changes related to the start of release (e.g. for removed options in 
> java.1) are handled separately.
> 
> This initial generation also picks up the unpublished changes from: 
> 
> - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC
> - JDK-8284500: Typo in Supported Named Extensions - BasicContraints
> - JDK-8324342: Doclet should default `@since` for a nested class to that of 
> its enclosing class
> 
> Thanks

David Holmes has updated the pull request incrementally with one additional 
commit since the last revision:

  Regenerated after merge

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19617/files
  - new: https://git.openjdk.org/jdk/pull/19617/files/8472c47d..e7563087

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19617=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=19617=01-02

  Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/19617.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617

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

Re: RFR: 8330205: Initial troff manpage generation for JDK 24 [v2]

[David Holmes]

> Sets the version to 24/24-ea and the copyright year to 2025.
> 
> Content changes related to the start of release (e.g. for removed options in 
> java.1) are handled separately.
> 
> This initial generation also picks up the unpublished changes from: 
> 
> - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC
> - JDK-8284500: Typo in Supported Named Extensions - BasicContraints
> - JDK-8324342: Doclet should default `@since` for a nested class to that of 
> its enclosing class
> 
> Thanks

David Holmes 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
 - 8330205: Initial troff manpage generation for JDK 24

-

Changes: https://git.openjdk.org/jdk/pull/19617/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19617=01
  Stats: 53 lines in 28 files changed: 12 ins; 3 del; 38 mod
  Patch: https://git.openjdk.org/jdk/pull/19617.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617

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

Re: RFR: 8332400: isspace argument should be a valid unsigned char

[David Holmes]

On Fri, 7 Jun 2024 06:07:17 GMT, Thomas Stuefe  wrote:

> "To use these functions safely with plain chars (or signed chars), the 
> argument should first be converted to unsigned char"

@tstuefe wow! Okay. That is a surprise to me. A cast to unsigned char doesn't 
actually do anything. Every char is "representable" as an unsigned char because 
it holds a bit pattern between 0x00 and 0xff i.e. the function is well defined 
if the incoming int is either EOF (int -1) or else in the range 0x00 to 0xff. 
But I did a bit of searching and it seems it comes down to potential arithmetic 
operations on the "char" the might behave differently depending on the 
signed-ness. :(

-

PR Comment: https://git.openjdk.org/jdk/pull/19567#issuecomment-2157677944

Re: RFR: 8330205: Initial troff manpage generation for JDK 24

[David Holmes]

On Mon, 10 Jun 2024 07:15:51 GMT, Alan Bateman  wrote:

>> Sets the version to 24/24-ea and the copyright year to 2025.
>> 
>> Content changes related to the start of release (e.g. for removed options in 
>> java.1) are handled separately.
>> 
>> This initial generation also picks up the unpublished changes from: 
>> 
>> - JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC
>> - JDK-8284500: Typo in Supported Named Extensions - BasicContraints
>> - JDK-8324342: Doclet should default `@since` for a nested class to that of 
>> its enclosing class
>> 
>> Thanks
>
> Marked as reviewed by alanb (Reviewer).

Thanks for the review @AlanBateman !

-

PR Comment: https://git.openjdk.org/jdk/pull/19617#issuecomment-2157606095

RFR: 8330205: Initial troff manpage generation for JDK 24

[David Holmes]

Sets the version to 24/24-ea and the copyright year to 2025.

Content changes related to the start of release (e.g. for removed options in 
java.1) are handled separately.

This initial generation also picks up the unpublished changes from: 

- JDK-8330807: Update Manpage for obsoletion of ScavengeBeforeFullGC
- JDK-8284500: Typo in Supported Named Extensions - BasicContraints
- JDK-8324342: Doclet should default @since for a nested class to that of its 
enclosing class

Thanks

-

Commit messages:
 - 8330205: Initial troff manpage generation for JDK 24

Changes: https://git.openjdk.org/jdk/pull/19617/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19617=00
  Issue: https://bugs.openjdk.org/browse/JDK-8330205
  Stats: 66 lines in 28 files changed: 12 ins; 13 del; 41 mod
  Patch: https://git.openjdk.org/jdk/pull/19617.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19617/head:pull/19617

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

Re: RFR: 8332400: isspace argument should be a valid unsigned char

[David Holmes]

On Wed, 5 Jun 2024 20:08:10 GMT, Robert Toyonaga  wrote:

> ### Summary
> This change ensures we don't get undefined behavior when 
> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html).
>   `isspace` accepts an `int` argument that "the application shall ensure is a 
> character representable as an unsigned char or equal to the value of the 
> macro EOF.". 
> 
> Previously, there was no checking of the values passed to `isspace`. I've 
> replaced direct calls with a new wrapper `os::is_space` that performs a range 
> check and prevents the possibility of undefined behavior from happening. For 
> instances outside of Hotspot, I've added casts to `unsigned char`. 
> 
> **Testing**
> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check 
> `os::is_space` is working correctly.
> - tier1

Sorry I think this is well-intentioned but unnecessary in nearly all cases. If 
you pass a char there is no potential problem. Only passing an actual int could 
be a problem.

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 664:

> 662: char after_key = line[key_len];
> 663: if (strncmp(line, key, key_len) == 0
> 664:   && os::is_space(after_key) != 0

I think this is excessive. The value is a char so cannot be a problem. The only 
calls to isspace that need checking are those that actually pass an int, and 
even then there could be adequate safeguards in place.

src/hotspot/os/linux/os_linux.cpp line 1356:

> 1354:   if (s) {
> 1355: // Skip blank chars
> 1356: do { s++; } while (s && os::is_space(*s));

Again not needed.

-

PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2103234054
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630265925
PR Review Comment: https://git.openjdk.org/jdk/pull/19567#discussion_r1630266490

Re: RFR: 8331977: Crash: SIGSEGV in dlerror()

[David Holmes]

On Mon, 3 Jun 2024 14:38:03 GMT, Alexey Semenyuk  wrote:

>> I am yet to see anything that actually explains the cause of the `dlerror` 
>> crash here ???
>
> @dholmes-ora there is no fix for the cause of the `dlerror` crash in this PR. 
> The PR fixes jpackage tests to rerun a launcher if it crashes. This 
> workaround for jpackage tests was first introduced in 
> [JDK-8269403](https://bugs.openjdk.org/browse/JDK-8269403) fix but 
> MainClassTest was left unfixed back then. This PR complements 
> [JDK-8269403](https://bugs.openjdk.org/browse/JDK-8269403).

@alexeysemenyukoracle But now there is no bug to investigate the dlerrror 
crash! If you wanted to make the test more robust a new JBS issue should have 
been filed for that.

-

PR Comment: https://git.openjdk.org/jdk/pull/19502#issuecomment-2146669916

Re: RFR: 8331977: Crash: SIGSEGV in dlerror()

[David Holmes]

On Fri, 31 May 2024 14:05:07 GMT, Alexey Semenyuk  wrote:

> Fix MainClassTest class to use HelloApp.AppOutputVerifier class to run app 
> launcher instead of raw Executor. This makes MainClassTest test run app 
> launchers with retries. This change addresses the primary issue.
> 
> Fix inconsistencies in HelloApp.AppOutputVerifier class. It used to provide 
> API allowing to run launchers without retries. It inconsistently allowed the 
> execution of launchers with suppressed output (stdout and stderr). It 
> inconsistently executed launchers with/without PATH removed from the 
> environment.
> 
> These loopholes were eliminated:
> 
>  - stdout and stderr of app launchers is never suppressed;
>  - PATH env variable is always deleted for app launchers on Windows. It is 
> not deleted on other platforms. This change sets the correct scope of 
> [JDK-8254920](https://bugs.openjdk.org/browse/JDK-8254920) fix that 
> introduced the removal of PATH env variable for app launchers;
>  - app launchers are always executed with retries unless the launcher is 
> executed with `jpackage.test.noexit` system property set to `true` indicating 
> the test app will not terminate on its own.
> 
> Other changes are due to changes in HelloApp.AppOutputVerifier class.

I am yet to see anything that actually explains the cause of the `dlerror` 
crash here ???

-

PR Comment: https://git.openjdk.org/jdk/pull/19502#issuecomment-2144616488

Re: RFR: 8332494: java/util/zip/EntryCount64k.java failing with java.lang.RuntimeException: '\\A\\Z' missing from stderr [v2]

[David Holmes]

On Mon, 20 May 2024 11:41:48 GMT, Axel Boldt-Christmas  
wrote:

>> test/lib/jdk/test/lib/process/OutputAnalyzer.java line 691:
>> 
>>> 689:  * @throws RuntimeException If the pattern was not found
>>> 690:  */
>>> 691: public OutputAnalyzer 
>>> stderrShouldMatchIgnoreDeprecatedWarnings(String pattern) {
>> 
>> Given we have `...IgnoreVMWarnings` this special case should really be 
>> called `...IgnoreDeprecatedVMWarnings`.
>
> The name was chosen based on: 
> https://github.com/openjdk/jdk/blob/77c8516085225a04bd5a954197fc5ef7e5c5ee61/test/lib/jdk/test/lib/process/OutputAnalyzer.java#L184
> 
> Should I still change it?

No. Sorry I missed the fact the inconsistency had already been introduced. And 
given that function exists we may as well add the new one too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19297#discussion_r1607458481

Re: RFR: 8332494: java/util/zip/EntryCount64k.java failing with java.lang.RuntimeException: '\\A\\Z' missing from stderr [v2]

[David Holmes]

On Mon, 20 May 2024 07:27:19 GMT, Axel Boldt-Christmas  
wrote:

>> Improve `java/util/zip/EntryCount64k.java` stderr parsing by ignoring 
>> deprecated warnings. Testing non-generational ZGC requires the use of a 
>> deprecated option.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update copyright year

looks good. Thanks

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19297#pullrequestreview-2067288156

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[David Holmes]

On 20/05/2024 10:12 pm, Aman Sharma wrote:

Hi David,


 > How did you try to intercept them? Hidden classes are not "loaded" in
the normal sense so won't trigger class load events.


I could not intercept them. I only see them when I pass `-verbose:class` 
in the Java CLI.


Yes that is why I asked how you tried to intercept them.



I also couldn't intercept them using JVMTI Class File Load Hook 
<https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#ClassFileLoadHook> event. However JEP 371 suggests that it should be possible to intercept them using JVMTI Class Load <https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#ClassLoad> event, but I won't have the bytecode at this stage. So is there no way to get its bytecode before it is linked and initialized in the JVM?


Hidden classes are not loaded so I would not expect any class load 
events. However the exact nature of the JVMTI class load event is 
unclear as it talks about "class or interface creation" which is neither 
loading or defining per se. But a class prepare event sounds like it 
should be issued. However neither give you access to the bytecode of the 
class AFAICS.


David
-




Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)
<http://www.kth.se><https://www.kth.se/profile/amansha><https://www.kth.se/profile/amansha>
<https://www.kth.se/profile/amansha>https://algomaster99.github.io/ 
<https://algomaster99.github.io/>

----
*From:* David Holmes 
*Sent:* Monday, May 20, 2024 2:59:17 AM
*To:* Aman Sharma; liangchenb...@gmail.com
*Cc:* core-libs-dev@openjdk.org; leyden-...@openjdk.org
*Subject:* Re: Deterministic naming of subclasses of 
`java/lang/reflect/Proxy`

On 17/05/2024 9:43 pm, Aman Sharma wrote:

Hi Chen,

  > java.lang.invoke.LambdaForm$MH/0x0200cc000400

I do see this as output when I pass -verbose:class. However, based on my 
experiments, I have seen that neither an agent passed via 'javaagent' 
nor an agent passed via 'agentpath' is able to intercept this hidden class.


How did you try to intercept them? Hidden classes are not "loaded" in
the normal sense so won't trigger class load events.

Also, I was a bit confused since I saw somewhere that the names of 
hidden classes are null. But thanks for clarifying here.


The JEP clearly defines the name format for hidden classes - though the
final component is VM specific (and typically a hashcode).

https://openjdk.org/jeps/371 <https://openjdk.org/jeps/371>

Cheers,
David
-


  > avoid dynamic class loading

I don't see dynamic class loading as a problem. I only mind some 
unstable generation aspects of them which make it hard to verify them 
based on an allowlist.


For example, if this hidden class is generated with the exact same name 
and the exact same bytecode during runtime as well, it would be easy to 
verify it. However, I do see the names are based on some sort of memory 
address so and I don't know what bytecode it has so I don't have 
suggestions to make them stable as of now. For Proxy classes, I feel it 
can be addressed unless you disagree or some involved in Project Leyden 
does. :) Thank you for forwarding my mail there.


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
https://algomaster99.github.io/ <https://algomaster99.github.io/> 

<https://algomaster99.github.io/ <https://algomaster99.github.io/>>



*From:* liangchenb...@gmail.com 
*Sent:* Friday, May 17, 2024 1:23:58 pm
*To:* Aman Sharma 
*Cc:* core-libs-dev@openjdk.org ; 
leyden-...@openjdk.org 
*Subject:* Re: Deterministic naming of subclasses of 
`java/lang/reflect/Proxy`


Hi Aman,
For `-verbose:class`, it's a JVM argument instead of a program argument; 
so when you run a java program like `java Main`, you should call it as 
`java -verbose:class Main`.

When done correctly, you should see hidden class outputs like:
[0.032s][info][class,load] 
java.lang.invoke.LambdaForm$MH/0x0200cc000400 source: 
__JVM_LookupDefineClass__
The loading of java.lang.invoke hidden classes requires your program to 
use MethodHandle features, like a lambda.


I think the problem you are exploring, that to avoid dynamic class 
loading and effectively turn Java Platform closed for security, is also 
being accomplished by project Leyden (as I've shared initially); Thus, I 
am forwarding this to leyden-dev instead, so you can see what approach 
Leyden uses to accomplish the same goal as yours.


Regards, Chen Liang

On Fri, May 17, 2024 at 4:40 AM Aman Sharma <mailto:aman...@kth.se <mailto:aman...@kth.se>>> wrote:


 __

 Hi Roger,


 Do you have ideas on how to intercept them

Re: RFR: 8332494: java/util/zip/EntryCount64k.java failing with java.lang.RuntimeException: '\\A\\Z' missing from stderr [v2]

[David Holmes]

On Mon, 20 May 2024 07:27:19 GMT, Axel Boldt-Christmas  
wrote:

>> Improve `java/util/zip/EntryCount64k.java` stderr parsing by ignoring 
>> deprecated warnings. Testing non-generational ZGC requires the use of a 
>> deprecated option.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update copyright year

I'm undecided whether it is really worth adding a new utility function to just 
ignore deprecated warnings, or whether we should just use the existing function 
to ignore all warnings.

If we special case for deprecated warnings then we may want to also add a 
special version of `asLinesWithoutVMWarnings`

test/lib/jdk/test/lib/process/OutputAnalyzer.java line 691:

> 689:  * @throws RuntimeException If the pattern was not found
> 690:  */
> 691: public OutputAnalyzer 
> stderrShouldMatchIgnoreDeprecatedWarnings(String pattern) {

Given we have `...IgnoreVMWarnings` this special case should really be called 
`...IgnoreDeprecatedVMWarnings`.

-

PR Review: https://git.openjdk.org/jdk/pull/19297#pullrequestreview-2065863128
PR Review Comment: https://git.openjdk.org/jdk/pull/19297#discussion_r1606551444

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[David Holmes]

On 17/05/2024 9:43 pm, Aman Sharma wrote:

Hi Chen,

 > java.lang.invoke.LambdaForm$MH/0x0200cc000400

I do see this as output when I pass -verbose:class. However, based on my 
experiments, I have seen that neither an agent passed via 'javaagent' 
nor an agent passed via 'agentpath' is able to intercept this hidden class.


How did you try to intercept them? Hidden classes are not "loaded" in 
the normal sense so won't trigger class load events.


Also, I was a bit confused since I saw somewhere that the names of 
hidden classes are null. But thanks for clarifying here.


The JEP clearly defines the name format for hidden classes - though the 
final component is VM specific (and typically a hashcode).


https://openjdk.org/jeps/371

Cheers,
David
-


 > avoid dynamic class loading

I don't see dynamic class loading as a problem. I only mind some 
unstable generation aspects of them which make it hard to verify them 
based on an allowlist.


For example, if this hidden class is generated with the exact same name 
and the exact same bytecode during runtime as well, it would be easy to 
verify it. However, I do see the names are based on some sort of memory 
address so and I don't know what bytecode it has so I don't have 
suggestions to make them stable as of now. For Proxy classes, I feel it 
can be addressed unless you disagree or some involved in Project Leyden 
does. :) Thank you for forwarding my mail there.


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
https://algomaster99.github.io/ 


*From:* liangchenb...@gmail.com 
*Sent:* Friday, May 17, 2024 1:23:58 pm
*To:* Aman Sharma 
*Cc:* core-libs-dev@openjdk.org ; 
leyden-...@openjdk.org 
*Subject:* Re: Deterministic naming of subclasses of 
`java/lang/reflect/Proxy`


Hi Aman,
For `-verbose:class`, it's a JVM argument instead of a program argument; 
so when you run a java program like `java Main`, you should call it as 
`java -verbose:class Main`.

When done correctly, you should see hidden class outputs like:
[0.032s][info][class,load] 
java.lang.invoke.LambdaForm$MH/0x0200cc000400 source: 
__JVM_LookupDefineClass__
The loading of java.lang.invoke hidden classes requires your program to 
use MethodHandle features, like a lambda.


I think the problem you are exploring, that to avoid dynamic class 
loading and effectively turn Java Platform closed for security, is also 
being accomplished by project Leyden (as I've shared initially); Thus, I 
am forwarding this to leyden-dev instead, so you can see what approach 
Leyden uses to accomplish the same goal as yours.


Regards, Chen Liang

On Fri, May 17, 2024 at 4:40 AM Aman Sharma > wrote:


__

Hi Roger,


Do you have ideas on how to intercept them? My javaagent is not able
to nor a JVMTI agent passed using `agentpath` option. It also does
not seem to show up in logs when I pass `-verbose:class`.


Also, what do you think of renaming the proxy classes as suggested
below?


Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Engineering and Computer Science (EECS)
Department of Theoretical Computer Science (TCS)


https://algomaster99.github.io/


*From:* core-libs-dev mailto:core-libs-dev-r...@openjdk.org>> on behalf of Roger Riggs
mailto:roger.ri...@oracle.com>>
*Sent:* Friday, May 17, 2024 4:57:46 AM
*To:* core-libs-dev@openjdk.org 
*Subject:* Re: Deterministic naming of subclasses of
`java/lang/reflect/Proxy`
Hi Aman,

You may also run into hidden classes (JEP 371: Hidden Classes) that
allow classes to be defined, at runtime, without names.
It has been proposed to use them for generated proxies but that
hasn't been implemented yet.
There are benefits to having nameless classes, because they can't be
referenced by name, only as a capability, they can be better
encapsulated.

fyi, Roger Riggs


On 5/16/24 8:11 AM, Aman Sharma wrote:


Hi,


Thanks for your response, Liang!


> I think you meant CVE-2021-42392 instead of 2022.


Sorry of the error. I indeed meant CVE-2021-42392
.


> Leyden mainly avoids this unstable generation by performing a
training run to collect classes loaded


Would love to know the details of Project Leyden and how they
worked so far to focus on this goal. In our case, the training run
is the test suite.


> GeneratedConstructorAccessor is already retired by JEP 416 [2]
in Java 18


I did see 

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

[David Holmes]

On Mon, 13 May 2024 15:32:27 GMT, Alan Bateman  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - Fix another typo
>>  - Fix typo
>>  - Add more comments
>
> src/hotspot/share/runtime/arguments.cpp line 2271:
> 
>> 2269: } else if (match_option(option, "--illegal-native-access=", 
>> )) {
>> 2270:   if (!create_module_property("jdk.module.illegal.native.access", 
>> tail, InternalProperty)) {
>> 2271: return JNI_ENOMEM;
> 
> I think it would be helpful to get guidance on if this is the right way to 
> add this system property, only because this one not a "module property". The 
> configuration (WriteableProperty + InternalProperty) look right.

So my recollection/understanding is that we use this mechanism to convert 
module-related `--` flags passed to the VM into system properties that the Java 
code can then read, but we set them up such that you are not allowed to specify 
them directly via `-D`. Is the question here whether this new property should 
be in the `jdk.module` namespace?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1600819327

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v4]

[David Holmes]

On Tue, 14 May 2024 18:10:28 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Address review comments
>Improve warning for JNI methods, similar to what's described in JEP 472
>Beef up tests
>  - Address review comments

Hotspot changes look good - notwithstanding discussion about properlty 
namespace placement. Manpage changes also look good.

-

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2056696636

Re: RFR: 8330205: Initial troff manpage generation for JDK 24

[David Holmes]

On Tue, 7 May 2024 11:53:19 GMT, Pavel Rappo  wrote:

> Please review this mechanical change to man pages. This PR should be 
> integrated after https://github.com/openjdk/jdk/pull/18787.

I think we may have to restore this to a standalone start-of-release change 
done after the other start-of-release changes. I had forgotten about the need 
to update the flags sections of java.1

src/java.base/share/man/java.1 line 3856:

> 3854: .SH REMOVED JAVA OPTIONS
> 3855: .PP
> 3856: These \f[V]java\f[R] options have been removed in JDK 24 and using them

This is incorrect. You can't just change 23 to 24 here as the actual set of 
flags listed below will be will be different.

-

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19119#pullrequestreview-2047411800
PR Review Comment: https://git.openjdk.org/jdk/pull/19119#discussion_r1595128505

Re: RFR: 8331518: Tests should not use the "Classpath" exception form of the legal header

[David Holmes]

On Thu, 2 May 2024 05:57:50 GMT, Tobias Hartmann  wrote:

> Removed the Classpath exception from the copyright header of some compiler 
> tests and benchmarks.
> 
> Thanks,
> Tobias

LGTM. Thanks. I'd consider this a trivial fix too.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19047#pullrequestreview-2035042618

Re: RFR: 8330182: Start of release updates for JDK 24 [v2]

[David Holmes]

On Wed, 17 Apr 2024 05:43:12 GMT, Joe Darcy  wrote:

>> Get JDK 24 underway.
>
> Joe Darcy has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Correct release date as observed in review feedback.
>  - Improve javadoc of class file update.

LGTM

Thanks

test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java line 1:

> 1: /*

There are further updates to this test in the pipeline (new deprecated flags in 
23) so you will need to keep updating to reflect that.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18787#pullrequestreview-2016422206
PR Review Comment: https://git.openjdk.org/jdk/pull/18787#discussion_r1575750313

Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v3]

[David Holmes]

On Wed, 17 Apr 2024 06:34:25 GMT, Jan Lahoda  wrote:

>> Consider code like:
>> 
>> public class MainClass {
>> public MainClass() {
>> System.out.println("Constructor called!");
>> }
>> public static void main() {
>> System.out.println("main called!");
>> }
>> }
>> 
>> and compile and run it, with preview enabled, like:
>> 
>> $ javac /tmp/MainClass.java 
>> $ java --enable-preview -classpath /tmp MainClass
>> Constructor called!
>> main called!
>> 
>> 
>> That is wrong, as the `main` method is static, and there is no need to 
>> create a new instance of the class.
>> 
>> The reason is that as launcher attempts to invoke the main method, it goes 
>> in the following order: 1) static-main-with-args; 2) 
>> instance-main-with-args; 3) static-main-without-args; 4) 
>> instance-main-without-args. But, for the instance variants, the code first 
>> creates a new instance of the given class, and only then attempts to lookup 
>> the `main` method, and will pass to the next option when the `main` method 
>> lookup fails. So, when invoking static-main-without-args, the current class 
>> instance may be created for instance-main-with-args, which will then fail 
>> due to the missing `main(String[])` method.
>> 
>> The proposed solution to this problem is to simply first do a lookup for the 
>> `main` method (skipping to the next variant when the given main method does 
>> not exist, without instantiating the current class).
>> 
>> There is also a relatively closely related problem: what happens when the 
>> constructor throws an exception?
>> 
>> public class MainClass {
>> public MainClass() {
>> if (true) throw new RuntimeException();
>> }
>> public void main() {
>> System.out.println("main called!");
>> }
>> }
>> 
>> 
>> when compiled an run, this produces no output whatsoever:
>> 
>> $ javac /tmp/MainClass.java 
>> $ java --enable-preview -classpath /tmp MainClass
>> $
>> 
>> 
>> This is because any exceptions thrown from the constructor are effectively 
>> ignored, and the launcher will continue with the next variant. This seems 
>> wrong - the exception should be printed for the user, like:
>> 
>> $ java --enable-preview -classpath /tmp/ MainClass 
>> Exception in thread "main" java.lang.RuntimeException
>> at MainClass.(MainClass.java:3)
>> 
>> 
>> This patch proposes to do that by not consuming the exceptions thrown from 
>> the constructor, and stop the propagation to the next variant.
>
> Jan Lahoda has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - Reflecting code formatting suggestion.
>  - First lookup the main method, and only then the constructor.
>  - Attempting to solve JDK-8329581 by only ignoring j.l.NoSuchMethodError

Okay on the update I suggested. Can't comment on the overall exception 
behaviour - not sure what is expected/specified.

-

PR Review: https://git.openjdk.org/jdk/pull/18753#pullrequestreview-2005279615

Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]

[David Holmes]

On Tue, 16 Apr 2024 13:26:43 GMT, Jan Lahoda  wrote:

>> src/java.base/share/native/libjli/java.c line 419:
>> 
>>> 417: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray 
>>> mainArgs) {
>>> 418: jmethodID constructor = (*env)->GetMethodID(env, mainClass, 
>>> "", "()V");
>>> 419: CHECK_EXCEPTION_FAIL();
>> 
>> Shouldn't this also not be checked until you know you have an instance 
>> method?
>
> You mean to first check whether the method exists, and only then try to 
> lookup the constructor? Not sure if that has observable effects, but I can do 
> that.
> 
> I missed [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581) before, 
> my plan is to adjust this "check for exception and clear it" check to only 
> work for `NoSuchMethodError`, and let the exception pass for any other 
> exception.

Yes that is what I meant. If the main method is static then the class need not 
have any such constructor (unless that is part of the spec for this 
functionality?).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1568370439

Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]

[David Holmes]

On Mon, 15 Apr 2024 07:36:05 GMT, Jan Lahoda  wrote:

>> Consider code like:
>> 
>> public class MainClass {
>> public MainClass() {
>> System.out.println("Constructor called!");
>> }
>> public static void main() {
>> System.out.println("main called!");
>> }
>> }
>> 
>> and compile and run it, with preview enabled, like:
>> 
>> $ javac /tmp/MainClass.java 
>> $ java --enable-preview -classpath /tmp MainClass
>> Constructor called!
>> main called!
>> 
>> 
>> That is wrong, as the `main` method is static, and there is no need to 
>> create a new instance of the class.
>> 
>> The reason is that as launcher attempts to invoke the main method, it goes 
>> in the following order: 1) static-main-with-args; 2) 
>> instance-main-with-args; 3) static-main-without-args; 4) 
>> instance-main-without-args. But, for the instance variants, the code first 
>> creates a new instance of the given class, and only then attempts to lookup 
>> the `main` method, and will pass to the next option when the `main` method 
>> lookup fails. So, when invoking static-main-without-args, the current class 
>> instance may be created for instance-main-with-args, which will then fail 
>> due to the missing `main(String[])` method.
>> 
>> The proposed solution to this problem is to simply first do a lookup for the 
>> `main` method (skipping to the next variant when the given main method does 
>> not exist, without instantiating the current class).
>> 
>> There is also a relatively closely related problem: what happens when the 
>> constructor throws an exception?
>> 
>> public class MainClass {
>> public MainClass() {
>> if (true) throw new RuntimeException();
>> }
>> public void main() {
>> System.out.println("main called!");
>> }
>> }
>> 
>> 
>> when compiled an run, this produces no output whatsoever:
>> 
>> $ javac /tmp/MainClass.java 
>> $ java --enable-preview -classpath /tmp MainClass
>> $
>> 
>> 
>> This is because any exceptions thrown from the constructor are effectively 
>> ignored, and the launcher will continue with the next variant. This seems 
>> wrong - the exception should be printed for the user, like:
>> 
>> $ java --enable-preview -classpath /tmp/ MainClass 
>> Exception in thread "main" java.lang.RuntimeException
>> at MainClass.(MainClass.java:3)
>> 
>> 
>> This patch proposes to do that by not consuming the exceptions thrown from 
>> the constructor, and stop the propagation to the next variant.
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reflecting review feedback.

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

> 417: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray 
> mainArgs) {
> 418: jmethodID constructor = (*env)->GetMethodID(env, mainClass, 
> "", "()V");
> 419: CHECK_EXCEPTION_FAIL();

Shouldn't this also not be checked until you know you have an instance method?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1567092479

Re: RFR: 8329596: Add test for virtual threads invoking synchronized native methods

[David Holmes]

On Fri, 5 Apr 2024 05:17:44 GMT, Alan Bateman  wrote:

>> test/jdk/java/lang/Thread/virtual/libSynchronizedNative.c line 29:
>> 
>>> 27: Java_SynchronizedNative_runWithSynchronizedNative(JNIEnv *env, jobject 
>>> obj, jobject task) {
>>> 28: jclass clazz = (*env)->GetObjectClass(env, obj);
>>> 29: jmethodID mid = (*env)->GetMethodID(env, clazz, "run", 
>>> "(Ljava/lang/Runnable;)V");
>> 
>> Does this generate a warning with `-Xcheck:jni` due to exceptions not being 
>> checked for?
>
>> Does this generate a warning with `-Xcheck:jni` due to exceptions not being 
>> checked for?
> 
> I run with`TEST_OPTS_JAVA_OPTIONS=-Xcheck:jni` and don't see any warnings. 
> Are you asking about the usage of GetObjectClass (which is not documented to 
> throw) or GetMethodID (the check for the jmethodID of NULL is at L30). Or 
> maybe you are saying that GetMethodID can return a jmethodID with a pending 
> exception?

Sorry I was thinking `GetObjectClass` could throw, but it can't.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18600#discussion_r1552955426

Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v2]

[David Holmes]

On Tue, 2 Apr 2024 02:16:07 GMT, David Holmes  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Use non-sse fill (old left in)
>
> This looks like it is still a Draft/work-in-progress. There is only code for 
> x64 and it doesn't appear it will build on other platforms. Also there are 
> still a bunch of `if 0` in the code that should not be there.

> @dholmes-ora Sorry for the dead code left in. It is gone now. Plus, this was 
> only requested for x86, thus no implementation for other platforms.

Only requested by whom? The JBS issue says nothing about that.

I'm not even sure how this avoids the  `CheckIntrinsics` check for missing 
intrinsics ... I guess it must only look for some kind of declaration not an 
actual implementation.

-

PR Comment: https://git.openjdk.org/jdk/pull/18555#issuecomment-2038941462

Re: RFR: 8329596: Add test for virtual threads invoking synchronized native methods

[David Holmes]

On Wed, 3 Apr 2024 10:52:10 GMT, Alan Bateman  wrote:

> This is a test-only addition to add a test for virtual threads invoking a 
> synchronized native method and invoking a native method that enter/exits a 
> monitor with JNI MonitorEnter/MonitorExit. The test has been in the loom repo 
> for some time, it can move to the main line in advance of changes to the 
> object monitor implementation.

test/jdk/java/lang/Thread/virtual/libSynchronizedNative.c line 29:

> 27: Java_SynchronizedNative_runWithSynchronizedNative(JNIEnv *env, jobject 
> obj, jobject task) {
> 28: jclass clazz = (*env)->GetObjectClass(env, obj);
> 29: jmethodID mid = (*env)->GetMethodID(env, clazz, "run", 
> "(Ljava/lang/Runnable;)V");

Does this generate a warning with `-Xcheck:jni` due to exceptions not being 
checked for?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18600#discussion_r1552648444

Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v2]

[David Holmes]

On Mon, 1 Apr 2024 21:30:19 GMT, Scott Gibbons  wrote:

>> This code makes an intrinsic stub for `Unsafe::setMemory`.  See [this 
>> PR](https://github.com/openjdk/jdk/pull/16760) for discussion around this 
>> change.
>> 
>> Overall, making this an intrinsic improves overall performance of 
>> `Unsafe::setMemory` by up to 4x for all buffer sizes.
>> 
>> Tested with tier-1 (and full CI).  I've added a table of the before and 
>> after numbers for the JMH I ran (`MemorySegmentZeroUnsafe`).
>> 
>> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt)
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use non-sse fill (old left in)

This looks like it is still a Draft/work-in-progress. There is only code for 
x64 and it doesn't appear it will build on other platforms. Also there are 
still a bunch of `if 0` in the code that should not be there.

-

PR Review: https://git.openjdk.org/jdk/pull/18555#pullrequestreview-1972492070

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v2]

[David Holmes]

On Thu, 21 Mar 2024 14:40:37 GMT, Adam Sotona  wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
>> additional class checks inspired by 
>> `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 26 commits:
> 
>  - Merge remote-tracking branch 'openjdk/master' into 
> JDK-8320396-verifier-extension
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - removed string templates from test
>  - work in progress
>  - work in progress
>  - work in progress
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/e41bc42d...54c4e9b9

@asotona  pardon my ignorance of the Classfile API usage, but I had thought 
that the API could be used to either write the bytecode representation of 
class, or else introspect on an existing class that has already been loaded. So 
I'm not clear at what point you would run these JVMS defined structural 
verification checks that you are adding?

-

PR Comment: https://git.openjdk.org/jdk/pull/16809#issuecomment-2014195638

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v2]

[David Holmes]

On Thu, 21 Mar 2024 14:40:37 GMT, Adam Sotona  wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
>> additional class checks inspired by 
>> `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 26 commits:
> 
>  - Merge remote-tracking branch 'openjdk/master' into 
> JDK-8320396-verifier-extension
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - removed string templates from test
>  - work in progress
>  - work in progress
>  - work in progress
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/e41bc42d...54c4e9b9

src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
 line 56:

> 54: 
> 55: /**
> 56:  * @see  href="https://raw.githubusercontent.com/openjdk/jdk/master/src/hotspot/share/classfile/classFileParser.cpp;>hotspot/share/classfile/classFileParser.cpp

A reference to JVMS Ch4(?) would be appropriate here as that is what any checks 
should be compliant with.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1534955072

Re: RFR: 8320396: Class-File API ClassModel::verify should include checks from hotspot/share/classfile/classFileParser.cpp [v2]

[David Holmes]

On Thu, 21 Mar 2024 14:40:37 GMT, Adam Sotona  wrote:

>> ClassFile API `jdk.internal.classfile.verifier.VerifierImpl` performed only 
>> bytecode-level class verification.
>> This patch adds `jdk.internal.classfile.verifier.ParserVerifier` with 
>> additional class checks inspired by 
>> `hotspot/share/classfile/classFileParser.cpp`.
>> 
>> Also new `VerifierSelfTest::testParserVerifier` has been added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 26 commits:
> 
>  - Merge remote-tracking branch 'openjdk/master' into 
> JDK-8320396-verifier-extension
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - work in progress
>  - removed string templates from test
>  - work in progress
>  - work in progress
>  - work in progress
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/e41bc42d...54c4e9b9

> class checks inspired by hotspot/share/classfile/classFileParser.cpp

Just to be clear any such checks are mandated by the JVMS

-

PR Comment: https://git.openjdk.org/jdk/pull/16809#issuecomment-2014188184

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v15]

[David Holmes]

On Thu, 21 Mar 2024 23:38:30 GMT, Brent Christian  wrote:

>> Classes in the `java.lang.ref` package would benefit from an update to bring 
>> the spec in line with how the VM already behaves. The changes would focus on 
>> _happens-before_ edges at some key points during reference processing.
>> 
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing 
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the 
>> registered cleaning action.
>> 
>> This will bring Cleaner in line (or close) with the memory visibility 
>> guarantees made for finalizers in [JLS 
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object 
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Elaborate on 'surprising and undesirable effects' in reachabilityFence()

src/java.base/share/classes/java/lang/ref/Reference.java line 576:

> 574:  * detects that there is no further need for an object. The garbage 
> collector
> 575:  * may reclaim an object even if values from that object's fields 
> are still
> 576:  * in use, or while a method on the object is still running, so long 
> as the

Suggestion: method _of_ the object

src/java.base/share/classes/java/lang/ref/Reference.java line 579:

> 577:  * object has otherwise become unreachable.
> 578:  * 
> 579:  * This may have surprising and undesirable effects, in particular 
> when using

Nit: I don't think a new paragraph is appropriate here as "This may ..." refers 
back to the subject of the preceding lines.

src/java.base/share/classes/java/lang/ref/Reference.java line 581:

> 579:  * This may have surprising and undesirable effects, in particular 
> when using
> 580:  * a Cleaner or finalizer for cleanup. If an object becomes 
> unreachable while
> 581:  * a method on the object is running, it can lead to a race between 
> the

Again suggest: method _of_ the object

src/java.base/share/classes/java/lang/ref/Reference.java line 585:

> 583:  * Cleaner or finalizer. For instance, the cleanup thread could 
> cleanup the
> 584:  * resource, followed by the program thread (still running the 
> method)
> 585:  * attempting to access the now-already-freed resource.

suggestion: s/cleanup/free/ to match the `now-already-freed` part.

src/java.base/share/classes/java/lang/ref/Reference.java line 586:

> 584:  * resource, followed by the program thread (still running the 
> method)
> 585:  * attempting to access the now-already-freed resource.
> 586:  * {@code reachabilityFence} can prevent this race by ensuring that 
> the

Suggestion: `Use of {@code reachabilityFence ...`

This avoids starting a sentence with a code font word that starts with a 
lower-case letter.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1534946886
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1534947996
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1534948233
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1534948894
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1534949965

Re: RFR: 8327176: UnreferencedExecutor.java can fail on libgraal with -Xcomp

[David Holmes]

On Sun, 3 Mar 2024 17:01:53 GMT, Doug Simon  wrote:

> The `java/util/concurrent/Executors/UnreferencedExecutor.java` test can fail 
> when run on libgraal and `-Xcomp` is specified. The problem is that libgraal 
> in `-Xcomp` temporarily causes some extra memory pressure (probably due to 
> [JDK-8310218](https://bugs.openjdk.org/browse/JDK-8310218)) which can cause 
> recoverable OOMEs to occur (memory is recovered when the relevant libgraal 
> compilations complete). It also seems related to async threads used for 
> cleaning weak references when using G1 or ZGC as I cannot reproduce the 
> failure under `-XX:+UseSerialGC`.
> 
> Installing a global `Thread.UncaughtExceptionHandler` that ignores 
> `OutOfMemoryError`s resolves the problem.

Changes requested by dholmes (Reviewer).

test/jdk/java/util/concurrent/Executors/UnreferencedExecutor.java line 58:

> 56: throw new RuntimeException(e);
> 57: }
> 58: }

I'm not clear exactly what you are trying to achieve with this - is it just to 
not print the stacktrace when OOME occurs?

A UEH should not rethrow exceptions as they are effectively ignored as we are 
already at the final level of handling exceptions (the VM will print a one line 
warning if a UEH itself throws). If I understand what you want then I think the 
following would be more correct:

// Ignore OOME but do default handling for any other uncaught exception.
public void uncaughtException(Thread t, Throwable e) {
if ( ! e instanceof OutOfMemoryError) {
System.err.print("Exception in thread "" + t.getName() + "" ");
e.printStackTrace(System.err);
}
}


This doesn't stop the thread from terminating of course - the OOME already 
caused it to unwind its stack if we get this far.

-

PR Review: https://git.openjdk.org/jdk/pull/18098#pullrequestreview-1950797938
PR Review Comment: https://git.openjdk.org/jdk/pull/18098#discussion_r1533273137

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v13]

[David Holmes]

On Tue, 19 Mar 2024 16:38:49 GMT, Y. Srinivas Ramakrishna  
wrote:

>> Sorry, my use of words was sloppy here. I think I did mean loose or somewhat 
>> informal and therefore slippery.
>> 
>> What I was saying is that using terms such as "any continuing computation" 
>> doesn't make sense because this is referring to a current state of the 
>> computation. I'm not sure what "any continuing computation" from a state is 
>> because the concept of what constitutes the notion of "a continuing 
>> computation" has not been defined before. To me it sounds like a computation 
>> tree with nodes as state and transitions as edges and a continuing 
>> computation as a path through that tree into the future. The way it is 
>> written then, it sounds to the naive reader, or to me at least, as if the 
>> object is perpetually reachable by every thread always. I assume I am 
>> misinterpreting the intention of the writing, but it sounds too loose for a 
>> definition being invoked here in the javadoc. May be it can be tightened up 
>> a bit.
>> 
>> Could one state instead that "An object is reachable at a given state when 
>> some thread is able to access the object through a sequence of steps 
>> starting at that state without other threads taking any steps."  ? Or 
>> something along those lines? Or at least something tighter than the current 
>> wording that is somewhat too loose.
>
> In fact, it appears as if the problem is with the use of "any", which is 
> universal in strength, whereas the intention here is existential in strength 
> (as suggested by. my wording). Indeed, you might achieve the same effect by 
> replacing "any" with "some" so that:
> 
> "An object is reachable if it can be accessed in some continuing computation 
> from some live thread."
> 
> You needn't even say live because dead threads can neither take steps nor 
> continue participating in the computation nor can they "access" objects for 
> whatever informal notion of access. The "some continuing computation" 
> subsumes "potential" (as in a possible future) so potential can be dropped.

I think you are overthinking this somewhat Ramki. I don't see a practical (non 
discrete-math) distinction between "some" and "any", so would not object to 
that single word change if it helps. But "potential" should remain as it covers 
branching in the program whereby if we proceed down one branch an object 
remains reachable, whereas if we precede down another then it may not.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1531191269

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v13]

[David Holmes]

On Tue, 19 Mar 2024 00:40:02 GMT, Y. Srinivas Ramakrishna  
wrote:

>> Brent Christian has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   further tweaks to reachability
>
> src/java.base/share/classes/java/lang/ref/package-info.java line 137:
> 
>> 135:  *
>> 136:  * A reachable object is any object that can be accessed in 
>> any potential
>> 137:  * continuing computation from any live thread (as stated in {@jls 
>> 12.6.1}).
> 
> This seems like somewhat loose and sloppy wording to me. "Any potential 
> continuing computation"? "Any live thread"? Could you share a pointer to JLS 
> 12.6.1 being referenced here?

https://docs.oracle.com/javase/specs/jls/se21/html/jls-12.html#jls-12.6.1

> A reachable object is any object that can be accessed in any potential 
> continuing computation from any live thread. 

It may be "loose" because the devil is in the details when it comes to 
reachability, but I disagree that it is "sloppy". This expresses reachability 
in simple terms, as a "first-order" or "Newtonian" model. There are of course 
"Quantum" effects that need to be dealt with in practice. The JLS alludes to 
this with:
> Optimizing transformations of a program can be designed that reduce the 
> number of objects that are reachable to be less than those which would 
> naively be considered reachable.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1529617062

Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]

[David Holmes]

On Tue, 12 Mar 2024 13:55:11 GMT, Magnus Ihse Bursie  wrote:

>> test/hotspot/jtreg/runtime/ErrorHandling/libTestDwarfHelper.h line 24:
>> 
>>> 22:  */
>>> 23: 
>>> 24: #include 
>> 
>> Seems unneeded.
>
> So what I did here was change:
> 
> #include "jni.h"
> #include 
> 
> 
> into 
> 
> #include 
> 
> #include "export.h"
> #include "jni.h"
> 
> 
> The reordering was strictly not needed, but putting user includes in front of 
> system ones looked like bad coding to me, and put me in a difficult spot in 
> where to add the new `#include "export.h"` -- next to the current user 
> include even if I thought that was wrong, or have the system includes 
> "sandwitched" between two user includes.
> 
> Or do you mean that it seems unneeded to include `jni.h` at all? Yes, I 
> agree, but it was there before, and I don't want to make any other changes to 
> the tests. This change is scary enough as it is :-). If you want, I can file 
> a follow-up to remove this include instead.

Yes I meant the include is actually unnecessary, but fine to not make that 
change here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1522659393

Re: RFR: 8327786: Add fluent setAccessible()

[David Holmes]

On Thu, 25 Jan 2024 21:35:45 GMT, Sergey  wrote:

> The feature allows to extract a private field value in a single expression, 
> like so:
> 
> object.getClass().getDeclaredField().setAccessible().get(object)

src/java.base/share/classes/java/lang/reflect/Field.java line 184:

> 182:  * @throws InaccessibleObjectException {@inheritDoc}
> 183:  * @throws SecurityException {@inheritDoc}
> 184:  */

Inherits from what?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17578#discussion_r1520897359

Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]

[David Holmes]

On Wed, 6 Mar 2024 13:43:00 GMT, Magnus Ihse Bursie  wrote:

>> Currently, our symbol visibility handling for tests are sloppy; we only 
>> handle it properly on Windows. We need to bring it up to the same levels as 
>> product code. This is a prerequisite for 
>> [JDK-8327045](https://bugs.openjdk.org/browse/JDK-8327045), which in turn is 
>> a building block for Hermetic Java.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update line number for dereference_null  in TestDwarf

Looks like a nice cleanup - great to see all the duplicated EXPORT code gone!

test/hotspot/jtreg/runtime/ErrorHandling/libTestDwarfHelper.h line 24:

> 22:  */
> 23: 
> 24: #include 

Seems unneeded.

test/hotspot/jtreg/runtime/ErrorHandling/libTestDwarfHelper.h line 27:

> 25: 
> 26: #include "export.h"
> 27: #include "jni.h"

Seems unneeded.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18135#pullrequestreview-1926822024
PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1519075894
PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1519076039

Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v4]

[David Holmes]

On Thu, 7 Mar 2024 08:16:26 GMT, Matthias Baesken  wrote:

>> We define the RESTARTABLE macro again and again at a lot of places in the 
>> JDK native codebase. This could be centralized to avoid repeating it again 
>> and again !
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   adjust COPYRIGHT year info

LGTM. Thanks

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18132#pullrequestreview-1922263102

Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]

[David Holmes]

On Thu, 7 Mar 2024 03:00:11 GMT, Guoxiong Li  wrote:

>> Matthias Baesken has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Introduce windows jni_util_md.h
>
> src/java.base/aix/native/libnio/ch/Pollset.c line 3:
> 
>> 1: /*
>> 2:  * Copyright (c) 2008, 2024, Oracle and/or its affiliates. All rights 
>> reserved.
>> 3:  * Copyright (c) 2012, 2024 SAP SE. All rights reserved.
> 
> It seems you only need to update the copyright of the company you work for.

Oracle requests/requires that the Oracle copyright always be updated when a 
file is modified.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1515660944

Re: RFR: 8327138: Clean up status management in cdsConfig.hpp and CDS.java [v3]

[David Holmes]

On Wed, 6 Mar 2024 23:36:09 GMT, Ioi Lam  wrote:

>> A few clean ups:
>> 
>> 1. Rename functions like "`s_loading_full_module_graph()` to 
>> `is_using_full_module_graph()`. The meaning of "loading" is not clear: it 
>> might be interpreted as to cover only the period where the artifact is being 
>> loaded, but not the period after the artifact is completely loaded. However, 
>> the function is meant to cover both periods, so "using" is a more precise 
>> term.
>> 
>> 2. The cumbersome sounding `disable_loading_full_module_graph()` is changed 
>> to `stop_using_full_module_graph()`, etc.
>> 
>> 3. The status of `is_using_optimized_module_handling()` is moved from 
>> metaspaceShared.hpp to cdsConfig.hpp, to be consolidated with other types of 
>> CDS status.
>> 
>> 4. The status of CDS was communicated to the Java class 
>> `jdk.internal.misc.CDS` by ad-hoc native methods. This is now changed to a 
>> single method, `CDS.getCDSConfigStatus()` that returns a bit field. That way 
>> we don't need to add a new native method for each type of status.
>> 
>> 5. `CDS.isDumpingClassList()` was a misnomer. It's changed to 
>> `CDS.isLoggingLambdaFormInvokers()`.
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   more alignment

Some drive-by nits. (But copyright needs fixing.)

src/hotspot/share/cds/cdsConfig.cpp line 2:

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

Incorrect copyright update - should be "2023, 2024,"

src/hotspot/share/cds/cdsConfig.cpp line 54:

> 52:  (is_dumping_static_archive()? IS_DUMPING_STATIC_ARCHIVE 
> : 0) |
> 53:  (is_logging_lambda_form_invokers()  ? 
> IS_LOGGING_LAMBDA_FORM_INVOKERS : 0) |
> 54:  (is_using_archive() ? IS_USING_ARCHIVE : 0);

You can remove one space before the ? in each line

src/hotspot/share/cds/cdsConfig.hpp line 56:

> 54:   static const int IS_DUMPING_STATIC_ARCHIVE  = 1 << 1;
> 55:   static const int IS_LOGGING_LAMBDA_FORM_INVOKERS= 1 << 2;
> 56:   static const int IS_USING_ARCHIVE   = 1 << 3;

why is the = sign so far away?

-

PR Review: https://git.openjdk.org/jdk/pull/18095#pullrequestreview-1921571088
PR Review Comment: https://git.openjdk.org/jdk/pull/18095#discussion_r1515626568
PR Review Comment: https://git.openjdk.org/jdk/pull/18095#discussion_r1515628561
PR Review Comment: https://git.openjdk.org/jdk/pull/18095#discussion_r1515629728

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]

[David Holmes]

On Tue, 5 Mar 2024 14:19:14 GMT, Erik Joelsson  wrote:

> Does the release note also looks ok?

Yes.

> Is there any process to formally review release notes?

No. Typically just add a comment to the RN sub-task indicating it is okay. My 
comment, that I had made some minor changes was meant as an indicator of that.

-

PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1980229076

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]

[David Holmes]

On Mon, 4 Mar 2024 23:44:14 GMT, Erik Joelsson  wrote:

>> Executables and dynamic libraries on Linux can encode a search path that the 
>> dynamic linker will use when looking up library dependencies, generally 
>> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature 
>> to set search paths relative to the location of the binary itself. Typically 
>> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find 
>> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to 
>> find each other.
>> 
>> There are two different types of such rpaths, RPATH and RUNPATH. The former 
>> is the earlier incantation but RUNPATH has been around since about 2003 and 
>> has been default in prominent Linux distros for a long time, and now also 
>> seems to be default in the linker directly from binutils. The toolchain used 
>> by Oracle defaulted to RPATH until at least JDK 11, but since then with some 
>> toolchain upgrade, the default was flipped to RUNPATH.
>> 
>> The main (relevant in this case) difference between the two is that RPATH is 
>> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is 
>> only considered after LD_LIBRARY_PATH. For libraries that are part of a 
>> Linux distribution, letting users, or the system, control and override 
>> builtin rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. 
>> However, for the JDK, there really is no usecase for having an externally 
>> configured LD_LIBRARY_PATH potentially getting in the way of the JDK 
>> libraries finding each other correctly. If a user environment sets 
>> LD_LIBRARY_PATH, and there is a library in that path with the same name as a 
>> JDK library (e.g. libnet.so or some other generically named library) that 
>> external library will be loaded instead of the JDK internal library and that 
>> is basically guaranteed to break the JDK. There is no supported usecase that 
>> I can think of for injecting other versions of such libraries in a JDK 
>> distribution.
>> 
>> I propose that we explicitly configure the JDK build to set RPATH instead of 
>> RUNPATH for Linux binaries. This is done with the linker flag 
>> "--disable-new-dtags".
>
> Erik Joelsson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   bug ref

Thanks for the further explanation and adding the comment.

LGTM.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18050#pullrequestreview-1916027301

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v2]

[David Holmes]

On Fri, 1 Mar 2024 13:58:08 GMT, Erik Joelsson  wrote:

>> Executables and dynamic libraries on Linux can encode a search path that the 
>> dynamic linker will use when looking up library dependencies, generally 
>> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature 
>> to set search paths relative to the location of the binary itself. Typically 
>> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find 
>> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to 
>> find each other.
>> 
>> There are two different types of such rpaths, RPATH and RUNPATH. The former 
>> is the earlier incantation but RUNPATH has been around since about 2003 and 
>> has been default in prominent Linux distros for a long time, and now also 
>> seems to be default in the linker directly from binutils. The toolchain used 
>> by Oracle defaulted to RPATH until at least JDK 11, but since then with some 
>> toolchain upgrade, the default was flipped to RUNPATH.
>> 
>> The main (relevant in this case) difference between the two is that RPATH is 
>> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is 
>> only considered after LD_LIBRARY_PATH. For libraries that are part of a 
>> Linux distribution, letting users, or the system, control and override 
>> builtin rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. 
>> However, for the JDK, there really is no usecase for having an externally 
>> configured LD_LIBRARY_PATH potentially getting in the way of the JDK 
>> libraries finding each other correctly. If a user environment sets 
>> LD_LIBRARY_PATH, and there is a library in that path with the same name as a 
>> JDK library (e.g. libnet.so or some other generically named library) that 
>> external library will be loaded instead of the JDK internal library and that 
>> is basically guaranteed to break the JDK. There is no supported usecase that 
>> I can think of for injecting other versions of such libraries in a JDK 
>> distribution.
>> 
>> I propose that we explicitly configure the JDK build to set RPATH instead of 
>> RUNPATH for Linux binaries. This is done with the linker flag 
>> "--disable-new-dtags".
>
> Erik Joelsson has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use @requires in test

I find it somewhat odd that we seem to be add odds with the general programming 
community when it comes to this behaviour. Giving precedence to 
`LD_LIBRARY_PATH` is the new behaviour, enabled via `--enable-new-dtags`; and 
at some point this was made the default behaviour so it must be considered 
generally desirable. But we have now decided we do not want this behaviour so 
we have to explicitly disable it via `--disable-new-dtags`.

make/autoconf/flags-cflags.m4 line 40:

> 38: # Default works for linux, might work on other platforms as well.
> 39: SHARED_LIBRARY_FLAGS='-shared'
> 40: SET_EXECUTABLE_ORIGIN='-Wl,-rpath,\$$ORIGIN[$]1 
> -Wl,--disable-new-dtags'

The reason we use `--disable-new-dtags` needs to be documented somewhere.

-

PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1975375484
PR Review Comment: https://git.openjdk.org/jdk/pull/18050#discussion_r1510411289

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries

[David Holmes]

On Wed, 28 Feb 2024 19:29:13 GMT, Erik Joelsson  wrote:

> Executables and dynamic libraries on Linux can encode a search path that the 
> dynamic linker will use when looking up library dependencies, generally 
> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature to 
> set search paths relative to the location of the binary itself. Typically 
> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find 
> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to 
> find each other.
> 
> There are two different types of such rpaths, RPATH and RUNPATH. The former 
> is the earlier incantation but RUNPATH has been around since about 2003 and 
> has been default in prominent Linux distros for a long time, and now also 
> seems to be default in the linker directly from binutils. The toolchain used 
> by Oracle defaulted to RPATH until at least JDK 11, but since then with some 
> toolchain upgrade, the default was flipped to RUNPATH.
> 
> The main (relevant in this case) difference between the two is that RPATH is 
> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is 
> only considered after LD_LIBRARY_PATH. For libraries that are part of a Linux 
> distribution, letting users, or the system, control and override builtin 
> rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. However, 
> for the JDK, there really is no usecase for having an externally configured 
> LD_LIBRARY_PATH potentially getting in the way of the JDK libraries finding 
> each other correctly. If a user environment sets LD_LIBRARY_PATH, and there 
> is a library in that path with the same name as a JDK library (e.g. libnet.so 
> or some other generically named library) that external library will be loaded 
> instead of the JDK internal library and that is basically guaranteed to break 
> the JDK. There is no supported usecase that I can think of for injecting 
> other versions of such libraries in a JDK distribution.
> 
> I propose that we explicitly configure the JDK build to set RPATH instead of 
> RUNPATH for Linux binaries. This is done with the linker flag 
> "--disable-new-dtags".

test/jdk/tools/launcher/RunpathTest.java line 27:

> 25:  * @test
> 26:  * @bug 7190813 8022719
> 27:  * @summary Check for extended RPATHs on Linux

Pre-existing but really the restriction to Linux should be via `@requires` not 
a runtime test.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18050#discussion_r1508535229

Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries

[David Holmes]

On Wed, 28 Feb 2024 19:29:13 GMT, Erik Joelsson  wrote:

> There is no supported usecase that I can think of for injecting other 
> versions of such libraries in a JDK distribution.

I can imagine it could be used to allow "hot patching" of the installed 
JDK/JRE. Whether anyone has ever needed to do so is another matter. I suggest 
at least adding a Release Note.

-

PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1970497315

Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]

[David Holmes]

On Thu, 22 Feb 2024 16:01:19 GMT, Matthias Baesken  wrote:

>> Currently assertEquals has in the failure case sometimes confusing output 
>> like :
>> 
>> java.lang.RuntimeException: VM output should contain exactly one RTM locking 
>> statistics entry for method 
>> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test: expected 0 to equal 1
>>at jdk.test.lib.Asserts.fail(Asserts.java:634)
>>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
>> 
>> (I don't think we really expected that for some reason 0 equals 1)
>> This should be improved.
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   assertEquals adjust output

Does "expected 0 to equal 1" really cause that much consternation? I just read 
it as "expected 0 to be equal to 1".

Aren't there existing test libraries that already "standardise" these kinds of 
utilities that we can emulate? testng? junit?

-

PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1963396567

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]

[David Holmes]

On Thu, 22 Feb 2024 01:42:17 GMT, Brent Christian  wrote:

>> Classes in the `java.lang.ref` package would benefit from an update to bring 
>> the spec in line with how the VM already behaves. The changes would focus on 
>> _happens-before_ edges at some key points during reference processing.
>> 
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing 
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the 
>> registered cleaning action.
>> 
>> This will bring Cleaner in line (or close) with the memory visibility 
>> guarantees made for finalizers in [JLS 
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object 
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Cleaner thread dequeue happens-before running cleaning action

Looks good.

It is a thumbs up from me.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16644#pullrequestreview-1894716637

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v8]

[David Holmes]

On Wed, 21 Feb 2024 01:20:24 GMT, Brent Christian  wrote:

>> Classes in the `java.lang.ref` package would benefit from an update to bring 
>> the spec in line with how the VM already behaves. The changes would focus on 
>> _happens-before_ edges at some key points during reference processing.
>> 
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing 
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the 
>> registered cleaning action.
>> 
>> This will bring Cleaner in line (or close) with the memory visibility 
>> guarantees made for finalizers in [JLS 
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object 
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Updates to reachabilityFence()

src/java.base/share/classes/java/lang/ref/package-info.java line 118:

> 116:  *
> 117:  * The Cleaner thread dequeues a reference to a registered object 
> before
> 118:  * running the cleaning action for that object.

Do you want to rephrase this so that it too mentions `happens-before`? e.g.

Dequeuing of a reference to a registered object, by the Cleaner thread, 
happens-before the execution of the cleaning action for that object.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1496767354

Re: RFR: JDK-8252802: java launcher should set MALLOCOPTIONS and LDR_CNTRL in order to use 64KB pages on AIX

[David Holmes]

On Mon, 19 Feb 2024 05:52:22 GMT, Varada M  wrote:

> DeCapo Benchmark Results (3 runs) :  
> 
> Before : 
> = DaCapo 9.12 h2 PASSED in 281402 msec =
> = DaCapo 9.12 h2 PASSED in 269818 msec =
> = DaCapo 9.12 h2 PASSED in 279279 msec =
>  
> After:
> = DaCapo 9.12 h2 PASSED in 279192 msec =
> = DaCapo 9.12 h2 PASSED in 269769 msec =
> = DaCapo 9.12 h2 PASSED in 271577 msec = 
> 
> Environmental variables LDR_CNTRL and MALLOCOPTIONS has caused 
> test/jdk/java/lang/ProcessBuilder/Basic.java failure.
> Additional environmental variables has to removed from 
> removeAixExpectedVars().
> 
> JBS Issue : [JDK-8252802](https://bugs.openjdk.org/browse/JDK-8252802)

I concur with Alan: is this the right way to fix this? And is it really the 
case that every single Java application on AIX wants this setting?

-

PR Comment: https://git.openjdk.org/jdk/pull/17906#issuecomment-1953585384

Integrated: 8326126: Update the java manpage with the changes from JDK-8322478

[David Holmes]

On Mon, 19 Feb 2024 07:04:07 GMT, David Holmes  wrote:

> Please reviews these manpage updates in relation to "JEP 458: Launch 
> Multi-File Source-Code Programs".  The manpage sources had previously been 
> updated internally at Oracle under JDK-8322478, but the open troff file was 
> not regenerated at the time in mainline (it was for JDK 22).
> 
> The main gist of the change is that with multi-file support now available,  
> occurrences of "single source-file" should just refer to "source-file".
> 
> Thanks

This pull request has now been integrated.

Changeset: a3d7f9f2
Author:David Holmes 
URL:   
https://git.openjdk.org/jdk/commit/a3d7f9f2422cb4b65de7a086dc27dadc0858bf82
Stats: 61 lines in 1 file changed: 27 ins; 5 del; 29 mod

8326126: Update the java manpage with the changes from JDK-8322478

Reviewed-by: alanb, cstein

-

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

Re: RFR: 8326126: Update the java manpage with the changes from JDK-8322478

[David Holmes]

On Mon, 19 Feb 2024 07:04:07 GMT, David Holmes  wrote:

> Please reviews these manpage updates in relation to "JEP 458: Launch 
> Multi-File Source-Code Programs".  The manpage sources had previously been 
> updated internally at Oracle under JDK-8322478, but the open troff file was 
> not regenerated at the time in mainline (it was for JDK 22).
> 
> The main gist of the change is that with multi-file support now available,  
> occurrences of "single source-file" should just refer to "source-file".
> 
> Thanks

Thanks for the reviews!

-

PR Comment: https://git.openjdk.org/jdk/pull/17908#issuecomment-1953205703

RFR: 8326126: Update the java manpage with the changes from JDK-8322478

[David Holmes]

Please reviews these manpage updates in relation to "JEP 458: Launch Multi-File 
Source-Code Programs".  The manpage sources had previously been updated 
internally at Oracle under JDK-8322478, but the open troff file was not 
regenerated at the time in mainline (it was for JDK 22).

The main gist of the change is that with multi-file support now available,  
occurrences of "single source-file" should just refer to "source-file".

Thanks

-

Commit messages:
 - 8326126: Update the java manpage with the changes from JDK-8322478

Changes: https://git.openjdk.org/jdk/pull/17908/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17908=00
  Issue: https://bugs.openjdk.org/browse/JDK-8326126
  Stats: 61 lines in 1 file changed: 27 ins; 5 del; 29 mod
  Patch: https://git.openjdk.org/jdk/pull/17908.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17908/head:pull/17908

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

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v3]

[David Holmes]

On Mon, 5 Feb 2024 09:06:24 GMT, Richard Reingruber  wrote:

>> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for 
>> correct (Dekker scheme) synchronization with concurrent execution of 
>> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176).
>> 
>> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot 
>> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance 
>> Suite, and SAP specific tests.
>> Testing was done with fastdebug and release builds on the main platforms and 
>> also on Linux/PPC64le and AIX.
>
> Richard Reingruber 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 six additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 
> 8323782__Race__Thread__interrupt_vs__AbstractInterruptibleChannel_begin
>  - Review Alan
>  - New version of LotsOfInterrupts.java supporting virtual threads
>  - Add Alan's LotsOfInterrupts.java test
>  - Must checkAccess before changing interrupt state of other thread
>  - 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin

Sorry for leaving this hanging. Change looks good.

Thanks

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17444#pullrequestreview-1881293498

Re: RFR: 8325163: Enable -Wpedantic on clang [v2]

[David Holmes]

On Mon, 5 Feb 2024 10:58:17 GMT, Magnus Ihse Bursie  wrote:

>> Inspired by (the later backed-out) 
>> [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to 
>> enable `-Wpedantic` for clang. This has already found some irregularities in 
>> the code, like mistakenly using `#import` instead of `#include`. In this 
>> patch, I disable warnings for these individual buggy or badly written files, 
>> but I intend to post follow-up issues on the respective teams to have them 
>> properly fixed.
>> 
>> Unfortunately, it is not possible to enable `-Wpedantic` on gcc, since 
>> individual warnings in `-Wpedantic` cannot be disabled. This means that code 
>> like this:
>> 
>> 
>> #define DEBUG_ONLY(code) code;
>> 
>> DEBUG_ONLY(foo());
>> 
>> 
>> will result in a `; ;`. This breaks the C standard, but is benign, and we 
>> use it all over the place. On clang, we can ignore this by 
>> `-Wno-extra-semi`, but this is not available on gcc.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   FIx dtrace build

I think given the warnings have been enabled globally, but disabled locally 
where they cause build failures, and JBS issues will be filed for each of those 
special cases, then this is a reasonable change to make. It is then up to 
component teams to fix code where applicable, and enable disabled warnings in 
the future.

-

PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1928824757

Re: RFR: 8325163: Enable -Wpedantic on clang

[David Holmes]

On Mon, 5 Feb 2024 15:42:40 GMT, Kim Barrett  wrote:

> I thought we didn't, because we were instead supposed to use INTPTR_FORMAT and
the (currently?) equivalent PTR_FORMAT. Those macros aren't legacy, they are
to provide consistent output across platforms. "%p" provides implementation
defined output. 

My understanding/recollection was that those issues with `%p` had gone away and 
so we could start using it.

-

PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1928814503

[jdk22] Integrated: 8322066: Update troff manpages in JDK 22 before RC

[David Holmes]

On Sun, 4 Feb 2024 22:43:28 GMT, David Holmes  wrote:

> This update drops the "ea" from the version string.
> 
> We also propagate the following fixes from the markdown sources to the troff 
> manpage file:
> 
> JDK-8322478: Update java manpage for multi-file source-code launcher
> JDK-8302233: HSS/LMS: keytool and jarsigner changes
> JDK-8318971: Better Error Handling for Jar Tool When Processing Non-existent 
> Files
> 
> 
> Thanks.

This pull request has now been integrated.

Changeset: ac7a3c00
Author:David Holmes 
URL:   
https://git.openjdk.org/jdk22/commit/ac7a3c00bbfde173ced08d8745b308bc0ac9649b
Stats: 160 lines in 28 files changed: 63 ins; 15 del; 82 mod

8322066: Update troff manpages in JDK 22 before RC

Reviewed-by: darcy, iris

-

PR: https://git.openjdk.org/jdk22/pull/104

Re: [jdk22] RFR: 8322066: Update troff manpages in JDK 22 before RC

[David Holmes]

On Mon, 5 Feb 2024 21:58:23 GMT, Joe Darcy  wrote:

>> This update drops the "ea" from the version string.
>> 
>> We also propagate the following fixes from the markdown sources to the troff 
>> manpage file:
>> 
>> JDK-8322478: Update java manpage for multi-file source-code launcher
>> JDK-8302233: HSS/LMS: keytool and jarsigner changes
>> JDK-8318971: Better Error Handling for Jar Tool When Processing Non-existent 
>> Files
>> 
>> 
>> Thanks.
>
> Marked as reviewed by darcy (Reviewer).

Thanks for the reviews @jddarcy  and @irisclark !

-

PR Comment: https://git.openjdk.org/jdk22/pull/104#issuecomment-1928783920

Re: RFR: 8325163: Enable -Wpedantic on clang

[David Holmes]

On Mon, 5 Feb 2024 03:07:43 GMT, Kim Barrett  wrote:

> I consider the "format '%p' expects argument of type 'void*" warnings to be 
> not at all helpful. Fortunately we don't use '%p' in HotSpot,

We do use it in hotspot. Not a huge amount as we have the legacy format 
specifiers for PTR_FORMAT etc.

-

PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1926298655

[jdk22] RFR: 8322066: Update troff manpages in JDK 22 before RC

[David Holmes]

This update drops the "ea" from the version string.

We also propagate the following fixes from the markdown sources to the troff 
manpage file:

JDK-8322478: Update java manpage for multi-file source-code launcher
JDK-8302233: HSS/LMS: keytool and jarsigner changes
JDK-8318971: Better Error Handling for Jar Tool When Processing Non-existent 
Files


Thanks.

-

Commit messages:
 - 8322066: Update troff pages in JDK 22 before RC

Changes: https://git.openjdk.org/jdk22/pull/104/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk22=104=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322066
  Stats: 160 lines in 28 files changed: 63 ins; 15 del; 82 mod
  Patch: https://git.openjdk.org/jdk22/pull/104.diff
  Fetch: git fetch https://git.openjdk.org/jdk22.git pull/104/head:pull/104

PR: https://git.openjdk.org/jdk22/pull/104

Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v4]

[David Holmes]

On Sat, 27 Jan 2024 18:24:45 GMT, Coleen Phillimore  wrote:

>> This mechanically replaces NULL with nullptr in hpp/cpp native files in test 
>> native code.  This didn't attempt to change NULL in comments to say null 
>> because nullptr is generally the right thing for the comment to say.  It 
>> does attempt to change NULL to "null" rather than "nullptr" in strings.  Any 
>> changes for "nullptr" to "null" in comments can be changed in a future RFE 
>> in a smaller patch. I didn't see any when it was scrolling by to make my 
>> script more complicated.
>> 
>> Ran tier1-4 testing.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix one nullptr in comments as found by @kevinw

Looks good. Thanks for doing the text changes as well, they are a necessary 
part of the cleanup.

A number of files are missing copyright updates - the ones I spotted all had a 
single copyright year so maybe your script missed them?

My browser found 21 places where nullptr is cast to something else, which 
should no longer be needed.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17593#pullrequestreview-1847855251

Re: RFR: JDK-8324598: use mem_unit when working with sysinfo memory and swap related information

[David Holmes]

On Wed, 24 Jan 2024 10:07:17 GMT, Matthias Baesken  wrote:

> According to the sysinfo manpage ( 
> https://man7.org/linux/man-pages/man2/sysinfo.2.html ) the memory and swap 
> related entries in the struct sysinfo are given as multiples of mem_unit 
> bytes.
> "In the above structure, sizes of the memory and swap fields are given as 
> multiples of mem_unit bytes."
> 
> This is considered in most parts of the OpenJDK codebase when sysinfo is used 
> but not all; should be added where it is missing.

Looks fine.

Minor nit with lack of parentheses.

Thanks

src/hotspot/os/linux/os_linux.cpp line 420:

> 418:   struct sysinfo si;
> 419:   sysinfo();
> 420:   return (julong)si.totalswap * si.mem_unit;

Suggestion: `(julong)(si.totalswap * si.mem_unit);

src/java.base/linux/native/libjava/CgroupMetrics.c line 57:

> 55:  return 0; // syinfo failed, treat as no swap
> 56: }
> 57: return (jlong)si.totalswap * si.mem_unit;

Suggestion: `(julong)(si.totalswap * si.mem_unit);

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17550#pullrequestreview-1842877781
PR Review Comment: https://git.openjdk.org/jdk/pull/17550#discussion_r1465868336
PR Review Comment: https://git.openjdk.org/jdk/pull/17550#discussion_r1465868493

Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]

[David Holmes]

On Wed, 24 Jan 2024 19:37:40 GMT, Paul Sandoz  wrote:

>> It is still weird to talk about expressions at this level. We really check 
>> if the value is constant, like the method name suggests now. Yes, this 
>> implicitly tests that the expression that produced that value is fully 
>> constant-folded. But that's a detail that we do not need to capture here. 
>> Let's rename `expr` -> `val`, and tighten up the javadoc for the method to 
>> mention we only test the constness of the final value.
>
> I agree. All values are produced by evaluating expressions. In this case we 
> want to query whether a value produced by the compiler evaluating its 
> expression is a constant value (inputs to the expression are constants and 
> the expression had no material side-effects). Meaning if the method returns 
> true then we could use that knowledge in subsequent expressions that may also 
> produce constants or some specific behavior.

> the method compilation has the expression in its original form

So the JIT analyses the bytecode used to place the result on the call stack, 
before the call, and from that determines if the expression were a constant? 
This kind of self-analysis is not something I was aware of.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1465846860

Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]

[David Holmes]

On Wed, 24 Jan 2024 13:47:17 GMT, Doug Simon  wrote:

> You need to check if class is already loaded by trying findLoadedClass first.

Thanks @xxDark . I knew it should work. :)

-

PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908961416

Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]

[David Holmes]

On Wed, 24 Jan 2024 08:56:10 GMT, Doug Simon  wrote:

> As far as I understand, even a non-delegating classloader cannot redefine a 
> class loaded by the boot loader. I modified the test to show this and get:
> 
> ```
> java.lang.LinkageError: loader LoadAlternativeJVMCI$1 @4a1f4d08 attempted 
> duplicate class definition for jdk.vm.ci.meta.ResolvedJavaType. 
> (jdk.vm.ci.meta.ResolvedJavaType is in unnamed module of loader 
> LoadAlternativeJVMCI$1 @4a1f4d08, parent loader 'bootstrap')
>   at java.base/java.lang.ClassLoader.defineClass1(Native Method)

Interesting. I'm not sure why that should be happening in this case. I can 
imagine a potential split-package issue with the bootloader that doesn't happen 
with the platform loader. I will look into it.

-

PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1907983591

Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]

[David Holmes]

On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon  wrote:

>> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform 
>> class loader instead of the boot class loader. This allows Native Image to 
>> load a version of JVMCI different than the version on top of which Native 
>> Image is running. This capability is demonstrated and tested by 
>> `LoadAlternativeJVMCI.java`.
>
> Doug Simon has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use null to denote boot class loader as delegation parent

Marked as reviewed by dholmes (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1841205744

Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]

[David Holmes]

On Wed, 24 Jan 2024 08:46:08 GMT, Doug Simon  wrote:

>> test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54:
>> 
>>> 52: 
>>> 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader();
>>> 54: URLClassLoader ucl = new URLClassLoader(cp, null);
>> 
>> I am missing something here, a `URLClassLoader` first delegates to its 
>> parent before searching its URLs, so how does this not find the platform 
>> loader versions of the JVMCI classes?
>
> With `new URLClassLoader(cp, null)`, the URL loader delegates directly to the 
> boot loader, by-passing the platform loader.



Thanks Doug.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464798827

Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]

[David Holmes]

On Tue, 23 Jan 2024 22:46:20 GMT, Quan Anh Mai  wrote:

>> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
>> 
>>> 54:  */
>>> 55: @IntrinsicCandidate
>>> 56: public static boolean isCompileConstant(boolean expr) {
>> 
>> Here and in other places: probably not `expr`, but just `val` or something?
>
> I think of this as an expression that is always evaluated to the same value. 
> The value itself is not interesting, it is the set of values that this 
> expression can take that we are talking about.

This seems really weird to me for Java code. The method doesn't get the 
original "expression" it only gets the value of that expression after it has 
been evaluated. Is there some kind of weird "magic" happening here?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464361310

Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]

[David Holmes]

On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon  wrote:

>> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform 
>> class loader instead of the boot class loader. This allows Native Image to 
>> load a version of JVMCI different than the version on top of which Native 
>> Image is running. This capability is demonstrated and tested by 
>> `LoadAlternativeJVMCI.java`.
>
> Doug Simon has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use null to denote boot class loader as delegation parent

The actual changes to load JVMCI via the platform loader seem fine.

I'm still puzzled by the need to do this as any non-delegating classloader 
would have allowed this even if JVMCI were loaded by the bootloader. And I 
don't understand how your test is working as it is using a delegating 
classloader.

test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54:

> 52: 
> 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader();
> 54: URLClassLoader ucl = new URLClassLoader(cp, null);

I am missing something here, a `URLClassLoader` first delegates to its parent 
before searching its URLs, so how does this not find the platform loader 
versions of the JVMCI classes?

-

PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1840477028
PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464348710

Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies

[David Holmes]

On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev  wrote:

> Some jtreg tests require resolvable external dependencies. This resolution is 
> delegated to JIB, which is not used in vanilla OpenJDK testing. It would be 
> convenient to add a keyword that marks tests that require these external 
> dependencies, so that we could exclude those tests from runs. This would 
> allow us to: a) run all tests in hotspot:tier4, which now excludes 
> `applications/` specifically; b) make all tests runs (#17422) cleaner on many 
> environments.
> 
> I provisionally call this flag `external-dep`, but I am open for other 
> suggestions.
> 
> Note that some tests that pull `@Artifact`-s provide special paths that do 
> limited testing anyway. However, there are tests which cannot run without 
> external dependencies at all. These include at least `applications/jcstress` 
> and `applications/scimark` tests.
> 
> Ironically, I cannot run the jcstress test generator because the dependencies 
> are lacking here. I regenerated those test using a self-built jcstress 0.16 
> bundle.
> 
> Additional testing:
>  - [x] `make test TEST=applications/` fails
>  - [x]  `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, 
> skipping most of the tests

Seems quite reasonable.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17421#pullrequestreview-1837713758

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[David Holmes]

On Thu, 18 Jan 2024 09:21:24 GMT, Richard Reingruber  wrote:

>>> It is really safe/correct to move this outside the synchronized block? I 
>>> know things have changed a bit with loom but we've "always" held a lock 
>>> when doing the actual interrupt. I'd have to check the VM logic to be sure 
>>> it can be called concurrently from multiple threads for the same target 
>>> thread.
>> 
>> This hasn't changed. The interruptLock is used to coordinate the add/remove 
>> of the nioBlocker. When there is no nioBlocker set then the interrupt status 
>> and unparking (as in JavaThread::interrupt) has always executed without the 
>> interruptLock (named "blockerLock" in the past).
>
> I think that interrupting is just asynchronous to some extent.
> E.g. a thread polls its interrupt status clearing it thereby (without lock) 
> before calling nio. A concurrent interrupt can be lost then even if the lock 
> is acquired.
> (Maybe clearing should not be done by a public method)

Yep my bad on the VM side of things - no change there. But in the nioBlocker 
case doesn't this inherently make things more racy? Now maybe those races are 
allowed, but this might lead to a change in behaviour.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1458188281

Re: Avoiding Side Effects of ForkJoinPool.managedBlock

[David Holmes]

On 18/01/2024 7:30 pm, Johannes Spangenberg wrote:

Hello,

I have a question about dealing with side effects caused by ForkJoinPool. I am 
not certain if this mailing list is the appropriate platform, but I hope it is. 
The issue I am facing is that running code within a ForkJoinPool may change the 
behavior of the code. These changes in behavior have resulted in 
non-deterministic test failures in one of the repositories I work on. JUnit 
uses a ForkJoinPool when you enable parallel test execution. I would like to 
disable or avoid the side effects of various methods if called from a 
ForkJoinPool. So far, I haven't found a solution, except for completely 
replacing the use of the ForkJoinPool in JUnit with some custom scheduler which 
is not a ForkJoinPool.

Is there a solution to force an "unmanaged" block (as opposed to 
ForkJoinPool.managedBlock)? Is there alternatively a good approach to transfer CPU bound 
subtasks to another thread pool while blocking the ForkJoinWorkerThread without 
compensation? I have implemented a workaround which I explain below, but I am not sure if 
this will remain effective in future JDK versions. I am currently using JDK 17 but were 
also able to reproduce the issues with JDK 21.

I have observed the following side-effects caused by managed blocks or similar 
mechanisms:

1. Parallel stream methods execute another task (i.e. JUnit test) from the pool 
recursively, which is particularly problematic if your code utilizes any 
ThreadLocal.


The parallel stream implementation utilises the current ForkJoinPool, if 
any, else the common FJP.


David
-


2. Blocking methods spawn around 256 threads in total to "compensate" for the 
blocking operation. Consequently, you end up with up to 256 tests running concurrently, 
each of them might or might not be CPU bound (unknown to the ForkJoinPool).

3. Blocking methods may throw a RejectedExecutionException when the thread 
limit is reached. This is effectively a race condition which may lead to 
exceptions.

I have not been able to determine under which circumstances each behavior 
occurs. I am unaware of any thorough documentation that clearly outlines the 
expected behavior in different scenarios with different blocking methods. While 
(1.) and (3.) have caused test failures, (2.) simply causes JUnit to run 256 
tests in parallel instead of the intended 12. I attached a JUnit test to 
reproduce (1.) and (3.), but it might not fail on every run.

Many of the blocking methods of the standard library include a check if the current 
thread is an instance of ForkJoinWorkerThread. My current workaround involves wrapping 
the code that makes blocking calls into a FutureTask which is executed on another thread 
and then joining this task afterwards. As of now, FutureTask.get() seems not to implement 
any of the side-effects. As the missing instanceof-check in FutureTask makes it 
inconsistent with other Futures like CompletableFuture, I fear it might be considered a 
"bug". I would like to know a safe solution which is specified to continue to 
work in future JDKs.

PS: There is also a ticket on the JUnit project about this topic, but it only 
talks about side-effect (2.), but not the other side-effects we observed.
https://github.com/junit-team/junit5/issues/3108

Thanks,
Johannes

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[David Holmes]

On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber  wrote:

>> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for 
>> correct (Dekker scheme) synchronization with concurrent execution of 
>> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176).
>> 
>> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot 
>> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance 
>> Suite, and SAP specific tests.
>> Testing was done with fastdebug and release builds on the main platforms and 
>> also on Linux/PPC64le and AIX.
>
> Richard Reingruber has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Review Alan

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

> 1707: // Setting the interrupt status must be done before reading 
> nioBlocker.
> 1708: interrupted = true;
> 1709: interrupt0();  // inform VM of interrupt

It is really safe/correct to move this outside the synchronized block? I know 
things have changed a bit with loom but we've "always" held a lock when doing 
the actual interrupt. I'd have to check the VM logic to be sure it can be 
called concurrently from multiple threads for the same target thread.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1457061745

Re: RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

[David Holmes]

On Thu, 18 Jan 2024 00:58:49 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to disable core file generation in 
> java/foreign/critical/TestCriticalUpcall.java.

Good and trivial.

Copyright year needs updating.

Thanks

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17476#pullrequestreview-1829051168

Re: RFR: 8323515: Create test alias "all" for all test roots

[David Holmes]

On Mon, 15 Jan 2024 11:05:09 GMT, Aleksey Shipilev  wrote:

> Since recent work to improve `tier4` performance, we actually test 
> `tier{1,2,3,4}` often, which includes all the tests in current tree. It would 
> be more convenient to just have the `all` test group/alias, so that we can do 
> `make test TEST=all`. This also gives a parallelism / run time benefit, as we 
> do not wait for tests in each tier to complete before moving to next tier. 
> 
> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some 
> environments one also needs to supply a few keywords like `!printer` to skip 
> tests that cannot complete without failure due to misconfiguration. I left 
> the keywords as is to show how would a failing run look. There is also an 
> existing shortcut in build system that allows to run this with `make 
> test-all`.
> 
> 
> % make test TEST=all
> 
> Test selection 'all', will run:
> * jtreg:test/hotspot/jtreg:all
> * jtreg:test/jdk:all
> * jtreg:test/langtools:all
> * jtreg:test/jaxp:all
> * jtreg:test/lib-test:all
> 
> (...about 6 hours later...)
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>>> jtreg:test/hotspot/jtreg:all   6731  670229 0 <<
>>> jtreg:test/jdk:all 9962  995111 0 <<
>jtreg:test/langtools:all   4469  4469 0 0  
>  
>jtreg:test/jaxp:all 513   513 0 0  
>  
>jtreg:test/lib-test:all  3232 0 0  
>  
> ==
> TEST FAILURE

Okay - change is harmless with no ongoing maintenance cost.

test/jdk/TEST.groups line 28:

> 26: #
> 27: 
> 28: all = \

Why no `jdk_all` definition in this case?

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17422#pullrequestreview-1822313872
PR Review Comment: https://git.openjdk.org/jdk/pull/17422#discussion_r1452781088

Re: RFR: 8323659: LinkedTransferQueue add and put methods call overridable offer [v3]

[David Holmes]

On Fri, 12 Jan 2024 15:06:01 GMT, Chris Hegarty  wrote:

>> Update LinkedTransferQueue add and put methods to not call overridable offer.
>
> Chris Hegarty has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   timed offer

With my CSR hat on, JDK-8301341 should never have made the changes it did 
without going through a CSR request. We have been bitten by this kind of 
problem many times. Unless a public method is specified to utilise another 
public method, we should never implement one public method in terms of another 
(non-final one) due to the overriding problem. Backporting such a change to 21u 
is then potentially very damaging.

-

PR Comment: https://git.openjdk.org/jdk/pull/17393#issuecomment-1891184409

Re: RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups

[David Holmes]

On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson  wrote:

> TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups 
> file still has a reference to it. This causes problems in our CI pipeline.

TEST.groups change is good but the other file shouldn't be there.

I will hit approve anyway and assume you will fix, as I have to go.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17344#pullrequestreview-1813099354

Re: RFR: 8309218: java/util/concurrent/locks/Lock/OOMEInAQS.java still times out with ZGC, Generational ZGC, and SerialGC [v2]

[David Holmes]

On Tue, 9 Jan 2024 11:08:39 GMT, Viktor Klang  wrote:

>> As per conversation in the issue.
>
> Viktor Klang has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Updating copyright year for zgc and zgc-gen problem lists
>  - Removing OOMEInAQS from zgc and zgc-gen problem lists

Looks good. Restricting to only using G1, which has not been failing, seems the 
best thing to do here.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17313#pullrequestreview-1812464169

Re: The default java.library.path on Linux does not include the library paths in the mulitarch-spec

[David Holmes]

On 5/01/2024 12:24 am, Glavo wrote:

I expect there are security reasons why the JDK tries to find
the file itself in these specific paths, rather than letting the
platform code search for it.


I think this should have nothing to do with security. If there is a 
vulnerability in the platform code, there is nothing the JDK can do to 
avoid it.


I did some archaeology and this has basically worked this way since it 
was introduced way back in 1998. As far as I can see there was never any 
discussion/suggestion that if we fail to find a library by the 
classloader mechanisms (using java.library.path etc.) that we just pass 
it through to the platform loader (e.g. dlopen) to try and find it. This 
may be an oversight, or it may relate to how the classloader has to 
maintain a mapping between library names and actual library files (which 
would be difficult if dlopen does the searching implicitly).



Well that is not something I would want to see implemented in hotspot.


This is the only way I can think of to get the JDK to behave 
consistently with ld.
Maybe we should wait and see other developers who are more familiar with 
this part.


Certainly. But IMHO neither core-libs folk, not hotspot folk, will want 
to be in the business of having to load, parse and interpret 
/etc/ld.so.conf and its related files.



I'm now sending this email to panama-dev as well.
I think this proposal is of great significance to Panama, as it will 
make it easier for developers to develop wrappers for platform libraries.


It will be interesting to see what they say.

Cheers,
David
-


Glavo

On Thu, Jan 4, 2024 at 3:12 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


On 4/01/2024 1:36 pm, Glavo wrote:
 > Hey David,
 >
 >     AFAICS java.library.path (and sun.boot.library.path) are input
 >     properties that can be set by the user. There are no
"default" values
 >     for these properties as such.
 >
 >
 > Although they can be set by the user, they have default values.
 > For Linux, the default value is set here:
 >
 >

https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555>
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555>>
 >
 > The default value on Linux is the LD_LIBRARY_PATH environment
variable
 > and the paths I mentioned earlier.
 >
 >     The library loading will ultimately rely
 >     on the behaviour of dlopen, if no additional paths have been
set, so it
 >     seems to me what you really want is for dlopen to act "the
same way"
 >     that ld does. Note that dlopen will already check the contents of
 >     /etc/ld.so.cache
 >
 >
 > System.loadLibrary looks for the library in sun.boot.library.path
and
 > java.library.path and passes the full path to the library to dlopen:
 >
 >

https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254>
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254>>
 >
 > Therefore, the behavior of finding native libraries has nothing
to do
 > with the behavior of dlopen, only sun.boot.library.path and
 > java.library.path.

I stand corrected - apologies. I expected a raw name to simply get
passed through. I thought both LD_LIBRARY_PATH and java.library.path
could be used to expand the set of directories where the platform code
looks for libs, not constrain things to only those paths (plus the
defaults). I expect there are security reasons why the JDK tries to
find
the file itself in these specific paths, rather than letting the
platform code search for it.

 >     This does not seem practical. On my system ld.so.conf just
contains
 >
 >     include ld.so.conf.d/*.conf
 >
 >     so now we (hotspot) have to read the top-level file, parse
it, interpret
 >     it and then rec

Re: The default java.library.path on Linux does not include the library paths in the mulitarch-spec

[David Holmes]

On 4/01/2024 1:36 pm, Glavo wrote:

Hey David,

AFAICS java.library.path (and sun.boot.library.path) are input
properties that can be set by the user. There are no "default" values
for these properties as such.


Although they can be set by the user, they have default values.
For Linux, the default value is set here:

https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555>

The default value on Linux is the LD_LIBRARY_PATH environment variable 
and the paths I mentioned earlier.


The library loading will ultimately rely
on the behaviour of dlopen, if no additional paths have been set, so it
seems to me what you really want is for dlopen to act "the same way"
that ld does. Note that dlopen will already check the contents of
/etc/ld.so.cache


System.loadLibrary looks for the library in sun.boot.library.path and 
java.library.path and passes the full path to the library to dlopen:



https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254
 
<https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254>

Therefore, the behavior of finding native libraries has nothing to do 
with the behavior of dlopen, only sun.boot.library.path and 
java.library.path.


I stand corrected - apologies. I expected a raw name to simply get 
passed through. I thought both LD_LIBRARY_PATH and java.library.path 
could be used to expand the set of directories where the platform code 
looks for libs, not constrain things to only those paths (plus the 
defaults). I expect there are security reasons why the JDK tries to find 
the file itself in these specific paths, rather than letting the 
platform code search for it.



This does not seem practical. On my system ld.so.conf just contains

include ld.so.conf.d/*.conf

so now we (hotspot) have to read the top-level file, parse it, interpret
it and then recursively read, parse and interpret more files.


Yes, this is exactly the behavior I want.


Well that is not something I would want to see implemented in hotspot.

Cheers,
David
-


Glavo

On Tue, Jan 2, 2024 at 10:08 AM David Holmes <mailto:david.hol...@oracle.com>> wrote:


Hi Glavo,

On 24/12/2023 4:18 am, Glavo wrote:
 > Hi,
 >
 > There are already many Linux distributions that are following the
 > multiarch-spec[1] and adding the following paths to the default
library
 > path list:
 >
 >   * /usr/local/lib/
 >   * /lib/
 >   * /usr/lib/
 >
 > But OpenJDK doesn't add these paths to the java.library.path by
default,
 > so System.loadLibrary(String) has annoying behavior differences
with ld.

AFAICS java.library.path (and sun.boot.library.path) are input
properties that can be set by the user. There are no "default" values
for these properties as such. The library loading will ultimately rely
on the behaviour of dlopen, if no additional paths have been set, so it
seems to me what you really want is for dlopen to act "the same way"
that ld does. Note that dlopen will already check the contents of
/etc/ld.so.cache

 > Many libraries already installed on the system cannot be found by
 > System.loadLibrary(String).
 >
 > I wish OpenJDK would parse the /etc/ld.so.conf to get the full
library
 > path list so it would be consistent with the behavior of ld.
 > Can anyone consider this suggestion?

This does not seem practical. On my system ld.so.conf just contains

include ld.so.conf.d/*.conf

so now we (hotspot) have to read the top-level file, parse it,
interpret
it and then recursively read, parse and interpret more files.

Cheers,
David



 > Glavo
 >
 > [1]: https://wiki.ubuntu.com/MultiarchSpec
<https://wiki.ubuntu.com/MultiarchSpec>
 > <https://wiki.ubuntu.com/MultiarchSpec
<https://wiki.ubuntu.com/MultiarchSpec>>

Re: RFR: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable

[David Holmes]

On Wed, 3 Jan 2024 09:51:24 GMT, Stefan Karlsson  wrote:

> Most functions in ProcessTools are declared to throw Exceptions, or one of 
> its subclasses. However, there are a small number of functions that are 
> unnecessarily declared to throw Throwable instead of Exception. I propose 
> that we change them to also be declared to throw Exception.
> 
> This is a trivial patch to make it easier to refactor tests to use the 
> updated functions.
> 
> Tested manually, but will wait for GHA to verify that the change is OK.

Seems fine and trivial. As long as this compiles it is correct.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17240#pullrequestreview-1801895740

Re: The default java.library.path on Linux does not include the library paths in the mulitarch-spec

[David Holmes]

Hi Glavo,

On 24/12/2023 4:18 am, Glavo wrote:

Hi,

There are already many Linux distributions that are following the 
multiarch-spec[1] and adding the following paths to the default library 
path list:


  * /usr/local/lib/
  * /lib/
  * /usr/lib/

But OpenJDK doesn't add these paths to the java.library.path by default, 
so System.loadLibrary(String) has annoying behavior differences with ld.


AFAICS java.library.path (and sun.boot.library.path) are input 
properties that can be set by the user. There are no "default" values 
for these properties as such. The library loading will ultimately rely 
on the behaviour of dlopen, if no additional paths have been set, so it 
seems to me what you really want is for dlopen to act "the same way" 
that ld does. Note that dlopen will already check the contents of 
/etc/ld.so.cache


Many libraries already installed on the system cannot be found by 
System.loadLibrary(String).


I wish OpenJDK would parse the /etc/ld.so.conf to get the full library 
path list so it would be consistent with the behavior of ld.

Can anyone consider this suggestion?


This does not seem practical. On my system ld.so.conf just contains

include ld.so.conf.d/*.conf

so now we (hotspot) have to read the top-level file, parse it, interpret 
it and then recursively read, parse and interpret more files.


Cheers,
David




Glavo

[1]: https://wiki.ubuntu.com/MultiarchSpec 

Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]

[David Holmes]

On Mon, 18 Dec 2023 17:09:59 GMT, Serguei Spitsyn  wrote:

>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical 
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, 
>> `getAndClearInterrupt()`, `threadState()`, `toString()`), 
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report 
>> comment.
>> In simple words, a virtual thread should not be suspended during 
>> 'interruptLock' critical sections.
>> 
>> The fix is to record that a virtual thread is in a critical section 
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about 
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any 
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>> 
>> Some of new notifications with `notifyJvmtiSync()` method is on a 
>> performance critical path. It is why this method has been intrincified.
>> 
>> New test was developed by Patricio:
>>  `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without 
>> the fix.
>> The test is never failing with this fix.
>> 
>> Testing:
>>  - tested with newly added test: 
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>>  - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge
>  - review: improve an assert message
>  - review: moved a couple of comments out of try blocks
>  - review: moved notifyJvmtiDisableSuspend(true) out of try-block
>  - review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
>  - review: (1) rename notifyJvmti method; (2) add try-final statements to 
> VirtualThread methods
>  - Resolved merge conflict in VirtualThread.java
>  - added @summary to new test SuspendWithInterruptLock.java
>  - add new test SuspendWithInterruptLock.java
>  - 8311218: fatal error: stuck in 
> JvmtiVTMSTransitionDisabler::VTMS_transition_disable

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

> 4022: #else
> 4023:   fatal("Should only be called with JVMTI enabled");
> 4024: #endif

You can't do this! The Java code knows nothing about JVM TI being 
enabled/disabled and will call this function unconditionally.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432241016

Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion

[David Holmes]

On Wed, 13 Dec 2023 13:29:43 GMT, Jorn Vernee  wrote:

>> test/jdk/java/foreign/TestStubAllocFailure.java line 51:
>> 
>>> 49: runInNewProcess(UpcallRunner.class, true, 
>>> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
>>> 50: .shouldNotHaveExitValue(0)
>>> 51: .shouldNotHaveFatalError();
>> 
>> Just curious what non-zero exit value is actually expected here and below?
>
> Any non-zero exit value is acceptable. The intent here is to check that the 
> process didn't complete normally.

A non-zero non-crashing value. Just wondering what that actually would be?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1427557140

Integrated: 8322065: Initial nroff manpage generation for JDK 23

[David Holmes]

On Thu, 14 Dec 2023 05:46:01 GMT, David Holmes  wrote:

> Updated the version to 23-ea and year to 2024.
> 
> This initial generation also picks up the unpublished changes from:
> 
> - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & 
> jarsigner)
> - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK 
> 23 backport)
> 
> Thanks

This pull request has now been integrated.

Changeset: 692be577
Author:David Holmes 
URL:   
https://git.openjdk.org/jdk/commit/692be577385844bf00a01ff10e390e014191569f
Stats: 193 lines in 27 files changed: 36 ins; 51 del; 106 mod

8322065: Initial nroff manpage generation for JDK 23

Reviewed-by: alanb

-

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

Re: RFR: 8322065: Initial nroff manpage generation for JDK 23 [v2]

[David Holmes]

> Updated the version to 23-ea and year to 2024.
> 
> This initial generation also picks up the unpublished changes from:
> 
> - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & 
> jarsigner)
> - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK 
> 23 backport)
> 
> Thanks

David Holmes has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert "8309981: Remove expired flags in JDK 23"
  
  This reverts commit 0324a90e936ae01e42ae099e7235156326cc318a.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17101/files
  - new: https://git.openjdk.org/jdk/pull/17101/files/65a8c9ed..8b052141

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17101=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=17101=00-01

  Stats: 23 lines in 2 files changed: 10 ins; 11 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/17101.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17101/head:pull/17101

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

Re: RFR: 8322065: Initial nroff manpage generation for JDK 23

[David Holmes]

On Thu, 14 Dec 2023 09:01:17 GMT, Alan Bateman  wrote:

> Initially I wondered if JDK-8309981 should be separated but include keeps 
> things in sync so I think okay.

Thanks for the review @AlanBateman .

Yeah I was in two minds there myself. I started fixing 
[JDK-8309981](https://bugs.openjdk.org/browse/JDK-8309981) only to discover 
that the start of release updates had not been done as part of the start of 
release, so I figured I may as well fix it all together given I'd generated all 
the updated files anyway. But I'm still a little unsure ... in fact I think I 
will remove it in the morning.

-

PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855761906

Re: RFR: 8322065: Initial nroff manpage generation for JDK 23

[David Holmes]

On Thu, 14 Dec 2023 09:17:05 GMT, Pavel Rappo  wrote:

> Thanks for doing this, David. I only note that the changes for JDK-8321384 
> were published in [JDK-8308715](https://bugs.openjdk.org/browse/JDK-8308715), 
> which was integrated in the mainline before JDK 22 RDP 1. So they are already 
> present in the mainline.

Ah I see. Thanks for correcting that, I will update the PR and JBS issue.

And thanks for looking at this @pavelrappo .

-

PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855755042

Re: Introduce constructor for PriorityQueue with existing collection and custom comparator

[David Holmes]

The current mailing list, core-libs-dev, is the correct one.

Cheers,
David

On 14/12/2023 9:37 am, Valeh Hajiyev wrote:

Hi Pavel,

Thanks for the reply. If I understand correctly, I need this change to 
be discussed in one of the mailing lists there, so that someone would 
sponsor me to create a tracking issue in JBS. Do you know which mailing 
list is the most relevant for me to propose the change?


Thanks,
Valeh

On Thu, Dec 14, 2023 at 12:26 AM Pavel Rappo > wrote:


Sorry, there's a necessary process that a PR must follow. You seem
to have signed OCA already. For the rest, see this resource:
https://openjdk.org/guide/ . In
particular, this part:
https://openjdk.org/guide/#contributing-to-an-openjdk-project
.

-Pavel

 > On 13 Dec 2023, at 23:09, Valeh Hajiyev mailto:valeh.haji...@gmail.com>> wrote:
 >
 > Hi all,
 >
 > I have raised the following PR, could someone please help me to
get it merged?
 >
 > https://github.com/openjdk/jdk/pull/17045

 >
 > More details:
 >
 > This commit addresses the current limitation in the
`PriorityQueue` implementation, which lacks a constructor to
efficiently create a priority queue with a custom comparator and an
existing collection. In order to create such a queue, we currently
need to initialize a new queue with custom comparator, and after
that populate the queue using `addAll()` method, which in the
background calls `add()` method (which takes `O(logn)` time) for
each element of the collection (`n` times).  This is resulting in an
overall time complexity of `O(nlogn)`.
 >
 > ```
 > PriorityQueue pq = new PriorityQueue<>(customComparator);
 > pq.addAll(existingCollection);
 > ```
 >
 > The pull request introduces a new constructor to streamline this
process and reduce the time complexity to `O(n)`.  If you create the
queue above using the new constructor, the contents of the
collection will be copied (which takes `O(n)` time) and then later 
`heapify()` operation (Floyd's algorithm) will be called once

(another `O(n)` time). Overall the operation will be reduced from
`O(nlogn)` to `O(2n)` -> `O(n)` time.
 >
 > ```
 > PriorityQueue pq = new
PriorityQueue<>(existingCollection, customComparator);
 > ```
 >
 > Best regards,
 > Valeh Hajiyev
 >

RFR: 8322065: Initial nroff manpage generation for JDK 23

[David Holmes]

Updated the version to 23-ea and year to 2024.

This initial generation also picks up the unpublished changes from:

- [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & 
jarsigner)
- [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK 23 
backport)
- [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc)


In addition this includes the updates for

- [JDK-8309981](https://bugs.openjdk.org/browse/8309981) Remove expired flags 
in JDK 23

Thanks

-

Commit messages:
 - 8322065: Initial nroff manpage generation for JDK 23
 - 8309981: Remove expired flags in JDK 23

Changes: https://git.openjdk.org/jdk/pull/17101/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17101=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322065
  Stats: 216 lines in 29 files changed: 47 ins; 61 del; 108 mod
  Patch: https://git.openjdk.org/jdk/pull/17101.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17101/head:pull/17101

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

Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion

[David Holmes]

On Mon, 11 Dec 2023 13:01:25 GMT, Jorn Vernee  wrote:

> Improve the test by being more lenient to related code cache exhaustion 
> errors. The important thing is that we don't terminate with a fatal error, 
> which the new code now checks for explicitly. The check for that is based on 
> what is done by 
> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
> 
> The existing `UpcallTestHelper.Output` class that was previously used to 
> assert on stdout/stderr contents did not have the capability to look for 
> patterns in the output. So, I've taken the opportunity to replace it with the 
> more canonical `OutputAnalyzer` which comes from the test library.
> 
> Finally, I've also added back the test for downcall stub allocation failure 
> which was removed as part of the initial patch because it was too 
> inconsistent [1]. With the new approach, it should pass reliably as well.
> 
> Testing: `jdk_foreign`  suite (which contains all the affected tests)
> 
> [1]: 
> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64

test/jdk/java/foreign/TestStubAllocFailure.java line 51:

> 49: runInNewProcess(UpcallRunner.class, true, 
> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
> 50: .shouldNotHaveExitValue(0)
> 51: .shouldNotHaveFatalError();

Just curious what non-zero exit value is actually expected here and below?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1424930505

Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]

[David Holmes]

On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato  wrote:

>> Currently, Locale-related system properties, such as `user.language` or 
>> `user.country`, are initialized when the `Locale` class is loaded. Making 
>> them static properties is safer than relying on the `Locale` class loading 
>> timing, which could potentially be changed depending on the implementation.
>
> Naoto Sato 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 four additional commits since 
> the last revision:
> 
>  - Reflects review comments
>  - Merge branch 'master' into JDK-8321206-Locale-static-properties
>  - Add exclusions in cdsHeapVerifier for new StaticProperties
>  - initial commit

@iklam or @calvinccheung  could either of you take a look at the CDS changes 
here please. Thanks.

-

PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1852949624

Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging

[David Holmes]

On Tue, 12 Dec 2023 08:57:08 GMT, Stefan Karlsson  wrote:

> Do you agree that it would be OK to remove the test?

Sure that's fine.

-

PR Comment: https://git.openjdk.org/jdk/pull/17052#issuecomment-1851644385

Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging

[David Holmes]

On Mon, 11 Dec 2023 11:16:05 GMT, Stefan Karlsson  wrote:

> There is some logging printed when tests spawns processes. This logging is 
> triggered from calls to `OutputAnalyzer`, when it delegates calls to 
> `LazyOutputBuffer`.
> 
> If we write code like this:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = new OutputAnalyzer(pb.start());
> int exitValue = output.getExitValue();
> 
> 
> We get the following logging:
> 
> [timestamp0] "Gathering output for process 
> [timestamp1] Waiting for completion for process 
> [timestamp2] Waiting for completion finished for process 
> 
> 
> The first line comes from the `OutputAnalyzer` constructor and the two other 
> lines comes from the `getExitValue` call, which calls logs the above messages 
> around the call to `waitFor`.
> 
> If instead write the code above as:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = ProcessTools.executeProcess(pb);
> int exitValue = output.getExitValue();
> 
> 
> We get the same logging, but timestamp1 and timestamp2 are almost the same. 
> This happens because there's an extra call to `waitFor` inside 
> `executeProcess`, but that `waitFor` does not have the "wait for" logging. 
> So, instead we get the logging for the no-op `waitFor` inside `getExitValue`.
> 
> I would like to propose a small workaround to solve this. The workaround is 
> that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` 
> via `OutputAnalyzer`.  This is a small, limited workaround for this issue. 
> Ideally I would like to extract the entire Process handling code out of 
> LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all 
> usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the 
> test directories, so I'm starting out with this suggestion.
> 
> We can see of this patch by looking at the timestamps generated from the 
> included test. Without the proposed workaround:
> 
> Without
> 
> testExecuteProcessExit
> [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719
> [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719
> [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process 
> 2547719
> 
> testExecuteProcessStdout
> [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741
> [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741
> [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process 
> 2547741
> 
> 
> testNewOutputAnalyzerExit
> [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782
> [2023...

Looks good.

Thanks

test/lib/jdk/test/lib/process/OutputAnalyzer.java line 111:

> 109: /**
> 110:  * Delegate waitFor to the OutputBuffer. This ensures that
> 111:  * the progress and timestmaps are logged correctly.

Typo: timestmaps

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17052#pullrequestreview-1776831148
PR Review Comment: https://git.openjdk.org/jdk/pull/17052#discussion_r1423565527

Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]

[David Holmes]

On Fri, 8 Dec 2023 21:11:57 GMT, Naoto Sato  wrote:

>> Naoto Sato 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 four additional 
>> commits since the last revision:
>> 
>>  - Reflects review comments
>>  - Merge branch 'master' into JDK-8321206-Locale-static-properties
>>  - Add exclusions in cdsHeapVerifier for new StaticProperties
>>  - initial commit
>
> Can you elaborate on it more?  I don't think it is possible to change the 
> default locale by modifying those system properties via the `setProperty()` 
> call  in recent JDK releases.

@naotoj  the hotspot changes were not approved by any Hotspot engineer. Unless 
trivial, all hotspot code changes require two reviews by hotspot developers.

-

PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1849333218

Re: RFR: 8321270: Virtual Thread.yield consumes parking permit [v2]

[David Holmes]

On Tue, 5 Dec 2023 09:51:50 GMT, Alan Bateman  wrote:

>> When a virtual thread continues after Thread.yield it currently consumes 
>> thread's parking permit. This is an oversight, the parking permit should 
>> only be consumed when continuing after park.
>> 
>> The changes are straight-forward. The internal "RUNNABLE" state is replaced 
>> with UNPARKED and YIELDED state, effectively encoding the previous state. So 
>> for the most part, it's just replacing the usages of RUNNABLE. The 
>> additional states require refactoring tryGetStackTrace, this is the method 
>> that is used for Thread::getStackTrace when the virtual thread is unmounted. 
>> It is also changed to not not swallow the REE if the reesubmit fails 
>> (tryStackTrace has to resubmit as the task for the thread may run, or the 
>> thread unparked, while "suspended" and sampling its stack trace). The 
>> changes are a subset of larger changes in the loom repo, future PRs will 
>> propose to bring in other changes to get main line up to date.
>> 
>> For testing the existing ThreadAPI has new test cases.
>> 
>> Testing: test1-5. This includes the JVMTI tests as it maps the thread states 
>> to JVMTI thread states.
>
> Alan Bateman 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 four additional 
> commits since the last revision:
> 
>  - Fix comment, remove space between case labels and colon
>  - Merge
>  - Leave onPinned to another PR
>  - Initial commit

Sorry I've deleted my earlier embarassingly stupid comment. The key point of 
this change is to split RUNNABLE into two states to indicated "runnable after 
yielding" and "runnable after being unparked".

-

PR Comment: https://git.openjdk.org/jdk/pull/16953#issuecomment-1842005657

Re: RFR: 8321270: Virtual Thread.yield consumes parking permit [v2]

[David Holmes]

On Tue, 5 Dec 2023 09:51:50 GMT, Alan Bateman  wrote:

>> When a virtual thread continues after Thread.yield it currently consumes 
>> thread's parking permit. This is an oversight, the parking permit should 
>> only be consumed when continuing after park.
>> 
>> The changes are straight-forward. The internal "RUNNABLE" state is replaced 
>> with UNPARKED and YIELDED state, effectively encoding the previous state. So 
>> for the most part, it's just replacing the usages of RUNNABLE. The 
>> additional states require refactoring tryGetStackTrace, this is the method 
>> that is used for Thread::getStackTrace when the virtual thread is unmounted. 
>> It is also changed to not not swallow the REE if the reesubmit fails 
>> (tryStackTrace has to resubmit as the task for the thread may run, or the 
>> thread unparked, while "suspended" and sampling its stack trace). The 
>> changes are a subset of larger changes in the loom repo, future PRs will 
>> propose to bring in other changes to get main line up to date.
>> 
>> For testing the existing ThreadAPI has new test cases.
>> 
>> Testing: test1-5. This includes the JVMTI tests as it maps the thread states 
>> to JVMTI thread states.
>
> Alan Bateman 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 four additional 
> commits since the last revision:
> 
>  - Fix comment, remove space between case labels and colon
>  - Merge
>  - Leave onPinned to another PR
>  - Initial commit

I find the switch from `RUNNABLE` to `UNPARKED` somewhat unnecessary. I don't 
see any problem with `RUNNABLE` that needed fixing. I found the distinction and 
transition between `RUNNABLE` and `RUNNING` to be very clear. ??

-

PR Review: https://git.openjdk.org/jdk/pull/16953#pullrequestreview-1766538038

Re: RFR: 8321206: Make Locale related system properties static properties

[David Holmes]

On Tue, 5 Dec 2023 23:04:55 GMT, Naoto Sato  wrote:

> Currently, Locale-related system properties, such as `user.language` or 
> `user.country`, are initialized when the `Locale` class is loaded. Making 
> them static properties is safer than relying on the class initialization 
> timing, which could potentially be changed depending on the implementation.

Can I also suggest that you change the bug and PR synopsis to say 
`StaticProperty` rather than `static properties` as I found it quite confusing 
to understand the issue.

> Making them static properties is safer than relying on the class 
> initialization timing

"class initialization" refers to the static initialization of a class. I assume 
that is not what you mean here but the creation of the instance of the class? 
Even `StaticProperty` still initializes these things during class 
initialization, so I'm unclear exactly what this is trying to say.

-

PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1841978376

Re: RFR: 8321206: Make Locale related system properties static properties

[David Holmes]

On Tue, 5 Dec 2023 23:04:55 GMT, Naoto Sato  wrote:

> Currently, Locale-related system properties, such as `user.language` or 
> `user.country`, are initialized when the `Locale` class is loaded. Making 
> them static properties is safer than relying on the class initialization 
> timing, which could potentially be changed depending on the implementation.

I'm not following the changes to cdsHeapVerifier.cpp. You've marked the new 
entries as `C` but the definition is:

// [C] A non-final static string that is assigned a string literal during class
// initialization; this string is never changed during -Xshare:dump.

and these are final static strings not non-final. ???

You also have a large number of test failures with this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1841972901
PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1841973970

Re: RFR: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed [v3]

[David Holmes]

On Tue, 5 Dec 2023 10:56:44 GMT, Jaikiran Pai  wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   remove micro optimization
>
> Thank you Stefan and Leonid for the reviews.

Sorry I missed this party :) Thanks for finding and fixing @jaikiran .

FWIW `exitCode` out numbers `exitValue` in source code 3:1 (and > 5:1 in test 
code).

-

PR Comment: https://git.openjdk.org/jdk/pull/16919#issuecomment-1841951455