Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c++11 thread_local objects don't get their destructors called on devices lower than android M #687

Closed
martinbonaciugome opened this issue May 4, 2018 · 8 comments
Assignees
Milestone

Comments

@martinbonaciugome
Copy link

Description

c++11 thread_local objects don't get their destructors called on devices lower than android M.

put this code in a thread and destroy it.

    struct aa
    {
      aa()
      {
        std::cout << "ctr " << std::uintptr_t(this);
      }
      ~aa()
      {
        std::cout << "dtr " << std::uintptr_t(this);
      }
    };
    static thread_local aa a{};

on api 23 the destructor gets called, on api 22 it does not.

Environment Details

  • NDK Version: 16b and 17-beta2
  • Build sytem: cmake
  • Host OS: Windows
  • Compiler: Clang
  • ABI:
  • STL: c++_static and c++_shared
  • NDK API level:
  • Device API level: 22
@DanAlbert DanAlbert self-assigned this May 4, 2018
@DanAlbert DanAlbert added this to the r18 milestone May 4, 2018
@rprichard rprichard assigned rprichard and unassigned DanAlbert May 5, 2018
@rprichard
Copy link
Collaborator

It looks like the libcxxabi __cxa_thread_atexit fallback suffers from the same issue recorded internally as http://b/78022094:

  • It's using pthread_key_create to register a per-thread call to run_dtors.
  • run_dtors accesses the __thread DtorList* dtors variable allocated by emutls, which deallocates storage using another pthread key destructor.

If the fallback destructor is called after the emutls destructor, then it will use deallocated storage.

The workaround for the internal issue (delay the emutls deallocation a few rounds) probably fixes this issue, too.

Android M adds __cxa_thread_atexit_impl, which ensures that C++ destructors are called before pthread key destructors. [Link] [Link]

@martinbonaciugome
Copy link
Author

Is this something a new ndk can fix, or are we totally hooped on older api's?

@enh
Copy link
Contributor

enh commented May 7, 2018

(the details of the problems with the various thread local choices seem like they should be added to the docs, given the number of folks hitting these now...)

@rprichard
Copy link
Collaborator

Currently, I think this bug can be fixed in libgcc (and/or compiler-rt, if we upgrade to it), which is packaged with the NDK and linked into app shared libs. You'd need a new NDK, and then apps would work correctly on pre-M platforms.

@martinbonaciugome
Copy link
Author

that's good news, thanks

@rprichard
Copy link
Collaborator

rprichard commented May 9, 2018

gnustl (which is deprecated) can fail on any Android version because, unlike libc++, its __cxa_thread_atexit fallback implementation doesn't defer to __cxa_thread_atexit_impl on newer platforms.

Like Android gnustl, mingw-w64 uses both emutls and libsupc++'s __cxa_thread_atexit, so it has the same bug. Apparently there was a mingw-w64 bug filed about this issue two weeks ago: https://sourceforge.net/p/mingw-w64/bugs/727/. Here's a test case that fails on all three configs (Android gnustl, Android libc++, mingw-w64): https://gist.github.com/rprichard/d0fad2161a3e2b7474b561d040ec5b40

Edit: s/__cxa_thread_atexit/__cxa_thread_atexit_impl/.

@rprichard
Copy link
Collaborator

@rprichard
Copy link
Collaborator

The NDK's libgcc.a prebuilts were updated in https://android-review.googlesource.com/c/platform/prebuilts/ndk/+/708782.

mehulagg pushed a commit to mehulagg/superproject that referenced this issue Dec 21, 2019
* Update ndk from branch 'master'
  to 3a21fd712122f16879ce937f0041c7b34a966c17
  - Merge "Test emutls pthread key deletion"
  - Test emutls pthread key deletion
    
    The key should be deleted when an solib is dlclose'd, but __thread
    variables should continue working during cleanup functions.
    
    Bug: b/80453944
    Test: manual
    Change-Id: Ie5719f6228911d4e9f8f58b86f994765a713691f
    

