Re: RFR: JDK-8229829: java/lang/management/ThreadMXBean/Locks.java fails with java.lang.RuntimeException: Thread WaitingThread is at WAITING state but is expected to be in Thread.State = WAITING
+1 Thanks, David On 15/05/2020 6:18 am, serguei.spit...@oracle.com wrote: Hi Alex, LGTM. Thanks, Serguei On 5/14/20 11:04, Alex Menkov wrote: I agree with the point. updated webrev (only WAITING handling is added): http://cr.openjdk.java.net/~amenkov/jdk15/Locks_waiting/webrev.2/ --alex On 05/13/2020 19:20, David Holmes wrote: Hi Alex, On 14/05/2020 10:55 am, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8229829 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/Locks_waiting/webrev/ The fix adds handling for WAITING That part is good. (and for consistency TIMED_WAITING But this could be a problem. I know what your intent is but we are waiting to observe a thread transition to a state that it can't escape from, but with TIMED_WAITING the thread can escape - when the timeout elapses. So it is possible that the target becomes TIMED_WAITING before you check the state, but then leaves that state due to timeout, and then you check the state and will loop until you trigger a failure. Cheers, David - which is not used in the test) states as it has for BLOCKED state. --alex
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil,
Thank you for the update!
It looks good to me.
> Regarding CR for the JDWP spec issues
I guess, you wanted to say "CSR", not CR. :)
Thanks,
Serguei
On 5/14/20 13:33, Daniil Titov wrote:
Hi Serguei and Chris,
Please review a new version of the change [1] that addresses your comments.
Testing: Mach5 tier1-tier5 tests successfully passed.
Regarding CR for the JDWP spec issues related to missing type information in
some of the protocol packet descriptions [3], as Chris has just noticed
we really don't need it, so I withdrew this CR.
[1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8241080
[3] https://bugs.openjdk.java.net/browse/JDK-8245057
Thank you,
Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, May 11, 2020 at 11:53 AM
To: Daniil Titov , serviceability-dev
Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability
tools
Hi Daniil,
It looks pretty good in general.
A couple of nits below.
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html
+void *cursor;
+jbyte argumentTag;
+jint argIndex = 0;
+jvalue *argument = request->arguments;;
. . .
void *cursor;
jint argIndex = 0;
+jbyte argumentTag;
jvalue *argument = request->arguments;
It is better if the local variables 'cursor' and 'argumentTag' get initialized.
There is double semicolon: "arguments;;"
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html
43 static inline jbyte basicType(const char *signature) {
It'd be nice to rename it to basicTypeTag() to get it unified with other
functions below.
It is more safe to run tier5 as well.
Thanks,
Serguei
On 5/9/20 09:29, Daniil Titov wrote:
Please review a change[1] that centralizes the signature processing in
serviceability tools to make it capable of being easily extensible in the
future.
Testing: Mach5 tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01
[2] https://bugs.openjdk.java.net/browse/JDK-8241080
Thank you,
Daniil
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Serguei and Chris,
Please review a new version of the change [1] that addresses your comments.
Testing: Mach5 tier1-tier5 tests successfully passed.
Regarding CR for the JDWP spec issues related to missing type information in
some of the protocol packet descriptions [3], as Chris has just noticed
we really don't need it, so I withdrew this CR.
[1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8241080
[3] https://bugs.openjdk.java.net/browse/JDK-8245057
Thank you,
Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, May 11, 2020 at 11:53 AM
To: Daniil Titov , serviceability-dev
Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability
tools
Hi Daniil,
It looks pretty good in general.
A couple of nits below.
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html
+void *cursor;
+jbyte argumentTag;
+jint argIndex = 0;
+jvalue *argument = request->arguments;;
. . .
void *cursor;
jint argIndex = 0;
+jbyte argumentTag;
jvalue *argument = request->arguments;
It is better if the local variables 'cursor' and 'argumentTag' get initialized.
There is double semicolon: "arguments;;"
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html
43 static inline jbyte basicType(const char *signature) {
It'd be nice to rename it to basicTypeTag() to get it unified with other
functions below.
It is more safe to run tier5 as well.
Thanks,
Serguei
On 5/9/20 09:29, Daniil Titov wrote:
Please review a change[1] that centralizes the signature processing in
serviceability tools to make it capable of being easily extensible in the
future.
Testing: Mach5 tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01
[2] https://bugs.openjdk.java.net/browse/JDK-8241080
Thank you,
Daniil
Re: RFR: JDK-8229829: java/lang/management/ThreadMXBean/Locks.java fails with java.lang.RuntimeException: Thread WaitingThread is at WAITING state but is expected to be in Thread.State = WAITING
Hi Alex, LGTM. Thanks, Serguei On 5/14/20 11:04, Alex Menkov wrote: I agree with the point. updated webrev (only WAITING handling is added): http://cr.openjdk.java.net/~amenkov/jdk15/Locks_waiting/webrev.2/ --alex On 05/13/2020 19:20, David Holmes wrote: Hi Alex, On 14/05/2020 10:55 am, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8229829 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/Locks_waiting/webrev/ The fix adds handling for WAITING That part is good. (and for consistency TIMED_WAITING But this could be a problem. I know what your intent is but we are waiting to observe a thread transition to a state that it can't escape from, but with TIMED_WAITING the thread can escape - when the timeout elapses. So it is possible that the target becomes TIMED_WAITING before you check the state, but then leaves that state due to timeout, and then you check the state and will loop until you trigger a failure. Cheers, David - which is not used in the test) states as it has for BLOCKED state. --alex
Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
Hi Alex, LGTM. Thanks, Serguei On 5/14/20 11:30, Alex Menkov wrote: Hi Alan, Serguei, updated webrev: http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.3/ --alex On 05/14/2020 04:25, Alan Bateman wrote: On 12/05/2020 20:57, Alex Menkov wrote: Hi Alan, Serguei, lets try one more time :) What about: Agents can transform classes in arbitrary ways at load time, transform modules, or transform the bytecode of methods of already loaded classes. Developers or administrators that deploy agents, deploy applications that package an agent with the application, or use tools that load agents into a running application, are responsible for verifying the trustworthiness of each agent including the content and structure of the agent JAR file. please let me know what do you thinks, I'll prepare & publish new webrev as soon as we get agreement about the paragraph. This version looks okay to me. -Alan
Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
On 14/05/2020 19:30, Alex Menkov wrote: Hi Alan, Serguei, updated webrev: http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.3/ Thanks. -Alan
Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
Hi Alan, Serguei, updated webrev: http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.3/ --alex On 05/14/2020 04:25, Alan Bateman wrote: On 12/05/2020 20:57, Alex Menkov wrote: Hi Alan, Serguei, lets try one more time :) What about: Agents can transform classes in arbitrary ways at load time, transform modules, or transform the bytecode of methods of already loaded classes. Developers or administrators that deploy agents, deploy applications that package an agent with the application, or use tools that load agents into a running application, are responsible for verifying the trustworthiness of each agent including the content and structure of the agent JAR file. please let me know what do you thinks, I'll prepare & publish new webrev as soon as we get agreement about the paragraph. This version looks okay to me. -Alan
Re: RFR: JDK-8229829: java/lang/management/ThreadMXBean/Locks.java fails with java.lang.RuntimeException: Thread WaitingThread is at WAITING state but is expected to be in Thread.State = WAITING
I agree with the point. updated webrev (only WAITING handling is added): http://cr.openjdk.java.net/~amenkov/jdk15/Locks_waiting/webrev.2/ --alex On 05/13/2020 19:20, David Holmes wrote: Hi Alex, On 14/05/2020 10:55 am, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8229829 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/Locks_waiting/webrev/ The fix adds handling for WAITING That part is good. (and for consistency TIMED_WAITING But this could be a problem. I know what your intent is but we are waiting to observe a thread transition to a state that it can't escape from, but with TIMED_WAITING the thread can escape - when the timeout elapses. So it is possible that the target becomes TIMED_WAITING before you check the state, but then leaves that state due to timeout, and then you check the state and will loop until you trigger a failure. Cheers, David - which is not used in the test) states as it has for BLOCKED state. --alex
RFR(XS): 8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD and/or OBSOLETE method(s) found
Please, review a fix for The Kitchensink bug: https://bugs.openjdk.java.net/browse/JDK-8222005 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/ Summary: The VM_RedefineClasses::doit() uses two helper classes to walk all VM classes. First is AdjustAndCleanMetadata to adjust method entries in the vtables/itables/cpcaches. Second is CheckClass to check that adjustments for all method entries are correct. The Kitchensink test is failing with two modes: - guarantee(false) failed: OLD and/or OBSOLETE method(s) found in the VM_RedefineClasses::CheckClass::do_klass() - SIGSEGV in the ConstantPoolCacheEntry::get_interesting_method_entry() in context of VM_RedefineClasses::CheckClass::do_klass() execution The second failure mode is rare. In is before the first one in the code path. The root cause of both is that the VM_RedefineClasses::AdjustAndCleanMetadata::do_klass() is skipping the cpcache update for classes that are being redefined assuming they are being redefined by the current VM_RedefineClasses operation. In such cases, the adjustment is not needed as the cpcache is empty. The problem is that the assumption above is wrong. The class can also be redefined by another VM_RedefineClasses operation which has already executed its doit_prologue. The cpcache djustment for such class is necessary. The fix is to always call the cp_cache->adjust_method_entries() even if the class is being redefined by the current VM_RedefineClasses operation. It is possible to skip it but it will add extra complexity to the code. The fix also includes minor tweak in the cpCache.cpp to include method's class name to the redefinition cpcache log. Testing: Ran Kitchensink test locally on a Linux server with the Instrumentation module enabled. The test does not fail anymore. In progress, a mach5 tiers 1-5 and runs and separate mach5 Kitchensink run. Thanks, Serguei
Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
On 12/05/2020 20:57, Alex Menkov wrote: Hi Alan, Serguei, lets try one more time :) What about: Agents can transform classes in arbitrary ways at load time, transform modules, or transform the bytecode of methods of already loaded classes. Developers or administrators that deploy agents, deploy applications that package an agent with the application, or use tools that load agents into a running application, are responsible for verifying the trustworthiness of each agent including the content and structure of the agent JAR file. please let me know what do you thinks, I'll prepare & publish new webrev as soon as we get agreement about the paragraph. This version looks okay to me. -Alan
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Serguei,
> Thank you for the bug report update - it is helpful.
> The fix/update looks good in general but I need more time to check some
> points.
Sure. I'd be happy if you could look at it again.
> I'm thinking it would be more safe to run full tier5.
> I can do it after you get all thumbs ups.
The patch goes through extensive testing here at SAP every night since many
weeks. Still it would be
great if you could run full tier5.
I'll wait then for a view more thumbs...
Thanks, Richard.
-Original Message-
From: serguei.spit...@oracle.com
Sent: Donnerstag, 14. Mai 2020 00:32
To: Reingruber, Richard ; Patricio Chilano
; Vladimir Ivanov
; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
Hi Richard,
Thank you for the bug report update - it is helpful.
The fix/update looks good in general but I need more time to check some
points.
I'm thinking it would be more safe to run full tier5.
I can do it after you get all thumbs ups.
Thanks,
Serguei
On 4/24/20 01:18, Reingruber, Richard wrote:
> Hi Patricio, Vladimir, and Serguei,
>
> now that direct handshakes are available, I've updated the patch to make use
> of them.
>
> In addition I have done some clean-up changes I missed in the first webrev.
>
> Finally I have implemented the workaround suggested by Patricio to avoid
> nesting the handshake
> into the vm operation VM_SetFramePop [1]
>
> Kindly review again:
>
> Webrev:http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/
> Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/
>
> I updated the JBS item explaining why the vm operation VM_EnterInterpOnlyMode
> can be replaced with a
> direct handshake:
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8238585
>
> Testing:
>
> * JCK and JTREG tests, also in Xcomp mode with fastdebug and release builds
> on all platforms.
>
> * Submit-repo: mach5-one-rrich-JDK-8238585-20200423-1436-10441737
>
> Thanks,
> Richard.
>
> [1] An assertion in Handshake::execute_direct() fails, if called be VMThread,
> because it is no JavaThread.
>
> -Original Message-
> From: hotspot-dev On Behalf Of
> Reingruber, Richard
> Sent: Freitag, 14. Februar 2020 19:47
> To: Patricio Chilano ;
> serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
> hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
> hotspot-gc-...@openjdk.java.net
> Subject: RE: RFR(S) 8238585: Use handshake for
> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
> methods on stack not_entrant
>
> Hi Patricio,
>
>> > I'm really glad you noticed the problematic nesting. This seems to be
> a general issue: currently a
>> > handshake cannot be nested in a vm operation. Maybe it should be
> asserted in the
>> > Handshake::execute() methods that they are not called by the vm thread
> evaluating a vm operation?
>> >
>> >> Alternatively I think you could do something similar to what we
> do in
>> >> Deoptimization::deoptimize_all_marked():
>> >>
>> >>EnterInterpOnlyModeClosure hs;
>> >>if (SafepointSynchronize::is_at_safepoint()) {
>> >> hs.do_thread(state->get_thread());
>> >>} else {
>> >> Handshake::execute(&hs, state->get_thread());
>> >>}
>> >> (you could pass “EnterInterpOnlyModeClosure” directly to the
>> >> HandshakeClosure() constructor)
>> >
>> > Maybe this could be used also in the Handshake::execute() methods as
> general solution?
>> Right, we could also do that. Avoiding to clear the polling page in
>> HandshakeState::clear_handshake() should be enough to fix this issue and
>> execute a handshake inside a safepoint, but adding that "if" statement
>> in Hanshake::execute() sounds good to avoid all the extra code that we
>> go through when executing a handshake. I filed 8239084 to make that
> change.
>
> Thanks for taking care of this and creating the RFE.
>
>>
>> >> I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode
> is
>> >> always called in a nested operation or just sometimes.
>> >
>> > At least one execution path without vm operation exists:
>> >
>> >
> JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *) : void
>> >
> JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState *) :
> jlong
>> >JvmtiEventControllerPrivate::recompute_enabled() : void
>> > JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent,
> bool) : void (2 matches)
>> >JvmtiEventController::change_field_watch(
RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Ok. Thanks for the feedback anyways.
Cheers, Richard.
-Original Message-
From: David Holmes
Sent: Donnerstag, 14. Mai 2020 07:29
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
methods on stack not_entrant
> Still not a review, or is it now?
I'd say still not a review as I'm only looking at the general structure.
Cheers,
David
On 14/05/2020 1:37 am, Reingruber, Richard wrote:
> Hi David,
>
>> On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
>>> // Trimmed the list of recipients. If the list gets too long then the
>>> message needs to be approved
>>> // by a moderator.
>
>> Yes I noticed that too :) In general if you send to hotspot-dev you
>> shouldn't need to also send to hotspot-X-dev.
>
> Makes sense. Will do so next time.
>
>>>
>>> This would be the post with the current webrev.1
>>>
>>>
>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
>
>> Sorry I missed that update. Okay so this is working with direct
>> handshakes now.
>
>> One style nit in jvmtiThreadState.cpp:
>
>> assert(SafepointSynchronize::is_at_safepoint() ||
>> ! (JavaThread *)Thread::current() == get_thread() ||
>> ! Thread::current() == get_thread()->active_handshaker(),
>> ! "bad synchronization with owner thread");
>
>> the ! lines should ident as follows
>
>> assert(SafepointSynchronize::is_at_safepoint() ||
>> (JavaThread *)Thread::current() == get_thread() ||
>> Thread::current() == get_thread()->active_handshaker(),
>>! "bad synchronization with owner thread");
>
> Sure.
>
>> Lets see how this plays out.
>
> Hopefully not too bad... :)
>
>>> Not a review but some general commentary ...
>
> Still not a review, or is it now?
>
> Thanks, Richard.
>
> -Original Message-
> From: David Holmes
> Sent: Mittwoch, 13. Mai 2020 07:43
> To: Reingruber, Richard ;
> serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net;
> hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net;
> hotspot-gc-...@openjdk.java.net
> Subject: Re: RFR(S) 8238585: Use handshake for
> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled
> methods on stack not_entrant
>
> On 4/05/2020 8:33 pm, Reingruber, Richard wrote:
>> // Trimmed the list of recipients. If the list gets too long then the
>> message needs to be approved
>> // by a moderator.
>
> Yes I noticed that too :) In general if you send to hotspot-dev you
> shouldn't need to also send to hotspot-X-dev.
>
>> Hi David,
>
> Hi Richard,
>
>>> On 28/04/2020 12:09 am, Reingruber, Richard wrote:
Hi David,
> Not a review but some general commentary ...
That's welcome.
>>
>>> Having had to take an even closer look now I have a review comment too :)
>>
>>> src/hotspot/share/prims/jvmtiThreadState.cpp
>>
>>> void JvmtiThreadState::invalidate_cur_stack_depth() {
>>> ! assert(SafepointSynchronize::is_at_safepoint() ||
>>> ! (Thread::current()->is_VM_thread() &&
>>> get_thread()->is_vmthread_processing_handshake()) ||
>>> (JavaThread *)Thread::current() == get_thread(),
>>> "must be current thread or at safepoint");
>>
>> You're looking at an outdated webrev, I'm afraid.
>>
>> This would be the post with the current webrev.1
>>
>>
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html
>
> Sorry I missed that update. Okay so this is working with direct
> handshakes now.
>
> One style nit in jvmtiThreadState.cpp:
>
> assert(SafepointSynchronize::is_at_safepoint() ||
> ! (JavaThread *)Thread::current() == get_thread() ||
> ! Thread::current() == get_thread()->active_handshaker(),
> ! "bad synchronization with owner thread");
>
> the ! lines should ident as follows
>
> assert(SafepointSynchronize::is_at_safepoint() ||
> (JavaThread *)Thread::current() == get_thread() ||
> Thread::current() == get_thread()->active_handshaker(),
>! "bad synchronization with owner thread");
>
> Lets see how this plays out.
>
> Cheers,
> David
>
>> Thanks, Richard.
>>
>> -Original Message-
>> From: David Holmes
>> Sent: Montag, 4. Mai 2020 08:51
>> To: Reingruber, Richard ; Yasumasa Suenaga
>> ; Patricio Chilano
>> ; serguei.spit...@oracle.com; Vladimir
>> Ivanov ; serviceability-dev@openjdk.java.net;
>> hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net;
>> hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
>> Subject: Re: RFR(S) 8238585: Use handshake for
>> JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make
>> compiled metho
