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

RESOLVED FIXED in mozilla7

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

Trunk
mozilla7
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

10.68 KB, patch
ted
: review+
Details | Diff | Splinter Review
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)
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
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
:(
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?
Attachment #533755 - Flags: review?(mh+mozilla)
The clang built finished fine. The try build is at

http://tbpl.mozilla.org/?tree=Try&rev=3d31d0f2c230
Created attachment 533840 [details] [diff] [review]
fancier version that handles host and target

Being tested at
http://tbpl.mozilla.org/?tree=Try&rev=64c0294ab016
Attachment #533755 - Attachment is obsolete: true
Attachment #533755 - Flags: review?(mh+mozilla)
Attachment #533840 - Flags: review?(mh+mozilla)
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
Attachment #533840 - Attachment is obsolete: true
Attachment #533840 - Flags: review?(mh+mozilla)
Attachment #533880 - Flags: review?(mh+mozilla)
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)
Attachment #533880 - Flags: review?(mh+mozilla) → review-
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
Attachment #533880 - Attachment is obsolete: true
Attachment #533955 - Flags: review?(mh+mozilla)
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 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.
(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.
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)
Attachment #533955 - Attachment is obsolete: true
Attachment #533955 - Flags: review?(mh+mozilla)
Attachment #533973 - Flags: review?(mh+mozilla)
Created attachment 534054 [details] [diff] [review]
version in python
Attachment #533973 - Attachment is obsolete: true
Attachment #533973 - Flags: review?(mh+mozilla)
Attachment #534054 - Flags: review?(mh+mozilla)
Created attachment 534095 [details] [diff] [review]
Fix to work with ccache, python version.
Attachment #534054 - Attachment is obsolete: true
Attachment #534054 - Flags: review?(mh+mozilla)
Attachment #534095 - Flags: review?(mh+mozilla)
The builds are slowly showing up on

http://tbpl.mozilla.org/?tree=Try&rev=248d2290dc13
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)
Attachment #534095 - Flags: review?(mh+mozilla) → review-
Created attachment 534580 [details] [diff] [review]
handle both host and target

http://tbpl.mozilla.org/?tree=Try&rev=d668b92740c7
Attachment #534095 - Attachment is obsolete: true
Attachment #534580 - Flags: review?(mh+mozilla)
Created attachment 534622 [details] [diff] [review]
also update js/src/config/rules.mk
Attachment #534580 - Attachment is obsolete: true
Attachment #534580 - Flags: review?(mh+mozilla)
Attachment #534622 - Flags: review?(mh+mozilla)
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.
Attachment #534622 - Flags: review?(ted.mielczarek)
Attachment #534622 - Flags: review?(mh+mozilla)
Attachment #534622 - Flags: review+
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.
Attachment #534622 - Attachment is obsolete: true
Attachment #534622 - Flags: review?(ted.mielczarek)
Attachment #534831 - Flags: review?(ted.mielczarek)
Assignee: nobody → respindola
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.
Attachment #534831 - Flags: review?(ted.mielczarek) → review+
Created attachment 537250 [details] [diff] [review]
updated patch with comments
Attachment #534831 - Attachment is obsolete: true
Attachment #537250 - Flags: review?(ted.mielczarek)
Created attachment 537257 [details] [diff] [review]
Had missed the comment about __main__. Fixed
Attachment #537250 - Attachment is obsolete: true
Attachment #537250 - Flags: review?(ted.mielczarek)
Attachment #537257 - Flags: review?(ted.mielczarek)
Comment on attachment 537257 [details] [diff] [review]
Had missed the comment about __main__. Fixed

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

Thanks!
Attachment #537257 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed
I've already landed this on mozilla-inbound.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/685f5ae6e7de
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Depends on: 664340
You need to log in before you can comment on or make changes to this bug.