657653 - stdc++compat.cpp checks for compiler version but needs libstdc++ version

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

stdc++compat.cpp uses many variations of

#if (__GNUC__ == 4) && (__GNUC_MINOR__ >= 2)

but what it does depends on the libstdc++ version, no the compiler versions. Those are normally the same with gcc (in fact, that is why this file exists), but they are different with clang.

Short of a better idea we should write a configure check to detect the libstdc++ version and change the checks to look like

#if (MOZ_LIBSTDCXX_MAJOR == 4) && (MOZ_LIBSTDCXX_MINOR >= 2)

Comment 1

[Mike Hommey [:glandium]]

There might actually be something simpler:
$ grep __GLIBCXX__ /usr/include/c++/*/*/bits/c++config.h
/usr/include/c++/4.3.5/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20100920 
/usr/include/c++/4.3/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20100920 
/usr/include/c++/4.4.6/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20110504 
/usr/include/c++/4.4/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20110504 
/usr/include/c++/4.5.3/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20110428 
/usr/include/c++/4.5/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20110428 
/usr/include/c++/4.6.1/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20110507
/usr/include/c++/4.6/x86_64-linux-gnu/bits/c++config.h:#define __GLIBCXX__ 20110507

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

A problem with __GLIBCXX__ is that it is the release date, so it doesn't map to a version easily:

$ svn cat svn://gcc.gnu.org/svn/gcc/tags/gcc_4_5_3_release/gcc/DATESTAMP
20110428

$ svn cat svn://gcc.gnu.org/svn/gcc/tags/gcc_4_6_0_release/gcc/DATESTAMP
20110325

Comment 3

[Mike Hommey [:glandium]]

:(

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: proposed patch

I am currently testing this by rebuilding from scratch with clang. I will push to try too, any other case you think I should test?

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

The clang built finished fine. The try build is at

http://tbpl.mozilla.org/?tree=Try&rev=3d31d0f2c230

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: fancier version that handles host and target

Being tested at
http://tbpl.mozilla.org/?tree=Try&rev=64c0294ab016

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: correct quotes

And finally one with all the quotes right!
Any idea why we use ccache on 32 bits but not 64? :-)

http://tbpl.mozilla.org/?tree=Try&rev=5ef11df9b3ee

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 533880 [details] [diff] [review]
correct quotes

Review of attachment 533880 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/libstdcxx.m4
@@ +5,5 @@
> +  run_test() {
> +    cxx="${1}"
> +    touch conftest.cc
> +    stdline=$($cxx -shared conftest.cc -o conftest.so -Wl,-t 2>&1 | \
> +              grep libstdc\+\+)

I don't know about clang, but g++ -shared -Wl,-t is enough for gcc.

@@ +6,5 @@
> +    cxx="${1}"
> +    touch conftest.cc
> +    stdline=$($cxx -shared conftest.cc -o conftest.so -Wl,-t 2>&1 | \
> +              grep libstdc\+\+)
> +    dso=$(echo $stdline | sed -e 's/.*(//' -e 's/).*//')

You can replace both with
dso=$($cxx -shared -Wl,-t | sed -n '/libstdc++/s/.*(\(.*\))/\1/p')
(though I think we tend to use `` instead of $())

@@ +9,5 @@
> +              grep libstdc\+\+)
> +    dso=$(echo $stdline | sed -e 's/.*(//' -e 's/).*//')
> +    rm -f conftest.cc conftest.so
> +    full_ver=$(readelf -V "$dso" | grep "Name: GLIBCXX" | sed "s/.*Name: //" | \
> +               tail -1)

full_ver=$(readelf -V "$dso" | awk '/Name: GLIBCXX/{v=$NF}END{print v}')
Though I'm not sure we can rely on the versions being ordered.

@@ +25,5 @@
> +  AC_DEFINE_UNQUOTED(MOZ_LIBSTDCXX_MAJOR, $major)
> +  AC_DEFINE_UNQUOTED(MOZ_LIBSTDCXX_MINOR, $minor)
> +  AC_DEFINE_UNQUOTED(MOZ_LIBSTDCXX_MICRO, $micro)
> +
> +  run_test "${CXX:-c++}"

Should be HOST_CXX or something like that, but cf. rest of the review, we don't need that.

::: build/stdc++compat.cpp
@@ +48,5 @@
> +   GLIBCXX_3.4.11 is from gcc 4.4.0
> +   GLIBCXX_3.4.12 is from gcc 4.4.1
> +   GLIBCXX_3.4.13 is from gcc 4.4.2
> +   GLIBCXX_3.4.14 is from gcc 4.5
> +   GLIBCXX_3.4.15 is from gcc 4.6 */

4.6 is GLIBCXX_3.4.16 here. I don't know where that places GLIBCXX_3.4.15.

