107976 - coreconf should allow CC and CCC to be overriden.

Status: RESOLVED FIXED

(NSS :: Build, enhancement, P1)

Description

[Wan-Teh Chang]

coreconf should allow CC and CCC (the C and C++
compilers) to be overriden.  This will impose
a new restriction on the values of CC and CCC
and require some cleanup in coreconf to ensure
that the new restriction is followed.  The
module (for example, PSM) that passes the values
of CC and CCC to coreconf must also follow the
restriction.

The new restriction is that CC and CCC must be
a single word, which is either the full pathname
or the name of the compiler.  CC and CC must not
contain any compiler options.

For example, some legal values of CC are:
CC = gcc
CC = /usr/local/bin/gcc
CC = /home/wtc/bin/gcc-2.95.3

Some illegal values of CC are:
CC = cc -Ae
CC = cc -xarch=v9CC = gcc -pthread

Please let me know if it is feasible to follow
this restriction and whether additional restrictions
are required.

Comment 1

[Wan-Teh Chang]

Correction: in the "Some illegal values of CC are"
paragraph, the line
  CC = cc -xarch=v9CC = gcc -pthread
should be two lines
  CC = cc -xarch=v9
  CC = gcc -pthread

Comment 2

[hacker formerly known as [e-mail]]

What's the reason for the single word restriction?  Building with CC='cc
-xarch=v9' is a valid Mozilla build flag.  We currently have a tinderboxes that
building with CC='cc -xarch=v9'.  I thought that the patch I attached to bug
93206 handled the multi-word case without a problem.  I haven't tested the
multi-word case recently though.

Comment 3

[Wan-Teh Chang]

Chris Seawood wrote:
> What's the reason for the single word restriction?
> Building with CC='cc -xarch=v9' is a valid Mozilla
> build flag.

1. The single word restriction must be imposed on the
   value of CC set by coreconf.  I believe we all
   agree this is necessary.  For example, if
   coreconf says
       CC = gcc -pthread
   and it gets overriden by
       CC = /home/wtc/bin/gcc
   on the command line, the critical -pthread flag
   will be lost.

2. So the only issue is whether the single word
   restriction should also be imposed on the value of
   CC passed to coreconf on the command line.
   As you pointed out, your patch handled the
   multi-word case without a problem, so it is in
   fact okay to pass a multi-word CC to coreconf.

   On the other hand, this restriction is not as bad
   as it seems.  For the specific case of
   CC='cc -xarch=v9' that you raised, I am asking you
   to only pass the first word (that is, CC=cc) to
   coreconf.  In this case, you will also be passing
   USE_64=1 to coreconf, which causes coreconf to add
   -xarch=v9 to CFLAGS.  So in the end you will get
   exactly what you want.

Allowing CC and CCC to be overridden will add a lot
of flexibility to coreconf, but it will also open
a lot of possible ways to break coreconf.  I am
concerned about the support cost down the road.
This is why I want us to think this through and
clearly state what can and cannot be done.  My
first proposal is intentionally more restrictive
than necessary.  I want to ensure that every step
we take is safe.

One question for you: is it more common to say
    CC='cc -xarch=v9'
or
    CC=cc CFLAGS=-xarch=v9
to specify a 64-bit Solaris build?  Does this
latter work too?

Comment 4

[hacker formerly known as [e-mail]]

