Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v2]
On Fri, 17 May 2024 02:08:34 GMT, Leonid Mesnik wrote: >> The JvmtiTrace::safe_get_thread_name sometimes crashes when called while >> current thread is in native thread state. >> >> It happens when thread_name is set for tracing from jvmti functions. >> See: >> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649 >> >> >> The setup is called and the thread name is used in tracing before the thread >> transition. There is no good location where this method could be called from >> vm thread_state only. Some functions like raw monitor enter/exit never >> transition in vm state. So sometimes it is needed to call this function from >> native thread state. >> >> The change should affect JVMTI trace mode only (-XX:TraceJVMTI). >> >> Verified by running jvmti/jdi/jdb tests with tracing enabled. > > Leonid Mesnik 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: > > - copyrights updated. > - Merge branch 'master' of https://github.com/openjdk/jdk into 8332259 > - include updated. > - 8332259 I have to wonder whether this solution will potentially cause problems because the code will now block for safepoints. We could fallback to `Thread::name()` if the current thread is in-native. - PR Review: https://git.openjdk.org/jdk/pull/19275#pullrequestreview-2062389545
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 00:38:07 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/prims/jvmti.xml line 1008: >> >>> 1006: function descriptions. Empty lists, arrays, sequences, etc are >>> 1007: returned as nullptr which is C programming language >>> 1008: null pointer. >> >> Perhaps instead something like >> >> "returned as a null pointer (C NULL or C++ >> nullptr)." >> >> "null pointer" is the generic phrase used in both the C and C++ standards. > > Thank you, Kim. I like this suggestion. Updated now. That part looks okay but I think all the parameters and error descriptions changed by JDK-8324680 will now need to change to use "null" instead of "nullptr". - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1604344850
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 02:00:29 GMT, David Holmes wrote: > But this clarification doesn't actually clarify that the rest of the spec > uses `nullptr`. Based on the proposed wording I would expect things like: > > ``` > The function may return nullptr > ``` > > to say > > ``` > The function may return a null pointer > ``` Looking at this again, I think I agree with @dholmes-ora . Some of the relevant places are text, and should be using "null pointer". Some are example code or the like. Those should be using NULL rather than nullptr, since we have this text early on: "Unless otherwise stated, all examples and declarations in this specification use the C language." I didn't find any that were described as C++ rather than C. So JDK-8324680 was somewhat mistaken about what needed to be done, and what was done. - PR Comment: https://git.openjdk.org/jdk/pull/19257#issuecomment-2116606245
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 00:43:18 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: corrected the nullptr clarification src/hotspot/share/prims/jvmti.xml line 1007: > 1005: explicitly deallocate. This is indicated in the individual > 1006: function descriptions. Empty lists, arrays, sequences, etc are > 1007: returned as a null pointer (C NULL or C++ > nullptr). This may be a little unnecessary rigor, but I believe that `nullptr` is not a null pointer. `nullptr` is the pointer literal that can be implicitly converted to a null pointer value of any pointer type and any pointer to member type. And I think the thing returned here is a null pointer, not `nullptr`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1604313245
Re: RFR: 8332259: JvmtiTrace::safe_get_thread_name fails if current thread is in native state [v2]
> The JvmtiTrace::safe_get_thread_name sometimes crashes when called while > current thread is in native thread state. > > It happens when thread_name is set for tracing from jvmti functions. > See: > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649 > > > The setup is called and the thread name is used in tracing before the thread > transition. There is no good location where this method could be called from > vm thread_state only. Some functions like raw monitor enter/exit never > transition in vm state. So sometimes it is needed to call this function from > native thread state. > > The change should affect JVMTI trace mode only (-XX:TraceJVMTI). > > Verified by running jvmti/jdi/jdb tests with tracing enabled. Leonid Mesnik 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: - copyrights updated. - Merge branch 'master' of https://github.com/openjdk/jdk into 8332259 - include updated. - 8332259 - Changes: - all: https://git.openjdk.org/jdk/pull/19275/files - new: https://git.openjdk.org/jdk/pull/19275/files/a2b1942b..c534c91b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19275=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19275=00-01 Stats: 30226 lines in 654 files changed: 18294 ins; 7842 del; 4090 mod Patch: https://git.openjdk.org/jdk/pull/19275.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19275/head:pull/19275 PR: https://git.openjdk.org/jdk/pull/19275
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 00:43:18 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: corrected the nullptr clarification But this clarification doesn't actually clarify that the rest of the spec uses `nullptr`. Based on the proposed wording I would expect things like: The function may return nullptr to say The function may return a null pointer - Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19257#pullrequestreview-2062190669
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Fri, 17 May 2024 00:43:18 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: corrected the nullptr clarification Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19257#pullrequestreview-2062184501
RFR: 8332259: JvmtiTrace::safe_get_thread_name is not safe
The JvmtiTrace::safe_get_thread_name sometimes crashes when called while current thread is in native thread state. It happens when thread_name is set for tracing from jvmti functions. See: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/jvmtiEnter.xsl#L649 The setup is called and the thread name is used in tracing before the thread transition. There is no good location where this method could be called from vm thread_state only. Some functions like raw monitor enter/exit never transition in vm state. So sometimes it is needed to call this function from native thread state. The change should affect JVMTI trace mode only (-XX:TraceJVMTI). Verified by running jvmti/jdi/jdb tests with tracing enabled. - Commit messages: - include updated. - 8332259 Changes: https://git.openjdk.org/jdk/pull/19275/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19275=00 Issue: https://bugs.openjdk.org/browse/JDK-8332259 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19275.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19275/head:pull/19275 PR: https://git.openjdk.org/jdk/pull/19275
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
> The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: corrected the nullptr clarification - Changes: - all: https://git.openjdk.org/jdk/pull/19257/files - new: https://git.openjdk.org/jdk/pull/19257/files/fd0e8d43..9fe639e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19257=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19257=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19257/head:pull/19257 PR: https://git.openjdk.org/jdk/pull/19257
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Thu, 16 May 2024 07:59:51 GMT, Kim Barrett wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: corrected the nullptr clarification > > src/hotspot/share/prims/jvmti.xml line 1008: > >> 1006: function descriptions. Empty lists, arrays, sequences, etc are >> 1007: returned as nullptr which is C programming language >> 1008: null pointer. > > Perhaps instead something like > > "returned as a null pointer (C NULL or C++ > nullptr)." > > "null pointer" is the generic phrase used in both the C and C++ standards. Thank you, Kim. I like this suggestion. Updated now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1604210615
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v2]
On Thu, 16 May 2024 19:26:07 GMT, Chris Plummer wrote: >> src/hotspot/share/prims/jvmti.xml line 1008: >> >>> 1006: function descriptions. Empty lists, arrays, sequences, etc are >>> 1007: returned as nullptr which is C programming language >>> 1008: null pointer. >> >> Shouldn't this be "NULL"? In any case, I think it would be helpful to >> expand this a bit to make it clear that usages of "nullptr" in parameter and >> error descriptions should be read or treated as "NULL" when developing an >> agent in C rather than C++. > > Yes, I think it should by NULL. Thank you for suggestions. I've changed it as Kim suggested below. Please, let me know if it is not good enough, or some additions are needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1604211567
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
On Thu, 16 May 2024 02:37:40 GMT, Serguei Spitsyn wrote: > The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) Changes requested by kbarrett (Reviewer). src/hotspot/share/prims/jvmti.xml line 1008: > 1006: function descriptions. Empty lists, arrays, sequences, etc are > 1007: returned as nullptr which is C programming language > 1008: null pointer. Perhaps instead something like "returned as a null pointer (C NULL or C++ nullptr)." "null pointer" is the generic phrase used in both the C and C++ standards. - PR Review: https://git.openjdk.org/jdk/pull/19257#pullrequestreview-2059896023 PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1602805633
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v2]
On Thu, 16 May 2024 19:53:48 GMT, Sean Mullan wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> IllegalArgumentException throws doc update
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
> line 959:
>
>> 957: * NotificationFilters. Elements of this array can
>> 958: * be null.
>> 959: * @param delegationSubjects should be {@code null}, but a non-null
>
> Would it be more clear to say: "should be {@code null}. However, an array
> where every entry is null is also accepted for compatibility reasons."
Yes, I like adding the "also".
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
> line 960:
>
>> 958: * be null.
>> 959: * @param delegationSubjects should be {@code null}, but a non-null
>> 960: * array is accepted for compatibilty reasons, which must not
>> contain
>
> Typo: s/compatibilty/compatibility/
oops yes thanks, done.
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
> line 969:
>
>> 967: * @throws IllegalArgumentException if names or
>> 968: * filters is null, or if names contains
>> 969: * a null element, or if these two arrays do not have the same size.
>
> Was this actually an oversight in the previous change to remove subject
> delegation? When `delegationSubjects` is null, then the 3 arrays are never
> going to be the same size.
Yes, this is an oversight from the previous change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603976969
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603974595
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603975797
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
> Running JConsole from a previous JDK, and attaching to jdk-23 (after > [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java > Management Extension (JMX) Subject Delegation feature), the MBean tab is > blank. > > In javax/management/remote/rmi/RMIConnectionImpl.java: > addNotificationListener rejects a non-null delegationSubjects array, but > older JDKs will send such an array. It could accept the array, and only > reject/throw if it contains a non-null Subject (i.e. if an attempt to use > subject delegation is really happening). > > Manually testing JConsole, the MBean tab is fully populated and usable. Kevin Walls has updated the pull request incrementally with two additional commits since the last revision: - add an 'also' - typo - Changes: - all: https://git.openjdk.org/jdk/pull/19253/files - new: https://git.openjdk.org/jdk/pull/19253/files/6c97b5ed..68c791e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19253=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19253=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19253.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19253/head:pull/19253 PR: https://git.openjdk.org/jdk/pull/19253
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v2]
On Thu, 16 May 2024 11:46:30 GMT, Kevin Walls wrote:
>> Running JConsole from a previous JDK, and attaching to jdk-23 (after
>> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
>> Management Extension (JMX) Subject Delegation feature), the MBean tab is
>> blank.
>>
>> In javax/management/remote/rmi/RMIConnectionImpl.java:
>> addNotificationListener rejects a non-null delegationSubjects array, but
>> older JDKs will send such an array. It could accept the array, and only
>> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
>> subject delegation is really happening).
>>
>> Manually testing JConsole, the MBean tab is fully populated and usable.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> IllegalArgumentException throws doc update
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 959:
> 957: * NotificationFilters. Elements of this array can
> 958: * be null.
> 959: * @param delegationSubjects should be {@code null}, but a non-null
Would it be more clear to say: "should be {@code null}. However, an array where
every entry is null is also accepted for compatibility reasons."
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 960:
> 958: * be null.
> 959: * @param delegationSubjects should be {@code null}, but a non-null
> 960: * array is accepted for compatibilty reasons, which must not contain
Typo: s/compatibilty/compatibility/
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 969:
> 967: * @throws IllegalArgumentException if names or
> 968: * filters is null, or if names contains
> 969: * a null element, or if these two arrays do not have the same size.
Was this actually an oversight in the previous change to remove subject
delegation? When `delegationSubjects` is null, then the 3 arrays are never
going to be the same size.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603957397
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603965135
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603963533
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Wed, 15 May 2024 06:00:46 GMT, Serguei Spitsyn wrote: >> I'm not sure this answered Chris' query properly. Or I'm reading Chris' >> query wrong. >> >> Perhaps this is not what Chris had in mind, but I'm wondering what happens >> in some >> Thread-A when it is checked and passed by but then Thread-A sets the flag in >> itself >> after the for-loop has passed it by. Does that Thread-A flag value get lost? > >> Perhaps this is not what Chris had in mind, but I'm wondering what happens >> in some >> Thread-A when it is checked and passed by but then Thread-A sets the flag in >> itself >> after the for-loop has passed it by. Does that Thread-A flag value get lost? > > Thank you for the question. > The Thread-A sets the flag optimistically and then re-checks if > `sync_protocol_enabled()` and any disabler exists. It can be global disbaler > (`_VTMS_transition_disable_for_all_count > 0`) or disabler of `Thread-A` only > (`java_lang_Thread::VTMS_transition_disable_count(vth()) > 0`). If any > disabler exists then `Thread-A` clears the optimistic settings and goes with > the pessimistic approach under protection of `JvmtiVTMSTransition_lock`. > > Please, let me know if you still have questions. This algorithm sounds correct. Thanks for closing the loop on my belated comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1603957324
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
On Thu, 16 May 2024 07:57:58 GMT, Alan Bateman wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > src/hotspot/share/prims/jvmti.xml line 1008: > >> 1006: function descriptions. Empty lists, arrays, sequences, etc are >> 1007: returned as nullptr which is C programming language >> 1008: null pointer. > > Shouldn't this be "NULL"? In any case, I think it would be helpful to expand > this a bit to make it clear that usages of "nullptr" in parameter and error > descriptions should be read or treated as "NULL" when developing an agent in > C rather than C++. Yes, I think it should by NULL. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1603929609
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 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 one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
src/java.base/share/classes/java/lang/System.java line 2023:
> 2021: * @throws NullPointerException if {@code filename} is {@code
> null}
> 2022: * @throws IllegalCallerException If the caller is in a module
> that
> 2023: * does not have native access enabled.
The exception description is fine, just noticed the other exception
descriptions start with a lowercase "if", this one is different.
src/java.base/share/man/java.1 line 587:
> 585: \f[V]deny\f[R]: This mode disables all illegal native access except for
> 586: those modules enabled by the \f[V]--enable-native-access\f[R]
> 587: command-line option.
"This mode disable all illegal native access except for those modules enabled
the --enable-native-access command-line option".
This can be read to mean that modules granted native access with the command
line option is also illegal native access An alternative is to make the second
part of the sentence a new sentence, something like "Only modules enabled by
the --enable-native-access command line option may perform native access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603878829
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603875920
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v7]
On Thu, 16 May 2024 15:31:15 GMT, Chris Plummer wrote: >> This PR adds ranked monitor support to the debug agent. The debug agent has >> a large number of monitors, and it's really hard to know which order to grab >> them in, and for that matter which monitors might already be held at any >> given moment. By imposing a rank on each monitor, we can check to make sure >> they are always grabbed in the order of their rank. Having this in place >> when I was working on >> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made >> it much easier to detect a deadlock that was occuring, and the reason for >> it. That's what motivated me to do this work >> >> There were 2 or 3 minor rank issues discovered as a result of these changes. >> I also learned a lot about some of the more ugly details of the locking >> support in the process. >> >> Tested with the following on all supported platforms and with virtual >> threads: >> >> com/sun/jdi >> vmTestbase/nsk/jdi >> vmTestbase/nsk/jdb >> vmTestbase/nsk/jdwp >> >> Still need to run tier2 and tier5. >> >> Details of the changes follow in the first comment. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify by getting rid of special logic around leaf monitors. src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 1781: > 1779: * popFrameEventLock. Meanwhile when the target thread gets the > SINGLE_STEP event, > 1780: * it always enters popFrameProceedLock first (which is always > available), then > 1781: * popFrameProceedLock second. It will always succeed because the > Reader thread Is that "then popFrameEventLock second" Drawing these out in two columns I can't see a deadlock either 8-) - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1603777908
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v7]
On Thu, 16 May 2024 15:31:15 GMT, Chris Plummer wrote: >> This PR adds ranked monitor support to the debug agent. The debug agent has >> a large number of monitors, and it's really hard to know which order to grab >> them in, and for that matter which monitors might already be held at any >> given moment. By imposing a rank on each monitor, we can check to make sure >> they are always grabbed in the order of their rank. Having this in place >> when I was working on >> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made >> it much easier to detect a deadlock that was occuring, and the reason for >> it. That's what motivated me to do this work >> >> There were 2 or 3 minor rank issues discovered as a result of these changes. >> I also learned a lot about some of the more ugly details of the locking >> support in the process. >> >> Tested with the following on all supported platforms and with virtual >> threads: >> >> com/sun/jdi >> vmTestbase/nsk/jdi >> vmTestbase/nsk/jdb >> vmTestbase/nsk/jdwp >> >> Still need to run tier2 and tier5. >> >> Details of the changes follow in the first comment. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify by getting rid of special logic around leaf monitors. src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 1749: > 1747: * the JDWP Command Reader thread and the PopFrame() target thread will > grab > 1748: * both of these locks. However, one curious trait of these two locks > is that > 1749: * the these two threads do not both grab them in the same order (and > they need nit: "that the these two" - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1603728625
Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Thu, 16 May 2024 11:00:33 GMT, Johan Sjölen wrote: > Please wait 24 hours before attempting to integrate. In other words: Do not > sponsor this until 24 hours has passed since opening the PR. OTOH, I would say this is good and _trivial_, so the usual 24 hour rule does not really apply. :) - PR Comment: https://git.openjdk.org/jdk/pull/19254#issuecomment-2115695457
Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin wrote: > The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has > [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd > ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs. > > This broke > [async-profiler](https://github.com/async-profiler/async-profiler/), since > the profiler needs this field to obtain jmethodID in some corner cases. > > There was no actual need for removal, as the field is still there in > InstanceKlass. So, I propose to return the field back to restore the broken > functionality of async-profiler. This is a no risk change, because it only > exports an offset of one field and does not affect the JVM otherwise. Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19254#pullrequestreview-2061331053
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v7]
> This PR adds ranked monitor support to the debug agent. The debug agent has a > large number of monitors, and it's really hard to know which order to grab > them in, and for that matter which monitors might already be held at any > given moment. By imposing a rank on each monitor, we can check to make sure > they are always grabbed in the order of their rank. Having this in place when > I was working on [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) > would have made it much easier to detect a deadlock that was occuring, and > the reason for it. That's what motivated me to do this work > > There were 2 or 3 minor rank issues discovered as a result of these changes. > I also learned a lot about some of the more ugly details of the locking > support in the process. > > Tested with the following on all supported platforms and with virtual threads: > > com/sun/jdi > vmTestbase/nsk/jdi > vmTestbase/nsk/jdb > vmTestbase/nsk/jdwp > > Still need to run tier2 and tier5. > > Details of the changes follow in the first comment. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Simplify by getting rid of special logic around leaf monitors. - Changes: - all: https://git.openjdk.org/jdk/pull/19044/files - new: https://git.openjdk.org/jdk/pull/19044/files/1c6a2e34..c3bd1716 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19044=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19044=05-06 Stats: 36 lines in 2 files changed: 3 ins; 23 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/19044.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19044/head:pull/19044 PR: https://git.openjdk.org/jdk/pull/19044
Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin wrote: > The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has > [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd > ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs. > > This broke > [async-profiler](https://github.com/async-profiler/async-profiler/), since > the profiler needs this field to obtain jmethodID in some corner cases. > > There was no actual need for removal, as the field is still there in > InstanceKlass. So, I propose to return the field back to restore the broken > functionality of async-profiler. This is a no risk change, because it only > exports an offset of one field and does not affect the JVM otherwise. This is fine to revert. I didn't see this field in SA so eagerly removed it. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19254#pullrequestreview-2060857374
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
> 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 one additional commit since the last revision: Add note on --illegal-native-access default value in the launcher help - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/1c45e5d5..3a0db276 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19213=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19213=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Thu, 16 May 2024 11:55:35 GMT, Jaikiran Pai wrote: >> We already do, see >> https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582 > > This is slightly different from what we do in the other PR for unsafe memory > access where we specify the default in the launcher's help text too > https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219. > > I don't have a strong opinion on this, I think either one is fine. Ah, apologies, I was looking in the wrong place. I agree that we should specify default in the launcher, as well as in the man pages. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603233038
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Thu, 16 May 2024 11:47:13 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/sun/launcher/resources/launcher.properties line >> 72: >> >>> 70: \ by code in modules for which native access is not >>> explicitly enabled.\n\ >>> 71: \ is one of "deny", "warn" or "allow".\n\ >>> 72: \ This option will be removed in a future release.\n\ >> >> Should this specify the current default value for this option if it isn't >> set? > > We already do, see > https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582 This is slightly different from what we do in the other PR for unsafe memory access where we specify the default in the launcher's help text too https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219. I don't have a strong opinion on this, I think either one is fine. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603205279
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Thu, 16 May 2024 11:42:48 GMT, Jaikiran Pai wrote: > Hello Maurizio, in the current mainline, we have code in `LauncherHelper` > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636 > where we enable native access to all unnamed modules if an executable jar > with `Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such > executable jars, what is the expected semantics when the launch also > explicitly has a `--enable-native-access=M1,M2` option. Something like: > > ``` > java --enable-native-access=M1,M2 -jar foo.jar > ``` > > where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest. The options are additive - e.g. the enable-native-access in the manifest will add up to the enable-native-access in the command line, so effectively it will be as if you were running with --enable-native-access=M1,M2,ALL-UNNAMED > src/java.base/share/classes/sun/launcher/resources/launcher.properties line > 72: > >> 70: \ by code in modules for which native access is not >> explicitly enabled.\n\ >> 71: \ is one of "deny", "warn" or "allow".\n\ >> 72: \ This option will be removed in a future release.\n\ > > Should this specify the current default value for this option if it isn't set? We already do, see https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582 - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115012361 PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603195671
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v2]
On Thu, 16 May 2024 11:38:28 GMT, Daniel Fuchs wrote: >> Yes, completely understand. I just don't think it has any benefit. > > Hmmm... the spec still says: > > * @throws IllegalArgumentException if names or > * filters is null, or if names contains > * a null element, or if the three arrays do not all have the same > * size. > ``` Thanks for additionally spotting the throws IAE needs updating, adding that now... (Will update the CSR also.) - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603191504
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v2]
> Running JConsole from a previous JDK, and attaching to jdk-23 (after > [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java > Management Extension (JMX) Subject Delegation feature), the MBean tab is > blank. > > In javax/management/remote/rmi/RMIConnectionImpl.java: > addNotificationListener rejects a non-null delegationSubjects array, but > older JDKs will send such an array. It could accept the array, and only > reject/throw if it contains a non-null Subject (i.e. if an attempt to use > subject delegation is really happening). > > Manually testing JConsole, the MBean tab is fully populated and usable. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: IllegalArgumentException throws doc update - Changes: - all: https://git.openjdk.org/jdk/pull/19253/files - new: https://git.openjdk.org/jdk/pull/19253/files/ef84485e..6c97b5ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19253=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19253=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19253.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19253/head:pull/19253 PR: https://git.openjdk.org/jdk/pull/19253
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Wed, 15 May 2024 16:08:17 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 one > additional commit since the last revision: > > Address review comment Hello Maurizio, in the current mainline, we have code in `LauncherHelper` https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636 where we enable native access to all unnamed modules if an executable jar with `Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such executable jars, what is the expected semantics when the launch also explicitly has a `--enable-native-access=M1,M2` option. Something like: java --enable-native-access=M1,M2 -jar foo.jar where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115005638
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Thu, 16 May 2024 10:37:36 GMT, Kevin Walls wrote: >> This shows that when SubjectDelegation was not used, a null-filled array of >> the same length as the two other arrays was expected before (in previous >> versions of the JDK where SubjectDelegation was supported, but in the case >> where it wasn't used). >> I am not suggesting to document the length requirement. The length >> requirement was enforced before and undocumented. I'm just suggesting that >> we allow null and null-filled but don't allow something (null filled array >> of wrong length) that would have been rejeceted in previous JDK versions. I >> would also suggest to check the length before the content - in case an array >> is supplied. > > Yes, completely understand. I just don't think it has any benefit. Hmmm... the spec still says: * @throws IllegalArgumentException if names or * filters is null, or if names contains * a null element, or if the three arrays do not all have the same * size. ``` - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603184648
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Wed, 15 May 2024 16:08:17 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 one > additional commit since the last revision: > > Address review comment src/java.base/share/classes/sun/launcher/resources/launcher.properties line 72: > 70: \ by code in modules for which native access is not > explicitly enabled.\n\ > 71: \ is one of "deny", "warn" or "allow".\n\ > 72: \ This option will be removed in a future release.\n\ Should this specify the current default value for this option if it isn't set? - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603157916
Integrated: 8332098: Add missing @ since tags to jdk.jdi
On Sun, 12 May 2024 01:58:38 GMT, Nizar Benalla wrote: > Please review this simple change where I add "@ since" tags to the > package-info file of the following packages > > com.sun.jdi > com.sun.jdi.connect > com.sun.jdi.connect.spi > com.sun.jdi.event > com.sun.jdi.request > > I used the unix grep command to find the oldest "@ since" in each package and > used that value, as it's hard to get the source code of JDK 1-5 > TIA This pull request has now been integrated. Changeset: a33cb904 Author:Nizar Benalla Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/a33cb9045b2c0cae7d894715d1484e77b7607de6 Stats: 15 lines in 5 files changed: 10 ins; 0 del; 5 mod 8332098: Add missing @ since tags to jdk.jdi Reviewed-by: alanb, cjplummer - PR: https://git.openjdk.org/jdk/pull/19200
Re: RFR: 8332098: Add missing @ since tags to jdk.jdi [v2]
On Mon, 13 May 2024 19:30:31 GMT, Nizar Benalla wrote: >> Please review this simple change where I add "@ since" tags to the >> package-info file of the following packages >> >> com.sun.jdi >> com.sun.jdi.connect >> com.sun.jdi.connect.spi >> com.sun.jdi.event >> com.sun.jdi.request >> >> I used the unix grep command to find the oldest "@ since" in each package >> and used that value, as it's hard to get the source code of JDK 1-5 >> TIA > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > empty commit No changes have been done to the PR since it was last approved by Chris few days back (that's a good thing). I'll go ahead and sponsor this now. - PR Comment: https://git.openjdk.org/jdk/pull/19200#issuecomment-2114924570
Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin wrote: > The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has > [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd > ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs. > > This broke > [async-profiler](https://github.com/async-profiler/async-profiler/), since > the profiler needs this field to obtain jmethodID in some corner cases. > > There was no actual need for removal, as the field is still there in > InstanceKlass. So, I propose to return the field back to restore the broken > functionality of async-profiler. This is a no risk change, because it only > exports an offset of one field and does not affect the JVM otherwise. Please wait 24 hours before attempting to integrate. In other words: Do not sponsor this until 24 hours has passed since opening the PR. - PR Comment: https://git.openjdk.org/jdk/pull/19254#issuecomment-2114911425
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Thu, 16 May 2024 10:26:25 GMT, Daniel Fuchs wrote: >> That the JConsole tab was blank shows that the older RMIConnector's >> addListenerWithSubject creates a new single-entry array from the given >> delegationSubject (which is null) and passes it onwards. The app is not >> creating the array itself., that's us. >> >> (also, maybe that JConsole relies on listeners in order to show a screen >> that doesn't really need to depend on them, but this change is obviously >> about being compatible with that) >> >> We all know that that is the only use case out there, the current wisdom is >> that this feature is not used, nobody is creating the Subject array and >> calling addListenersWithSubjects (plural) with it... >> >> IF we find such an app app, we are going to ignore the array unless it >> contains a non-null entry. This seems safe and efficient. We are >> documenting that it should be null, and it is weird to document a length >> requirement for something that should be null... 8-) > > This shows that when SubjectDelegation was not used, a null-filled array of > the same length as the two other arrays was expected before (in previous > versions of the JDK where SubjectDelegation was supported, but in the case > where it wasn't used). > I am not suggesting to document the length requirement. The length > requirement was enforced before and undocumented. I'm just suggesting that we > allow null and null-filled but don't allow something (null filled array of > wrong length) that would have been rejeceted in previous JDK versions. I > would also suggest to check the length before the content - in case an array > is supplied. Yes, completely understand. I just don't think it has any benefit. - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603106058
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Thu, 16 May 2024 10:16:58 GMT, Kevin Walls wrote: >> Well my thinking was this: the fact that the jconsole tab was blank shows >> that the array may being passed. The previous code verified that all three >> arrays had the same length - so it would have failed if the array had a >> length different than the other two. So I would prefer if we kept on >> throwing in that case. In other words, we now allow and prefer `null` - but >> if non-null - we will allow a null-filled array passed by an older client >> but we should not accept something that would have been invalid then. > > That the JConsole tab was blank shows that the older RMIConnector's > addListenerWithSubject creates a new single-entry array from the given > delegationSubject (which is null) and passes it onwards. The app is not > creating the array itself., that's us. > > (also, maybe that JConsole relies on listeners in order to show a screen that > doesn't really need to depend on them, but this change is obviously about > being compatible with that) > > We all know that that is the only use case out there, the current wisdom is > that this feature is not used, nobody is creating the Subject array and > calling addListenersWithSubjects (plural) with it... > > IF we find such an app app, we are going to ignore the array unless it > contains a non-null entry. This seems safe and efficient. We are > documenting that it should be null, and it is weird to document a length > requirement for something that should be null... 8-) This shows that when SubjectDelegation was not used, a null-filled array of the same length as the two other arrays was expected before (in previous versions of the JDK where SubjectDelegation was supported, but in the case where it wasn't used). I am not suggesting to document the length requirement. The length requirement was enforced before and undocumented. I'm just suggesting that we allow null and null-filled but don't allow something (null filled array of wrong length) that would have been rejeceted in previous JDK versions. I would also suggest to check the length before the content - in case an array is supplied. - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603087272
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Thu, 16 May 2024 09:40:58 GMT, Daniel Fuchs wrote: >> That seems pretty extreme -- it's an array we explicitly don't want and are >> going to ignore? If somebody passes a non-null member, we will throw as >> unsupported. I was thinking that was enough, hence removing the sbjs array >> to be quite certain we can't pass on any supplied Subjects. > > Well my thinking was this: the fact that the jconsole tab was blank shows > that the array may being passed. The previous code verified that all three > arrays had the same length - so it would have failed if the array had a > length different than the other two. So I would prefer if we kept on throwing > in that case. In other words, we now allow and prefer `null` - but if > non-null - we will allow a null-filled array passed by an older client but > we should not accept something that would have been invalid then. That the JConsole tab was blank shows that the older RMIConnector's addListenerWithSubject creates a new single-entry array from the given delegationSubject (which is null) and passes it onwards. The app is not creating the array itself., that's us. (also, maybe that JConsole relies on listeners in order to show a screen that doesn't really need to depend on them, but this change is obviously about being compatible with that) We all know that that is the only use case out there, the current wisdom is that this feature is not used, nobody is creating the Subject array and calling addListenersWithSubjects (plural) with it... IF we find such an app app, we are going to ignore the array unless it contains a non-null entry. This seems safe and efficient. We are documenting that it should be null, and it is weird to document a length requirement for something that should be null... 8-) - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603064299
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 20:02:26 GMT, Kevin Walls wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
>> line 984:
>>
>>> 982: }
>>> 983: if (names.length != filters.length) {
>>> 984: final String msg = "The lengths of names and filters
>>> parameters are not same.";
>>
>> I wonder if we should check that if present, `delegationSubjects.length ==
>> names.length`?
>
> That seems pretty extreme -- it's an array we explicitly don't want and are
> going to ignore? If somebody passes a non-null member, we will throw as
> unsupported. I was thinking that was enough, hence removing the sbjs array
> to be quite certain we can't pass on any supplied Subjects.
Well my thinking was this: the fact that the jconsole tab was blank shows that
the array may being passed. The previous code verified that all three arrays
had the same length - so it would have failed if the array had a length
different than the other two. So I would prefer if we kept on throwing in that
case. In other words, we now allow and prefer `null` - but if non-null - we
will allow a null-filled array passed by an older client but we should not
accept something that would have been invalid then.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603000965
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 22:44:03 GMT, Serguei Spitsyn wrote:
>> Running JConsole from a previous JDK, and attaching to jdk-23 (after
>> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
>> Management Extension (JMX) Subject Delegation feature), the MBean tab is
>> blank.
>>
>> In javax/management/remote/rmi/RMIConnectionImpl.java:
>> addNotificationListener rejects a non-null delegationSubjects array, but
>> older JDKs will send such an array. It could accept the array, and only
>> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
>> subject delegation is really happening).
>>
>> Manually testing JConsole, the MBean tab is fully populated and usable.
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 979:
>
>> 977: for (Subject s: delegationSubjects) {
>> 978: if (s != null) {
>> 979: throw new UnsupportedOperationException("Subject
>> Delegation has been removed.");
>
> Q1: Would it make sense to provide any details about the non-null
> `delegationSubject` element?
> Q2: Does this fix needs a CSR? My guess is that this issue is very minor, so
> CSR is not needed.
Q1 We have a couple of places where we use that text, when a non-null Subject
is given. It's quite generic and that is a good fit. Your app would have to
go to a significant effort to use the Subject Delegation feature, and we aren't
aware of any apps that use it. If JConsole were using the feature, you would
know as you would have had to specifically configure it somehow (as well as on
the remote end with SM and policies), so I really think the generic message is
explicit enough.
Q2 Yes, Daniel very quickly added the CSR flag. I was presuming that
"paperwork" would need doing, although yes as spec changes go, it is very very
minor.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1602917924
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 23:40:00 GMT, Chris Plummer wrote: > If jconsole is passing null, why is it triggering this exception? JConsole passes null, but when running on an older jdk, the older RMIConnector actually "promotes" it to an array before making the remote call. If you connect to a jdk-23 with the removal, the exception is thrown. (JConsole running on jdk-23 can connect to jdk-23 fine.) - PR Comment: https://git.openjdk.org/jdk/pull/19253#issuecomment-2114453521
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
On Thu, 16 May 2024 02:37:40 GMT, Serguei Spitsyn wrote: > The following RFE was fixed recently: > [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with > nullptr in JVMTI generated code > > It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI > agents can be developed in C or C++. > This update is to make it clear that `nullptr` is C programming language > `null` pointer. > > I think we do not need a CSR for this fix. > > Testing: N/A (not needed) src/hotspot/share/prims/jvmti.xml line 1008: > 1006: function descriptions. Empty lists, arrays, sequences, etc are > 1007: returned as nullptr which is C programming language > 1008: null pointer. Shouldn't this be "NULL"? In any case, I think it would be helpful to expand this a bit to make it clear that usages of "nullptr" in parameter and error descriptions should be read or treated as "NULL" when developing an agent in C rather than C++. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1602803174
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Wed, 15 May 2024 13:32:38 GMT, Magnus Ihse Bursie wrote:
> Hi Julian, sorry for not getting back to you.
>
> The problem from my PoV is that this is a very large and intrusive patch in
> the build of the actual product, for a claimed problem in the tangential
> hsdis library. I have not understood a pressing business need to get a
> Windows/gcc port for hsdis, which means I can't really prioritize trying to
> understand this patch.
>
> As you know, the build system does not currently really separate between "the
> OS is Windows" and "the toolchain is Microsoft". I understand that you want
> to fix that, and in theory I support it. However, you must also realize that
> making a complete fix of this requires a lot of work, for something that
> there is no clearly defined need. (After all, cl.exe works fine to create the
> build, has no apparent issues, and is as far as an "official" compiler for
> Windows as you can get.) That makes it fall squarely in the WIBNIs bucked
> ("wouldn't it be nice if...").
>
> If you can fix just the hsdis build without changing anything outside the
> hsdis Makefiles, that would be a different story. Such a change would be
> limited in scope, easy to say it will not affect the product proper, and be
> easier to review.
Oh, no worries. Sorry for sounding a little impatient.
The problem is that there are areas in the build system that will need changing
to support hsdis compilation, not just the hsdis Makefile and autoconf file
itself. I can try to work on minimizing the amount of changes to non-hsdis
files (I was hoping the current changes were small enough, but it seems they
are not), but I believe it's impossible to achieve this by only touching the
hsdis Makefile and lib-hsdis.m4. That is, there will necessarily be changes, no
matter how minimal, to non-hsdis files.
As for why do this at all, I was mainly driven by seeing the hack in place for
the binutils backend on Windows. It only supports Cygwin, of which the gcc
installation is subpar compared to the newer gcc provided by some MSYS2
subsystems (MINGW64 gcc is primarily battle tested on MSYS2, on Cygwin it
doesn't get as much attention and misses out further fixes and enhancements
from the MSYS2 team, for instance it even links to the obsolete msvcrt
library), and the hack itself has several flaws that don't seem apparent at
first (For instance, -lpthread will link to libwinpthread.dll and result in an
invisible dependency on that dll depending on the Windows platform, which will
cause loading hsdis to silently fail depending on how it's loaded for seemingly
random reasons!). I wanted to replace that (or I guess provide a better
alternative) by adding semi proper support to the hsdis build for the more
modern and better battle tested gcc that some MSYS2 subsystems provide, to
streamline comp
iling the binutils hsdis on Windows. So this is mainly about hsdis-binutils on
Windows. hsdis-capstone I also decided to support because it's small and
relatively easy to pile on top of the existing work, and MSYS2 also provides
Capstone as well. The same unfortunately could not be said for hsdis-llvm,
which was significantly more complex than Capstone is
I'm not sure where to go from here. I could support gcc for hsdis binutils by
extending the hack that exists for Cygwin, but I _really_ don't want to do
that. I could enhance the build system to semi properly support gcc for hsdis
only, as I've done, but the changes will necessarily touch non hsdis files as
well, no matter how minimal they are. I'll return this to draft for the time
being I suppose, I'd be interested in hearing which way forward you prefer
though
But while I work on making this change even smaller and easier to review, could
I ask the above questions again? (Well, except for the FIXPATH one, you can
ignore that)
> @magicus I think I might need some help here. Currently all the Cygwin stuff
> is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be
> behind isBuildOsEnv cygwin checks instead? I don't know how to add support
> for Cygwin gcc for most of hsdis, since it is quite different from the more
> modern gcc distributions that are found in places like MSYS2 and MINGW64. But
> most of the existing logic seems to accomodate more for the Microsoft
> compiler than it is concerned about the OS environment being used, and for
> this reason I can't tell which of the 2 checks to use for the existing hack
> that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but
> Microsoft does, but I don't want to check for microsoft inside
> TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get
> FIXPATH while Microsoft does?
> Also @magicus, what is the typical path passed to --with-binutils like on
> Windows? Something like
> --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work
> correctly, since the include path to dis-asm.h
