Add gxx_version

The status quo is this:

build/compiler_version.py detects the version of the c++ compiler, which is assumed to be g++. That version then becomes gcc_version in gyp. gcc_version is used for CFLAGS and CXXFLAGS

Nothing keeps me from using gcc-4.4 for CC and g++-4.5 for CXX, though (let alone clang). I think splitting that up into two variables, one for c++ and one for c would benefit clarity while introducing minimal overhead. I've attached a patch that changes checks for gcc_version that are actually checks for the version of g++ into gxx_version plus everything that's required for that to work. This patch is against the chromium-9.0.570.0 tarball.

BUG=62207
and TEST="manual"

[Evan Martin, 2010-11-08 17:56:05]

Why would you want to mix different versions of the same compiler?  I am
confused about the use case, and I'm generally scared of extra complexity.

[pipping.elias, 2010-11-08 23:07:44]

This isn't really about a reasonable scenario. It's only the tip of the iceberg.
Even though I see no reason to forbid using CC=gcc-4.4 in conjunction with
CXX=gcc-4.5, I doubt that anyone does it.

What is a reasonable combination is e.g. CC=clang-2.8 CXX=g++-4.4.

That combination makes perfectly clear that retrieving the version of CXX from
CC does not make any sense.

Currently, CC is assumed to be gcc and CXX is assumed to be g++. Passing clang=1
makes that into CC=clang and CXX=clang++ instead. I think that's unnecessarily
restrictive. clang and gcc, clang++ and g++ will probably both be around for the
next ten years, each with their own benefits, caveats, flags and bugs. So code
will always have to check what compiler is used. Coupling CC and CXX the way
it's currently done is unnecessary and restrictive in my opinion. So decoupling
them is what I'm aiming for.

Eventually, every piece of C++ code that has problems with g++ 4.5 should say
that. Rather then checking gcc_version, it should check if CXX is clang++, g++
or something else and in the second case, check if the version of g++ falls into
the affected range. Not only because it adds flexibility, but also because
asking CC for the version of CXX does not make any sense if you can ask CXX
itself.

[Evan Martin, 2010-11-08 23:24:06]

On 2010/11/08 23:07:44, pipping.elias wrote:
> What is a reasonable combination is e.g. CC=clang-2.8 CXX=g++-4.4.

Hm, what is the reason to build like this?

The two accept different e.g. warning flags, so you'd also need to split up how
we pass C and C++ flags into two separate blocks depending on the compiler. 
(E.g. -fno-strict-aliasing goes to both, but warning-related settings only go to
one or the other.)

> Eventually, every piece of C++ code that has problems with g++ 4.5 should say
> that. Rather then checking gcc_version, it should check if CXX is clang++, g++
> or something else and in the second case, check if the version of g++ falls
into
> the affected range. Not only because it adds flexibility, but also because
> asking CC for the version of CXX does not make any sense if you can ask CXX
> itself.

Currently we have no case where we need to test for g++ in the code.  (We only
need the version to work around bugs in prerelease versions of g++.)
The only cases where we test for clang are places where there are clang bugs,
and we should remove all of them.  (There are only two cases, it seems -- one
where we have a bug on file, and the other due to seccomp which doesn't even
work on gcc yet.)


Sorry to be annoying about accepting this change -- it's just that our build is
already way too complicated and I'd really prefer to remove all this code rather
than add more... :\

[Evan Martin, 2010-11-08 23:24:55]

On 2010/11/08 23:24:06, Evan Martin wrote:
> Currently we have no case where we need to test for g++ in the code.  (We only
> need the version to work around bugs in prerelease versions of g++.)

Sorry, left out a sentence -- we only test for versions to set compile-time
flags, not #ifdefs.  And compile-time flags are applied a target at a time, we
can't adjust them on a per-file basis.

[pipping.elias, 2010-11-08 23:29:12]

On 2010/11/08 23:24:06, Evan Martin wrote:
> Currently we have no case where we need to test for g++ in the code.  (We only
> need the version to work around bugs in prerelease versions of g++.)

r64263 adds -fno-ipa-cp (erroneously to cflags rather than cxxflags) to make
template_url.cc (thus, c++ code) work with g++ 4.5.

[pipping.elias, 2010-11-08 23:34:39]

On 2010/11/08 23:24:55, Evan Martin wrote:
> And compile-time flags are applied a target at a time, we
> can't adjust them on a per-file basis.
I forgot about that. That is a problem.

[pipping.elias, 2010-11-08 23:49:28]

On 2010/11/08 23:24:06, Evan Martin wrote:
> The two accept different e.g. warning flags, so you'd also need to split up
how
> we pass C and C++ flags into two separate blocks depending on the compiler. 
> (E.g. -fno-strict-aliasing goes to both, but warning-related settings only go
to
> one or the other.)

What's wrong with doing that? It only needs to be done once in one place.

