Last Comment Bug 657653 - stdc++compat.cpp checks for compiler version but needs libstdc++ version
: stdc++compat.cpp checks for compiler version but needs libstdc++ version
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla7
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on: 664340
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-17 08:59 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-06-21 22:12 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (7.17 KB, patch)
2011-05-19 12:33 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
fancier version that handles host and target (9.05 KB, patch)
2011-05-19 16:27 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
correct quotes (8.64 KB, patch)
2011-05-19 21:31 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mh+mozilla: review-
Details | Diff | Splinter Review
updated patch (8.63 KB, patch)
2011-05-20 06:52 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
updated patch (8.63 KB, patch)
2011-05-20 07:58 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
version in python (8.17 KB, patch)
2011-05-20 12:05 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Fix to work with ccache, python version. (8.18 KB, patch)
2011-05-20 13:52 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mh+mozilla: review-
Details | Diff | Splinter Review
handle both host and target (8.90 KB, patch)
2011-05-23 14:42 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
also update js/src/config/rules.mk (9.71 KB, patch)
2011-05-23 16:45 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mh+mozilla: review+
Details | Diff | Splinter Review
OBJ_SUFFIX (9.76 KB, patch)
2011-05-24 10:45 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
ted: review+
Details | Diff | Splinter Review
updated patch with comments (10.64 KB, patch)
2011-06-03 15:27 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Had missed the comment about __main__. Fixed (10.68 KB, patch)
2011-06-03 15:56 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
ted: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-17 08:59:53 PDT
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] 2011-05-17 09:08:58 PDT
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) 2011-05-19 10:31:49 PDT
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] 2011-05-19 10:47:44 PDT
:(
Comment 4 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-19 12:33:10 PDT
Created attachment 533755 [details] [diff] [review]
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) 2011-05-19 12:57:26 PDT
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) 2011-05-19 16:27:33 PDT
Created attachment 533840 [details] [diff] [review]
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) 2011-05-19 21:31:40 PDT
Created attachment 533880 [details] [diff] [review]
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] 2011-05-20 00:43:11 PDT
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) 2011-05-20 06:52:32 PDT
Created attachment 533955 [details] [diff] [review]
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) 2011-05-20 06:59:36 PDT
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] 2011-05-20 07:49:51 PDT
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] 2011-05-20 07:52:34 PDT
(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) 2011-05-20 07:58:24 PDT
Created attachment 533973 [details] [diff] [review]
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) 2011-05-20 12:05:47 PDT
Created attachment 534054 [details] [diff] [review]
version in python
Comment 15 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-20 13:52:33 PDT
Created attachment 534095 [details] [diff] [review]
Fix to work with ccache, python version.
Comment 16 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-20 15:58:55 PDT
The builds are slowly showing up on

http://tbpl.mozilla.org/?tree=Try&rev=248d2290dc13
Comment 17 Mike Hommey [:glandium] 2011-05-23 09:15:11 PDT
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) 2011-05-23 14:42:36 PDT
Created attachment 534580 [details] [diff] [review]
handle both host and target

http://tbpl.mozilla.org/?tree=Try&rev=d668b92740c7
Comment 19 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-23 16:45:45 PDT
Created attachment 534622 [details] [diff] [review]
also update js/src/config/rules.mk
Comment 20 Mike Hommey [:glandium] 2011-05-24 08:35:54 PDT
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) 2011-05-24 10:45:16 PDT
Created attachment 534831 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2011-06-03 11:07:05 PDT
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) 2011-06-03 15:27:02 PDT
Created attachment 537250 [details] [diff] [review]
updated patch with comments
Comment 24 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-06-03 15:56:09 PDT
Created attachment 537257 [details] [diff] [review]
Had missed the comment about __main__. Fixed
Comment 25 Ted Mielczarek [:ted.mielczarek] 2011-06-08 10:46:46 PDT
Comment on attachment 537257 [details] [diff] [review]
Had missed the comment about __main__. Fixed

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

Thanks!
Comment 26 :Ehsan Akhgari (away Aug 1-5) 2011-06-09 15:02:27 PDT
I've already landed this on mozilla-inbound.
Comment 27 :Ehsan Akhgari (away Aug 1-5) 2011-06-09 15:36:35 PDT
http://hg.mozilla.org/mozilla-central/rev/685f5ae6e7de

Note You need to log in before you can comment on or make changes to this bug.