From 651ccc0c3a3f507b8ab0895e4ba018b9a6dedbfa Mon Sep 17 00:00:00 2001 From: Frank Barchard Date: Tue, 23 May 2017 18:43:40 -0700 Subject: [PATCH] Fix data races in libyuv::TestCpuFlag(). Detect the compiler's support of C11 atomics, and use C11 atomics when available. Note that libyuv::MaskCpuFlags() is still not thread-safe. BUG=libyuv:641 TEST= cpu_thread_test.cc adds a pthread based test R=wangcheng@google.com Change-Id: If05b1e16da833105a0159ed67ef20f4e61bc7abd Reviewed-on: https://chromium-review.googlesource.com/510079 Commit-Queue: Frank Barchard Reviewed-by: Cheng Wang --- Android.mk | 1 + BUILD.gn | 1 + README.chromium | 2 +- include/libyuv/cpu_id.h | 7 ++++- include/libyuv/version.h | 2 +- libyuv_test.gyp | 1 + source/compare_win.cc | 2 +- source/cpu_id.cc | 26 +++++++++++----- unit_test/cpu_thread_test.cc | 60 ++++++++++++++++++++++++++++++++++++ 9 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 unit_test/cpu_thread_test.cc diff --git a/Android.mk b/Android.mk index 36c02adf3..e03459264 100644 --- a/Android.mk +++ b/Android.mk @@ -89,6 +89,7 @@ LOCAL_SRC_FILES := \ unit_test/compare_test.cc \ unit_test/convert_test.cc \ unit_test/cpu_test.cc \ + unit_test/cpu_thread_test.cc \ unit_test/math_test.cc \ unit_test/planar_test.cc \ unit_test/rotate_argb_test.cc \ diff --git a/BUILD.gn b/BUILD.gn index 1971161ab..8057cffa7 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -238,6 +238,7 @@ if (libyuv_include_tests) { "unit_test/compare_test.cc", "unit_test/convert_test.cc", "unit_test/cpu_test.cc", + "unit_test/cpu_thread_test.cc", "unit_test/math_test.cc", "unit_test/planar_test.cc", "unit_test/rotate_argb_test.cc", diff --git a/README.chromium b/README.chromium index 61b74f5bc..d02617472 100644 --- a/README.chromium +++ b/README.chromium @@ -1,6 +1,6 @@ Name: libyuv URL: http://code.google.com/p/libyuv/ -Version: 1656 +Version: 1657 License: BSD License File: LICENSE diff --git a/include/libyuv/cpu_id.h b/include/libyuv/cpu_id.h index bcddb32ea..77471fda5 100644 --- a/include/libyuv/cpu_id.h +++ b/include/libyuv/cpu_id.h @@ -59,7 +59,12 @@ int ArmCpuCaps(const char* cpuinfo_name); // returns non-zero if instruction set is detected static __inline int TestCpuFlag(int test_flag) { LIBYUV_API extern int cpu_info_; - return (!cpu_info_ ? InitCpuFlags() : cpu_info_) & test_flag; +#ifdef __ATOMIC_RELAXED + int cpu_info = __atomic_load_n(&cpu_info_, __ATOMIC_RELAXED); +#else + int cpu_info = cpu_info_; +#endif + return (!cpu_info ? InitCpuFlags() : cpu_info) & test_flag; } // For testing, allow CPU flags to be disabled. diff --git a/include/libyuv/version.h b/include/libyuv/version.h index 0dc150ca1..2d03771d2 100644 --- a/include/libyuv/version.h +++ b/include/libyuv/version.h @@ -11,6 +11,6 @@ #ifndef INCLUDE_LIBYUV_VERSION_H_ #define INCLUDE_LIBYUV_VERSION_H_ -#define LIBYUV_VERSION 1656 +#define LIBYUV_VERSION 1657 #endif // INCLUDE_LIBYUV_VERSION_H_ diff --git a/libyuv_test.gyp b/libyuv_test.gyp index 87e7a5bb1..4222cf26d 100644 --- a/libyuv_test.gyp +++ b/libyuv_test.gyp @@ -39,6 +39,7 @@ 'unit_test/color_test.cc', 'unit_test/convert_test.cc', 'unit_test/cpu_test.cc', + 'unit_test/cpu_thread_test.cc', 'unit_test/math_test.cc', 'unit_test/planar_test.cc', 'unit_test/rotate_argb_test.cc', diff --git a/source/compare_win.cc b/source/compare_win.cc index de1d07b5c..5ca59178d 100644 --- a/source/compare_win.cc +++ b/source/compare_win.cc @@ -14,7 +14,7 @@ #include "libyuv/row.h" #if defined(_MSC_VER) -#include // For __popcnt +#include // For __popcnt #endif #ifdef __cplusplus diff --git a/source/cpu_id.cc b/source/cpu_id.cc index afb5d282b..044039675 100644 --- a/source/cpu_id.cc +++ b/source/cpu_id.cc @@ -43,6 +43,9 @@ extern "C" { #define SAFEBUFFERS #endif +// cpu_info_ variable for SIMD instruction sets detected. +LIBYUV_API int cpu_info_ = 0; + // Low level cpuid for X86. #if (defined(_M_IX86) || defined(_M_X64) || defined(__i386__) || \ defined(__x86_64__)) && \ @@ -192,10 +195,6 @@ LIBYUV_API SAFEBUFFERS int MipsCpuCaps(const char* cpuinfo_name, return 0; } -// CPU detect function for SIMD instruction sets. -LIBYUV_API -int cpu_info_ = 0; // cpu_info is not initialized yet. - // Test environment variable for disabling CPU features. Any non-zero value // to disable. Zero ignored to make it easy to set the variable on/off. #if !defined(__native_client__) && !defined(_M_ARM) @@ -215,7 +214,7 @@ static LIBYUV_BOOL TestEnv(const char*) { } #endif -LIBYUV_API SAFEBUFFERS int InitCpuFlags(void) { +static SAFEBUFFERS int GetCpuFlags(void) { int cpu_info = 0; #if !defined(__pnacl__) && !defined(__CLR_VER) && defined(CPU_X86) uint32 cpu_info0[4] = {0, 0, 0, 0}; @@ -321,14 +320,27 @@ LIBYUV_API SAFEBUFFERS int InitCpuFlags(void) { cpu_info = 0; } cpu_info |= kCpuInitialized; - cpu_info_ = cpu_info; return cpu_info; } // Note that use of this function is not thread safe. LIBYUV_API void MaskCpuFlags(int enable_flags) { - cpu_info_ = InitCpuFlags() & enable_flags; +#ifdef __ATOMIC_RELAXED + __atomic_store_n(&cpu_info_, GetCpuFlags() & enable_flags, __ATOMIC_RELAXED); +#else + cpu_info_ = GetCpuFlags() & enable_flags; +#endif +} + +LIBYUV_API +int InitCpuFlags(void) { + MaskCpuFlags(-1); +#ifdef __ATOMIC_RELAXED + return __atomic_load_n(&cpu_info_, __ATOMIC_RELAXED); +#else + return cpu_info_; +#endif } #ifdef __cplusplus diff --git a/unit_test/cpu_thread_test.cc b/unit_test/cpu_thread_test.cc new file mode 100644 index 000000000..185e36348 --- /dev/null +++ b/unit_test/cpu_thread_test.cc @@ -0,0 +1,60 @@ +/* + * Copyright 2017 The LibYuv Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include + +#include "libyuv/cpu_id.h" + +// TODO(fbarchard): Port to other platforms +#if defined(__linux__) +#define LIBYUV_HAVE_PTHREAD 1 +#endif + +#ifdef LIBYUV_HAVE_PTHREAD +#include +#endif + +namespace libyuv { + +#ifdef LIBYUV_HAVE_PTHREAD +void* ThreadMain(void* arg) { + int* flags = static_cast(arg); + + *flags = TestCpuFlag(kCpuHasSSSE3); + return nullptr; +} +#endif // LIBYUV_HAVE_PTHREAD + +// Call TestCpuFlag() from two threads. ThreadSanitizer should not report any +// data race. +TEST(LibYUVCpuThreadTest, TestCpuFlagMultipleThreads) { +#ifdef LIBYUV_HAVE_PTHREAD + int cpu_flags1; + int cpu_flags2; + int ret; + pthread_t thread1; + pthread_t thread2; + + MaskCpuFlags(0); // Reset to 0 to allow auto detect. + ret = pthread_create(&thread1, nullptr, ThreadMain, &cpu_flags1); + ASSERT_EQ(ret, 0); + ret = pthread_create(&thread2, nullptr, ThreadMain, &cpu_flags2); + ASSERT_EQ(ret, 0); + ret = pthread_join(thread1, nullptr); + EXPECT_EQ(ret, 0); + ret = pthread_join(thread2, nullptr); + EXPECT_EQ(ret, 0); + EXPECT_EQ(cpu_flags1, cpu_flags2); +#else + printf("pthread unavailable; Test skipped."); +#endif // LIBYUV_HAVE_PTHREAD +} + +} // namespace libyuv