search

BinomialLLC / basis_universal

Basis Universal GPU Texture Codec
Apache License 2.0
2.73k stars 267 forks source link

Should interval_timer initialize globals via C++ static ctors? #317

Closed zeux closed 2 years ago

zeux commented 2 years ago

Right now interval_timer uses lazy initialization.

This is fine by itself, but when Basis is used from multiple threads, it results in thread sanitizer issues, see report below. By itself it's probably benign - the threads would end up initializing more or less the same data, and on x64 at least the order of reads/writes is probably going to work out. However, that makes it more difficult to validate that other issues don't occur.

Let me know if you'd accept a PR fixing this by moving interval_timer field initialization to C++ static construction.

WARNING: ThreadSanitizer: data race (pid=551485)
  Read of size 8 at 0x55e2d19bb5f0 by thread T9:
    #0 basisu::interval_timer::interval_timer() basis_universal/encoder/basisu_enc.cpp:260 (gltfpack+0x4ca57)
    #1 basisu::basis_compressor::generate_mipmaps(basisu::image const&, basisu::vector<basisu::image>&, bool) basis_universal/encoder/basisu_comp.cpp:383 (gltfpack+0x4185c)
    #2 basisu::basis_compressor::read_source_images() basis_universal/encoder/basisu_comp.cpp:703 (gltfpack+0x42f66)
    #3 basisu::basis_compressor::process() basis_universal/encoder/basisu_comp.cpp:222 (gltfpack+0x40924)
    #4 encodeInternal gltf/basisenc.cpp:119 (gltfpack+0xdc14)
    #5 encodeImage gltf/basisenc.cpp:143 (gltfpack+0xe074)
    #6 operator() gltf/basisenc.cpp:171 (gltfpack+0xe270)
    #7 __invoke_impl<void, encodeImages(std::string*, const cgltf_data*, const std::vector<ImageInfo>&, char const*, const Settings&)::<lambda()>&> /usr/include/c++/11/bits/invoke.h:61 (gltfpack+0xeeb1)
    #8 __invoke_r<void, encodeImages(std::string*, const cgltf_data*, const std::vector<ImageInfo>&, char const*, const Settings&)::<lambda()>&> /usr/include/c++/11/bits/invoke.h:154 (gltfpack+0xecea)
    #9 _M_invoke /usr/include/c++/11/bits/std_function.h:290 (gltfpack+0xe9c3)
    #10 std::function<void ()>::operator()() const <null> (gltfpack+0x292c70)
    #11 basisu::job_pool::job_run(basisu::job_pool::item&, std::unique_lock<std::mutex>&) basis_universal/encoder/basisu_enc.cpp:1595 (gltfpack+0x53a3a)
    #12 basisu::job_pool::job_thread(unsigned int) basis_universal/encoder/basisu_enc.cpp:1627 (gltfpack+0x53c76)
    #13 operator() basis_universal/encoder/basisu_enc.cpp:1517 (gltfpack+0x53248)
    #14 __invoke_impl<void, basisu::job_pool::job_pool(uint32_t)::<lambda()> > /usr/include/c++/11/bits/invoke.h:61 (gltfpack+0x266874)
    #15 __invoke<basisu::job_pool::job_pool(uint32_t)::<lambda()> > /usr/include/c++/11/bits/invoke.h:96 (gltfpack+0x2667eb)
    #16 _M_invoke<0> /usr/include/c++/11/bits/std_thread.h:253 (gltfpack+0x26674c)
    #17 operator() /usr/include/c++/11/bits/std_thread.h:260 (gltfpack+0x2666f2)
    #18 _M_run /usr/include/c++/11/bits/std_thread.h:211 (gltfpack+0x2666a8)
    #19 <null> <null> (libstdc++.so.6+0xdc2c2)

  Previous write of size 8 at 0x55e2d19bb5f0 by main thread:
    #0 basisu::interval_timer::init() basis_universal/encoder/basisu_enc.cpp:297 (gltfpack+0x4cdd0)
    #1 basisu::interval_timer::interval_timer() basis_universal/encoder/basisu_enc.cpp:261 (gltfpack+0x4ca78)
    #2 basisu::basis_compressor::generate_mipmaps(basisu::image const&, basisu::vector<basisu::image>&, bool) basis_universal/encoder/basisu_comp.cpp:383 (gltfpack+0x4185c)
    #3 basisu::basis_compressor::read_source_images() basis_universal/encoder/basisu_comp.cpp:703 (gltfpack+0x42f66)
    #4 basisu::basis_compressor::process() basis_universal/encoder/basisu_comp.cpp:222 (gltfpack+0x40924)
    #5 encodeInternal gltf/basisenc.cpp:119 (gltfpack+0xdc14)
    #6 encodeImage gltf/basisenc.cpp:143 (gltfpack+0xe074)
    #7 operator() gltf/basisenc.cpp:171 (gltfpack+0xe270)
    #8 __invoke_impl<void, encodeImages(std::string*, const cgltf_data*, const std::vector<ImageInfo>&, char const*, const Settings&)::<lambda()>&> /usr/include/c++/11/bits/invoke.h:61 (gltfpack+0xeeb1)
    #9 __invoke_r<void, encodeImages(std::string*, const cgltf_data*, const std::vector<ImageInfo>&, char const*, const Settings&)::<lambda()>&> /usr/include/c++/11/bits/invoke.h:154 (gltfpack+0xecea)
    #10 _M_invoke /usr/include/c++/11/bits/std_function.h:290 (gltfpack+0xe9c3)
    #11 std::function<void ()>::operator()() const <null> (gltfpack+0x292c70)
    #12 basisu::job_pool::job_run(basisu::job_pool::item&, std::unique_lock<std::mutex>&) basis_universal/encoder/basisu_enc.cpp:1595 (gltfpack+0x53a3a)
    #13 basisu::job_pool::wait_for_all(std::atomic<unsigned int>*) basis_universal/encoder/basisu_enc.cpp:1566 (gltfpack+0x5384c)
    #14 encodeImages(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, cgltf_data const*, std::vector<ImageInfo, std::allocator<ImageInfo> > const&, char const*, Settings const&) gltf/basisenc.cpp:178 (gltfpack+0xe7b1)
    #15 process gltf/gltfpack.cpp:463 (gltfpack+0x2e3b90)
    #16 gltfpack(char const*, char const*, char const*, Settings) gltf/gltfpack.cpp:992 (gltfpack+0x2e7700)
    #17 main gltf/gltfpack.cpp:1499 (gltfpack+0x2eb2e6)

  Location is global 'basisu::interval_timer::g_timer_freq' of size 8 at 0x55e2d19bb5f0 (gltfpack+0x0000004a65f0)
zeux commented 2 years ago

Alternative easy solution is to initialize this in basisu_encoder_init.

richgel999 commented 2 years ago

This is fine by itself, but when Basis is used from multiple threads, it results in thread sanitizer issues, see report below. Alternative easy solution is to initialize this in basisu_encoder_init.

This is how the code was designed to work. Until you initialize the encoder via this global function you shouldn't use the library.

zeux commented 2 years ago

Ok great - submitted https://github.com/BinomialLLC/basis_universal/pull/319