@@ +54,2 @@
>  namespace std {
> +#if MOZ_LIBSTDCXX_MICRO >= 9

Please enclose the whole thing with a check for major and minor version

::: build/unix/elfhack/Makefile.in
@@ +114,5 @@
>  # on mozilla buildbots
>  OS_CXXFLAGS := $(filter-out -pedantic,$(OS_CXXFLAGS))
>  
> +# FIXME: Is this the correct variable to set?
> +OS_CXXFLAGS := -include $(DEPTH)/mozilla-host-config.h $(OS_CXXFLAGS)

You can simplify the problem by adding -static-libstdc++ in HOST_LDFLAGS and removing $(STDCXX_COMPAT) from HOST_CPPSRCS for elfhack.

::: configure.in
@@ +7577,5 @@
>      STDCXX_COMPAT=stdc++compat.cpp)
>  
>  AC_SUBST(STDCXX_COMPAT)
>  
> +MOZ_FIND_LIBSTDCXX_SYM_VER()

if test -n "$STDCXX_COMPAT"; then
MOZ_FIND_LIBSTDCXX_SYM_VER()
fi

(not sure it's worth putting in a separate file)

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: updated patch

Unfortunately, the suggested line

dso=$($cxx -shared -Wl,-t | sed -n '/libstdc++/s/.*(\(.*\))/\1/p')

only works in gnu ld, gold doesn't add the parentheses:

clang++ -shared -Wl,-t 2>&1 | grep libstd
/usr/lib/gcc/x86_64-redhat-linux/4.5.1/libstdc++.so

I have kept the $() and the code in a new file in this patch, but I can change that if you want.

I had forgotten to check for backports in the gcc branches. I have done so and added the revisions to the table to make it easier to check.

The builds are running at
http://tbpl.mozilla.org/?tree=Try&rev=b2d6aefa7dd8

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I forgot to mention the version sorting issue. The first version of the patch had a call to sort -V, but the version of sort in the bots is too old to have -V. Any ideas?

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 533955 [details] [diff] [review]
updated patch

Review of attachment 533955 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/libstdcxx.m4
@@ +1,5 @@
> +dnl detect the symbol versions provided by libstdc++
> +
> +AC_DEFUN(MOZ_FIND_LIBSTDCXX_SYM_VER,
> +[
> +  run_test() {

You don't need that to be a function anymore

@@ +4,5 @@
> +[
> +  run_test() {
> +    cxx="${1}"
> +    stdline=$($cxx -shared -Wl,-t 2>&1 | grep libstdc\+\+)
> +    dso=$(echo $stdline | sed -e 's/.*(//' -e 's/).*//')

This should work with both GNU ld and gold:
g++ -shared -Wl,-t 2>&1 | sed -n '/libstdc++/{s/.*(\(.*\))/\1/;p}'

::: build/stdc++compat.cpp
@@ +49,5 @@
> +   GLIBCXX_3.4.12 is from gcc 4.4.1 (147138)
> +   GLIBCXX_3.4.13 is from gcc 4.4.2 (151127)
> +   GLIBCXX_3.4.14 is from gcc 4.5.0 (151126)
> +   GLIBCXX_3.4.15 is from gcc 4.6.0 (160071)
> +   GLIBCXX_3.4.16 is form gcc 4.6.1 (172240) */

from

@@ +153,5 @@
>      }
>  }
>  #endif
>  
> +#endif /*MOZ_LIBSTDCXX_MICRO >= 14*/

doesn't match anymore

::: build/unix/elfhack/Makefile.in
@@ +114,5 @@
>  OS_CXXFLAGS := $(filter-out -pedantic,$(OS_CXXFLAGS))
>  
> +# need this because STDCXX_COMPAT is built with information about the target
> +# libstdc++, not the host one.
> +HOST_LDFLAGS += -static-libstdc++

Note that the same might apply to other host programs, though :-/
Even if not right now, it could apply in the future... I'll try to think of something.

Comment 12

[Mike Hommey [:glandium]]

(In reply to comment #10)
> I forgot to mention the version sorting issue. The first version of the
> patch had a call to sort -V, but the version of sort in the bots is too old
> to have -V. Any ideas?

I think we need a small python program to handle that. It could also handle the check in config/config.mk in a better way at the same time.

Comment 13

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: updated patch

I had the wrong mozconfig in my home machine, so the tests were disabled :-(

This one fixes the version comparisons.

Do you want a python scrip to implement just the "sort -V"? I can check if the versions are guaranteed to be sorted if you want.

http://tbpl.mozilla.org/?tree=Try&rev=94b0c0eab013

(running to the office now)

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: version in python

Comment 15

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Fix to work with ccache, python version.

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

The builds are slowly showing up on

http://tbpl.mozilla.org/?tree=Try&rev=248d2290dc13

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 534095 [details] [diff] [review]
Fix to work with ccache, python version.

Review of attachment 534095 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer the python part to ted, but before that, we actually still have the HOST problem. You're not catching that on try because you're not running tests, but i'd expect at least one test to fail.

What I'd suggest is to set two variables with the corresponding version for host or target libstdc++, and set the value in config/rules.mk with something like:
stdcxxcompat.o: DEFINES+=-DVERSION=$(VER)
host_stdcxxcompat.o: DEFINES+=-DVERSION=$(HOST_VER)
(with proper variable names and use of variables for suffixes)

Comment 18

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: handle both host and target

http://tbpl.mozilla.org/?tree=Try&rev=d668b92740c7

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: also update js/src/config/rules.mk

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 534622 [details] [diff] [review]
also update js/src/config/rules.mk

Review of attachment 534622 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a couple little nits.
I leave the python part to ted.

::: config/rules.mk
@@ +906,5 @@
>  endif # NO_PROFILE_GUIDED_OPTIMIZE
>  
>  ##############################################
>  
> +stdc++compat.o: CXXFLAGS+=-DMOZ_LIBSTDCXX_VERSION=$(MOZ_LIBSTDCXX_TARGET_VERSION)

make that stdc++compat.${OBJ_SUFFIX}

::: configure.in
@@ +7570,5 @@
>  
>  AC_SUBST(STDCXX_COMPAT)
>  
> +if test -n "$STDCXX_COMPAT"; then
> +   eval $($_topsrcdir/build/autoconf/libstdcxx.py)

I'd go for something like:
MOZ_LIBSTDCXX_TARGET_VERSION=`$_topsrcdir/build/autoconf/libstdcxx.py $CXX`
MOZ_LIBSTDCXX_HOST_VERSION=`$_topsrcdir/build/autoconf/libstdcxx.py $HOST_CXX`

and rename libstdcxx.py to libstdcxx_version.py

But it's fine for me either way.

Comment 21

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: OBJ_SUFFIX

builds at

http://tbpl.mozilla.org/?tree=Try&rev=c9b8f0843598

I have kept the single invocation to the pyhton script. Python is not very fast to start, so it is probably better to print all the information we need at once.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 534831 [details] [diff] [review]
OBJ_SUFFIX

Review of attachment 534831 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few nits.

::: build/autoconf/libstdcxx.py
@@ +1,1 @@
> +#!/usr/bin/python

Can you put a comment block up top describing the purpose of this file? Also, a few comments in function bodies would be nice for the next person that stumbles upon this file and tries to figure out what it's doing.

@@ +20,5 @@
> +
> +def cmp_ver(a, b):
> +    parts_a = split_ver(a)
> +    parts_b = split_ver(b)
> +    for (i, j) in zip(parts_a, parts_b):

nit: You don't really need the temporary vars here.

@@ +33,5 @@
> +def find_version(e):
> +    args = e.split()
> +    args +=  ['-shared', '-Wl,-t']
> +    p = subprocess.Popen(args, stderr=subprocess.STDOUT, stdout=subprocess.PIPE)
> +    canditates = [x for x in p.stdout.readlines() if 'libstdc++.so' in x]

typo: "candidates"

Also you can drop the .readlines() and just iterate over p.stdout.

@@ +39,5 @@
> +    libstdcxx = parse_ld_line(canditates[-1])
> +
> +    p = subprocess.Popen(['readelf', '-V', libstdcxx], stdout=subprocess.PIPE)
> +    versions = [parse_readelf_line(x)
> +                for x in p.stdout.readlines() if 'Name: GLIBCXX' in x]

Drop the .readlines() here too.

@@ +43,5 @@
> +                for x in p.stdout.readlines() if 'Name: GLIBCXX' in x]
> +    last_version = sorted(versions, cmp = cmp_ver)[-1]
> +    return encode_ver(last_version)
> +
> +cxx_env = os.environ.get('CXX', 'c++')

I think you should just error if $CXX isn't set. It should always be set in the context you're running it in.

@@ +46,5 @@
> +
> +cxx_env = os.environ.get('CXX', 'c++')
> +print 'MOZ_LIBSTDCXX_TARGET_VERSION=%s' % find_version(cxx_env)
> +host_cxx_env = os.environ.get('HOST_CXX', cxx_env)
> +print 'MOZ_LIBSTDCXX_HOST_VERSION=%s' % find_version(host_cxx_env)

Can you put these top-level lines in a block like:
if __name__ == '__main__':
    ...

We typically write all our scripts this way so they're usable as imported modules if we want to use them that way in the future.

Comment 23

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: updated patch with comments

Comment 24

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Had missed the comment about __main__. Fixed

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 537257 [details] [diff] [review]
Had missed the comment about __main__. Fixed

Review of attachment 537257 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 26

[(no longer active)]

I've already landed this on mozilla-inbound.

Comment 27

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/685f5ae6e7de