Re: [ft] freetype2 and Win64

[mpsuzuki]

Hi,

Just I've built Win64 development tools, the problems
you reported are completely reproduced. However, I don't
have the extra licenses/machines to test Win64 binaries.
I will have to ask for your help to finish this issue...

On Wed, 2 Sep 2009 18:27:22 -0400
NightStrike <address@hidden> wrote:
>/home/nightstrike/work/vlc/extras/contrib/src/freetype2/src/cff/cffgload.c:
>In function ‘cff_decoder_parse_charstrings’:
>/home/nightstrike/work/vlc/extras/contrib/src/freetype2/src/cff/cffgload.c:866:12:
>warning: cast from pointer to integer of different size

[snip]

>All of these are related to castings that assume that pointers are the
>size of long.  On this platform, they are not.

Correct. Win64 is LLP64, we cannot calculate the pointer
with unsigned long variables. When I've ever worked for
16-bit and 64-bit systems, I was thinking about ILP64 and
LP64 systems, but I slipped to take LLP64 into account.
It is my error, I'm sorry.

>                                                I looked into some of
>them, and I see for instance this:
>
>  /*************************************************************************/
>  /*                                                                       */
>  /* <Type>                                                                */
>  /*    FT_Fixed                                                           */
>  /*                                                                       */
>  /* <Description>                                                         */
>  /*    This type is used to store 16.16 fixed float values, like scaling  */
>  /*    values or matrix coefficients.                                     */
>  /*                                                                       */
>  typedef signed long  FT_Fixed;
>
>Well, that's an easy fix.  Just change signed long to intptr_t, and
>problems are solved.

Yet I've not tested this, but maybe it could remove most
warnings. However, I prefer to use ptrdiff_t that is
available since C89, because intptr_t is available since C99.

But, we cannot do such, because FT_Fixed is public interface
and changing its type to different size will break the binary
compatibility.

>However, the comments in the typedef say that FT_Fixed is expected to
>be 16 bits by 16 bits.  This isn't the cast even on a linux 64 system,
>where long is 32.32.  Further, why is a type like this being used to
>store pointers?  Surely a pointer isn't a matrix coefficient.

Yes, if we restrict FT_Fixed to the shortest type covering
the essential part (16.16 int), the problem will occur on
LP64 systems either.

As you described, the proposed fix just solves the warning
against invalid casts, it is not good solution. As you wrote,
the interplaying between FT_Fixed and the pointer is the root
of this issue, it is what should be fixed.

>I want to be able to properly port this, but it seems like this isn't
>as trivial as just replacing a bunch of long's with intptr_t's.

There are 3 groups of the problem.

A) bdflib.c

In this module, the length of property name should be
typed FT_Offset (= size_t) to take the return value
by strlen(). On C89-conforming implementations, size_t
won't exceed the unsigned long [1], so taking strlen()
return value by unsigned long is ok, but it is not ok
for LLP64 implementations that 64-bit size_t exceeds
32-bit unsigned long. The fix would be something like...

diff --git a/src/bdf/bdf.h b/src/bdf/bdf.h
index e3088a2..780168f 100644
--- a/src/bdf/bdf.h
+++ b/src/bdf/bdf.h
@@ -216,7 +216,7 @@ FT_BEGIN_HEADER
     bdf_glyph_t*     unencoded;      /* Unencoded glyphs themselves.        */
 
     unsigned long    props_size;     /* Font properties allocated.          */
-    unsigned long    props_used;     /* Font properties used.               */
+    size_t           props_used;     /* Font properties used.               */
     bdf_property_t*  props;          /* Font properties themselves.         */
 
     char*            comments;       /* Font comments.                      */
