From 2dda038306fd2953c2bbd01a15795994fbc32af2 Mon Sep 17 00:00:00 2001 From: Andrey Semashev Date: Mon, 14 Jun 2021 22:09:15 +0300 Subject: [PATCH] Reworked function pointers use and definitions. Instead of using atomic<> to access global function pointers, use raw pointers and atomic_ref to access them safely in multi-threaded builds. This allows to ensure constant initialization of the function pointers, even in C++03. This also solves the problem of undefined dynamic initialization order that we previously tried to solve with the init_priority attribute. The attribute turns out to not work if the pointers were raw pointers (in single-threaded builds). It is also not supported by Intel Compiler and possibly other, which required us to avoid using the function pointer for fill_random. The resulting code should be simpler and more portable. In order to check for C++20 std::atomic_ref availability, the older check for header was replaced with a check for std::atomic_ref. If not available, we're using Boost.Atomic, as before. --- CMakeLists.txt | 10 ++---- build/Jamfile.v2 | 11 ++++--- config/Jamfile.v2 | 2 ++ config/has_cxx20_atomic_ref.cpp | 19 ++++++++++++ src/{atomic.hpp => atomic_ref.hpp} | 14 ++++----- src/atomic_tools.hpp | 50 +++++++++++++++++++++++++----- src/operations.cpp | 24 +++++++------- src/private_config.hpp | 8 ++--- src/unique_path.cpp | 20 +++--------- 9 files changed, 98 insertions(+), 60 deletions(-) create mode 100644 config/has_cxx20_atomic_ref.cpp rename src/{atomic.hpp => atomic_ref.hpp} (61%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 76e1de5..227aa7e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,6 +22,7 @@ set(BOOST_FILESYSTEM_DISABLE_BCRYPT OFF CACHE BOOL "Disable usage of BCrypt API set(CMAKE_REQUIRED_INCLUDES "${CMAKE_CURRENT_SOURCE_DIR}/src" "${CMAKE_CURRENT_SOURCE_DIR}/../..") +check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/config/has_cxx20_atomic_ref.cpp>" BOOST_FILESYSTEM_HAS_CXX20_ATOMIC_REF) check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/config/has_stat_st_blksize.cpp>" BOOST_FILESYSTEM_HAS_STAT_ST_BLKSIZE) check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/config/has_stat_st_mtim.cpp>" BOOST_FILESYSTEM_HAS_STAT_ST_MTIM) check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/config/has_stat_st_mtimensec.cpp>" BOOST_FILESYSTEM_HAS_STAT_ST_MTIMENSEC) @@ -43,12 +44,6 @@ endif() unset(CMAKE_REQUIRED_INCLUDES) -set(CMAKE_REQUIRED_INCLUDES "${CMAKE_CURRENT_SOURCE_DIR}/../..") -set(CMAKE_REQUIRED_DEFINITIONS "-DTEST_BOOST_NO_CXX11_HDR_ATOMIC") -check_cxx_source_compiles("#include <${CMAKE_CURRENT_SOURCE_DIR}/../config/checks/test_case.cpp>" BOOST_FILESYSTEM_HAS_CXX11_HDR_ATOMIC) -unset(CMAKE_REQUIRED_DEFINITIONS) -unset(CMAKE_REQUIRED_INCLUDES) - set(BOOST_FILESYSTEM_SOURCES src/codecvt_error_category.cpp src/exception.cpp @@ -153,7 +148,8 @@ target_link_libraries(boost_filesystem Boost::predef ) -if(NOT BOOST_FILESYSTEM_HAS_CXX11_HDR_ATOMIC) +if(NOT BOOST_FILESYSTEM_HAS_CXX20_ATOMIC_REF) + target_compile_definitions(boost_filesystem PRIVATE BOOST_FILESYSTEM_NO_CXX20_ATOMIC_REF) target_link_libraries(boost_filesystem PRIVATE Boost::atomic) endif() diff --git a/build/Jamfile.v2 b/build/Jamfile.v2 index 634e98b..6977041 100644 --- a/build/Jamfile.v2 +++ b/build/Jamfile.v2 @@ -79,16 +79,17 @@ rule check-statx ( properties * ) return $(result) ; } -# The rule checks if the standard header is supported -rule check-cxx11-hdr-atomic ( properties * ) +# The rule checks if std::atomic_ref is supported +rule check-cxx20-atomic-ref ( properties * ) { local result ; if ! single in $(properties) { - if ! [ configure.builds ../../config/checks//cxx11_hdr_atomic : $(properties) : "has header" ] + if ! [ configure.builds ../config//has_cxx20_atomic_ref : $(properties) : "has std::atomic_ref" ] { - result = /boost/atomic//boost_atomic ; + result = BOOST_FILESYSTEM_NO_CXX20_ATOMIC_REF ; + result += /boost/atomic//boost_atomic ; } } else @@ -112,7 +113,7 @@ project boost/filesystem [ check-target-builds ../config//has_stat_st_birthtimespec "has stat::st_birthtimespec" : BOOST_FILESYSTEM_HAS_STAT_ST_BIRTHTIMESPEC ] @check-statx @select-windows-crypto-api - @check-cxx11-hdr-atomic + @check-cxx20-atomic-ref windows:BOOST_USE_WINDOWS_H windows:WIN32_LEAN_AND_MEAN windows:NOMINMAX diff --git a/config/Jamfile.v2 b/config/Jamfile.v2 index fa671ca..fd0157c 100644 --- a/config/Jamfile.v2 +++ b/config/Jamfile.v2 @@ -5,6 +5,8 @@ # http://www.boost.org/LICENSE_1_0.txt) # +obj has_cxx20_atomic_ref : has_cxx20_atomic_ref.cpp : ../src ; +explicit has_cxx20_atomic_ref ; obj has_stat_st_blksize : has_stat_st_blksize.cpp : ../src ; explicit has_stat_st_blksize ; obj has_stat_st_mtim : has_stat_st_mtim.cpp : ../src ; diff --git a/config/has_cxx20_atomic_ref.cpp b/config/has_cxx20_atomic_ref.cpp new file mode 100644 index 0000000..1fb407f --- /dev/null +++ b/config/has_cxx20_atomic_ref.cpp @@ -0,0 +1,19 @@ +// Copyright 2021 Andrey Semashev + +// Distributed under the Boost Software License, Version 1.0. +// See http://www.boost.org/LICENSE_1_0.txt + +// See library home page at http://www.boost.org/libs/filesystem + +#include + +typedef void func_t(); + +int main() +{ + func_t* func = 0; + std::atomic_ref< func_t* > ref(func); + ref.load(std::memory_order_relaxed); + ref.store((func_t*)0, std::memory_order_relaxed); + return 0; +} diff --git a/src/atomic.hpp b/src/atomic_ref.hpp similarity index 61% rename from src/atomic.hpp rename to src/atomic_ref.hpp index adc0a5f..6e8208d 100644 --- a/src/atomic.hpp +++ b/src/atomic_ref.hpp @@ -9,24 +9,24 @@ //--------------------------------------------------------------------------------------// -#ifndef BOOST_FILESYSTEM_SRC_ATOMIC_HPP_ -#define BOOST_FILESYSTEM_SRC_ATOMIC_HPP_ +#ifndef BOOST_FILESYSTEM_SRC_ATOMIC_REF_HPP_ +#define BOOST_FILESYSTEM_SRC_ATOMIC_REF_HPP_ #include -#if !defined(BOOST_NO_CXX11_HDR_ATOMIC) +#if !defined(BOOST_FILESYSTEM_NO_CXX20_ATOMIC_REF) #include namespace atomic_ns = std; -#else // !defined(BOOST_NO_CXX11_HDR_ATOMIC) +#else // !defined(BOOST_FILESYSTEM_NO_CXX20_ATOMIC_REF) #include -#include +#include namespace atomic_ns = boost; -#endif // !defined(BOOST_NO_CXX11_HDR_ATOMIC) +#endif // !defined(BOOST_FILESYSTEM_NO_CXX20_ATOMIC_REF) -#endif // BOOST_FILESYSTEM_SRC_ATOMIC_HPP_ +#endif // BOOST_FILESYSTEM_SRC_ATOMIC_REF_HPP_ diff --git a/src/atomic_tools.hpp b/src/atomic_tools.hpp index 769f20e..a60e5d3 100644 --- a/src/atomic_tools.hpp +++ b/src/atomic_tools.hpp @@ -16,17 +16,53 @@ #if !defined(BOOST_FILESYSTEM_SINGLE_THREADED) -#include "atomic.hpp" +#include "atomic_ref.hpp" -#define BOOST_FILESYSTEM_ATOMIC_VAR(type, name, init) atomic_ns::atomic< type > name(init) -#define BOOST_FILESYSTEM_ATOMIC_LOAD_RELAXED(a) a.load(atomic_ns::memory_order_relaxed) -#define BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(a, x) a.store(x, atomic_ns::memory_order_relaxed) +namespace boost { +namespace filesystem { +namespace detail { + +//! Atomically loads the value +template< typename T > +BOOST_FORCEINLINE T atomic_load_relaxed(T& a) +{ + return atomic_ns::atomic_ref< T >(a).load(atomic_ns::memory_order_relaxed); +} + +//! Atomically stores the value +template< typename T > +BOOST_FORCEINLINE void atomic_store_relaxed(T& a, T val) +{ + atomic_ns::atomic_ref< T >(a).store(val, atomic_ns::memory_order_relaxed); +} + +} // namespace detail +} // namespace filesystem +} // namespace boost #else // !defined(BOOST_FILESYSTEM_SINGLE_THREADED) -#define BOOST_FILESYSTEM_ATOMIC_VAR(type, name, init) type name = init -#define BOOST_FILESYSTEM_ATOMIC_LOAD_RELAXED(a) a -#define BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(a, x) a = x +namespace boost { +namespace filesystem { +namespace detail { + +//! Atomically loads the value +template< typename T > +BOOST_FORCEINLINE T atomic_load_relaxed(T const& a) +{ + return a; +} + +//! Atomically stores the value +template< typename T > +BOOST_FORCEINLINE void atomic_store_relaxed(T& a, T val) +{ + a = val; +} + +} // namespace detail +} // namespace filesystem +} // namespace boost #endif // !defined(BOOST_FILESYSTEM_SINGLE_THREADED) diff --git a/src/operations.cpp b/src/operations.cpp index 7c17ce1..d97fc32 100644 --- a/src/operations.cpp +++ b/src/operations.cpp @@ -506,12 +506,11 @@ int statx_fstatat(int dirfd, const char* path, int flags, unsigned int mask, str typedef int statx_t(int dirfd, const char* path, int flags, unsigned int mask, struct ::statx* stx); //! Pointer to the actual implementation of the statx implementation -BOOST_FILESYSTEM_INIT_PRIORITY(BOOST_FILESYSTEM_FUNC_PTR_INIT_PRIORITY) -BOOST_FILESYSTEM_ATOMIC_VAR(statx_t*, statx_ptr, &statx_fstatat); +statx_t* statx_ptr = &statx_fstatat; inline int invoke_statx(int dirfd, const char* path, int flags, unsigned int mask, struct ::statx* stx) BOOST_NOEXCEPT { - return BOOST_FILESYSTEM_ATOMIC_LOAD_RELAXED(statx_ptr)(dirfd, path, flags, mask, stx); + return filesystem::detail::atomic_load_relaxed(statx_ptr)(dirfd, path, flags, mask, stx); } //! A wrapper for the statx syscall. Disable MSAN since at least on clang 10 it doesn't @@ -525,7 +524,7 @@ int statx_syscall(int dirfd, const char* path, int flags, unsigned int mask, str const int err = errno; if (BOOST_UNLIKELY(err == ENOSYS)) { - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(statx_ptr, &statx_fstatat); + filesystem::detail::atomic_store_relaxed(statx_ptr, &statx_fstatat); return statx_fstatat(dirfd, path, flags, mask, stx); } } @@ -545,7 +544,7 @@ inline void init_statx_impl(unsigned int major_ver, unsigned int minor_ver, unsi if (major_ver > 4u || (major_ver == 4u && minor_ver >= 11u)) stx = &statx_syscall; - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(statx_ptr, stx); + filesystem::detail::atomic_store_relaxed(statx_ptr, stx); #endif // !defined(BOOST_FILESYSTEM_HAS_STATX) && defined(BOOST_FILESYSTEM_HAS_STATX_SYSCALL) } @@ -748,8 +747,7 @@ int copy_file_data_read_write(int infile, int outfile, uintmax_t size, std::size typedef int copy_file_data_t(int infile, int outfile, uintmax_t size, std::size_t blksize); //! Pointer to the actual implementation of the copy_file_data implementation -BOOST_FILESYSTEM_INIT_PRIORITY(BOOST_FILESYSTEM_FUNC_PTR_INIT_PRIORITY) -BOOST_FILESYSTEM_ATOMIC_VAR(copy_file_data_t*, copy_file_data, ©_file_data_read_write); +copy_file_data_t* copy_file_data = ©_file_data_read_write; #if defined(BOOST_FILESYSTEM_USE_SENDFILE) @@ -783,7 +781,7 @@ int copy_file_data_sendfile(int infile, int outfile, uintmax_t size, std::size_t if (err == ENOSYS) { - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(copy_file_data, ©_file_data_read_write); + filesystem::detail::atomic_store_relaxed(copy_file_data, ©_file_data_read_write); goto fallback_to_read_write; } } @@ -851,10 +849,10 @@ int copy_file_data_copy_file_range(int infile, int outfile, uintmax_t size, std: if (err == ENOSYS) { #if defined(BOOST_FILESYSTEM_USE_SENDFILE) - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(copy_file_data, ©_file_data_sendfile); + filesystem::detail::atomic_store_relaxed(copy_file_data, ©_file_data_sendfile); goto fallback_to_sendfile; #else - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(copy_file_data, ©_file_data_read_write); + filesystem::detail::atomic_store_relaxed(copy_file_data, ©_file_data_read_write); goto fallback_to_read_write; #endif } @@ -934,7 +932,7 @@ inline void init_copy_file_data_impl(unsigned int major_ver, unsigned int minor_ cfd = &check_fs_type< ©_file_data_copy_file_range >; #endif - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(copy_file_data, cfd); + filesystem::detail::atomic_store_relaxed(copy_file_data, cfd); #endif // defined(BOOST_FILESYSTEM_USE_SENDFILE) || defined(BOOST_FILESYSTEM_USE_COPY_FILE_RANGE) } @@ -961,7 +959,7 @@ struct syscall_initializer } }; -BOOST_FILESYSTEM_INIT_PRIORITY(BOOST_FILESYSTEM_FUNC_PTR_INIT_INIT_PRIORITY) +BOOST_FILESYSTEM_INIT_PRIORITY(BOOST_FILESYSTEM_FUNC_PTR_INIT_PRIORITY) const syscall_initializer syscall_init; #endif // defined(linux) || defined(__linux) || defined(__linux__) @@ -1901,7 +1899,7 @@ bool copy_file(path const& from, path const& to, unsigned int options, error_cod } // Note: Use block size of the target file since it is most important for writing performance. - err = BOOST_FILESYSTEM_ATOMIC_LOAD_RELAXED(detail::copy_file_data)(infile.fd, outfile.fd, get_size(from_stat), get_blksize(to_stat)); + err = filesystem::detail::atomic_load_relaxed(filesystem::detail::copy_file_data)(infile.fd, outfile.fd, get_size(from_stat), get_blksize(to_stat)); if (BOOST_UNLIKELY(err != 0)) goto fail; // err already contains the error code diff --git a/src/private_config.hpp b/src/private_config.hpp index c86d21f..2dab5a6 100644 --- a/src/private_config.hpp +++ b/src/private_config.hpp @@ -29,13 +29,11 @@ #endif // According to https://gcc.gnu.org/bugzilla//show_bug.cgi?id=65115, -// the default C++ object initializers priority is 65535. We want to +// the default C++ object initializers priority is 65535. We would like to // initialize function pointers earlier than that (with lower priority values), -// and construct the function pointers initializer after that but preferably -// before the other global objects initializers. Other than this, +// before the other global objects initializers are run. Other than this, // these priority values are arbitrary. -#define BOOST_FILESYSTEM_FUNC_PTR_INIT_PRIORITY 32766 -#define BOOST_FILESYSTEM_FUNC_PTR_INIT_INIT_PRIORITY 32767 +#define BOOST_FILESYSTEM_FUNC_PTR_INIT_PRIORITY 32767 #if defined(__has_feature) && defined(__has_attribute) #if __has_feature(memory_sanitizer) && __has_attribute(no_sanitize) diff --git a/src/unique_path.cpp b/src/unique_path.cpp index ae5438d..d48f8ed 100644 --- a/src/unique_path.cpp +++ b/src/unique_path.cpp @@ -166,14 +166,8 @@ int fill_random_dev_random(void* buf, std::size_t len) typedef int fill_random_t(void* buf, std::size_t len); -#if defined(BOOST_FILESYSTEM_HAS_INIT_PRIORITY) //! Pointer to the implementation of fill_random. -//! Since the variable may be a boost::atomic with a non-constexpr constructor, dynamic initialization may be happening. -//! To avoid dynamic initialization order issues, initialization priority must be supported by the compiler so that -//! this pointer is initialized before the syscall initializer in operations.cpp. -BOOST_FILESYSTEM_INIT_PRIORITY(BOOST_FILESYSTEM_FUNC_PTR_INIT_PRIORITY) -BOOST_FILESYSTEM_ATOMIC_VAR(fill_random_t*, fill_random, &fill_random_dev_random); -#endif // defined(BOOST_FILESYSTEM_HAS_INIT_PRIORITY) +fill_random_t* fill_random = &fill_random_dev_random; //! Fills buffer with cryptographically random data obtained from getrandom() int fill_random_getrandom(void* buf, std::size_t len) @@ -194,9 +188,7 @@ int fill_random_getrandom(void* buf, std::size_t len) if (err == ENOSYS && bytes_read == 0u) { -#if defined(BOOST_FILESYSTEM_HAS_INIT_PRIORITY) - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(fill_random, &fill_random_dev_random); -#endif + filesystem::detail::atomic_store_relaxed(fill_random, &fill_random_dev_random); return fill_random_dev_random(buf, len); } @@ -220,11 +212,7 @@ void system_crypt_random(void* buf, std::size_t len, boost::system::error_code* #if defined(BOOST_FILESYSTEM_HAS_GETRANDOM) || defined(BOOST_FILESYSTEM_HAS_GETRANDOM_SYSCALL) -#if defined(BOOST_FILESYSTEM_HAS_INIT_PRIORITY) - int err = BOOST_FILESYSTEM_ATOMIC_LOAD_RELAXED(fill_random)(buf, len); -#else - int err = fill_random_getrandom(buf, len); -#endif + int err = filesystem::detail::atomic_load_relaxed(fill_random)(buf, len); if (BOOST_UNLIKELY(err != 0)) emit_error(err, ec, "boost::filesystem::unique_path"); @@ -310,7 +298,7 @@ void init_fill_random_impl(unsigned int major_ver, unsigned int minor_ver, unsig if (major_ver > 3u || (major_ver == 3u && minor_ver >= 17u)) fr = &fill_random_getrandom; - BOOST_FILESYSTEM_ATOMIC_STORE_RELAXED(fill_random, fr); + filesystem::detail::atomic_store_relaxed(fill_random, fr); #endif }