Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-26 Thread serguei.spit...@oracle.com 

Hi David,

Sorry for pushing it before your final approval.
I thought you were okay with the update as it was implementation of your 
suggestion.


Thanks,
Serguei


On 5/23/20 05:49, David Holmes wrote:

Update looks fine - though I see you already pushed it.

David

On 22/05/2020 7:32 pm, serguei.spit...@oracle.com wrote:

Hi David,

The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/ 



Thanks,
Serguei


On 5/22/20 00:43, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for the comments!


On 5/21/20 23:58, David Holmes wrote:

Hi Serguei,

On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:

PING: This is pretty small and easy to review fix.

Thanks!
Serguei


On 5/19/20 09:28, serguei.spit...@oracle.com wrote:

Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 



Summary:
  There are two places in the native part of test that cause 
assert and WARNING with the -Xcheck:jni.
  The assert is because there is no check for pending exception 
after the call to:

jni->CallBooleanMethod(klass, is_hid_mid);
  Using a JNI ExceptionCheck()after the call fixes the issue.


  bool res = jni->CallBooleanMethod(klass, is_hid_mid);
  if (jni->ExceptionCheck()) {
    LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
  }
  return res;

That will fix the pending_jni_exception_check error, but if an 
exception actually occurs what will be returned? And whatever is 
returned, the callers of this method don't themselves check for 
pending exceptions so they will treat it as if the exception didn't 
occur - at least until we finally return to Java code. Perhaps any 
exception should result in jni->FatalError as happens with any 
JVMTI error?

You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception 
details.

The exception can be printed before call to FatalError.



  The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, , _classes);
  produces the warning (with a java level stack trace): WARNING: 
JNI local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of 
local references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the JVM TI call also 
fixes the issue.


The warning suggests using 1024 is a bit of overkill. :)


What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.

Thanks!
Serguei


Cheers,
David



Testing:
  Running the test 
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.

  Will run a mach5 job as well.

Thanks,
Serguei

Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-23 Thread David Holmes 

Update looks fine - though I see you already pushed it.

David

On 22/05/2020 7:32 pm, serguei.spit...@oracle.com wrote:

Hi David,

The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/ 



Thanks,
Serguei


On 5/22/20 00:43, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for the comments!


On 5/21/20 23:58, David Holmes wrote:

Hi Serguei,

On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:

PING: This is pretty small and easy to review fix.

Thanks!
Serguei


On 5/19/20 09:28, serguei.spit...@oracle.com wrote:

Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 



Summary:
  There are two places in the native part of test that cause assert 
and WARNING with the -Xcheck:jni.
  The assert is because there is no check for pending exception 
after the call to:

jni->CallBooleanMethod(klass, is_hid_mid);
  Using a JNI ExceptionCheck()after the call fixes the issue.


  bool res = jni->CallBooleanMethod(klass, is_hid_mid);
  if (jni->ExceptionCheck()) {
    LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
  }
  return res;

That will fix the pending_jni_exception_check error, but if an 
exception actually occurs what will be returned? And whatever is 
returned, the callers of this method don't themselves check for 
pending exceptions so they will treat it as if the exception didn't 
occur - at least until we finally return to Java code. Perhaps any 
exception should result in jni->FatalError as happens with any JVMTI 
error?

You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception 
details.

The exception can be printed before call to FatalError.



  The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, , _classes);
  produces the warning (with a java level stack trace): WARNING: 
JNI local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of local 
references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the JVM TI call also 
fixes the issue.


The warning suggests using 1024 is a bit of overkill. :)


What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.

Thanks!
Serguei


Cheers,
David



Testing:
  Running the test 
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.

  Will run a mach5 job as well.

Thanks,
Serguei

Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-22 Thread serguei.spit...@oracle.com 



Thank you for the review, Chris!
Serguei


On 5/22/20 11:57, Chris Plummer wrote:

Hi Serguei,

Looks good, and I agree with David's comments. I was thinking the same 
thing when I first looked at your original changes.


thanks,

Chris

On 5/22/20 2:32 AM, serguei.spit...@oracle.com wrote:

Hi David,

The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/ 



Thanks,
Serguei


On 5/22/20 00:43, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for the comments!


On 5/21/20 23:58, David Holmes wrote:

Hi Serguei,

On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:

PING: This is pretty small and easy to review fix.

Thanks!
Serguei


On 5/19/20 09:28, serguei.spit...@oracle.com wrote:

Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 



Summary:
  There are two places in the native part of test that cause 
assert and WARNING with the -Xcheck:jni.
  The assert is because there is no check for pending exception 
after the call to:

jni->CallBooleanMethod(klass, is_hid_mid);
  Using a JNI ExceptionCheck()after the call fixes the issue.


  bool res = jni->CallBooleanMethod(klass, is_hid_mid);
  if (jni->ExceptionCheck()) {
    LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
  }
  return res;

