Re: [8u] RFR: 8225690: Multiple AttachListener threads can be created

[Yi Yang]

Hi Paul,

In my initial changes, I used enum AttachListenerState. Enum cannot be automatically converted to jlong, the compiler complains about this, so forced type casting is required. At that time you were worried that forced type conversion might cause problems:

> the AttachListenerState enum's underlying type will be int, so casting to jlong for the atomic operations will clobber whatever is beyond _state, and won't work at all on big-endian machines. C++11 adds enum base type specification, but we can't use C++11 features in 8u. I would define const int AL_* values and declare _state to be an int.

It is always correct to use constants, so I am using constant(macro definition) now.

Thanks!


------------------------------------------------------------------
From:Hohensee, Paul <hohensee at amazon.com>
Send Time:2021 Nov. 11 (Thu.) 02:44
To:"YANG, Yi" <qingfeng.yy at alibaba-inc.com>; jdk8u-dev at openjdk.java.net <jdk8u-dev at openjdk.java.net>
Subject:Re: [8u] RFR: 8225690: Multiple AttachListener threads can be created

  
Apologies for the delay.
Lgtm for 8274953, except that I’d use the original AttachListenerState enum type definition in attachListener.hpp rather than #defines. The enums will be converted properly to jlongs, and #defines of constants aren’t typed so shouldn’t be used unless absolutely necessary.
Lgtm for  8227815 and 8227738.
Thanks,
Paul
From: Yi Yang <qingfeng.yy at alibaba-inc.com>
Reply-To: Yi Yang <qingfeng.yy at alibaba-inc.com>
Date: Tuesday, November 9, 2021 at 1:47 AM
To: "YANG, Yi" <qingfeng.yy at alibaba-inc.com>, "Hohensee, Paul" <hohensee at amazon.com>, "jdk8u-dev at openjdk.java.net" <jdk8u-dev at openjdk.java.net>
Subject: RE:  [8u] RFR: 8225690: Multiple AttachListener threads can be created 
Gentle PING :-

 Thanks! 
   
------------------------------------------------------------------ 
From:YANG, Yi <qingfeng.yy at alibaba-inc.com> 
Send Time:2021 Oct. 21 (Thu.) 16:08 
To:"Hohensee, Paul" <hohensee at amazon.com>; jdk8u-dev at openjdk.java.net <jdk8u-dev at openjdk.java.net> 
Subject:Re: [8u] RFR: 8225690: Multiple AttachListener threads can be created 
  
Hi Paul,
Currently, Atomic::load only accepts jlong as a parameter, so I use some macros to define AL_XX values and change type of _state to `volatile jlong`, what do you think? 

 Introduction LingeredApp has a relatively large effort and may also be relatively risky. There are already similar examples of removing @test tag when using LingeredApp(ClhsdbJstackXcompStress, LingeredAppWithRecComputation), so this may be a feasible solution. These tests are all passed(w/ some modifications to allow it working on JDK8u). Nevertheless, I could investigate if it's possible to backport LingeredApp to  after these patches. 

 I have prepared all related changeset. Please help review it, thank you. 

 -yyang 

 1. 8225690: Multiple AttachListener threads can be created 
Original Issue: https://bugs.openjdk.java.net/browse/JDK-8225690 
Original Changeset: https://hg.openjdk.java.net/jdk/jdk/rev/70fab3a8ff02 
Backport Issue: https://bugs.openjdk.java.net/browse/JDK-8274953 
Backport Changeset: http://cr.openjdk.java.net/~yyang/8274953/webrev.00/ 
Review Effort: M 

 2. 8227815: Minimal VM: set_state is not a member of AttachListener 
Original Issue: https://bugs.openjdk.java.net/browse/JDK-8227815 
Original Changeset: https://hg.openjdk.java.net/jdk/jdk/rev/2660d47140da 
Backport Issue: https://bugs.openjdk.java.net/browse/JDK-8275694 
Backport Changeset: https://cr.openjdk.java.net/~yyang/8275694/webrev/ 
Review Effort: XS 

 3. 8227738: jvmti/DataDumpRequest/datadumpreq001 failed due to "exit code is 134" 
