[OpenJDK 2D-Dev] Request for review [XS]: The XRender backend fails to render any glyphs on 64-bit Big-endian architectures

[Phil Race]

Volker .. thanks for the patch looks good although I ask
that you break the source code lines at no more than 80 chars ..
that's the norm/standard we have always used

There actually is an open bug on this you can use :-

7191872: The panel does not include any information for each case on 
solaris11-sparc.


-phil.

On 4/26/2013 7:02 AM, Volker Simonis wrote:
> Hi,
>
> I've found and fixed a problem of the XRender pipeline on 64-bit
> Big-endian architectures which leads to the effect that all AWT/Swing
> applications will run as expected but won't actually display any
> character glyphs at all! This seems to be a "day-one" bug which still
> exists in both, JDK7 and JDK8 but probably didn't pop up until now
> because the XRender backend wasn't enabled by default. This has
> however changed recently in JDK8.
>
> Could somebody please open a bug for this issue and review the change:
>
> http://cr.openjdk.java.net/~simonis/webrevs/webrev_xrender.v1/
>
> The fix is small and straightforward.
> You can read the full story below (or the HTML-version in the webrev
> for a better reading experience).
>
> Thank you and best regards,
> Volker
>
>
>
> Problem description
>
> The XRender backend writes some native data-structures which have the
> size of a native pointer (i.e. 32-bit on 32-bit architectures and
> 64-bit on 64-bit architectures) by using Unsafe.putInt(). This is of
> course not safe and breaks on 64-bit Big-endian architectures like
> SPARC, PPC64 or HPUX/IA64. The concrete problem is in the method
> setGlyphID in src/solaris/classes/sun/font/XRGlyphCacheEntry.java:
>
> public static void setGlyphID(long glyphInfoPtr, int id) {
>      StrikeCache.unsafe.putInt(glyphInfoPtr + StrikeCache.cacheCellOffset, id);
> }
>
> which writes the offending field and in the function
> Java_sun_java2d_xr_XRBackendNative_XRAddGlyphsNative in
> src/solaris/native/sun/java2d/x11/XRBackendNative.c which reads this
> field:
>
> for (i=0; i < glyphCnt; i++) {
>    GlyphInfo *jginfo = (GlyphInfo *) jlong_to_ptr(glyphInfoPtrs[i]);
>
>    gid[i] = (Glyph) (0x0ffffffffL & ((unsigned long)(jginfo->cellInfo)));
>
> Actually, they both access the cellInfo field of a GlypInfo structure
> which has the size of a native pointer (see
> src/share/native/sun/font/fontscalerdefs.h):
>
> struct GlyphInfo {
>      float advanceX;
>      float advanceY;
>      UInt16 width;
>      UInt16 height;
>      UInt16 rowBytes;
>      UInt8 managed;
>      float topLeftX;
>      float topLeftY;
>      void *cellInfo;
>      UInt8 *image;
> }
>
> The native reader reads the field as a void* and casts it into an
> unsigned long. On Big-endian architectures this shifts the integer
> value which had been previously written in setGlyphID() by 32 bits to
> the left such that the final 'anding' with 0x0ffffffffL will always
> lead to a zero result (on Little-endian architectures there's no
> problem because the 4-byte integer value lies in the 'right' half of
> the 8-byte long value).
>
> How to reproduce the problem
>
> This finally leads to the effect that all AWT/Swing applications will
> run as expected but won't actually display any character glyphs at
> all! This behavior is reproducible with every JDK7 and JDK8. You just
> have to set the property -Dsun.java2d.xrender=True and make sure that
> the actual Xserver which displays the application window really
> supports the XRender extension. Using -Dsun.java2d.xrender=True (with
> capital 'T') will print a debugging line which confirms whether the
> XRender pipeline will be used ('XRender pipeline enabled') or not
> ('Could not enable XRender pipeline').
>
> Notice that the XRender pipeline was switched off by default in JDK7
> but is currently switched on by default in JDK8. However if the
> displaying X-server doesn't support XRender there's always a silent
> fallback to the plain X11 rendering pipeline. So you need an XRender
> capable X server to reproduce this problem.
>
> How to fix the problem
>
> Fixing the problem is easy: we just have to replace the offending
> Unsafe.putInt()/Unsafe.getInt() accesses with corresponding
> Unsafe.putAddress()/Unsafe.getAddress() calls. These routines
> write/read 4 byte in 32-bit VMs and 8 byte on 64-bit VMs (actually
> they read/write Unsafe.addressSize() bytes which corresponds to the
> mentioned values).
>
> Other cleanups in the patch
>
> Once I've fixed the problem, I've also cleaned up several other
> occurrences of the following pattern:
>
> long pixelDataAddress;
> if (StrikeCache.nativeAddressSize == 4) {
>      pixelDataAddress = 0xffffffff &
> StrikeCache.unsafe.getInt(glyphInfoPtr + StrikeCache.pixelDataOffset);
> } else {
>      pixelDataAddress = StrikeCache.unsafe.getLong(glyphInfoPtr +
> StrikeCache.pixelDataOffset);
> }
> long pixelDataAddress = StrikeCache.unsafe.getAddress(glyphInfoPtr +
> StrikeCache.pixelDataOffset);
>
> The intention of this code is to load a native pointer and store it
> into a Java long variable. On 32-bit architectures it uses
> Unsafe.getInt() to read the value and masks the result with 0xffffffff
> after converting it into a Java long value (which is actually the same
> as doing a zero-extension of a 32-bit value during the conversion into
> a 64-bit value). On 64-bit architectures it simply uses
> Unsafe.getLong() to read the value.
>
> The previous code can be simplified into a single expression by always
> using Unsafe.getAddress():
>
> long pixelDataAddress = StrikeCache.unsafe.getAddress(glyphInfoPtr +
> StrikeCache.pixelDataOffset);
>
> because Unsafe.getAddress() does exactly the same thing: it reads a
> native pointer and returns it as a Java long value. In the case where
> the native pointer is less than 64 bits wide, it is extended as an
> unsigned number to a Java long (see JavaDoc of
> sun.misc.Unsafe.getAddress()).
>
> I've also simplified the following line in the function
> Java_sun_java2d_xr_XRBackendNative_XRAddGlyphsNative in
> src/solaris/native/sun/java2d/x11/XRBackendNative.c:
>
> -      gid[i] = (Glyph) (0x0ffffffffL & ((unsigned long)(jginfo->cellInfo)));
> +      // 'jginfo->cellInfo' is of type 'void*' (see definition of
> 'GlyphInfo' in fontscalerdefs.h)
> +      // 'Glyph' is typedefed to 'unsigned long' (see
> http://www.x.org/releases/X11R7.7/doc/libXrender/libXrender.txt)
> +      // Maybe we should assert that (sizeof(void*) == sizeof(Glyph)) ?
> +      gid[i] = (Glyph) (jginfo->cellInfo);
>
> because Glyph is typedefed to unsigned long anyway. The
> 'zero-extension' of the result is unnecessary as well because we're
> actually reading an integer value here and not a pointer.