Re: [OpenJDK 2D-Dev] RFR: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux [v2]
On Fri, 2 Jul 2021 04:02:45 GMT, Sergey Bylokhov wrote: >> Maxim Kartashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Addressed PR comments >> >> 1. Allowed test to run on any platform. >> 2. Trimmed comments to fit in with 80 columns. >> 3. Removed unnecessayr comments. >> 4. Made the ExceptionDescribe() calls conditional on the value of >> FontUtilities.debugFonts() > > Looks fine to me, I'll run the tests. > @mrserb Any news on that front? The new test fails on all platforms: - Linux/MacOS: The test should be marked as headful, otherwise it may run on headless systems where "No X11 DISPLAY variable was set" - WIndows: The jni warnings are generated in the log. - PR: https://git.openjdk.java.net/jdk/pull/4572
Re: [OpenJDK 2D-Dev] RFR: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux [v2]
On Fri, 2 Jul 2021 04:02:45 GMT, Sergey Bylokhov wrote: > Looks fine to me, I'll run the tests. @mrserb Any news on that front? - PR: https://git.openjdk.java.net/jdk/pull/4572
Re: [OpenJDK 2D-Dev] RFR: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux [v2]
On Wed, 30 Jun 2021 10:42:44 GMT, Maxim Kartashev wrote: >> Added an `ExceptionCheck()` followed by `ExceptionDescribe()` and >> `ExceptionClear()` immediately after the Java calls made from the callback >> function `ReadTTFontFileFunc()` in `freetypeScaler.c`. >> >> The exception(s) need to be cleared because we're not returning immediately >> to Java that would've been able to handle them gracefully. And in order not >> to loose the exception entirely (even though the return value would also >> indicate an error condition), print out the exception with >> `ExceptionDescribe()` to aid in debugging. > > Maxim Kartashev has updated the pull request incrementally with one > additional commit since the last revision: > > Addressed PR comments > > 1. Allowed test to run on any platform. > 2. Trimmed comments to fit in with 80 columns. > 3. Removed unnecessayr comments. > 4. Made the ExceptionDescribe() calls conditional on the value of > FontUtilities.debugFonts() Looks fine to me, I'll run the tests. - PR: https://git.openjdk.java.net/jdk/pull/4572
Re: [OpenJDK 2D-Dev] RFR: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux [v2]
On Tue, 29 Jun 2021 23:19:43 GMT, Sergey Bylokhov wrote: >> Maxim Kartashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Addressed PR comments >> >> 1. Allowed test to run on any platform. >> 2. Trimmed comments to fit in with 80 columns. >> 3. Removed unnecessayr comments. >> 4. Made the ExceptionDescribe() calls conditional on the value of >> FontUtilities.debugFonts() > > src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 146: > >> 144: freeNativeResources(env, scalerInfo); >> 145: (*env)->CallVoidMethod(env, scaler, invalidateScalerMID); >> 146: // NB: Exceptions must not be cleared (and therefore no JNI calls >> performed) after calling this method > > Please split long lines to 80 chars per line Done. > src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 199: > >> 197: bBuffer, offset, numBytes); >> 198: // This is a callback, we are not returning immediately to >> Java and better report exceptions now >> 199: CHECK_EXCEPTION(env); > > Probably we should report it only if "debugFonts" was set? Done. > test/jdk/java/awt/font/JNICheck/FreeTypeScalerJNICheck.java line 28: > >> 26: * @bug 8269223 >> 27: * @summary Verifies that -Xcheck:jni issues no warnings from >> freetypeScaler.c >> 28: * @requires os.family == "linux" > > Can we run this test on all platforms? Since this bug was not found, means we > did not cover this code by the tests, and it will be useful to test it even > if the code path will be different on other platforms. Sure; dropped the `@requires` clause. - PR: https://git.openjdk.java.net/jdk/pull/4572
Re: [OpenJDK 2D-Dev] RFR: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux [v2]
On Wed, 30 Jun 2021 00:08:22 GMT, Phil Race wrote: >> Maxim Kartashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Addressed PR comments >> >> 1. Allowed test to run on any platform. >> 2. Trimmed comments to fit in with 80 columns. >> 3. Removed unnecessayr comments. >> 4. Made the ExceptionDescribe() calls conditional on the value of >> FontUtilities.debugFonts() > > src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 53: > >> 51: (*(env))->ExceptionDescribe(env);\ >> 52: (*(env))->ExceptionClear(env); \ >> 53: } > > https://docs.oracle.com/en/java/javase/11/docs/specs/jni/functions.html#exceptiondescribe > > "The pending exception is cleared as a side-effect of calling this function" > > So you certainly don't need both of these and I would prefer that Describe > only be used if really debugging where we think there's a REAL chance of an > exception rather than just to keep JNI happy. > > And the upcall that is likely (readBlock) itself will log any IOException > (and catch it) in the event of an I/O error so I really think this is > unlikely to be useful here. Yes, makes sense. I made `CHECK_EXCEPTION()` call either `Clear...` or `Describe...` based on the value of `debugFonts`. Please, take a look. > src/java.desktop/share/native/libfontmanager/freetypeScaler.c line 198: > >> 196: sunFontIDs.ttReadBlockMID, >> 197: bBuffer, offset, numBytes); >> 198: // This is a callback, we are not returning immediately to >> Java and better report exceptions now > > I think the comment is un-needed .. since the only reason to call > CHECK_EXCEPTION() is because of this. > Same for the other case. OK, comments removed. - PR: https://git.openjdk.java.net/jdk/pull/4572
Re: [OpenJDK 2D-Dev] RFR: 8269223: -Xcheck:jni WARNINGs working with fonts on Linux [v2]
> Added an `ExceptionCheck()` followed by `ExceptionDescribe()` and > `ExceptionClear()` immediately after the Java calls made from the callback > function `ReadTTFontFileFunc()` in `freetypeScaler.c`. > > The exception(s) need to be cleared because we're not returning immediately > to Java that would've been able to handle them gracefully. And in order not > to loose the exception entirely (even though the return value would also > indicate an error condition), print out the exception with > `ExceptionDescribe()` to aid in debugging. Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Addressed PR comments 1. Allowed test to run on any platform. 2. Trimmed comments to fit in with 80 columns. 3. Removed unnecessayr comments. 4. Made the ExceptionDescribe() calls conditional on the value of FontUtilities.debugFonts() - Changes: - all: https://git.openjdk.java.net/jdk/pull/4572/files - new: https://git.openjdk.java.net/jdk/pull/4572/files/1622a169..d1bc82e8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4572&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4572&range=00-01 Stats: 20 lines in 2 files changed: 7 ins; 4 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/4572.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4572/head:pull/4572 PR: https://git.openjdk.java.net/jdk/pull/4572
