Last Comment Bug 328090 - remove -fpascal-strings from mac build options
: remove -fpascal-strings from mac build options
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Ludovic Hirlimann [:Usul]
:
:
Mentors:
http://devworld.apple.com/documentati...
Depends on: 658218
Blocks: 700519 700745
  Show dependency treegraph
 
Reported: 2006-02-21 12:28 PST by Ludovic Hirlimann [:Usul]
Modified: 2011-11-08 11:44 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove -fpascal-string from build config (1.43 KB, patch)
2006-02-21 12:59 PST, Ludovic Hirlimann [:Usul]
mark: review-
Details | Diff | Splinter Review
Hg version of the patch (949 bytes, patch)
2011-02-17 00:13 PST, Ludovic Hirlimann [:Usul]
ted: review+
mbeltzner: approval2.0-
Details | Diff | Splinter Review

Description Ludovic Hirlimann [:Usul] 2006-02-21 12:28:54 PST
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] 2006-02-21 12:59:59 PST
Created attachment 212631 [details] [diff] [review]
remove -fpascal-string from build config

I've successfully build Camino with pacth applied.
Asking Mento to review.
Comment 2 Mark Mentovai 2006-02-21 19:10:56 PST
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 2007-12-20 14:06:46 PST
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 2009-02-25 14:01:01 PST
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] 2009-02-26 01:14:10 PST
(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] 2011-02-17 00:13:16 PST
Created attachment 513055 [details] [diff] [review]
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] 2011-02-25 05:18:54 PST
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] 2011-02-25 11:50:11 PST
Comment on attachment 513055 [details] [diff] [review]
Hg version of the patch

Nope, into risk reduction, so no.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-02-25 11:55:29 PST
Feel free to land this on the build-system branch.
Comment 10 Nomis101 2011-02-25 11:56:39 PST
(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) 2011-02-28 04:29:35 PST
Checked into build-system repo:

http://hg.mozilla.org/projects/build-system/rev/71c17252918a
Comment 12 Ludovic Hirlimann [:Usul] 2011-02-28 05:10:25 PST
(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 2011-05-18 14:09:40 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-05-18 16:53:26 PDT
Feel free to get someone to push this to the try server to check.
Comment 15 Ludovic Hirlimann [:Usul] 2011-05-19 01:06:26 PDT
http://hg.mozilla.org/try/pushloghtml?changeset=bbf9f6e88788
Comment 16 Ludovic Hirlimann [:Usul] 2011-05-19 01:55:01 PDT
(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] 2011-05-20 10:00:24 PDT
Got a green build !!!
Comment 18 Dão Gottwald [:dao] 2011-05-22 08:29:50 PDT
http://hg.mozilla.org/mozilla-central/rev/b6202b4a98f1

Note You need to log in before you can comment on or make changes to this bug.