That will fix the pending_jni_exception_check error, but if an 
exception actually occurs what will be returned? And whatever is 
returned, the callers of this method don't themselves check for 
pending exceptions so they will treat it as if the exception didn't 
occur - at least until we finally return to Java code. Perhaps any 
exception should result in jni->FatalError as happens with any 
JVMTI error?

You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception 
details.

The exception can be printed before call to FatalError.



  The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, , _classes);
  produces the warning (with a java level stack trace): WARNING: 
JNI local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of 
local references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the JVM TI call also 
fixes the issue.


The warning suggests using 1024 is a bit of overkill. :)


What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.

Thanks!
Serguei


Cheers,
David



Testing:
  Running the test 
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.

  Will run a mach5 job as well.

Thanks,
Serguei

Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-22 Thread Chris Plummer 

Hi Serguei,

Looks good, and I agree with David's comments. I was thinking the same 
thing when I first looked at your original changes.


thanks,

Chris

On 5/22/20 2:32 AM, serguei.spit...@oracle.com wrote:

Hi David,

The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/ 



Thanks,
Serguei


On 5/22/20 00:43, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for the comments!


On 5/21/20 23:58, David Holmes wrote:

Hi Serguei,

On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:

PING: This is pretty small and easy to review fix.

Thanks!
Serguei


On 5/19/20 09:28, serguei.spit...@oracle.com wrote:

Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 



Summary:
  There are two places in the native part of test that cause 
assert and WARNING with the -Xcheck:jni.
  The assert is because there is no check for pending exception 
after the call to:

jni->CallBooleanMethod(klass, is_hid_mid);
  Using a JNI ExceptionCheck()after the call fixes the issue.


  bool res = jni->CallBooleanMethod(klass, is_hid_mid);
  if (jni->ExceptionCheck()) {
    LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
  }
  return res;

That will fix the pending_jni_exception_check error, but if an 
exception actually occurs what will be returned? And whatever is 
returned, the callers of this method don't themselves check for 
pending exceptions so they will treat it as if the exception didn't 
occur - at least until we finally return to Java code. Perhaps any 
exception should result in jni->FatalError as happens with any JVMTI 
error?

You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception 
details.

The exception can be printed before call to FatalError.



  The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, , _classes);
  produces the warning (with a java level stack trace): WARNING: 
JNI local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of 
local references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the JVM TI call also 
fixes the issue.


The warning suggests using 1024 is a bit of overkill. :)


What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.

Thanks!
Serguei


Cheers,
David



Testing:
  Running the test 
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.

  Will run a mach5 job as well.

Thanks,
Serguei

Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-22 Thread serguei.spit...@oracle.com 

Hi David,

The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/

Thanks,
Serguei


On 5/22/20 00:43, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for the comments!


On 5/21/20 23:58, David Holmes wrote:

Hi Serguei,

On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:

PING: This is pretty small and easy to review fix.

Thanks!
Serguei


On 5/19/20 09:28, serguei.spit...@oracle.com wrote:

Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 



Summary:
  There are two places in the native part of test that cause assert 
and WARNING with the -Xcheck:jni.
  The assert is because there is no check for pending exception 
after the call to:

jni->CallBooleanMethod(klass, is_hid_mid);
  Using a JNI ExceptionCheck()after the call fixes the issue.


  bool res = jni->CallBooleanMethod(klass, is_hid_mid);
  if (jni->ExceptionCheck()) {
    LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
  }
  return res;

That will fix the pending_jni_exception_check error, but if an 
exception actually occurs what will be returned? And whatever is 
returned, the callers of this method don't themselves check for 
pending exceptions so they will treat it as if the exception didn't 
occur - at least until we finally return to Java code. Perhaps any 
exception should result in jni->FatalError as happens with any JVMTI 
error?

You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception 
details.

The exception can be printed before call to FatalError.



  The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, , _classes);
  produces the warning (with a java level stack trace): WARNING: 
JNI local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of local 
references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the JVM TI call also 
fixes the issue.


The warning suggests using 1024 is a bit of overkill. :)


What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.

Thanks!
Serguei


Cheers,
David



Testing:
  Running the test 
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.

  Will run a mach5 job as well.

Thanks,
Serguei

Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-22 Thread serguei.spit...@oracle.com 

Hi David,

Thank you for the comments!


On 5/21/20 23:58, David Holmes wrote:

Hi Serguei,

On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:

PING: This is pretty small and easy to review fix.

Thanks!
Serguei


On 5/19/20 09:28, serguei.spit...@oracle.com wrote:

Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 



Summary:
  There are two places in the native part of test that cause assert 
