1389422 - [gcc6] check_stdcxx | We do not want these libstdc++ symbol versions to be used: 2 symbols@GLIBCXX_3.4.22

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Mike Hommey [:glandium]]

INFO -  TEST-UNEXPECTED-FAIL | check_stdcxx | We do not want these libstdc++ symbol versions to be used:
INFO -    _ZNSt6thread15_M_start_threadESt10unique_ptrINS_6_StateESt14default_deleteIS1_EEPFvvE@GLIBCXX_3.4.22
INFO -    _ZNSt6thread6_StateD2Ev@GLIBCXX_3.4.22
INFO -  /home/worker/workspace/build/src/config/rules.mk:717: recipe for target 'libxul.so' failed

They both come from the use of std::thread in https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/media/libcubeb/src/cubeb_log.cpp#76

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1389422 - Avoid @GLIBCXX_3.4.22 symbols from the use of std::thread when building with GCC 6.

That the wrapper implementation works has been verified by creating a
dummy program such as:

  $ cat test.cc
  #include <thread>

  int main() {
    std::thread([]() {
      printf("foo\n");
    }).join();
    return 0;
  }

And compiling it with and without the hack:

  $ g++ -fno-rtti -o test test.cc -lpthread
  $ objdump -TC test | grep UND.*GLIBCXX_3.4.22
  0000000000000000      DF *UND*	0000000000000000  GLIBCXX_3.4.22 std::thread::_State::~_State()
  0000000000000000      DF *UND*	0000000000000000  GLIBCXX_3.4.22 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)())

  $ ./test
  foo

  $ g++ -fno-rtti -o test test.cc $objdir/build/unix/stdc++compat/stdc++compat.o -lpthread
  $ objdump -TC test | grep UND.*GLIBCXX_3.4.22
  $ ./test
  foo

Review commit: https://reviewboard.mozilla.org/r/167448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167448/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8896178 [details]
Bug 1389422 - Avoid @GLIBCXX_3.4.22 symbols from the use of std::thread when building with GCC 6.

https://reviewboard.mozilla.org/r/167448/#review172754

This is fine in principle, but some questions below.  I am particularly curious about how `thread::_M_start_thread(shared_ptr<_Impl_base>, void(*)())` works.

::: build/unix/stdc++compat/stdc++compat.cpp:121
(Diff revision 1)
> +   * There is an intermediate ABI at version 3.4.21, with
> +   * thread::_M_start_thread(shared_ptr<_Impl_base>, void(*)()).

This is convenient!

I am missing something here, though; this function is at 3.4.21, which the documentation above states that is GCC 5.0...but this symbol *is* present in my GCC 4.9 `libstdc++.so`.  How does that work?

And then how do we get that 3.4.21 symbol in our binaries in the first place, since we're checking for compat with 3.4.16?  I can't see that we're linking stdc++ statically...

::: build/unix/stdc++compat/stdc++compat.cpp:126
(Diff revision 1)
> +   * There is an intermediate ABI at version 3.4.21, with
> +   * thread::_M_start_thread(shared_ptr<_Impl_base>, void(*)()).
> +   * The void(*)() parameter is only there to keep a reference to pthread_create
> +   * on the caller side, and is unused in the implementation
> +   * We're creating an entry point for the new ABI, and make that call the old ABI,
> +   * so we need to a _Impl_base-derived class wrapping that _State. */

Nit: "...we need to have a `_Impl_base`-derived" or "...we need to create a `_Impl_base`-derived" or something similar?

::: build/unix/stdc++compat/stdc++compat.cpp:134
(Diff revision 1)
> +
> +    StateWrapper(unique_ptr<thread::_State> aState)
> +    : mState(std::move(aState))
> +    { }
> +
> +    void _M_run()

Maybe tag this `override`, to be clear that we're providing a virtual function here?

::: build/unix/stdc++compat/stdc++compat.cpp:144
(Diff revision 1)
> +
> +  __attribute__((weak))
> +  void thread::_M_start_thread(unique_ptr<_State> aState, void (*)())
> +  {
> +    auto impl = std::make_shared<StateWrapper>(std::move(aState));
> +    _M_start_thread(impl);

`_M_start_thread(std::move(impl))` for micro-efficiency?

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8896178 [details]
Bug 1389422 - Avoid @GLIBCXX_3.4.22 symbols from the use of std::thread when building with GCC 6.

https://reviewboard.mozilla.org/r/167448/#review172754

> This is convenient!
> 
> I am missing something here, though; this function is at 3.4.21, which the documentation above states that is GCC 5.0...but this symbol *is* present in my GCC 4.9 `libstdc++.so`.  How does that work?
> 
> And then how do we get that 3.4.21 symbol in our binaries in the first place, since we're checking for compat with 3.4.16?  I can't see that we're linking stdc++ statically...

Oh, I guess we're currently saying our minimum supported GCC version is 4.9?  We could delete some of the code in this file, then?

Comment 4

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> ::: build/unix/stdc++compat/stdc++compat.cpp:121
> (Diff revision 1)
> > +   * There is an intermediate ABI at version 3.4.21, with
> > +   * thread::_M_start_thread(shared_ptr<_Impl_base>, void(*)()).
> 
> This is convenient!
> 
> I am missing something here, though; this function is at 3.4.21, which the
> documentation above states that is GCC 5.0...but this symbol *is* present in
> my GCC 4.9 `libstdc++.so`.  How does that work?

The symbol in gcc 4.9 libstdc++.so is for thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>) (without the extra void (*)() argument.

> And then how do we get that 3.4.21 symbol in our binaries in the first
> place, since we're checking for compat with 3.4.16?  I can't see that we're
> linking stdc++ statically...

We get a 3.4.2*2* symbol *reference* in our binaries by means of using GCC 6.
 
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Oh, I guess we're currently saying our minimum supported GCC version is 4.9?
> We could delete some of the code in this file, then?

Our minimum supported libstdc++ version is 4.6.1. That is, we're maintaining binary compatibility with a version that is older than the minimum version of GCC we support. Thus we need hacks for versions above 3.4.16.

Comment 5

[Nathan Froyd [:froydnj]]

Ah, I see, I was missing that there were two overloads with shared_ptr<> as their first argument.  OK, this makes sense.  So we have to implement the 3.4.21 symbol, and we'll use the non-void (*)() overload (which is 3.4.11) to do that.

Comment 6

[Mike Hommey [:glandium]]

We have to implement the 3.4.22 symbol. But I'm also going to add the 3.4.21 one too, there's no reason not to. It might help make things clearer too.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8896178 [details]
Bug 1389422 - Avoid @GLIBCXX_3.4.22 symbols from the use of std::thread when building with GCC 6.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167448/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f92327a6d1f7
Avoid @GLIBCXX_3.4.22 symbols from the use of std::thread when building with GCC 6. r=froydnj

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f92327a6d1f7