294244 - nsDeviceContextMac.cpp compiling fails with gcc-4.0.0

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Ginn Chen]

Compliing Firefox 1.0.x fails with gcc version 4.0.0 (Apple Computer, Inc. build 5018) on Mac OS 10.4
I built gcc-4.0.0 from its CVS branch.
I modified some code in Firefox to get here.
(as Phase 1 patch of 292530)

Here's the message.

nsDeviceContextMac.cpp: At global scope:
nsDeviceContextMac.cpp:950: error: prototype for 'bool nsDeviceContextMac::GetMacFontNumber
(const nsString&, short int&)' does not match any in class 'nsDeviceContextMac'
nsDeviceContextMac.h:115: error: candidate is: static bool nsDeviceContextMac::GetMacFontNumber
(const nsString&, int&)
nsDeviceContextMac.cpp: In member function 'bool nsDeviceContextMac::GetMacFontNumber(const 
nsString&, short int&)':
nsDeviceContextMac.cpp:956: error: cast from 'void*' to 'short int' loses precision
nsDeviceContextMac.cpp: In function 'PRBool EnumerateFont(nsHashKey*, void*, void*)':
nsDeviceContextMac.cpp:1157: error: cast from 'void*' to 'FMFontFamily' loses precision
nsDeviceContextMac.cpp: In member function 'virtual nsresult 
nsDeviceContextMac::CheckFontExistence(const nsString&)':
nsDeviceContextMac.cpp:453: warning: control reaches end of non-void function
make[1]: *** [nsDeviceContextMac.o] Error 1
make: *** [all] Error 2

Comment 1

[Ginn Chen]

It's easy to fix the first error.
But I don't know what to do with the FMFontFamily one.

Comment 2

[Ginn Chen]

Sorry, I forgot I modified the header.
The right meesage is,

nsDeviceContextMac.cpp: In static member function 'static bool nsDeviceContextMac::GetMacFontNumber
(const nsString&, short int&)':
nsDeviceContextMac.cpp:956: error: cast from 'void*' to 'short int' loses precision
nsDeviceContextMac.cpp: In function 'PRBool EnumerateFont(nsHashKey*, void*, void*)':
nsDeviceContextMac.cpp:1157: error: cast from 'void*' to 'FMFontFamily' loses precision
make[1]: *** [nsDeviceContextMac.o] Error 1
make: *** [all] Error 2

Comment 3

[Ginn Chen]

A simple fix.

@@ -953,7 +953,7 @@ bool nsDeviceContextMac :: GetMacFontNum
        //                              fontNum, nsFontMetricsMac::SetFont() wouldn't need to call this at all.
        InitFontInfoList();
     FontNameKey key(aFontName);
-       aFontNum = (short)gFontInfoList->Get(&key);
+       aFontNum = (short)(int)gFontInfoList->Get(&key);
        return (aFontNum != 0);
 }
 
@@ -1154,7 +1154,7 @@ EnumerateFont(nsHashKey *aKey, void *aDa
   PRBool match = PR_FALSE;
 #if TARGET_CARBON
   // we need to match the cast of FMFontFamily in nsDeviceContextMac :: InitFontInfoList()
-  FMFontFamily fontFamily = (FMFontFamily) aData;
+  FMFontFamily fontFamily = (FMFontFamily)(int) aData;
   TextEncoding fontEncoding;
   OSStatus status = ::FMGetFontFamilyTextEncoding(fontFamily, &fontEncoding);
   if (noErr == status) {

Comment 4

[Mark Mentovai]

Attachment: Build with gcc 4.0 on Mac

There are a few more Mac-specific parts of the codebase that need to be updated
for gcc 4.  Here is the patch I am using.  You can do a full build with it
using the gcc 4 from cvs (combined with the stuff over in bug 292530). 
However, the results are still unstable.  The profile manager doesn't work, and
the app tends to crash.  I haven't had time to look in to it thoroughly yet.

Comment 5

[Ginn Chen]

FYI:
I just made a release build of Firefox 1.0.4 with gcc-4.0.0.
with "-fpermissive",
create a link of libstdc++.a to /usr/lib.
modified dist/private/nss/oiddata.h by hand.

The profile manager works for me.

Comment 6

[Mark Mentovai]

I tried your method on the trunk, using -fpermissive, and got the same results
that I get when I use the patch above.  gcc-5018.

Comment 7

[Mark Mentovai]

Comment on attachment 183689 [details] [diff] [review]
Build with gcc 4.0 on Mac

gcc 4 on the Mac just got a whole lot more important.  Xcode 2.1 includes gcc
4.0, Apple build 5026.	The resultant Mozilla won't necessarily run [without
crashing].  That's not the fault of this patch.  After a preliminary diagnosis,
it looks like a compiler bug.

At this point, it's probably a good idea to get the build patch in, and work
from there.

I've been building with this patch using both gcc 4.0 (from Apple's cvs branch
and now Xcode 2.1) and 3.3 (under a variety of Xcode versions) for about a
month with no ill effects.

Comment 8

[Mark Mentovai]

Good-ish news.  A Firefox build by the gcc 4.0.0-5026 bundled with Xcode 2.1
runs, provided certain build settings are used.

When using --disable-optimize, the application is no good.  It crashes in
QuickDraw calls.  Running firefox-bin -profilemanager results in a crash, as
does attempting to visit http://www.google.com/

When using --enable-optimize-"-mcpu=750 -O2 -falitivec", the application appears
stable.  Neither of the above two cases cause it to crash.

Switching between a gcc 3.3-built app and a gcc 4.0-built app requires you to
trash the compreg.dat file from your profile.  This is no surprise: the same
thing happens when switching between debug and non-debug builds.  It would be
nice if this case were handled better than having the app die with no feedback.

Comment 9

[Darin Fisher]

Comment on attachment 183689 [details] [diff] [review]
Build with gcc 4.0 on Mac

>Index: mozilla/gfx/src/mac/nsUnicodeFallbackCache.h

>+		return (((ScriptCode) NS_PTR_TO_INT32(v1)) == 
>+		    ((ScriptCode) NS_PTR_TO_INT32(v2)));

why does this code need to cast to ScriptCode before comparing values?
why can't it just compare the raw pointer values?


sr=darin provided josh is happy with the patch.

Comment 10

[Mark Mentovai]

Hold off on this for a moment, it looks like Josh is bringing back a nifty gift
from Apple.

Comment 11

[Josh Aas]

I should have Apple's patches soon and we can compare/contrast before landing
this stuff. I'd rather make sure we're doing it right the first time as long as
other patches that have actually been tested on Intel macs exist.

Comment 12

[Mark Mentovai]

Picking this back up.

(In reply to comment #9)
> why does this code need to cast to ScriptCode before comparing values?
> why can't it just compare the raw pointer values?

It could, but the casts were already there and make the functions a clearer, if
only a little.  The extra casts shouldn't result in any extra instructions being
emitted.

The landing of bug 224305 renders the patch to nsAppRunner.cpp inapplicable. 
Ignore that part of the patch.  gcc 4 compiles the new nsAppRunner.cpp without
any hand-holding.

The Apple patch is pretty similar to this, except it uses explicit casts instead
of the NS_PTR_TO_INT32 macro, resulting in unsightly double casts.  They also
didn't handle TV2FP and FP2TV in the plugin code.  The last difference is that
their patch conforms the types in nsDragService.cpp by making them signed, and I
chose unsigned because that's what GetFlavorData uses.

There'll be a new bug for Thunderbird and anything else that needs similar changes.

Comment 13

[Josh Aas]

Comment on attachment 183689 [details] [diff] [review]
Build with gcc 4.0 on Mac

patch fails to apply

otherwise it looks good

Comment 14

[Mark Mentovai]

Attachment: up-to-date

ns4xPlugin.cpp was updated today.  This patch has the new context for that file
and doesn't include the obsolete diff for nsAppRunner.cpp.

Comment 15

[Josh Aas]

Can you explain this stuff please:

+#define TV2FP(tvp) _TV2FP((void *)tvp)
+
 static void*
-TV2FP(void *tvp)
+_TV2FP(void *tvp)

Comment 16

[Mark Mentovai]

(In reply to comment #15)
> Can you explain this stuff please:

Sure.  We start with FP2TV, which converts function pointers into CFM tvectors.
 Vice-versa for TV2FP.  This maintains CFM plugin compatibility.  Because we're
passing those functions arbitrary function pointers with arbitrary signatures,
we pass them as generic function pointers, void *.  We only care about the
function address, so we only need the pointer value.  TV2FP and FP2TV don't call
the functions, so it's safe.  It's also illegal C++, although it's perfectly
valid C.  C++ doesn't let you do void * generic function pointers.

In g++ 3.3, the compiler would print a loud warning but let you slide with an
implicit cast.  In g++ 4.0, it's a hard error.  Here's an example:

ns4xPlugin.cpp: In constructor 'ns4xPlugin::ns4xPlugin(NPPluginFuncs*,
PRLibrary*, NPError (*)(), nsIServiceManagerObsolete*)':
ns4xPlugin.cpp:446: error: invalid conversion from 'NPError (*)(char*, NPP_t*,
uint16, int16, char**, char**, NPSavedData*)' to 'void*'
ns4xPlugin.cpp:446: error:   initializing argument 1 of 'void* TV2FP(void*)'

So, move the real function out of the way and replace it with a macro that
provides the cast.  With the arguments explicitly casted to void *, the warnings
disappear under g++ 3.3, and the errors are gone under 4.0.

Comment 17

[Josh Aas]

landed on trunk - thanks Mark!

Comment 18

[Mark Mentovai]

*** Bug 294250 has been marked as a duplicate of this bug. ***

Comment 19

[Mark Mentovai]

*** Bug 257098 has been marked as a duplicate of this bug. ***

Comment 20

[Mark Mentovai]

*** Bug 257133 has been marked as a duplicate of this bug. ***