Original Issue: https://bugs.openjdk.java.net/browse/JDK-8227738 
Original Changeset: https://hg.openjdk.java.net/jdk/jdk/rev/4888ccfc234e 
Backport Issue: https://bugs.openjdk.java.net/browse/JDK-8275695 
Backport Changeset: https://cr.openjdk.java.net/~yyang/8275695/webrev/ 
Review Effort: XS 
  
------------------------------------------------------------------ 
From:Hohensee, Paul <hohensee at amazon.com> 
Send Time:2021 Oct. 16 (Sat.) 04:39 
To:"YANG, Yi" <qingfeng.yy at alibaba-inc.com>; jdk8u-dev at openjdk.java.net <jdk8u-dev at openjdk.java.net> 
Subject:Re: [8u] RFR: 8225690: Multiple AttachListener threads can be created 
  
In attachListener.hpp, the AttachListenerState enum's underlying type will be int, so casting to jlong for the atomic operations will clobber whatever is beyond _state, and won't work at all on big-endian machines. C++11 adds enum base type specification, but we can't use C++11 features in 8u. I would define const int AL_* values and declare _state to be an int.

 I'm leery of disabling the test. Please see if you can backport 8165500, which introduced LingeredApp.

 You have code from 8237499 in the patch which has already been backported. Please remove it. Also, please post review requests for 8227815 and 8227738 backports, with the intent to push all three at once.

 Thanks,
 Paul

 -----Original Message-----
 From: jdk8u-dev <jdk8u-dev-retn at openjdk.java.net> on behalf of Yi Yang <qingfeng.yy at alibaba-inc.com>
 Reply-To: Yi Yang <qingfeng.yy at alibaba-inc.com>
 Date: Friday, October 8, 2021 at 4:21 AM
 To: "jdk8u-dev at openjdk.java.net" <jdk8u-dev at openjdk.java.net>
 Subject: [8u] RFR: 8225690: Multiple AttachListener threads can be created

 Hi,

 Can I have a review for this backport of JDK-8225690?
 Original Issue: https://bugs.openjdk.java.net/browse/JDK-8225690
 Original Changeset: https://hg.openjdk.java.net/jdk/jdk/rev/70fab3a8ff02
 Backport Issue: https://bugs.openjdk.java.net/browse/JDK-8274953
 Backport Changeset: http://cr.openjdk.java.net/~yyang/8274953/

 VM can't be attached If someone cleans up the /tmp directory. This patch
 addresses the problem. It can not apply cleanly, some minor conflicts are as follows:

 1. Atomic::store(new_state, &_state)
 jdk8u does not have templated Atomic function family, use explicitly type
 conversion `Atomic::store((jlong)new_state, (volatile jlong*)&_state)`.

 2. IsRegisteredEnum<AttachListenerState>
 jdk8u does not have metaprogramming type traits,
 `template<> struct IsRegisteredEnum<AttachListenerState> : public TrueType {};`
 simply omit it.

 3. os::naked_yield
 os::naked_yield is missing due to https://bugs.openjdk.java.net/browse/JDK-8047714,
 use os::yield instead.

 4. log_debug(attach)("...")
 jdk8u does not have unified logging system. Use the following code:
 `debug_only(warning("..."))`

 5. Test
 They can not work because of the usages of LingeredApp. Remove @test tag to
 disable them while keeping their source code.

 Test cases have been passed manually. The original issue has related bugs:
https://bugs.openjdk.java.net/browse/JDK-8227815
https://bugs.openjdk.java.net/browse/JDK-8235211
https://bugs.openjdk.java.net/browse/JDK-8227738
 I'd like to create follow-up backports(JDK-8227815 and JDK-8227738) once this
 merged. JDK-8235211 is not necessary since RemovingUnixDomainSocketTest.java
 will be ignored as I said before.

 Thanks!