Change some Mac OS X compiler flags

RESOLVED FIXED in 4.8

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: wtc, Assigned: wtc)

Tracking

other
x86
macOS

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

10 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
The attached patch changes some Mac OS X compiler flags used by NSPR.

1. Use -x assembler-with-cpp with assembly files so that we can use
#ifdef's.

2. Replace -Wmost by -Wall.  (-Wmost is an Apple extension.  Standard
GCC doesn't recognize -Wmost.)  (Mozilla uses -Wall on all platforms.)

3. Change the optimize flag from -O to -O2.  (-O seems to be the same
as -O1.)  Mozilla uses -O2 on Mac OS X.
Attachment #365604 - Flags: review?(ted.mielczarek)
Assignee

Comment 1

10 years ago
Comment on attachment 365604 [details] [diff] [review]
Proposed patch

>+    AS='$(CC) -x assembler-with-cpp'

NSPR has an assembly file that uses #ifdef's:
http://mxr.mozilla.org/nspr/source/nsprpub/pr/src/md/unix/os_Darwin.s

The GCC in Xcode understands #ifdef's without -x assembler-with-cpp,
but the standard GCC needs -x assembler-with-cpp to assemble os_Darwin.s.
Assignee

Updated

10 years ago
Attachment #365604 - Flags: review?(glen.beasley)
Assignee

Comment 2

10 years ago
Comment on attachment 365604 [details] [diff] [review]
Proposed patch

Glen, could you review this NSPR Mac patch?

We should also use -Wall instead of -Wmost in
mozilla/security/coreconf/Darwin.mk.

NSS/coreconf doesn't need the other two changes -- it
already uses -O2, and we can add -x assembler-with-cpp
when we have assembly files for Mac that use C
preprocessor directives such as #include and #ifdef.
Comment on attachment 365604 [details] [diff] [review]
Proposed patch

+    AS='$(CC) -x assembler-with-cpp'

I think you should only set AS if it's not already set, so it can be overridden from the commandline.

r=me with that change.
Attachment #365604 - Flags: review?(ted.mielczarek) → review+

Updated

10 years ago
Attachment #365604 - Flags: review?(glen.beasley) → review+
Assignee

Comment 4

10 years ago
I don't know how to implement Ted's suggestion in comment 3,
so I removed the AS change and checked the rest of the patch
in on the NSPR trunk (NSPR 4.8).  I will see if I can fix os_Darwin.s
issue by renaming the file with the .S suffix.

Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.257; previous revision: 1.256
done
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.252; previous revision: 1.251
done
Attachment #365604 - Attachment is obsolete: true
Assignee

Comment 5

10 years ago
Renaming os_Darwin.s with the .S suffix is more work
than I expected.  First, CVS seems to have problems
with two file names that differ only in case.  Second,
NSPR's build system assumes that all the assembly files
for one platform have the same suffix (ASM_SUFFIX),
so we can't have most Mac OS X assembly files with
the .s suffix and one with the .S suffix.

I'm marking this bug fixed because I'm not motivated
to figure out how to make the AS change properly.
It's not that important to support the standard,
non-Apple gcc.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I was simply thinking of replacing what you had in the patch with:
if -z "$AS";
  AS='$(CC) -x assembler-with-cpp'
fi
Assignee

Comment 7

10 years ago
Ted,

Thanks for the code snippet.  I believe we need more changes.
Looking at nsprpub/configure (with all m4 macros expanded),
I see these "AS=xxx" assignments (shown with line numbers
on the left):

1775 AS="$ac_cv_prog_AS"

1784 test -n "$AS" || AS="echo"

2404 AS="$ac_cv_path_AS"

2413 test -n "$AS" || AS="$CC"

2786 AS='$(CC)'

These assignments come from the following code in nsprpub/configure.in:

476     AC_CHECK_PROGS(AS, $AS "${target_alias}-as" "${target}-as", echo)

492     AC_PATH_PROGS(AS, as, $CC)

647 AS='$(CC)'

So, if we add the following code to the *-darwin*) section at line 3342:

  if test -z "$AS"; then
    AS='$(CC) -x assembler-with-cpp'
  fi

I believe "$AS" cannot be an empty string because of
the assignment at line 2413 or line 2786.
Ah, yes, I missed that one. In that case, I'm fine with your change. It would be nice if the user could override AS, but it's not critical. (Especially since we set it to CC and that should work anyway.)
Assignee

Comment 9

10 years ago
With Ted's approval in comment 8, I checked in the remaining
change on the NSPR trunk (NSPR 4.8).

Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.258; previous revision: 1.257
done
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.253; previous revision: 1.252
done
You need to log in before you can comment on or make changes to this bug.