[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v
From:       Phil Race <prr () openjdk ! java ! net>
Date:       2021-01-29 19:56:45
Message-ID: Qbm3YYbnAeQycSXj8M4mukJXusNpxlRDTSwWELN6nQM=.e3325063-cf69-4816-adaa-6a54c6a867a1 () github ! com
[Download RAW message or body]

On Fri, 29 Jan 2021 16:23:20 GMT, Gerard Ziemski <gziemski@openjdk.org> wrote:

> > Phil Race has updated the pull request incrementally with one additional commit \
> > since the last revision: 
> > 8260616: Removing remaining JNF dependencies in the java.desktop modul
> 
> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 601:
> 
> > 599:         jchar unichars[len];
> > 600:         (*env)->GetStringRegion(env, str, 0, len, unichars);
> > 601:         CHECK_EXCEPTION();
> 
> Are `JNF_CHECK_AND_RETHROW_EXCEPTION` and `CHECK_EXCEPTION` equivalent?
> 
> `JNF_CHECK_AND_RETHROW_EXCEPTION` seems to throw exception, but `CHECK_EXCEPTION` \
> does not?

JNF_CHECK_AND_RETHROW_EXCEPTION is this


// JNF_CHECK_AND_RETHROW_EXCEPTION - rethrows exceptions from Java
//
// Takes an exception thrown from Java, and transforms it into an
// NSException. The NSException should bubble up to the upper-most
// JNF_COCOA_ENTER/JNF_COCOA_EXIT pair, and then be re-thrown as
// a Java exception when returning from JNI. This check should be
// done after raw JNI operations which could cause a Java exception
// to be be thrown. The JNF{Get/Set/Call}  macros below do this
// check automatically.
#define JNF_CHECK_AND_RETHROW_EXCEPTION(env)                                          \
\ {                                                                                   \
\  jthrowable _exception = (*env)->ExceptionOccurred(env);                     \
    if (_exception) [JNFException raise:env throwable:_exception];      \
}

So what it actually does is clear the exception, raise an NSException and when
the code reaches JNF_COCOA_EXIT - which then has to be there - it clears the \
NSException and re-throws the Java exception.

So the name of the macro is misleading since this does NOT itself rethrow the \
exception, it relies on other code to do that.

CHECK_EXCEPTION will amount to the same - it throws an NSException if there is a Java \
exception pending. And it will clear the NSException before exiting back to Java.
The difference is that it doesn't clear the Java Exception and later rethrow it since \
there is no need There is one exception to this in  CHECK_EXCEPTION - if this is the \
main (ie appkit) thread the java exception is cleared since there's no one to return \
it to ...

> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 608:
> 
> > 606:     {
> > 607:         // Get string to draw and the length
> > 608:         const jchar *unichars = JNFGetStringUTF16UniChars(env, str);
> 
> According to `JNFString.h`, the `JNFGetStringUTF16UniChars()` API:
> 
> /*
> * Gets UTF16 unichars from a Java string, and checks for errors and raises a \
>                 JNFException if
> * the unichars cannot be obtained from Java.
> */
> 
> raises a (JNFException) if it can't get the chars, but now we simply return if \
> `GetStringChars()` can not return the chars. 
> Seems like we handle this case differently in the new code now - is this change \
> desired and correct?

You are correct, but I decided that here it is fine.
If GetStringChars fails it will return NULL (it does not raise any excptions itself).
I added a check for NULL if NULL we then return from the JNI method to Java.
In the old case it also would return but would additionally propagate that raised \
exception to Java which JNF decided to throw into the mix when there's already a \
standard way of testing failure. And since we already know str != NULL we are in the \
realms of something went badly wrong inside the VM for this to fail. So I decided \
this was all fine.

> src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 83:
> 
> > 81:                   stringWithFileSystemRepresentation:chs length:len];
> > 82:     return result;
> > 83: }
> 
> Why are we doing:
> 
> `java_string -> chars -> NSString -> chars -> [NSFileManager \
> stringWithFileSystemRepresentation]` 
> ?
> 
> Why not just get the raw characters form JNI and feed them directly into \
> `[NSFileManager  stringWithFileSystemRepresentation]`, ie: 
> `java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]`
> 
> and skip the `JavaStringToNSString` step altogether?

I tried to explain the odd-ness of this in the comments preceding the function \
definition. Near as I could figure out that intermediate call is needed to do the \
decomposition since the NSFileManager won't do that. But this is not exactly my area \
of expertise and there may be a better way. Who is an expert on the intricacies of \
the macOS file system naming ?

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

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


[prev in list] [next in list] [prev in thread] [next in thread]