diff --git a/src/bdf/bdflib.c b/src/bdf/bdflib.c
index c8afc01..54d380b 100644
--- a/src/bdf/bdflib.c
+++ b/src/bdf/bdflib.c
@@ -971,7 +971,7 @@
                        int          format,
                        bdf_font_t*  font )
   {
-    unsigned long    n;
+    FT_Offset        n;
     bdf_property_t*  p;
     FT_Memory        memory = font->memory;
     FT_Error         error = BDF_Err_Ok;
@@ -991,7 +991,7 @@
     p = font->user_props + font->nuser_props;
     FT_ZERO( p );
 
-    n = (unsigned long)( ft_strlen( name ) + 1 );
+    n = ( ft_strlen( name ) + 1 );
 
     if ( FT_NEW_ARRAY( p->name, n ) )
       goto Exit;
@@ -1019,7 +1019,7 @@
                     bdf_font_t*  font )
   {
     hashnode       hn;
-    unsigned long  propid;
+    FT_Offset      propid;
 
 
     if ( name == 0 || *name == 0 )
@@ -1028,7 +1028,7 @@
     if ( ( hn = hash_lookup( name, &(font->proptbl) ) ) == 0 )
       return 0;
 
-    propid = (unsigned long)hn->data;
+    propid = (FT_Offset)hn->data;
     if ( propid >= _num_bdf_properties )
       return font->user_props + ( propid - _num_bdf_properties );
 
@@ -1261,7 +1261,7 @@
                      char*        name,
                      char*        value )
   {
-    unsigned long   propid;
+    FT_Offset       propid;
     hashnode        hn;
     bdf_property_t  *prop, *fp;
     FT_Memory       memory = font->memory;
@@ -1273,7 +1273,7 @@
     {
       /* The property already exists in the font, so simply replace */
       /* the value of the property with the current value.          */
-      fp = font->props + (unsigned long)hn->data;
+      fp = font->props + (FT_Offset)hn->data;
 
       switch ( fp->format )
       {
@@ -1335,7 +1335,7 @@
       font->props_size++;
     }
 
-    propid = (unsigned long)hn->data;
+    propid = (FT_Offset)hn->data;
     if ( propid >= _num_bdf_properties )
       prop = font->user_props + ( propid - _num_bdf_properties );
     else
@@ -2044,7 +2044,7 @@
       p->memory    = 0;
 
       { /* setup */
-        unsigned long    i;
+        FT_Offset        i;
         bdf_property_t*  prop;
 
 
@@ -2472,7 +2472,7 @@
 
     hn = hash_lookup( name, (hashtable *)font->internal );
 
-    return hn ? ( font->props + (unsigned long)hn->data ) : 0;
+    return hn ? ( font->props + (FT_Offset)hn->data ) : 0;
   }
 

However, if I make a BDF font including a string longer
than 32-bit length, I will find more bugs of FT2.

B) t1decode.c & cffgload.c

In these modules, a seed of random number to decode the
encrypted Adobe Type1 & Type2 font program is calculated
from the memory address of the parameters. I'm not sure
which is the best to use the calculation "FT_Fixed",
"FT_Int32", "FT_Long", or "FT_PtrDist". I have to check
Adobe documentation.

It won't be long work, please wait.


C) ftcbasic.c & ftccmap.c

In this module, a hash value FTC_FaceID for a FT_Face
object is calculated by the memory address of the object. 

The hash value is designed to be 32-bit even on non
32-bit platforms. The public interface
include/freetype/ftcache.h has a macro FTC_FACE_ID_HASH()
that includes a cast to FT_UInt32, like this:

#define FT_POINTER_TO_ULONG( p )  ( (FT_ULong)(FT_Pointer)(p) )

#define FTC_FACE_ID_HASH( i )                                \
          ((FT_UInt32)(( FT_POINTER_TO_ULONG( i ) >> 3 ) ^   \
                       ( FT_POINTER_TO_ULONG( i ) << 7 ) ) )

Even if we cannot use 64-bit hash value, we should care
the overflow to avoid the hash value conflict. I guess
Win64 is the first LLP64 platform that FreeType2 runs on,
maybe we can insert some change if it won't break the
compatibilities of non-LLP64 systems.

I checked if how many FT2 client softwares use FTC_FACE_ID_HASH()
macro by google, but I could not find.

Fixing this (possibly the most important task) might be
long work, please wait a while.

Regards,
mpsuzuki


P.S.
BTW, testing the fix for LLP64 for the overflow unsigned
long by the pointer, or invalid cast from the pointer to
unsigned long, I wish if there is a memory allocater
returning "low" block whose address fits to 32-bit variable
and another allocator returning "high" block whose address
exceeds the 32-bit variable. If you know anything like
that, please let me know.

[1]
http://coding.derkeiler.com/Archive/C_CPP/comp.lang.c/2007-03/msg02514.html

I didn't know that C89-conforming ILP32 systems cannot
have 64-bit size_t, even if they have 64-bit long long.