and WARNING with the -Xcheck:jni.
  The assert is because there is no check for pending exception 
after the call to:

jni->CallBooleanMethod(klass, is_hid_mid);
  Using a JNI ExceptionCheck()after the call fixes the issue.


  bool res = jni->CallBooleanMethod(klass, is_hid_mid);
  if (jni->ExceptionCheck()) {
    LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
  }
  return res;

That will fix the pending_jni_exception_check error, but if an 
exception actually occurs what will be returned? And whatever is 
returned, the callers of this method don't themselves check for 
pending exceptions so they will treat it as if the exception didn't 
occur - at least until we finally return to Java code. Perhaps any 
exception should result in jni->FatalError as happens with any JVMTI 
error?

You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception details.
The exception can be printed before call to FatalError.



  The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, , _classes);
  produces the warning (with a java level stack trace): WARNING: JNI 
local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of local 
references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the JVM TI call also 
fixes the issue.


The warning suggests using 1024 is a bit of overkill. :)


What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.

Thanks!
Serguei


Cheers,
David



Testing:
  Running the test 
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.

  Will run a mach5 job as well.

Thanks,
Serguei

Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-22 Thread David Holmes 

Hi Serguei,

On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:

PING: This is pretty small and easy to review fix.

Thanks!
Serguei


On 5/19/20 09:28, serguei.spit...@oracle.com wrote:

Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/

Summary:
  There are two places in the native part of test that cause assert 
and WARNING with the -Xcheck:jni.
  The assert is because there is no check for pending exception after 
the call to:

jni->CallBooleanMethod(klass, is_hid_mid);
  Using a JNI ExceptionCheck()after the call fixes the issue.


  bool res = jni->CallBooleanMethod(klass, is_hid_mid);
  if (jni->ExceptionCheck()) {
LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
  }
  return res;

That will fix the pending_jni_exception_check error, but if an exception 
actually occurs what will be returned? And whatever is returned, the 
callers of this method don't themselves check for pending exceptions so 
they will treat it as if the exception didn't occur - at least until we 
finally return to Java code. Perhaps any exception should result in 
jni->FatalError as happens with any JVMTI error?



  The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, , _classes);
  produces the warning (with a java level stack trace): WARNING: JNI 
local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of local 
references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the JVM TI call also fixes 
the issue.


The warning suggests using 1024 is a bit of overkill. :)

Cheers,
David



Testing:
  Running the test test/hotspot/jtreg/serviceability/jvmti/HiddenClass 
locally.

  Will run a mach5 job as well.

Thanks,
Serguei

PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-22 Thread serguei.spit...@oracle.com 

  
  
PING: This is pretty small and easy to
  review fix.
  
  Thanks!
  Serguei
  
  
  On 5/19/20 09:28, serguei.spit...@oracle.com wrote:


  Please, review fix for:
    https://bugs.openjdk.java.net/browse/JDK-8244571
  
  Webrev:
   
  http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/
  
  Summary:
    There are two places in the native part of test that cause
  assert and WARNING with the -Xcheck:jni.
    The assert is because there is no check for pending exception
  after the call to:
   jni->CallBooleanMethod(klass,
is_hid_mid);
  Using a JNI ExceptionCheck() after the call fixes the issue.

  The following call to the JVM TI function:
 err = jvmti->GetClassLoaderClasses(loader,
  , _classes);
    produces the warning (with a java level stack trace): WARNING:
  JNI local refs: 94, exceeds capacity: 32
    It is because the GetClassLoaderClasses returns an array of
  local references to the loader classes.
    Using a JNI EnsureLocalCapacity() before the
JVM TI call also fixes the issue.

Testing:
  Running the test
  test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
  Will run a mach5 job as well.

Thanks,
Serguei

RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

2020-05-19 Thread serguei.spit...@oracle.com 

  
  
Please, review fix for:
    https://bugs.openjdk.java.net/browse/JDK-8244571
  
  Webrev:
   
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/
  
  Summary:
    There are two places in the native part of test that cause
  assert and WARNING with the -Xcheck:jni.
    The assert is because there is no check for pending exception
  after the call to:
   jni->CallBooleanMethod(klass,
  is_hid_mid);
    Using a JNI ExceptionCheck() after the call fixes the issue.
  
    The following call to the JVM TI function:
   err = jvmti->GetClassLoaderClasses(loader,
, _classes);
  produces the warning (with a java level stack trace): WARNING: JNI
local refs: 94, exceeds capacity: 32
  It is because the GetClassLoaderClasses returns an array of local
references to the loader classes.
  Using a JNI EnsureLocalCapacity() before the
  JVM TI call also fixes the issue.
  
  Testing:
    Running the test
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
    Will run a mach5 job as well.
  
  Thanks,
  Serguei