Code Review for "Fix Xrender backend on 64-bit Big-endian architectures"

Prepared by:simonis on Fri Apr 26 15:41:23 CEST 2013
Contributed by:volker.simonis@gmail.com
Workspace:/net/usr.work/d046063/OpenJDK/ppc-aix-port/jdk8/jdk
Compare against: http://hg.openjdk.java.net/ppc-aix-port/jdk8/jdk
Compare against version:6828
Summary of changes: 41 lines changed: 14 ins; 19 del; 8 mod; 2616 unchg
Patch of changes: jdk.patch
Author comments:

The XRender backend fails to render any glyphs on 64-bit Big-endian architectures

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.

Legend: Modified file
Deleted file
New file

Cdiffs Udiffs Sdiffs Frames Old New Patch Raw src/share/classes/sun/font/FileFontStrike.java

rev 6829 : Fix Xrender backend on  64-bit Big-endian architectures
8 lines changed: 0 ins; 6 del; 2 mod; 943 unchg

Cdiffs Udiffs Sdiffs Frames Old New Patch Raw src/share/classes/sun/font/GlyphList.java

rev 6829 : Fix Xrender backend on  64-bit Big-endian architectures
10 lines changed: 0 ins; 8 del; 2 mod; 496 unchg

Cdiffs Udiffs Sdiffs Frames Old New Patch Raw src/solaris/classes/sun/font/XRGlyphCacheEntry.java

rev 6829 : Fix Xrender backend on  64-bit Big-endian architectures
19 lines changed: 11 ins; 5 del; 3 mod; 198 unchg

Cdiffs Udiffs Sdiffs Frames Old New Patch Raw src/solaris/native/sun/java2d/x11/XRBackendNative.c

rev 6829 : Fix Xrender backend on  64-bit Big-endian architectures
4 lines changed: 3 ins; 0 del; 1 mod; 979 unchg

This code review page was prepared using /sapmnt/home1/d046063/webrev.ksh (vers 23.18-hg).