* Update prebuilts/ndk from branch 'dev'
  to f0908f8e2856919868742937450d197b08d37073
  - Merge "Update prebuilt GCC to build 4886780." into dev
  - Update prebuilt GCC to build 4886780.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: android/ndk#505
    Bug: http://b/80453944
    Change-Id: I1bd7fe862704a10f24806de23bc67ec3f15e4cb4
    
  - Merge "Update prebuilt GCC to build 4856068." into dev
  - Update prebuilt GCC to build 4856068.
    
    Update libgcc.a with a change to defer per-thread emutls cleanup by one
    round.
    
    Bug: android/ndk#687
    Bug: b/78022094
    Test: ./checkbuild.py && ./run_tests.py --filter=emutls-dealloc
    Change-Id: I413b4bd926472bb50865d4b91431302f803a49e6
    
  - Merge "Update NDK platform prebuilts to build 4828202." into dev
  - Update NDK platform prebuilts to build 4828202.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: None
    Change-Id: Ib963ec9d8afe1ff84dc9586eb774aad6c9d1236b
    
  - Merge "Update NDK platform prebuilts to build 4811751." into dev
  - Update NDK platform prebuilts to build 4811751.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: None
    Change-Id: If2aaaf6fe05ea63c8e340908f8f00d218219ddfb
    
  - Merge "Update NDK platform prebuilts to build 4794421." into dev
  - Update NDK platform prebuilts to build 4794421.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: android/ndk#702
    Change-Id: I84362b1b3f1bec6236866cdae617a811abfd5f52
    
  - Merge "Update prebuilt Clang to build r328903." into dev
  - Merge "Adapt to the new Clang naming scheme." into dev
  - Adapt to the new Clang naming scheme.
    
    Clang prebuilts are now named as clang-r${REVISION}${PATCH} rather
    than as clang-${BUILD_NUMBER}.
    
    Test: ./symlink-clang.py
    Bug: None
    Change-Id: I758feecb05ad568560333f63d3e0e9cdc25f9bcd
    
  - Update prebuilt Clang to build r328903.
    
    Change-Id: I07c5832446733f09aa48ff8a0c13d89516e69370
    
  - Merge "Update NDK platform prebuilts to build 4753280." into dev
  - Update NDK platform prebuilts to build 4753280.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: None
    Change-Id: Ic062d17cf3a7de3c82ea1d9a875c6cb3dc141b23
    
  - Merge "Update prebuilt binutils to build 4724899." into dev
  - Merge "Don't fetch the repo.prop for platform prebuilts." into dev
  - Don't fetch the repo.prop for platform prebuilts.
    
    Not necessary.
    
    Test: ./update_platform.py ...
    Bug: None
    Change-Id: I079c65caedaf5c1a3f270af721133051aee00c0b
    
  - Update prebuilt binutils to build 4724899.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: None
    Change-Id: I951f53a8bfc258eb4501df8d84e255bd1e1cc1f1
    
  - Merge "Update prebuilt Clang to build 4691093." into dev
  - Update prebuilt Clang to build 4691093.
    
    Change-Id: Ibe8c95936a0bbb46a172f8efcd4a00386dfa5e8c
    
  - Merge "Update Validation layer binaries" into dev
  - Merge "Update prebuilt Clang to build 4639204." into dev
  - Update Validation layer binaries
    
    Build: 4634174
    
    https://android-build.googleplex.com/builds/branch-dashboard/aosp-master-ndk-vulkan-validation-layers?build_id=4634174
    
    Change-Id: I55d4ca67aa390d288c6c0f04bbafb21dee31aab8
    
  - Merge "Update NDK platform prebuilts to build 4634113." into dev
  - Update prebuilt Clang to build 4639204.
    
    Change-Id: I456b400bc534de579078665a48ae5235869fb3f3
    
  - Update NDK platform prebuilts to build 4634113.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: None
    Change-Id: I9e7aa333073c61ba2724a6cd39f7b7e30ff2fef6
    
  - Merge "Update prebuilt Clang to build 4630689." into dev
  - Merge "Update NDK platform prebuilts to build 4634436." into dev
  - Update NDK platform prebuilts to build 4634436.
    
    Test: ndk/checkbuild.py && ndk/run_tests.py
    Bug: None
    Change-Id: Idd13dfe5dc9b928b81b48a0e72dc8ae669739275
    
  - Update prebuilt Clang to build 4630689.
    
    Change-Id: I852722d2bdfb4aeaa42fe943677d7adbf679d61b
Talustus pushed a commit to Ubuntu-Touch-GalaxyS/android_toolchain_gcc that referenced this issue Jan 23, 2021
With Android/Bionic, delay deallocation to round 2 of 4. It must run after
C++ thread_local destructors have been called, but before the final 2
rounds, because emutls calls free, and jemalloc then needs another 2
rounds to free its thread-specific data.

Bug: android/ndk#687
Bug: b/78022094
Test: manual
Test: ./run_tests.py --rebuild --filter emutls-dealloc
Change-Id: I01bd634d97b7d22161b5cc8ca71b3cb94064a03e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants