328090 - remove -fpascal-strings from mac build options

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ludovic Hirlimann [:Usul]]

We pass a flag to gcc so it can recognize and use pascal strings. This is now discouraged by Apple (see url). We don't seem to use pascal strings. Hence I'm opening this bug so we don't pass this flag to gcc.

Comment 1

[Ludovic Hirlimann [:Usul]]

Attachment: remove -fpascal-string from build config

I've successfully build Camino with pacth applied.
Asking Mento to review.

Comment 2

[Mark Mentovai]

Comment on attachment 212631 [details] [diff] [review]
remove -fpascal-string from build config

I could swear that we're still using "\p" in places where breakage might not be all that apparent.  If you want to kill -fpascal-strings, you'll need to kill that code too.

I see one use of "\p" in nsMacResources.cpp, for example.

Comment 3

[Mark Mentovai]

I just grepped the current trunk for '"\\p' and looked through the results, filtering out code that's not part of any current build as far as I know.  The following uses of "\p" still need to be removed:

modules/plugin/samples/default/mac/npmac.cpp
xpcom/io/nsAppFileLocationProvider.cpp
xpfe/bootstrap/appleevents/nsAEApplicationClass.cpp
xpfe/bootstrap/appleevents/nsAEDefs.h
xpfe/bootstrap/appleevents/nsMacUtils.cpp
xpinstall/src/nsInstallFileOpItem.cpp

All other uses of "\p" are either in files that are never built or are in sections of files that are never built.  For example, there's still some code only used in pre-Mac OS X or CFM builds that are no longer of any consequence to us.  Not all of the files above are always built.  xpinstall, for one, is frequently not built, but it should be fixed because as far as I know it hasn't yet been formally deprecated and it's still possible to build it.

Some of these are just DebugStr only in debug builds, and can be converted to some other form of logging or just eliminated.  Some just use "\p" to initialize empty Pascal strings (Str255 s = "\p";), which can also be done with {0} (Str255 s = {0};).  Some are still doing things with resources that probably no longer work.

These should all be easy to eliminate.

Comment 4

[Simon Strandman]

If this is fixed it would perhaps be possible to compile firefox with a vanilla gcc on OSX. It currently fails because -fpascal-strings is in Apple's gcc only.

Comment 5

[Ludovic Hirlimann [:Usul]]

(In reply to comment #4)
> If this is fixed it would perhaps be possible to compile firefox with a vanilla
> gcc on OSX. It currently fails because -fpascal-strings is in Apple's gcc only.

Unfortunatly I got lazy and never did the proper patch. Feel free to remove the pascal strings in the code.

Comment 6

[Ludovic Hirlimann [:Usul]]

Attachment: Hg version of the patch

(In reply to comment #3)
> I just grepped the current trunk for '"\\p' and looked through the results,
> filtering out code that's not part of any current build as far as I know.  The
> following uses of "\p" still need to be removed:
> 
> modules/plugin/samples/default/mac/npmac.cpp
> xpcom/io/nsAppFileLocationProvider.cpp
> xpfe/bootstrap/appleevents/nsAEApplicationClass.cpp
> xpfe/bootstrap/appleevents/nsAEDefs.h
> xpfe/bootstrap/appleevents/nsMacUtils.cpp
> xpinstall/src/nsInstallFileOpItem.cpp

When I was able to find the file in mxr - I didn't find any occurrences of \p anymore in them.

> All other uses of "\p" are either in files that are never built or are in
> sections of files that are never built.  For example, there's still some code
> only used in pre-Mac OS X or CFM builds that are no longer of any consequence
> to us.  Not all of the files above are always built.  xpinstall, for one, is
> frequently not built, but it should be fixed because as far as I know it hasn't
> yet been formally deprecated and it's still possible to build it.
> 

I also grepped my local copy of comm-central and nothing poped out.

Comment 7

[Ludovic Hirlimann [:Usul]]

Comment on attachment 513055 [details] [diff] [review]
Hg version of the patch

Requesting approval. removing unused code.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 513055 [details] [diff] [review]
Hg version of the patch

Nope, into risk reduction, so no.

Comment 9

[(not currently active) Ted Mielczarek]

Feel free to land this on the build-system branch.

Comment 10

[Nomis101]

(In reply to comment #6)
> Created attachment 513055 [details] [diff] [review]
> Hg version of the patch

There is the same -fpascal-strings in /js/src/configure.in, is it also possible to remove it from there?

Comment 11

[Mark Banner (:standard8)]

Checked into build-system repo:

http://hg.mozilla.org/projects/build-system/rev/71c17252918a

Comment 12

[Ludovic Hirlimann [:Usul]]

(In reply to comment #11)
> Checked into build-system repo:
> 
> http://hg.mozilla.org/projects/build-system/rev/71c17252918a

Which burns on /builds/slave/bld-system-osx-dbg/build/browser/components/migration/src/nsMacIEProfileMigrator.cpp:155

Comment 13

[Hanspeter Niederstrasser]

browser/components/migration/src/nsMacIEProfileMigrator.cpp was removed in bug 656205.  Does the patch still cause burning on a current tree in build-system?

Comment 14

[(not currently active) Ted Mielczarek]

Feel free to get someone to push this to the try server to check.

Comment 15

[Ludovic Hirlimann [:Usul]]

http://hg.mozilla.org/try/pushloghtml?changeset=bbf9f6e88788

Comment 16

[Ludovic Hirlimann [:Usul]]

(In reply to comment #13)
> browser/components/migration/src/nsMacIEProfileMigrator.cpp was removed in
> bug 656205.  

It does because of components/migration/src/nsSafariProfileMigrator.cpp:1285: error: unknown escape sequence '\p'

Comment 17

[Ludovic Hirlimann [:Usul]]

Got a green build !!!

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b6202b4a98f1