Re: RFR: 8264711: More runtime TRAPS cleanups [v2]

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264711: More runtime TRAPS cleanups [v2]

Harold Seigel-2
On Mon, 5 Apr 2021 23:28:38 GMT, David Holmes <[hidden email]> wrote:

>> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Undo change to ObjectSynchronizer::jni_exit()
>
> Hi Harold,
>
> Lots of good cleanup here but also a couple of things that I think are incorrect. Platform string creation can throw exceptions; and I also believe the ClassFileLoadHook can too.
>
> Thanks,
> David

Please review this latest version of this change containing reviewer suggested changes.

Thanks, Harold

> src/hotspot/share/classfile/javaClasses.cpp line 396:
>
>> 394:
>> 395: // Converts a C string to a Java String based on current encoding
>> 396: Handle java_lang_String::create_from_platform_dependent_str(JavaThread* current, const char* str) {
>
> This change is incorrect. JNU_NewStringPlatform can raise an exception so TRAPS here is correct.

Thanks for finding this.  I reverted this change and the change to as_platform_dependent_str() because it calls GetStringPlatformChars() which calls JNU_GetStringPlatformChars() which can throw an exception.

> src/hotspot/share/prims/jvmtiEnv.cpp line 709:
>
>> 707:
>> 708:     // need the path as java.lang.String
>> 709:     Handle path = java_lang_String::create_from_platform_dependent_str(THREAD, segment);
>
> This change will need reverting as an exception is possible as previously noted.
>
> But note that if there was no possibility of exception here then the following check of HAS_PENDING_EXCEPTION should also have been removed.

Reverted.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3345