RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is incorrect and corresponsing logic seems to be broken

Tue Jun 9 19:45:52 UTC 2020

http://cr.openjdk.java.net/~fmatte/8243451/webrev.09/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/TestDebuggerType2.java.udiff.html

I see that isJFRActive() depends on "nsk.share.jdi.HeapwalkingDebuggee". 
It is not going to work of debugee is not 
"nsk.share.jdi.HeapwalkingDebuggee".

Shouldn't it be placed in HeapWalkingDebugger?

Leonid

On 6/8/20 9:26 PM, serguei.spitsyn at oracle.com wrote:
> Hi Fairoz,
>
> LGTM.
>
> Thanks,
> Serguei
>
>
> On 6/8/20 21:20, Fairoz Matte wrote:
>> Hi Serguei,
>>
>> Thanks for the clarifications,
>> I have incorporated the 2nd suggestion, below is the webrev,
>> http://cr.openjdk.java.net/~fmatte/8243451/webrev.09/
>>
>> Thanks,
>> Fairoz
>>
>> From: Serguei Spitsyn
>> Sent: Monday, June 8, 2020 10:34 PM
>> To: Fairoz Matte <fairoz.matte at oracle.com>; Erik Gahlin 
>> <erik.gahlin at oracle.com>
>> Cc: serviceability-dev at openjdk.java.net
>> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is 
>> incorrect and corresponsing logic seems to be broken
>>
>> Hi Fairoz,
>>
>>
>> On 6/8/20 02:08, mailto:serguei.spitsyn at oracle.com wrote:
>> Hi Fairoz,
>>
>> There are two different isJFRActive() methods, one is on debuggee 
>> side and another on the debugger side.
>> The one on debuggee side is better to keep in Debuggee.java (where it 
>> was before) instead of moving it to HeapwalkingDebuggee.java.
>> It is okay to keep the call to it in the HeapwalkingDebuggee.java.
>>
>> Please, skip this suggestion as Debugger.java is not one of supers of 
>> HeapwalkingDebuggee.java as I've assumed.
>>
>> Thanks,
>> Serguei
>>
>>
>> +    protected boolean isJFRActive() {
>> +        boolean isJFRActive = false;
>> +        ReferenceType referenceType = 
>> debuggee.classByName("nsk.share.jdi.HeapwalkingDebuggee");
>> +        if (referenceType == null)
>> +           throw new RuntimeException("Debugeee is not initialized 
>> yet");
>> +
>> +        Field isJFRActiveFld = 
>> referenceType.fieldByName("isJFRActive");
>> +        isJFRActive = 
>> ((BooleanValue)referenceType.getValue(isJFRActiveFld)).value();
>> +        return isJFRActive;
>>       }
>> It is better to remove the line:
>> +        boolean isJFRActive = false;
>> and just change this one:
>> +        boolean isJFRActive = 
>> ((BooleanValue)referenceType.getValue(isJFRActiveFld)).value();
>>
>> Otherwise, it looks good to me.
>> I hope, it really works now.
>>
>> Thanks,
>> Serguei
>>
>> On 6/8/20 00:26, Fairoz Matte wrote:
>> Hi Serguei, Erik,
>>   Thanks for the reviews,
>> Below webrev contains the suggested changes,
>> http://cr.openjdk.java.net/~fmatte/8243451/webrev.08/
>>   The only thing I couldn’t do is to keep the local copy of 
>> isJFRActive() in HeapwalkingDebugger,
>> The method is called in debugee code.
>> In debugger, we have access to debugee before test started or after 
>> test completes.
>> isJFRActive() method need to be executed during the test execution. 
>> Hence I didn’t find place to initialize and cannot make local copy.
>>   Thanks,
>> Fairoz
>>   From: Serguei Spitsyn
>> Sent: Tuesday, June 2, 2020 7:57 AM
>> To: Fairoz Matte mailto:fairoz.matte at oracle.com; Erik Gahlin 
>> mailto:erik.gahlin at oracle.com
>> Cc: mailto:serviceability-dev at openjdk.java.net
>> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is 
>> incorrect and corresponsing logic seems to be broken
>>   On 6/1/20 12:30, mailto:serguei.spitsyn at oracle.com wrote:
>> Hi Fairoz,
>>
>> It looks okay in general.
>> But I'm not sure this check is going to work.
>> The problem is the HeapwalkingDebuggee.useStrictCheck method is 
>> invoked in the
>> context of the HeapwalkingDebugger process, not the 
>> HeapwalkingDebuggee process.
>>
>> Probably, you wanted to get this bit of information from the Debuggee 
>> process.
>> The debuggee has to evaluate it itself and store in some field.
>> The debugger should use the JDI to get this value from the debuggee.
>>
>> Thanks,
>> Serguei
>>
>> I'm not sure, what exactly you wanted to do here.
>> It can occasionally work for you as long as both processes are run 
>> with the same options.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 6/1/20 08:52, Fairoz Matte wrote:
>> Hi Erik,
>>   Thanks for the review, below is the updated webrev.
>> http://cr.openjdk.java.net/~fmatte/8243451/webrev.02/
>>   Thanks,
>> Fairoz
>>   -----Original Message-----
>> From: Erik Gahlin
>> Sent: Monday, June 1, 2020 4:26 PM
>> To: Fairoz Matte mailto:fairoz.matte at oracle.com
>> Cc: mailto:serviceability-dev at openjdk.java.net
>> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is 
>> incorrect
>> and corresponsing logic seems to be broken
>>   Hi Fairoz,
>>   What I think you need to do is something like this:
>>             if (className.equals("java.lang.Thread")) {
>>               return !isJfrInitialized();
>>           }
>>   ...
>>         private static boolean isJfrInitialized() {
>>           try {
>>               Class<?> clazz = Class.forName("jdk.jfr.FlightRecorder");
>>               Method method = clazz.getDeclaredMethod("isInitialized",
>> new Class[0]);
>>               return (boolean) method.invoke(null, new Object[0]);
>>           } catch (Exception e) {
>>               return false;
>>           }
>>       }
>>   Erik
>>   On 2020-06-01 12:30, Fairoz Matte wrote:
>> Hi Erik,
>>   Thanks for your quick response,
>> Below is the updated webrev to handle if jfr module is not present
>> http://cr.openjdk.java.net/~fmatte/8243451/webrev.01/
>>   Thanks,
>> Fairoz
>>   -----Original Message-----
>> From: Erik Gahlin
>> Sent: Monday, June 1, 2020 2:31 PM
>> To: Fairoz Matte mailto:fairoz.matte at oracle.com
>> Cc: mailto:serviceability-dev at openjdk.java.net
>> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is
>> incorrect and corresponsing logic seems to be broken
>>   Hi Fairoz,
>>   If the test needs to run with builds where the JFR module is not
>> present(?), you need to do the check using reflection.
>>   If not, looks good.
>>   Erik
>>   On 1 Jun 2020, at 10:27, Fairoz Matte 
>> mailto:fairoz.matte at oracle.com wrote:
>>   Hi,
>>   Please review this small test infra change to identify at runtime
>> the JFR is
>> active or not.
>> JBS - https://bugs.openjdk.java.net/browse/JDK-8243451
>> Webrev - http://cr.openjdk.java.net/~fmatte/8243451/webrev.00/
>>   Thanks,
>> Fairoz
>>
>