93206 - PSM doesn't pass CC variable to NSS make

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Brian Ryner (not reading)]

When security/manager/Makefile.in builds NSS, it should pass Mozilla's CC
variable as a parameter.  This will ensure that the same compiler is used to
build NSS as is used for the rest of the Mozilla build.  (We allow specifying an
alternate compiler by setting CC before running configure).

Comment 1

[Stephane Saux]

P3
->ddrinan
t->2.1

Comment 2

[cls]

Attachment: Pass CC & CXX to NSS build.

Comment 3

[Brian Ryner (not reading)]

r=bryner

Comment 4

[cls]

Attachment: Oops. should be += not =

Comment 5

[Stephane Saux]

Move to future. Won't have time to fix these for 2.1

Comment 6

[cls]

wtc, can you sr this?

Comment 7

[Wan-Teh Chang]

Allowing CC and CCC to be overriden on the gmake command
line was not a goal of coreconf when it was designed.  So
we would first need to retrofit coreconf so that CC and CCC
can be overriden safely.  Makefile fragments in coreconf
like the following would need to be changed.

AIX.mk:
ifeq ($(CC),xlC_r)

HP-UXB.11.mk:
    CCC                 = /opt/aCC/bin/aCC -ext

OS2.mk:
CC                     = icc -q -DXP_OS2 -DOS2=4 -N10

SunOS5.mk:
        CCC        = g++
        CCC       += -Wall -Wno-format

ruleset.mk:
ifneq ($(DEFAULT_COMPILER), $(CC))

Comment 8

[cls]

Taking the bug as I spent a few hrs this evening wondering why the upgraded
nebiros couldn't build NSS so I'm a bit motivated to get this fixed now.

So, this patch is going to look wacky because:
1) previously, there was no CXXFLAGS.
2) CCC was being set with extra (presumably c++ only) flags
3) C++ files were being compiled with CCC & CFLAGS. 

Now,
1) CCC is just the compiler
2) the extra flags are being added to OS_CXXFLAGS 
3) CXXFLAGS == CFLAGS + OS_CXXFLAGS
4) c++ files are being compiled with CCC & CXXFLAGS

Comment 9

[cls]

Attachment: Introducing CXXFLAGS to coreconf

Comment 10

[cls]

Attachment: Set CCC not CXX

Comment 11

[Stephane Saux]

Chris, thanks for taking this in.
Adding patch, review.
We need to assign a target for this.  If this is absolutely necessary for the
0.9.4 branch, then it should be targetted 2.1 otherwise Future. Let me know.

Comment 12

[Wan-Teh Chang]

Chris's patch to introduce CXXFLAGS to coreconf
is the right solution.  I still need to review it
again carefully to make sure that all the necessary
changes have been made.  For example, in coreconf/OS2.mk,
the line:
CC                     = icc -q -DXP_OS2 -DOS2=4 -N10
should also be broken into two lines:
CC                     = icc
OS_CFLAGS              += icc -q -DXP_OS2 -DOS2=4 -N10

In coreconf/ruleset.mk, the line:
        COMPILER_TAG = _$(CC)
should now read:
        COMPILER_TAG = _$(firstword $(basename $(CC)))
Also, the expression $(firstword $(basename $(DEFAULT_COMPILER)))
may not be necessary because I believe DEFAULT_COMPILER is
always a simple command name.

Comment 13

[Stephane Saux]

assigning a 2.2 target.

Comment 14

[cls]

The NSS_CLIENT_TAG tagged version of coreconf/OS2.mk (v1.7) doesn't have the
'CC=icc ...' line.  Here's the updated version of coreconf/ruleset.mk (the rest
of the patch is the same):

Index: mozilla/security/coreconf/ruleset.mk
===================================================================
RCS file: /cvsroot/mozilla/security/coreconf/ruleset.mk,v
retrieving revision 1.12
diff -u -r1.12 ruleset.mk
--- ruleset.mk  2001/06/02 06:03:14     1.12
+++ ruleset.mk  2001/09/19 07:43:58
@@ -111,14 +111,14 @@
 #
 
 ifndef COMPILER_TAG
-ifneq ($(DEFAULT_COMPILER), $(CC))
+ifneq ($(DEFAULT_COMPILER), $(firstword $(basename $(CC))))
 #
 # Temporary define for the Client; to be removed when binary release is used
 #
        ifdef MOZILLA_CLIENT
                COMPILER_TAG =
        else
-               COMPILER_TAG = _$(CC)
+               COMPILER_TAG = _$(firstword $(basename $(CC)))
        endif
 else
        COMPILER_TAG =

Comment 15

[Wan-Teh Chang]

I created an NSS build system RFE (bug 107976) for
the coreconf work.

Comment 16

[Kai Engert [:KaiE:]]

Comment on attachment 48745 [details] [diff] [review]
Set CCC not CXX

r=kaie

Is it necessary to land bug  107976 and bug 93206 at the same time?

Comment 17

[hacker formerly known as [e-mail]]

The PSM patch requires the patch to NSS (actually coreconf) in order to work
correctly for all platforms.

Comment 18

[hacker formerly known as [e-mail]]

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

Comment 19

[Wan-Teh Chang]

Attachment: Minimal patch: only override CC

This patch should be used in conjunction with the coreconf
minimal patch (attachment 114058 [details] [diff] [review]) in bug 107976.

Note the two simplifications:
1. Only CC is overriden.  CCC (the C++ compiler) is not
overridden, which is fine because NSS has no C++ code.
2. I'm taking the first word of $(CC) and passing it to
NSS.  The reason is that I am not sure if I can allow
passing arbitrary compiler flags to NSS.  I propose that
we check in this patch first with this limitation and see
how things go.

Comment 20

[hacker formerly known as [e-mail]]

