+1 from me .. yes push to jdk/jdk10.
-phil. On 12/18/17, 1:51 AM, Volker Simonis wrote:
Hi Matthias, the change looks good and I can sponsor it. I'd just propose to slightly change the comment to "..by the overloaded versions of 'cmp' in IntType" if you don't mind. There's no need for a new webrew tough - I can make that change before pushing. I'm just waiting for one more review from the 2D group. Afterwards I'll push directly to jdk/jdk10. From my understanding, the fix will than be automatically forward-integrated into jdk/jdk. Regards, Volker On Fri, Dec 15, 2017 at 2:24 PM, Baesken, Matthias <matthias.baes...@sap.com> wrote:Hello, I created a second webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8193515.1/Whatever we do, can it be wrapped in an appropriate #ifdef AIX so that other platforms are guaranteed to be unaffected ?The new webrev uses the simpler cast-version proposed by Volki , and guards the AIX change by ifdef (suggested by Phil).In the meantime we try to find out how latest xlC version 13 behaves .In the meantime I checked with XLC13 as well , but we see the error with XLC 13 too (same as XLC 12). Please review the change . It should go later into jdk10 as well (because jdk10 contains the new Harfbuzz 1.7.1 too ). Thanks, Matthias-----Original Message----- From: Baesken, Matthias Sent: Donnerstag, 14. Dezember 2017 18:03 To: 'Phil Race'<philip.r...@oracle.com>; Volker Simonis <volker.simo...@gmail.com> Cc: Simonis, Volker<volker.simo...@sap.com>; 2d-dev@openjdk.java.net Subject: RE: [OpenJDK 2D-Dev] RFR : 8193515 : AIX : new Harfbuzz 1.7.1 version fails to compile Hi Phil, hi Volki, I think wrapping Volkis version into #ifdef _AIX plus adding a small comment sounds like a good idea . In the meantime we try to find out how latest xlC version 13 behaves . Best regards, Matthias-----Original Message----- From: Phil Race [mailto:philip.r...@oracle.com] Sent: Donnerstag, 14. Dezember 2017 17:53 To: Volker Simonis<volker.simo...@gmail.com>; Baesken, Matthias <matthias.baes...@sap.com> Cc: Simonis, Volker<volker.simo...@sap.com>; 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] RFR : 8193515 : AIX : new Harfbuzz 1.7.1 version fails to compile Whatever we do, can it be wrapped in an appropriate #ifdef AIX so that other platforms are guaranteed to be unaffected ? For upstream you can report it at github https://github.com/harfbuzz/harfbuzz and see how Behdad would like to handle it. I expect he would want it removed once the compiler bug is fixed. -pgil. On 12/14/2017 08:13 AM, Volker Simonis wrote:Hi Matthias, thanks for addressing this issue! I'm pretty sure that his is a compiler bug and I have a short reproducer which I'll send to IBM. The problem is that xlC can't distinguish a static member function (which uses an ordinary function call) from a non-static member function (which uses a member function call) with the same name. I've just found a slightly more elegant (and less intrusive) fix. We can help the compiler to find the correct method by simply casting it to the correct version: diff -r be065f758154 src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hh--- a/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hhThu Dec 14 12:49:47 2017 +0530 +++ b/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hhThu Dec 14 17:11:49 2017 +0100 @@ -77,7 +77,7 @@ /* Bubble-sort or something equally good! * May not be good-enough for presidential candidate interviews, but good-enough for us... */ - hb_stable_sort (&glyphs[0], num_glyphs, OT::GlyphID::cmp,&substitutes[0]);+ hb_stable_sort (&glyphs[0], num_glyphs, (int(*)(const OT::GlyphID *, const OT::GlyphID *)) OT::GlyphID::cmp,&substitutes[0]); OT::Supplier<OT::GlyphID> glyphs_supplier (glyphs, num_glyphs); OT::Supplier<OT::GlyphID> substitutes_supplier (substitutes,num_glyphs);@@ -126,7 +126,7 @@ first_glyphs_indirection[num_first_glyphs] = first_glyph_idx; num_first_glyphs++; } - hb_stable_sort (&first_glyphs[0], num_first_glyphs, OT::GlyphID::cmp,&first_glyphs_indirection[0]); + hb_stable_sort (&first_glyphs[0], num_first_glyphs, (int(*)(const OT::GlyphID *, const OT::GlyphID *)) OT::GlyphID::cmp, &first_glyphs_indirection[0]); /* Now that the first-glyphs are sorted, walk again, populate ligatures.*/for (unsigned int i = 0; i< num_first_glyphs; i++) I'll also try to bring this change upstream into Harfbuzz, but we need to push this into the new jdk/jdk10 repo because there will be no more HarfBuzz integration for Java 10. Regards, Volker On Thu, Dec 14, 2017 at 4:10 PM, Baesken, Matthias <matthias.baes...@sap.com> wrote:Hello, after upgrading to new Harfbuzz 1.7.1 the openjdk build fails on AIX. I created the following bug : https://bugs.openjdk.java.net/browse/JDK-8193515 The compile error we get on AIX (using XLC 12.1) is : === Output from failing command(s) repeated here === * For target support_native_java.desktop_libfontmanager_hb-ot-shape-complex-arabic.o:" /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hh",line 80.3: 1540-0218 (S) The call does not match any parameter list for "hb_stable_sort". " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-private.hh",line 723.1: 1540-1283 (I) "template<class T, class T2> hb_stable_sort(T *, unsigned int, int (*)(const T *, const T *), T2 *)" is not a viable candidate. " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hh",line 80.43: 1540-0298 (I) Template argument deduction cannot beperformedusing the function "template int cmp(Type2) const". " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-private.hh",line 748.1: 1540-1283 (I) "template<class T> hb_stable_sort(T *,unsignedint, int (*)(const T *, const T *))" is not a viable candidate. " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hh",line 80.3: 1540-0215 (I) The wrong number of arguments has beenspecifiedfor "template<class T> hb_stable_sort(T *, unsigned int, int (*)(const T*,const T *))". " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hh",line 129.3: 1540-0218 (S) The call does not match any parameter list for "hb_stable_sort". " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-private.hh",line 723.1: 1540-1283 (I) "template<class T, class T2> hb_stable_sort(T *, unsigned int, int (*)(const T *, const T *), T2 *)" is not a viable candidate. " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hh",line 129.55: 1540-0298 (I) Template argument deduction cannot beperformedusing the function "template int cmp(Type2) const". " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-private.hh",line 748.1: 1540-1283 (I) "template<class T> hb_stable_sort(T *,unsignedint, int (*)(const T *, const T *))" is not a viable candidate. " /jdk/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-shape-complex-arabic-fallback.hh",line 129.3: 1540-0215 (I) The wrong number of arguments has beenspecifiedfor "template<class T> hb_stable_sort(T *, unsigned int, int (*)(const T*,const T *))". The compilation “complains” about the hb_stable_sort template usedinhb-ot-shape-complex-arabic-fallback.hh . After looking a bit into this , the third parameter OT::GlyphID::cmp of hb_stable_sort (&glyphs[0], num_glyphs, OT::GlyphID::cmp, &substitutes[0]); seems to trigger this XLC 12 issue . XLC 12 does not like the fact that we have two cmp functions (one a template) in hb-open-type-private.hh : 610 template<typename Type, unsigned int Size> 611 struct IntType 612 { .... 617 static inline int cmp (const IntType<Type,Size> *a, const IntType<Type,Size> *b) { return b->cmp (*a); } 622 623 template<typename Type2> 624 inline int cmp (Type2 a) const 625 { ( GlyphID is an IntType ) This looks like an XLC bug, however it is pretty easy to workaround it by using a helper compare-function with a unique name (issue with cmp isthatit is not unique, that confuses XLC ). See this webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8193515/ Please review it. Thanks, Matthias
