Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274794: Make Thread::_owned_locks available in product #5896

Closed
wants to merge 3 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Oct 11, 2021

Make thread owned locks available in product and change the error message printing to print by thread so that not only locks in mutexLocker are printed in the hs_err file.
Tested with tier1-3 and with product build, and tested hs_err files manually with some selected temporary ShouldNotReachHere, also new test.


Progress

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

Issue

  • JDK-8274794: Make Thread::_owned_locks available in product

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5896/head:pull/5896
$ git checkout pull/5896

Update a local copy of the PR:
$ git checkout pull/5896
$ git pull https://git.openjdk.java.net/jdk pull/5896/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5896

View PR using the GUI difftool:
$ git pr show -t 5896

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5896.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 11, 2021

👋 Welcome back coleenp! 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 openjdk bot added the rfr label Oct 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 11, 2021

@coleenp The following label will be automatically applied to this pull request:

  • hotspot

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 pull request command.

@openjdk openjdk bot added the hotspot label Oct 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 11, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Coleen,

Sorry but I have significant concerns with parts of this change. First it isn't safe to print the owned locks of all threads during error reporting as those threads are still running and could be updating their list of owned threads.

I also think there is still need for the _mutex_array because that allows you to check what each mutex thinks its state is, independently of what the threads think ie. if there's a bug in the owned_locks code and a locked lock was mistakenly elided, then you now have no way to know that.

My understanding of the motivation for this change was to allow printing of the dynamically defined locks held by the current thread during error reporting - but this change tries to go way beyond that.

Thanks,
David

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Oct 12, 2021

in opposite order, I've found the mutex_array to be not useful at all for debugging - didn't know it existed for 20yrs. You can check the lock you're interested in directly because it's a global name. The small chance of a thread adding a lock during error reporting and potential crash, is handled by the error reporting "STEP" framework, so I don't see that as a concern.
There are a few locks that would be useful to see in error handling (like the HandshakeState_lock) that are not covered by the mutex_array because they're not global. It would have helped debug some bug I was looking for several months ago.
I was actually motivated to do this because of some awful looking code in #5590, but apparently that's been removed.
My real motivation, besides accurate error reporting, is to remove the 'def' macros in mutexLocker.cpp so that the default case of _allow_vm_block isn't specified, so that we can see some safepoint checking locks that pass true which block safepoints that really should not pass true. Some of these are cut/paste code.
This is the change that wanted to get to:
38b58c5

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Oct 12, 2021

I have a better patch, withdrawing this one.

@coleenp coleenp closed this Oct 12, 2021
@coleenp coleenp deleted the owned-locks branch Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot rfr
2 participants