Comment on attachment 114059 [details] [diff] [review]
Minimal patch: only override CC

I still think the single word restriction is unnecessary.  It's also fairly
easy, if highly annoying, to get around that restriction by using a wrapper
script.  If you're going to accept an arbitrary $CC, I don't see the how
accepting arbitrary args to that $CC is any worse.

Comment 21

[Wan-Teh Chang]

Some code in NSS must be compiled with certain compiler flags
so that we only use certain instruction sets.  If we accept
arbitrary args to $(CC), we will lose that control of the
instruction set used.

We will not be able to honor all the build customizations.
The NSS coreconf build system and the original NSPR build
system ares designed to ensure that everyone builds our software
the same way as we do, using the compiler flags we have tested.
We recognize that it is common for people to have multiple
versions of a compiler installed and the ability to specify
a particular version of a compiler with its full pathname is
useful.  This is why we want to allow people to do this.
I know you don't like the single word restriction.  Please
look at it this way: this patch, with its single word restriction,
is still a step forward and better than what we have now.
So I suggest that we check it in first and see if the single
word restriction is really a problem.

Comment 22

[hacker formerly known as [e-mail]]

> Some code in NSS must be compiled with certain compiler flags
> so that we only use certain instruction sets.  If we accept
> arbitrary args to $(CC), we will lose that control of the
> instruction set used.

If you allow the user to set CC, then you've already lost that control.  

cat >/bin/annoy-nss-cc<<EOF
#!/bin/sh
exec gcc -mpentium4 $*
EOF
chmod 755 /bin/annoy-nss-cc

env CC=annoy-nss-cc CXX='g++ -mpentium4' ../mozilla/configure

It's already generally accepted that we cannot support every arbitrary compiler
flag given to configure via --enable-optimize, CFLAGS/CXXFLAGS or CC/CXX.  We
routinely have to tell people to not use -fomit-frame-pointer, a common gcc
optimization, as it kills xptcall which uses the frame pointer.  However, we do
not specifically prevent them from using *any* arbitrary flag because a handful
of them may cause problems.  If the user uses flags other than the default ones
in the build system, then it's up to them to prove that problems caused by those
flags aren't compiler issues.

> The NSS coreconf build system and the original NSPR build
> system ares designed to ensure that everyone builds our software
> the same way as we do, using the compiler flags we have tested.

If someone does a default NSS build, then they will build with those defaults. 
However, anyone could set CC on the commandline and add additional compiler
flags to the build.  You haven't protected against setting CC (or CFLAGS for
that matter) in the environment in the NSS Makefiles.  And as I pointed out
earlier, the proposed change does not prevent me from working around that
restriction though the workaround is inconvenient IMO.  Given that the
restriction is ineffective for its intended purpose, I don't see the point in
adding the restriction.  And I believe my points at
http://bugzilla.mozilla.org/show_bug.cgi?id=107976#c6 are still valid.

Comment 23

[hacker formerly known as [e-mail]]

Attachment: Only set CC

Comment 24

[Wan-Teh Chang]

Comment on attachment 114059 [details] [diff] [review]
Minimal patch: only override CC

Chris, I fully understand what you said.  Checking in the
patch I proposed would still be a *strict* improvement over
what we have right now, and my patch does not preclude
checking in your patch (removing the 'firstword' function)
in the future.	We should check in portions of your patch
that everyone thinks are safe first, otherwise we are not
making any progress on this bug.

Comment 25

[hacker formerly known as [e-mail]]

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

Comment 26

[Ari Pollak]

Marking as OS-neutral, as this is not Linux-specific.

Comment 27

[Wan-Teh Chang]

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

Comment 28

[Wan-Teh Chang]

Attachment: Only set CC, with corresponding NSS patch

Comment 29

[Wan-Teh Chang]

Comment on attachment 131805 [details] [diff] [review]
Only set CC, with corresponding NSS patch

cls, please review this patch.

It's too bad that the COMPILER_TAG variable will
have the wrong value if CC is "ccache gcc".  But
I can't come up with a good DEFAULT_COMPILER test
that works in this case.  One possible solution
is to handle programs like ccache as a special
case and use a new make variable like
CC_FRONTEND for it.

Comment 30

[Wan-Teh Chang]

Comment on attachment 131805 [details] [diff] [review]
Only set CC, with corresponding NSS patch

Julien, please review the NSS change in this patch.

I'd like to check in the NSS change on the tip and
the NSS_3_9_BRANCH.  This change (calling the
firstword function on $(CC)) is a no-op for our
own NSS builds because coreconf always sets CC to
a value with only one word.  However, the PSM change
in this patch will override the value of CC in
coreconf, so coreconf needs to protect against a
value of CC with more than one word (e.g.,
"ccache gcc".  see bug 250153).

Note that with my proposed NSS change, if CC is
"ccache gcc", COMPILER_TAG will be "ccache", which
is wrong.  A better long-term fix is probably to
delete all the coreconf code related to DEFAULT_COMPILER
and the automatic setting of COMPILER_TAG for
non-default compilers.	In other words, we would
set COMPILER_TAG manually.  Let me know if you'd
like me to do this instead.

Comment 31

[Wan-Teh Chang]

Comment on attachment 131805 [details] [diff] [review]
Only set CC, with corresponding NSS patch

I checked in the NSS coreconf change in this patch
on the NSS trunk (NSS 3.10).

We still need to get the NSS coreconf change into
NSS_CLIENT_TAG and check in the PSM change in this
patch.

Comment 32

[cls]

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

Comment 33

[Mike Shaver (:shaver emeritus)]

Comment on attachment 131805 [details] [diff] [review]
Only set CC, with corresponding NSS patch

a=shaver

Comment 34

[cls]

The patch has been checked in.