I misread what you said.  I thought that you were setting a restriction on the
value of CC passed to make.  Yes, I agree that the value set in the
coreconf/*.mk files needs to be a single word, otherwise we'll clobber the
useful flags.

For the specific case that I cited, the restriction works but only because you
have another mechanism to handle that specific case.  If I wanted to do a CC='cc
-o32' build on Irix (to get plugin compatibility with 4.x), then we'd be back to
square one.

Right now, it's more common to say  CC='cc -xarch=v9' to get a 64-bit build as
it is guaranteed to work across most packages. In theory, either method should
work but I haven't tested the latter.   The problem is that some cflags, like
-xarch=v9, change the behavior of the compiler so much that you must make sure
that all binaries are compiled with that flag.  Usually, setting CC is the only
way to keep that guarantee.
  

Comment 5

[Wan-Teh Chang]

Chris Seawood wrote:
> I misread what you said.  I thought that you were setting
> a restriction on the value of CC passed to make.

Actually I want to set that restriction as well.  I know it
will be okay to pass a multi-word value of CC to make, at
least I don't have a real-world counterexample, but I just
feel that I need to think more about it.

For your new example of CC='cc -o32' on IRIX, you can use
the coreconf option USE_N32=, although coreconf won't do
the right thing if the version of cc you are using generates
n32 binaries by default.  This can be fixed by modifying
coreconf to use -o32 if USE_N32 is undefined or adding a new
coreconf option USE_O32.

My point is that coreconf usually has an option for each
compiler flag that you want to specify in the value of CC.
For example, coreconf has USE_64=1 for CC='cc -xarch=v9'
and USE_N32= for CC='cc -o32'.

I propose that we first proceed with the restriction that
*both* the value of CC passed to make and the value set in
the coreconf/*.mk files need to be a single word, and get
your patch checked in.  Then, I will think it over whether
we can allow passing a multi-word value of CC to make.

How does this sound?

Comment 6

[hacker formerly known as [e-mail]]

> My point is that coreconf usually has an option for each
> compiler flag that you want to specify in the value of CC.

And when coreconf doesn't have an equivalent option, then we are blocked waiting
for someone to find the time to:
1) verify that our problem is actually a coreconf problem
and
2) deem it important enough to devote resources to fixing

Historically, neither of those steps has typically been done in a timely manner.
 Why make the developers bother going through that timesink when we can just
build flexibility into the system to allow multi-word CCs to be passed in? 
Coreconf loses nothing by allowing multi-word CCs to be passed in and developers
gain the flexibility of being able to use their "non-standard" configurations
without being unnecessarily blocked.

Comment 7

[Wan-Teh Chang]

I haven't had time to work on this. Sigh.  I am moving
the target to NSS 4.0.

Comment 8

[Dennis Haney]

Is something happening to this?

Comment 9

[Dan Rosen]

I'd like to gently push for this as well. I'm building mozilla for the iPaq, 
and I have to cross-compile, because the iPaq doesn't have enough resources to 
compile natively. Being able to override CC and CCC would greatly simplify the 
task of cross-compiling security.

I'd say that in general, this is a moderately important bug for getting Mozilla 
running on embedded devices.

Comment 10

[Dan Rosen]

Hm. Chris Seawood just pointed me at bug 104541. Is this a dup or otherwise 
deserving of abandonment?

Comment 11

[hacker formerly known as [e-mail]]

No, this isn't a dupe of 104541.  There are valid uses of overriding CC/CXX
other than just for cross-compiling.  And according to bug 104541#c2, just
overriding CC/CXX isn't enough to get a proper cross-compilation env setup for NSS.

Comment 12

[hacker formerly known as [e-mail]]

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

Comment 13

[Kai Engert (:KaiE:)]

What do you think about the patch I had attached to bug 166305?

Comment 14

[hacker formerly known as [e-mail]]

The patch from bug 93206 was more robust.

Comment 15

[Wan-Teh Chang]

Yes.  We should move that patch (attachment 48743 [details] [diff] [review]) to
this bug.

Comment 16

[cls]

Attachment: Add CXXFLAGS to coreconf

Attachment 48743 [details] [diff] applied to the current NSS_CLIENT_TAG pull.

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 98203 [details] [diff] [review]
Add CXXFLAGS to coreconf

Dauphin asked me to review the patch. It looks good to me, but I suggest that
Wan-Teh should review as well, since I'm not really a NSS person.

Comment 18

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 19

[Wan-Teh Chang]

Attachment: Minimal patch for coreconf

This patch is the absolute minimum changes required
for Mozilla to override the compilers used by NSS.

Note the two simplifications:
1. It only allows the C compiler to be overridden.
This is sufficient for NSS because NSS does not have
any C++ code.
2. It requires that the value of CC be a pathname,
with no command-line options.  I understand that
this may turn out to be a limitation, but I suggest
that we check in this patch first because it is
still a step forward.

I will attach the corresponding PSM patch to bug
93206.

Comment 20

[Wan-Teh Chang]

Attachment: Minimal patch for coreconf, v2

I found that GNU make's "basename" function does not
do the same thing as C's "basename" function :-)  The
GNU make function we want is "notdir".

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 22

[cls]

A form of this patch has been checked into the NSS tree some time ago.  See
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/security/coreconf/ruleset.mk .