8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… #123

lmesnik · 2020-09-10T23:38:45Z

The NULL oops are corrupted by CheckUnhandledOops and should be re-written with NULL to pass testing with -XX:+CheckUnhandledOops.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize(...) ⚠️ Title mismatch between PR and JBS.

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/123/head:pull/123
$ git checkout pull/123

…(...)

bridgekeeper · 2020-09-10T23:40:06Z

👋 Welcome back lmesnik! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-09-10T23:41:07Z

@lmesnik The following label will be automatically applied to this pull request: hotspot-runtime.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

mlbridge · 2020-09-10T23:44:35Z

Webrevs

01: Full - Incremental (c89edef)
00: Full (0db863a)

lmesnik · 2020-09-10T23:54:56Z

/label serviceability

openjdk · 2020-09-10T23:55:13Z

@lmesnik
The serviceability label was successfully added.

coleenp · 2020-09-11T00:19:47Z

src/hotspot/share/services/threadService.cpp

@@ -886,6 +886,9 @@ void ThreadSnapshot::initialize(ThreadsList * t_list, JavaThread* thread) {
      _thread_status == java_lang_Thread::IN_OBJECT_WAIT_TIMED) {

    Handle obj = ThreadService::get_current_contended_monitor(thread);


There must be a safepoint here then.
I think this would be better and safer if blocker_object and blocker_object_owner are Handles. Can you change them to Handles?

I can't see anywhere a safepoint check would occur in that code. This issue was flagged as being in Loom so perhaps the loom code is different and is what introduces the safepoint check?
But I agree with Coleen that the best solution is to just use Handles.

It is not loom-specific and reproduced n jdk/jdk with -XX:+CheckUnhandledOops.

What do you think about moving
Handle obj = ThreadService::get_current_contended_monitor(thread);
out of scope of block_object oop visibility?
It is my second patch.

I'm missing something. How can a NULL oop get corrupted even if there is a GC?

This is a specific of "CheckUnhandledOops"
I've written in bug comment "Another possible fix would be to disable corruption of NULL unhandled oops. They couldn't be changed really."

We discussed it with Coleen and seems that moving NULL oops out of possible safepoint or handling them seems easier option than changing UnhandledOops.cpp to don't corrupt NULL. It is here:

jdk/src/hotspot/share/runtime/unhandledOops.cpp

Line 113 in 77bdc30

void UnhandledOops::clear_unhandled_oops() {

ThreadService::get_current_contended_monitor calls Thread::check_for_dangling_thread_pointer calls ThreadsSMRSupport::is_a_protected_JavaThread_with_lock((JavaThread *) thread),

The potential safepoint is here, where CheckUnhandledOops puts junk in any oop on the stack.

inline bool ThreadsSMRSupport::is_a_protected_JavaThread_with_lock(JavaThread *thread) {
MutexLocker ml(Threads_lock->owned_by_self() ? NULL : Threads_lock);
return is_a_protected_JavaThread(thread);
}

Thanks Coleen. I'm still not sure that CheckUnhandledOops should be touching NULL oops but ...

Leonid the workaround seems okay.

dholmes-ora · 2020-09-11T02:49:04Z

src/hotspot/share/services/threadService.cpp

@@ -886,6 +886,9 @@ void ThreadSnapshot::initialize(ThreadsList * t_list, JavaThread* thread) {
      _thread_status == java_lang_Thread::IN_OBJECT_WAIT_TIMED) {

    Handle obj = ThreadService::get_current_contended_monitor(thread);


I can't see anywhere a safepoint check would occur in that code. This issue was flagged as being in Loom so perhaps the loom code is different and is what introduces the safepoint check?
But I agree with Coleen that the best solution is to just use Handles.

…(...)

openjdk · 2020-09-11T18:02:56Z

@lmesnik This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize…

Reviewed-by: coleenp, dholmes

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 59 commits pushed to the master branch:

5c0d985: 8253060: remove test/hotspot/jtreg/test_env.sh
9525186: 8253031: git jcheck complains about invalid tags in jdk repo after fix for JDK-8252844
7f27d0b: 8236142: DocTrees should provide getCharacters(EntityTree)
e7a1b9b: 8252981: ObjectMonitor::object() cleanup changes extracted from JDK-8247281
040c8f5: 8250914: Matcher::stack_direction() is unused
9687dca: 8252494: C2: TypeAryPtr::cast_to_autobox_cache does not use ptr_type
8777ded: 8253008: Remove develop flags TraceLongCompiles/LongCompileThreshold
c7062dc: 8253026: Remove dummy call to gc alot from VM Thread
5144190: 8243208: Clean up JVMFlag implementation
976acdd: 8252406: Introduce Thread::as_Java_thread() convenience function
... and 49 more: https://git.openjdk.java.net/jdk/compare/5d2e79e2c38ff79c1d5712aa77d5ac98eef9f97c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 5c0d985abf7ef89f9a035692fe9db3ef4001bc2c.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

lmesnik · 2020-09-11T23:02:50Z

/integrate

openjdk · 2020-09-11T23:03:54Z

@lmesnik Since your change was applied there have been 59 commits pushed to the master branch:

5c0d985: 8253060: remove test/hotspot/jtreg/test_env.sh
9525186: 8253031: git jcheck complains about invalid tags in jdk repo after fix for JDK-8252844
7f27d0b: 8236142: DocTrees should provide getCharacters(EntityTree)
e7a1b9b: 8252981: ObjectMonitor::object() cleanup changes extracted from JDK-8247281
040c8f5: 8250914: Matcher::stack_direction() is unused
9687dca: 8252494: C2: TypeAryPtr::cast_to_autobox_cache does not use ptr_type
8777ded: 8253008: Remove develop flags TraceLongCompiles/LongCompileThreshold
c7062dc: 8253026: Remove dummy call to gc alot from VM Thread
5144190: 8243208: Clean up JVMFlag implementation
976acdd: 8252406: Introduce Thread::as_Java_thread() convenience function
... and 49 more: https://git.openjdk.java.net/jdk/compare/5d2e79e2c38ff79c1d5712aa77d5ac98eef9f97c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 306b166.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize…

0db863a

…(...)

openjdk bot added the rfr Pull request is ready for review label Sep 10, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 10, 2020

openjdk bot added the serviceability serviceability-dev@openjdk.org label Sep 10, 2020

coleenp suggested changes Sep 11, 2020

View reviewed changes

dholmes-ora suggested changes Sep 11, 2020

View reviewed changes

8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize…

c89edef

…(...)

coleenp approved these changes Sep 11, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 11, 2020

dholmes-ora approved these changes Sep 11, 2020

View reviewed changes

openjdk bot closed this Sep 11, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 11, 2020

lmesnik deleted the checkhandle branch September 30, 2020 22:11

pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024

MethodKey.compareTo in StringMethodConstraints is broken. openjdk#123

0bc5d59

pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024

MethodKey.compareTo in StringMethodConstraints is broken. openjdk#123

1596580

pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024

MethodKey.compareTo in StringMethodConstraints is broken. openjdk#123

a407b46

pfirmstone pushed a commit to pfirmstone/jdk-with-authorization that referenced this pull request Nov 18, 2024

MethodKey.compareTo in StringMethodConstraints is broken. openjdk#123

3414cad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… #123

8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… #123

lmesnik commented Sep 10, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 10, 2020

openjdk bot commented Sep 10, 2020

mlbridge bot commented Sep 10, 2020 •

edited

Loading

lmesnik commented Sep 10, 2020

openjdk bot commented Sep 10, 2020

coleenp Sep 11, 2020

dholmes-ora Sep 11, 2020

lmesnik Sep 11, 2020

lmesnik Sep 11, 2020

dholmes-ora Sep 11, 2020

lmesnik Sep 11, 2020

coleenp Sep 11, 2020

dholmes-ora Sep 11, 2020

dholmes-ora Sep 11, 2020

openjdk bot commented Sep 11, 2020 •

edited

Loading

lmesnik commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

		@@ -886,6 +886,9 @@ void ThreadSnapshot::initialize(ThreadsList * t_list, JavaThread* thread) {
		_thread_status == java_lang_Thread::IN_OBJECT_WAIT_TIMED) {

		Handle obj = ThreadService::get_current_contended_monitor(thread);

8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… #123

8253033: CheckUnhandledOops check fails in ThreadSnapshot::initialize… #123

Conversation

lmesnik commented Sep 10, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 10, 2020

openjdk bot commented Sep 10, 2020

mlbridge bot commented Sep 10, 2020 • edited Loading

Webrevs

lmesnik commented Sep 10, 2020

openjdk bot commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Sep 11, 2020 • edited Loading

lmesnik commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

lmesnik commented Sep 10, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 10, 2020 •

edited

Loading

openjdk bot commented Sep 11, 2020 •

edited

Loading