[Craig, 2010-11-09 17:28:39]

On 2010/11/08 23:29:12, pipping.elias wrote:
> On 2010/11/08 23:24:06, Evan Martin wrote:
> > Currently we have no case where we need to test for g++ in the code.  (We
only
> > need the version to work around bugs in prerelease versions of g++.)
> 
> r64263 adds -fno-ipa-cp (erroneously to cflags rather than cxxflags) to make
> template_url.cc (thus, c++ code) work with g++ 4.5.

Yes, that's an oversight of mine, sorry. In gyp, the variables are called
cflags, cflags_c and cflags_cc so I should have used cflags_cc there.

As Evan said, the idea is to get rid of all the special cases and the
template_url.cc 'miscompilation' is still on my todo list to sort out properly.

I'm also guilty of naming some of the stuff colloquially (i.e. gcc) rather than
pedantically (i.e. g++). At the moment we don't seem to need both a gcc_version
and a gxx_version though ... we are only using 'g++ -dumpversion' via the
misnamed 'gcc_version' variable in a small handful of places and nothing needs
the output of 'gcc -dumpversion' AFAICS. Right?

So to me there are two issues:
- Do we fix the naming issues to be pedantically correct i.e. rename gcc_version
to gxx_version and clean up the comments?
- Do we need the results of 'gcc -dumpversion' too?

I'd tend to say the former is debatable and the latter is perhaps a no since
it's not required anywhere yet. Evan?

PS. Your patch misses two other uses of gcc_version btw. - in chrome_tests.gypi
(arguably a NOP right now since strict-aliasing is off) and v8.gyp which is a
separate song-and-dance process to change.

[pipping.elias, 2010-11-09 20:47:23]

On 2010/11/09 17:28:39, Craig wrote:
> On 2010/11/08 23:29:12, pipping.elias wrote:
> > On 2010/11/08 23:24:06, Evan Martin wrote:
> > > Currently we have no case where we need to test for g++ in the code.  (We
> only
> > > need the version to work around bugs in prerelease versions of g++.)
> > 
> > r64263 adds -fno-ipa-cp (erroneously to cflags rather than cxxflags) to make
> > template_url.cc (thus, c++ code) work with g++ 4.5.
> 
> Yes, that's an oversight of mine, sorry. In gyp, the variables are called
> cflags, cflags_c and cflags_cc so I should have used cflags_cc there.
> 
> As Evan said, the idea is to get rid of all the special cases and the
> template_url.cc 'miscompilation' is still on my todo list to sort out
properly.

In a perfect world...

> I'm also guilty of naming some of the stuff colloquially (i.e. gcc) rather
than
> pedantically (i.e. g++). At the moment we don't seem to need both a
gcc_version
> and a gxx_version though ... we are only using 'g++ -dumpversion' via the
> misnamed 'gcc_version' variable in a small handful of places and nothing needs
> the output of 'gcc -dumpversion' AFAICS. Right?

Right. I knew (because I followed that bug) that the "gcc_version==45" in
chrome_browser.gypi was actually a check for the version of g++. I was aware of
the other two instances where gcc_version is accessed and let the cflags
variable fool me into thinking they dealt with C code (calling the combination
of CFLAGS and CXXFLAGS just cflags is a horrible misnomer imho).

> So to me there are two issues:
> - Do we fix the naming issues to be pedantically correct i.e. rename
gcc_version
> to gxx_version and clean up the comments?
> - Do we need the results of 'gcc -dumpversion' too?
> 
> I'd tend to say the former is debatable and the latter is perhaps a no since
> it's not required anywhere yet. Evan?

given that gcc refers both to the c compiler /and/ the gnu compiler collection,
thus everything including g++, and the version of the c compiler is unnecessary,
the name gcc_version will do just fine.

> PS. Your patch misses two other uses of gcc_version btw. - in
chrome_tests.gypi
> (arguably a NOP right now since strict-aliasing is off) and v8.gyp which is a
> separate song-and-dance process to change.

see above.

to sum up; this patch is wholly unnecessary. it is based on a couple of
assumptions of mine that turned out to be wrong. here's why the above patch is
nonsense:

  * the version of gcc is actually never needed. the value of cflags in gyp
actually affects both (what is usually referred to as) CFLAGS and CXXFLAGS.

here's why explicitly allowing CC=clang and CXX=g++ wouldn't work:

  * gyp does not allow per-file adjustments to CXXFLAGS, just per-target (this
is a horrible shortcoming of gyp imho, given i'm not familiar with scons it
might have been necessary, though)

Thanks for the discussion. Please reject this patch.

[Craig, 2010-11-10 04:43:51]

On 2010/11/09 20:47:23, pipping.elias wrote:
> Thanks for the discussion. Please reject this patch.

I'll close it for you (edit issue/tick close). I'll close the bug too.