Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed [v3]
On Wed, 24 Apr 2024 02:49:56 GMT, Serguei Spitsyn wrote:
>> This is a simple fix of three similar asserts.
>> The `_target_jt->jvmti_vthread()` has to be used instead of
>> `_target_jt->vthread()`.
>> The `_target_jt->vthread()` can be outdated in some specific contexts as
>> shown in the `hs_err` stack trace.
>>
>> I've seen similar issue and already fixed it in this fragment of code:
>>
>> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
>> . . .
>> void do_vthread(Handle target_h) {
>> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
>> check");
>> // use jvmti_vthread() as vthread() can be outdated
>> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
>> target_h(), "sanity check");
>> . . .
>>
>> The issue above was fixed by replacing `_target_jt->vthread()` with
>> `_target_jt->jvmti_vthread()`.
>>
>> There are three places which need to be fixed the same way:
>> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
>> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
>> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>>
>> Testing:
>> - Run mach5 tiers 1-6
>
> Serguei Spitsyn 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:
>
> - “Merge”
> - review: updated same clarifying comment in several spots
> - add comments explaining that the vthread() can return outdated oop
> - 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() ==
> target_h()) failed
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2079:
> 2077: void
> 2078: GetSingleStackTraceClosure::do_vthread(Handle target_h) {
> 2079: // Use jvmti_vthread() instead of vthread() as target could have
> temporary changed
Suggestion:
// Use jvmti_vthread() instead of vthread() as target could have temporarily
changed
src/hotspot/share/prims/jvmtiEnvBase.hpp line 509:
> 507: void do_vthread(Handle target_h) {
> 508: assert(_target_jt != nullptr, "sanity check");
> 509: // Use jvmti_vthread() instead of vthread() as target could have
> temporary changed
Suggestion:
// Use jvmti_vthread() instead of vthread() as target could have
temporarily changed
src/hotspot/share/prims/jvmtiEnvBase.hpp line 531:
> 529: void do_vthread(Handle target_h) {
> 530: assert(_target_jt != nullptr, "sanity check");
> 531: // Use jvmti_vthread() instead of vthread() as target could have
> temporary changed
Suggestion:
// Use jvmti_vthread() instead of vthread() as target could have
temporarily changed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577299642
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577299918
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1577300305
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v5]
On Wed, 24 Apr 2024 00:01:45 GMT, Serguei Spitsyn wrote: >> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` >> event but has not released the `lockCheck` monitor yet. It has been fixed to >> wait for each `WaitingTask` thread to really reach the `WAITING` state. The >> same approach is used for `EnteringTask` threads. It has been fixed to wait >> for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state. >> >> Testing: >> - Reran the test >> `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally >> - TBD: submit the mach5 tiers 1-6 as well > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: rename ensureWaitsToBeNotified to ensureWaitingToBeNotified Looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18778#pullrequestreview-2018891935
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed [v3]
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Serguei Spitsyn 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:
- “Merge”
- review: updated same clarifying comment in several spots
- add comments explaining that the vthread() can return outdated oop
- 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() ==
target_h()) failed
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18806/files
- new: https://git.openjdk.org/jdk/pull/18806/files/6ac3b54f..c5ad80d0
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18806=02
- incr: https://webrevs.openjdk.org/?repo=jdk=18806=01-02
Stats: 52905 lines in 671 files changed: 26069 ins; 23980 del; 2856 mod
Patch: https://git.openjdk.org/jdk/pull/18806.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18806/head:pull/18806
PR: https://git.openjdk.org/jdk/pull/18806
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed [v2]
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: updated same clarifying comment in several spots
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18806/files
- new: https://git.openjdk.org/jdk/pull/18806/files/55a8ed10..6ac3b54f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18806=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18806=00-01
Stats: 8 lines in 3 files changed: 4 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/18806.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18806/head:pull/18806
PR: https://git.openjdk.org/jdk/pull/18806
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Thank you for review, Leonid!
-
PR Comment: https://git.openjdk.org/jdk/pull/18806#issuecomment-2073695339
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v5]
> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` > event but has not released the `lockCheck` monitor yet. It has been fixed to > wait for each `WaitingTask` thread to really reach the `WAITING` state. The > same approach is used for `EnteringTask` threads. It has been fixed to wait > for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state. > > Testing: > - Reran the test > `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally > - TBD: submit the mach5 tiers 1-6 as well Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: rename ensureWaitsToBeNotified to ensureWaitingToBeNotified - Changes: - all: https://git.openjdk.org/jdk/pull/18778/files - new: https://git.openjdk.org/jdk/pull/18778/files/0b2500d5..24322947 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18778=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18778=03-04 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18778/head:pull/18778 PR: https://git.openjdk.org/jdk/pull/18778
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Marked as reviewed by lmesnik (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/18806#pullrequestreview-2018506993
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Tue, 23 Apr 2024 19:05:38 GMT, Patricio Chilano Mateo
wrote:
>> This is a simple fix of three similar asserts.
>> The `_target_jt->jvmti_vthread()` has to be used instead of
>> `_target_jt->vthread()`.
>> The `_target_jt->vthread()` can be outdated in some specific contexts as
>> shown in the `hs_err` stack trace.
>>
>> I've seen similar issue and already fixed it in this fragment of code:
>>
>> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
>> . . .
>> void do_vthread(Handle target_h) {
>> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
>> check");
>> // use jvmti_vthread() as vthread() can be outdated
>> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
>> target_h(), "sanity check");
>> . . .
>>
>> The issue above was fixed by replacing `_target_jt->vthread()` with
>> `_target_jt->jvmti_vthread()`.
>>
>> There are three places which need to be fixed the same way:
>> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
>> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
>> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>>
>> Testing:
>> - Run mach5 tiers 1-6
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 2079:
>
>> 2077: void
>> 2078: GetSingleStackTraceClosure::do_vthread(Handle target_h) {
>> 2079: // use jvmti_vthread() as vthread() can be outdated
>
> The only reason I can see of why just using vthread() doesn't work is because
> of the case where we are in a temporary switch to carrier thread. So maybe
> change comment to be: "use jvmti_vthread() instead of vthread() as target
> could have temporary changed identity to carrier thread (see
> VirtualThread.switchToCarrierThread)". Same in the other places.
Thank you for the suggestion. Will fix.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1576959637
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Thank you for review, Patricio and Chris!
-
PR Comment: https://git.openjdk.org/jdk/pull/18806#issuecomment-2073550087
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v4]
On Tue, 23 Apr 2024 20:47:34 GMT, Chris Plummer wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: tweak in wait_for_state func to allow exp_state bit mask > > test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java > line 58: > >> 56: native static int setTestedMonitor(Object monitor); >> 57: native static void ensureBlockedOnEnter(Thread thread); >> 58: native static void ensureWaitsToBeNotified(Thread thread); > > This should be named "Waiting" instead of "Waits". Thank you for the suggestion. Will fix. - PR Review Comment: https://git.openjdk.org/jdk/pull/18778#discussion_r1576957455
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Looks good.
-
Marked as reviewed by cjplummer (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18806#pullrequestreview-2018277846
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v4]
On Thu, 18 Apr 2024 21:56:09 GMT, Serguei Spitsyn wrote: >> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` >> event but has not released the `lockCheck` monitor yet. It has been fixed to >> wait for each `WaitingTask` thread to really reach the `WAITING` state. The >> same approach is used for `EnteringTask` threads. It has been fixed to wait >> for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state. >> >> Testing: >> - Reran the test >> `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally >> - TBD: submit the mach5 tiers 1-6 as well > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: tweak in wait_for_state func to allow exp_state bit mask Changes requested by cjplummer (Reviewer). test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java line 58: > 56: native static int setTestedMonitor(Object monitor); > 57: native static void ensureBlockedOnEnter(Thread thread); > 58: native static void ensureWaitsToBeNotified(Thread thread); This should be named "Waiting" instead of "Waits". - PR Review: https://git.openjdk.org/jdk/pull/18778#pullrequestreview-2018273130 PR Review Comment: https://git.openjdk.org/jdk/pull/18778#discussion_r1576875339
Integrated: 8329113: Deprecate -XX:+UseNotificationThread
On Fri, 19 Apr 2024 01:16:46 GMT, Alex Menkov wrote: > The fix deprecates -XX:+UseNotificationThread VM option > > Testing: tier1-3,hs-tier5-svc This pull request has now been integrated. Changeset: 25551662 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/2555166247230497453503318ccbf4dd4f047193 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod 8329113: Deprecate -XX:+UseNotificationThread Reviewed-by: dcubed, dholmes, coleenp - PR: https://git.openjdk.org/jdk/pull/18852
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v7]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons 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 with upstream/master
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18527=06
Stats: 485 lines in 59 files changed: 387 ins; 3 del; 95 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Looks good. Just small suggestion.
Thanks,
Patricio
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2079:
> 2077: void
> 2078: GetSingleStackTraceClosure::do_vthread(Handle target_h) {
> 2079: // use jvmti_vthread() as vthread() can be outdated
The only reason I can see of why just using vthread() doesn't work is because
of the case where we are in a temporary switch to carrier thread. So maybe
change comment to be: "use jvmti_vthread() instead of vthread() as target could
have temporary changed identity to carrier thread (see
VirtualThread.switchToCarrierThread)". Same in the other places.
-
Marked as reviewed by pchilanomate (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18806#pullrequestreview-2018092388
PR Review Comment: https://git.openjdk.org/jdk/pull/18806#discussion_r1576768011
Re: RFR: 8329113: Deprecate -XX:+UseNotificationThread [v2]
> The fix deprecates -XX:+UseNotificationThread VM option > > Testing: tier1-3,hs-tier5-svc Alex Menkov 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 branch 'master' into deprecate_notif_thread - fix - Changes: https://git.openjdk.org/jdk/pull/18852/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18852=01 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18852.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18852/head:pull/18852 PR: https://git.openjdk.org/jdk/pull/18852
Re: RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
On Wed, 17 Apr 2024 00:29:52 GMT, Serguei Spitsyn wrote:
> This is a simple fix of three similar asserts.
> The `_target_jt->jvmti_vthread()` has to be used instead of
> `_target_jt->vthread()`.
> The `_target_jt->vthread()` can be outdated in some specific contexts as
> shown in the `hs_err` stack trace.
>
> I've seen similar issue and already fixed it in this fragment of code:
>
> class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
> . . .
> void do_vthread(Handle target_h) {
> assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity
> check");
> // use jvmti_vthread() as vthread() can be outdated
> assert(_target_jt == nullptr || _target_jt->jvmti_vthread() ==
> target_h(), "sanity check");
> . . .
>
> The issue above was fixed by replacing `_target_jt->vthread()` with
> `_target_jt->jvmti_vthread()`.
>
> There are three places which need to be fixed the same way:
> - `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
> - `SetForceEarlyReturn::do_vthread(Handle target_h)`
> - `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
>
> Testing:
> - Run mach5 tiers 1-6
Ping!! Still need this reviewed.
-
PR Comment: https://git.openjdk.org/jdk/pull/18806#issuecomment-2073075364
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v4]
On Thu, 18 Apr 2024 21:56:09 GMT, Serguei Spitsyn wrote: >> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` >> event but has not released the `lockCheck` monitor yet. It has been fixed to >> wait for each `WaitingTask` thread to really reach the `WAITING` state. The >> same approach is used for `EnteringTask` threads. It has been fixed to wait >> for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state. >> >> Testing: >> - Reran the test >> `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally >> - TBD: submit the mach5 tiers 1-6 as well > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: tweak in wait_for_state func to allow exp_state bit mask Ping!! Need one more review, please. - PR Comment: https://git.openjdk.org/jdk/pull/18778#issuecomment-2073073521
Re: RFR: 8330155: Serial: Remove TenuredSpace [v3]
On Tue, 23 Apr 2024 17:22:41 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch removes the class `TenuredSpace` and adjusts its usages. After >> removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove >> this file and change the included header file to `space.hpp`. >> >> The test `make test-tier1_gc` passed locally. Thanks for taking the time to >> review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li has updated the pull request incrementally with one additional > commit since the last revision: > > Fix included header file error after merging master. I merged the master branch in order to solve the file conflict and added the missed header file after merging. Please take a look at the newest code. - PR Comment: https://git.openjdk.org/jdk/pull/18894#issuecomment-2072993947
Re: RFR: 8330155: Serial: Remove TenuredSpace [v3]
> Hi all, > > This patch removes the class `TenuredSpace` and adjusts its usages. After > removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove > this file and change the included header file to `space.hpp`. > > The test `make test-tier1_gc` passed locally. Thanks for taking the time to > review. > > Best Regards, > -- Guoxiong Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision: Fix included header file error after merging master. - Changes: - all: https://git.openjdk.org/jdk/pull/18894/files - new: https://git.openjdk.org/jdk/pull/18894/files/0796e0b4..5478742c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18894=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18894=01-02 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18894.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18894/head:pull/18894 PR: https://git.openjdk.org/jdk/pull/18894
Re: RFR: 8330155: Serial: Remove TenuredSpace [v2]
> Hi all, > > This patch removes the class `TenuredSpace` and adjusts its usages. After > removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove > this file and change the included header file to `space.hpp`. > > The test `make test-tier1_gc` passed locally. Thanks for taking the time to > review. > > Best Regards, > -- Guoxiong Guoxiong Li has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Remove file. - Merge branch 'master' into REMOVE_TENURED_SPACE # Conflicts: #src/hotspot/share/gc/serial/defNewGeneration.inline.hpp - JDK-8330155 - Changes: https://git.openjdk.org/jdk/pull/18894/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18894=01 Stats: 161 lines in 20 files changed: 10 ins; 127 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/18894.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18894/head:pull/18894 PR: https://git.openjdk.org/jdk/pull/18894
Re: RFR: 8330171: Lazy W^X switch implementation
On Fri, 19 Apr 2024 07:44:17 GMT, Richard Reingruber wrote: >> An alternative for preemptively switching the W^X thread mode on macOS with >> an AArch64 CPU. This implementation triggers the switch in response to the >> SIGBUS signal if the *si_addr* belongs to the CodeCache area. With this >> approach, it is now feasible to eliminate all WX guards and avoid >> potentially costly operations. However, no significant improvement or >> degradation in performance has been observed. Additionally, considering the >> issue with AsyncGetCallTrace, the patched JVM has been successfully operated >> with [asgct_bottom](https://github.com/parttimenerd/asgct_bottom) and >> [async-profiler](https://github.com/async-profiler/async-profiler). >> >> Additional testing: >> - [x] MacOS AArch64 server fastdebug *gtets* >> - [ ] MacOS AArch64 server fastdebug *jtreg:hotspot:tier4* >> - [ ] Benchmarking >> >> @apangin and @parttimenerd could you please check the patch on your >> scenarios?? > > What about granting `WXWrite` only if the current thread is in > `_thread_in_vm`? > That would be more restrictive and roughly equivalent how it currently works. > Likely there are some places then that should be granted `WXWrite` eagerly > because they need `WXWrite` without `_thread_in_vm`. E.g. the JIT compiler > threads should have `WXWrite` and never `WXExec` (I assume) which should be > checked in the signal handler. > The patch doesn't protect against native agents, as this is obviously > impossible. The current code doesn't do that either. For the bytecode, it > doesn't prevent the attacker from abusing unsafe api to modify code cache. > However unsafe functions are already considered "safe" and we proactively > enable WXWrite as well as move thread to `_thread_in_vm` state (@reinrich). > JITed code can't write to the cache either with or without the patch. > > I totally get the sense of loss of security. But is this really the case? I think it is. W^X is intended (amongst other things) to protect against the use of gadgets, from buffer overflow exploits in non-java code to ROP programming. At present, in order to generate code and execute it, you first have to be able to make the JIT code writable, then write the code, then make it executable. then jump to the code. And the exploit writer might have to do some or all of this by finding gadgets. If we were to merge this patch then all the attacker would have to do is write code to memory and find a way to jump to it, and the automatic switch-on-segfault in this patch would do the all the work the attacker needs. It makes far more sense to tag those places that actually need to change W^X access, and only switch there. You could argue that any switching of W^X on a write to code space, then switching it back on jumping (or returning) to Java code, even what we already do, is effectively the same thing. Kinda, but it's not on just any attempt to write to code space or any attempt to jump into code, it's at the places we choose, and we can be careful to limit those places. But surely the JDK is not the most vulnerable part of the stack anyway? I'd agree with that, of course, but I don't think that's sufficient reason to decide to bypass an OS security mechanism. We are trying to reduce the size of the attack surface. - PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2072639243
Integrated: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 15:26:52 GMT, Matias Saavedra Silva wrote: > Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), > [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic > operands needed to be rewritten to encoded values to better distinguish indy > entries from other cp cache entries. The above changes now distinguish > between entries with `to_cp_index()` using the bytecode, which is now > propagated by the callers. > > The encoding flips the bits of the index so the encoded index is always > negative, leading to access errors if there is no matching decode call. These > calls are removed with some methods adjusted to distinguish between indices > with the bytecode. Verified with tier 1-5 tests. The changes show no issues > when tested against libgraal. This pull request has now been integrated. Changeset: 383fe6ea Author:Matias Saavedra Silva URL: https://git.openjdk.org/jdk/commit/383fe6eaab423a1218c9915362f691472e3773e7 Stats: 225 lines in 37 files changed: 15 ins; 137 del; 73 mod 8330388: Remove invokedynamic cache index encoding Reviewed-by: cjplummer, dlong, coleenp - PR: https://git.openjdk.org/jdk/pull/18819
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:48:16 GMT, Dean Long wrote: >> Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), >> [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and >> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic >> operands needed to be rewritten to encoded values to better distinguish indy >> entries from other cp cache entries. The above changes now distinguish >> between entries with `to_cp_index()` using the bytecode, which is now >> propagated by the callers. >> >> The encoding flips the bits of the index so the encoded index is always >> negative, leading to access errors if there is no matching decode call. >> These calls are removed with some methods adjusted to distinguish between >> indices with the bytecode. Verified with tier 1-5 tests. The changes show no >> issues when tested against libgraal. > > Did you consider minimizing changes by leaving > decode_invokedynamic_index/encode_invokedynamic_index calls in place, but > having the implementations not change the value? Thanks for the reviews @dean-long @gilles-duboscq @coleenp and @plummercj! - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2072603376
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 14:25:22 GMT, Magnus Ihse Bursie wrote: > There's a huge amount of changes for just hsdis... You might have to separate > out the infrastructure changes that seem to amount to most of the changes > here. > > This is going to take me a while to get through. Sorry, it's still a WIP, and I believe something broke and scattered changes from one of my other local branches into this current changeset - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2072483937
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 13:56:32 GMT, Julian Waters wrote: > WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. There's a huge amount of changes for just hsdis... You might have to separate out the infrastructure changes that seem to amount to most of the changes here. This is going to take me a while to get through. - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2072458273
RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
WIP This changeset contains hsdis for Windows/gcc Port. It supports both the binutils and capstone backends, though the LLVM backend is left out due to compatibility issues encountered during the build. Currently, which gcc distributions are supported is still to be clarified, as several, ranging from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, the build system hack in place at the moment to compile the binutils backend on Windows is still left in place, for now. - Commit messages: - Implementation of hsdis for 8288293: Windows/gcc Port Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18915=00 Issue: https://bugs.openjdk.org/browse/JDK-8330988 Stats: 347 lines in 28 files changed: 161 ins; 22 del; 164 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8329113: Deprecate -XX:+UseNotificationThread
On Fri, 19 Apr 2024 01:16:46 GMT, Alex Menkov wrote: > The fix deprecates -XX:+UseNotificationThread VM option > > Testing: tier1-3,hs-tier5-svc Looks good. Thank you for doing this. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18852#pullrequestreview-2017009355
Re: RFR: 8330171: Lazy W^X switch implementation
On Fri, 19 Apr 2024 07:44:17 GMT, Richard Reingruber wrote: >> An alternative for preemptively switching the W^X thread mode on macOS with >> an AArch64 CPU. This implementation triggers the switch in response to the >> SIGBUS signal if the *si_addr* belongs to the CodeCache area. With this >> approach, it is now feasible to eliminate all WX guards and avoid >> potentially costly operations. However, no significant improvement or >> degradation in performance has been observed. Additionally, considering the >> issue with AsyncGetCallTrace, the patched JVM has been successfully operated >> with [asgct_bottom](https://github.com/parttimenerd/asgct_bottom) and >> [async-profiler](https://github.com/async-profiler/async-profiler). >> >> Additional testing: >> - [x] MacOS AArch64 server fastdebug *gtets* >> - [ ] MacOS AArch64 server fastdebug *jtreg:hotspot:tier4* >> - [ ] Benchmarking >> >> @apangin and @parttimenerd could you please check the patch on your >> scenarios?? > > What about granting `WXWrite` only if the current thread is in > `_thread_in_vm`? > That would be more restrictive and roughly equivalent how it currently works. > Likely there are some places then that should be granted `WXWrite` eagerly > because they need `WXWrite` without `_thread_in_vm`. E.g. the JIT compiler > threads should have `WXWrite` and never `WXExec` (I assume) which should be > checked in the signal handler. The patch doesn't protect against native agents, as this is obviously impossible. The current code doesn't do that either. For the bytecode, it doesn't prevent the attacker from abusing unsafe api to modify code cache. However unsafe functions are already considered "safe" and we proactively enable WXWrite as well as move thread to `_thread_in_vm` state (@reinrich). JITed code can't write to the cache either with or without the patch. I totally get the sense of loss of security. But is this really the case? - PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2071988941
Re: RFR: 8330155: Serial: Remove TenuredSpace
On Mon, 22 Apr 2024 16:24:06 GMT, Guoxiong Li wrote: > Hi all, > > This patch removes the class `TenuredSpace` and adjusts its usages. After > removing `TenuredSpace`, the file `space.inline.hpp` is empty, so I remove > this file and change the included header file to `space.hpp`. > > The test `make test-tier1_gc` passed locally. Thanks for taking the time to > review. > > Best Regards, > -- Guoxiong Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18894#pullrequestreview-2016759032
