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

[Phil Race]

On 5/6/2013 5:25 AM, Volker Simonis wrote:
> On Wed, May 1, 2013 at 9:30 PM, Phil Race <philip.race at oracle.com> wrote:
>> 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
>>
> I read the guidlines but after I realized that some of the files I
> changed already contain lines with more than 130 characters I thought
> that the guildines may be antiquated in that respect :) Nevertheless,
> I'm not against coding standards at all, so please find the '80
> chars'-version of my patch below:
>
> http://cr.openjdk.java.net/~simonis/webrevs/7191872

I have commited this as 'simonis' and pushed to the 2d forest.

>
>> 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.
>>
> Thanks. I've used it in the new webrev.
> Unfortunately searching the bug database for 'xrender' only returns a
> single hit and omits the bug you mentioned. But that's a totally
> different discussion :)

Yes .. its visible as http://bugs.sun.com/view_bug.do?bug_id=7191872
but search has been "bust" for a while.

thanks ..

-phil.

>
> Regards,
> Volker
>
>> -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.
>>