From f7d2b13ef2466a5a3b7bcc535c49d3962a5c89f9 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Mon, 22 Jan 2024 09:09:23 -0800 Subject: Remove code pieces for no longer supported GCC versions. The minimum supported version today is GCC 7 (`__GNUC__ >= 7`). PiperOrigin-RevId: 600475215 Change-Id: I1aa46384f1e75f268649a48dbe2b42f3475bb07f --- absl/flags/flag_test.cc | 8 -------- 1 file changed, 8 deletions(-) (limited to 'absl/flags') diff --git a/absl/flags/flag_test.cc b/absl/flags/flag_test.cc index 8d14ba8d..53ad4635 100644 --- a/absl/flags/flag_test.cc +++ b/absl/flags/flag_test.cc @@ -1044,13 +1044,7 @@ TEST_F(FlagTest, MacroWithinAbslFlag) { // -------------------------------------------------------------------- -#if defined(__GNUC__) && !defined(__clang__) && __GNUC__ <= 5 -#define ABSL_SKIP_OPTIONAL_BOOL_TEST_DUE_TO_GCC_BUG -#endif - -#ifndef ABSL_SKIP_OPTIONAL_BOOL_TEST_DUE_TO_GCC_BUG ABSL_FLAG(absl::optional, optional_bool, absl::nullopt, "help"); -#endif ABSL_FLAG(absl::optional, optional_int, {}, "help"); ABSL_FLAG(absl::optional, optional_double, 9.3, "help"); ABSL_FLAG(absl::optional, optional_string, absl::nullopt, "help"); @@ -1064,7 +1058,6 @@ ABSL_FLAG(std::optional, std_optional_int64, std::nullopt, "help"); namespace { -#ifndef ABSL_SKIP_OPTIONAL_BOOL_TEST_DUE_TO_GCC_BUG TEST_F(FlagTest, TestOptionalBool) { EXPECT_FALSE(absl::GetFlag(FLAGS_optional_bool).has_value()); EXPECT_EQ(absl::GetFlag(FLAGS_optional_bool), absl::nullopt); @@ -1083,7 +1076,6 @@ TEST_F(FlagTest, TestOptionalBool) { } // -------------------------------------------------------------------- -#endif TEST_F(FlagTest, TestOptionalInt) { EXPECT_FALSE(absl::GetFlag(FLAGS_optional_int).has_value()); -- cgit v1.2.3 From 780bfc194d807dbd56363635ca40bf96743aa00b Mon Sep 17 00:00:00 2001 From: Shahriar Rouf Date: Wed, 31 Jan 2024 10:07:48 -0800 Subject: Replace `testonly = 1` with `testonly = True` in abseil BUILD files. https://bazel.build/build/style-guide#other-conventions PiperOrigin-RevId: 603084345 Change-Id: Ibd7c9573d820f88059d12c46ff82d7d322d002ae --- absl/base/BUILD.bazel | 18 +++++++++--------- absl/container/BUILD.bazel | 40 ++++++++++++++++++++-------------------- absl/crc/BUILD.bazel | 2 +- absl/flags/BUILD.bazel | 2 +- absl/hash/BUILD.bazel | 6 +++--- absl/log/internal/BUILD.bazel | 2 +- absl/numeric/BUILD.bazel | 2 +- absl/profiling/BUILD.bazel | 2 +- absl/random/BUILD.bazel | 6 +++--- absl/random/internal/BUILD.bazel | 8 ++++---- absl/strings/BUILD.bazel | 8 ++++---- absl/synchronization/BUILD.bazel | 10 +++++----- absl/time/BUILD.bazel | 2 +- 13 files changed, 54 insertions(+), 54 deletions(-) (limited to 'absl/flags') diff --git a/absl/base/BUILD.bazel b/absl/base/BUILD.bazel index 0eb735da..1eb8f098 100644 --- a/absl/base/BUILD.bazel +++ b/absl/base/BUILD.bazel @@ -294,7 +294,7 @@ cc_library( cc_library( name = "atomic_hook_test_helper", - testonly = 1, + testonly = True, srcs = ["internal/atomic_hook_test_helper.cc"], hdrs = ["internal/atomic_hook_test_helper.h"], copts = ABSL_DEFAULT_COPTS, @@ -380,7 +380,7 @@ cc_test( cc_library( name = "exception_testing", - testonly = 1, + testonly = True, hdrs = ["internal/exception_testing.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -404,7 +404,7 @@ cc_library( cc_library( name = "exception_safety_testing", - testonly = 1, + testonly = True, srcs = ["internal/exception_safety_testing.cc"], hdrs = ["internal/exception_safety_testing.h"], copts = ABSL_TEST_COPTS, @@ -470,7 +470,7 @@ cc_test( # AbslInternalSpinLockDelay and AbslInternalSpinLockWake. cc_library( name = "spinlock_test_common", - testonly = 1, + testonly = True, srcs = ["spinlock_test_common.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -507,7 +507,7 @@ cc_test( cc_library( name = "spinlock_benchmark_common", - testonly = 1, + testonly = True, srcs = ["internal/spinlock_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -527,7 +527,7 @@ cc_library( cc_binary( name = "spinlock_benchmark", - testonly = 1, + testonly = True, copts = ABSL_DEFAULT_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, tags = ["benchmark"], @@ -608,7 +608,7 @@ cc_test( cc_binary( name = "no_destructor_benchmark", - testonly = 1, + testonly = True, srcs = ["no_destructor_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -710,7 +710,7 @@ cc_test( cc_library( name = "scoped_set_env", - testonly = 1, + testonly = True, srcs = ["internal/scoped_set_env.cc"], hdrs = ["internal/scoped_set_env.h"], linkopts = ABSL_DEFAULT_LINKOPTS, @@ -784,7 +784,7 @@ cc_test( cc_binary( name = "strerror_benchmark", - testonly = 1, + testonly = True, srcs = ["internal/strerror_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/container/BUILD.bazel b/absl/container/BUILD.bazel index bc5b2201..91633948 100644 --- a/absl/container/BUILD.bazel +++ b/absl/container/BUILD.bazel @@ -108,7 +108,7 @@ cc_test( cc_binary( name = "fixed_array_benchmark", - testonly = 1, + testonly = True, srcs = ["fixed_array_benchmark.cc"], copts = ABSL_TEST_COPTS + ["$(STACK_FRAME_UNLIMITED)"], linkopts = ABSL_DEFAULT_LINKOPTS, @@ -151,7 +151,7 @@ cc_library( cc_library( name = "test_allocator", - testonly = 1, + testonly = True, copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, textual_hdrs = ["internal/test_allocator.h"], @@ -181,7 +181,7 @@ cc_test( cc_binary( name = "inlined_vector_benchmark", - testonly = 1, + testonly = True, srcs = ["inlined_vector_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -210,7 +210,7 @@ cc_test( cc_library( name = "test_instance_tracker", - testonly = 1, + testonly = True, srcs = ["internal/test_instance_tracker.cc"], hdrs = ["internal/test_instance_tracker.h"], copts = ABSL_DEFAULT_COPTS, @@ -449,7 +449,7 @@ cc_test( cc_library( name = "hash_generator_testing", - testonly = 1, + testonly = True, srcs = ["internal/hash_generator_testing.cc"], hdrs = ["internal/hash_generator_testing.h"], copts = ABSL_TEST_COPTS, @@ -465,7 +465,7 @@ cc_library( cc_library( name = "hash_policy_testing", - testonly = 1, + testonly = True, hdrs = ["internal/hash_policy_testing.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -707,7 +707,7 @@ cc_test( cc_binary( name = "raw_hash_set_benchmark", - testonly = 1, + testonly = True, srcs = ["internal/raw_hash_set_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -724,7 +724,7 @@ cc_binary( cc_binary( name = "raw_hash_set_probe_benchmark", - testonly = 1, + testonly = True, srcs = ["internal/raw_hash_set_probe_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = select({ @@ -798,7 +798,7 @@ cc_test( cc_binary( name = "layout_benchmark", - testonly = 1, + testonly = True, srcs = ["internal/layout_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -814,7 +814,7 @@ cc_binary( cc_library( name = "tracked", - testonly = 1, + testonly = True, hdrs = ["internal/tracked.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -825,7 +825,7 @@ cc_library( cc_library( name = "unordered_map_constructor_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_map_constructor_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -838,7 +838,7 @@ cc_library( cc_library( name = "unordered_map_lookup_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_map_lookup_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -851,7 +851,7 @@ cc_library( cc_library( name = "unordered_map_modifiers_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_map_modifiers_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -864,7 +864,7 @@ cc_library( cc_library( name = "unordered_set_constructor_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_set_constructor_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -878,7 +878,7 @@ cc_library( cc_library( name = "unordered_set_members_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_set_members_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -890,7 +890,7 @@ cc_library( cc_library( name = "unordered_map_members_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_map_members_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -902,7 +902,7 @@ cc_library( cc_library( name = "unordered_set_lookup_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_set_lookup_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -915,7 +915,7 @@ cc_library( cc_library( name = "unordered_set_modifiers_test", - testonly = 1, + testonly = True, hdrs = ["internal/unordered_set_modifiers_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -1008,7 +1008,7 @@ cc_library( cc_library( name = "btree_test_common", - testonly = 1, + testonly = True, hdrs = ["btree_test.h"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -1059,7 +1059,7 @@ cc_test( cc_binary( name = "btree_benchmark", - testonly = 1, + testonly = True, srcs = [ "btree_benchmark.cc", ], diff --git a/absl/crc/BUILD.bazel b/absl/crc/BUILD.bazel index d923aec4..9dc81819 100644 --- a/absl/crc/BUILD.bazel +++ b/absl/crc/BUILD.bazel @@ -203,7 +203,7 @@ cc_test( cc_binary( name = "crc32c_benchmark", - testonly = 1, + testonly = True, srcs = ["crc32c_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/flags/BUILD.bazel b/absl/flags/BUILD.bazel index d3b06227..657f8d2c 100644 --- a/absl/flags/BUILD.bazel +++ b/absl/flags/BUILD.bazel @@ -404,7 +404,7 @@ cc_test( cc_binary( name = "flag_benchmark", - testonly = 1, + testonly = True, srcs = [ "flag_benchmark.cc", ], diff --git a/absl/hash/BUILD.bazel b/absl/hash/BUILD.bazel index 1e8ad451..fe567e91 100644 --- a/absl/hash/BUILD.bazel +++ b/absl/hash/BUILD.bazel @@ -61,7 +61,7 @@ cc_library( cc_library( name = "hash_testing", - testonly = 1, + testonly = True, hdrs = ["hash_testing.h"], linkopts = ABSL_DEFAULT_LINKOPTS, deps = [ @@ -128,7 +128,7 @@ cc_test( cc_binary( name = "hash_benchmark", - testonly = 1, + testonly = True, srcs = ["hash_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -148,7 +148,7 @@ cc_binary( cc_library( name = "spy_hash_state", - testonly = 1, + testonly = True, hdrs = ["internal/spy_hash_state.h"], copts = ABSL_DEFAULT_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/log/internal/BUILD.bazel b/absl/log/internal/BUILD.bazel index 1be13499..2a8c1a47 100644 --- a/absl/log/internal/BUILD.bazel +++ b/absl/log/internal/BUILD.bazel @@ -400,7 +400,7 @@ cc_library( cc_binary( name = "vlog_config_benchmark", - testonly = 1, + testonly = True, srcs = ["vlog_config_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/numeric/BUILD.bazel b/absl/numeric/BUILD.bazel index db02b9c0..41c015db 100644 --- a/absl/numeric/BUILD.bazel +++ b/absl/numeric/BUILD.bazel @@ -46,7 +46,7 @@ cc_library( cc_binary( name = "bits_benchmark", - testonly = 1, + testonly = True, srcs = ["bits_benchmark.cc"], copts = ABSL_DEFAULT_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/profiling/BUILD.bazel b/absl/profiling/BUILD.bazel index 86f205f9..abe127ec 100644 --- a/absl/profiling/BUILD.bazel +++ b/absl/profiling/BUILD.bazel @@ -126,7 +126,7 @@ cc_test( cc_binary( name = "periodic_sampler_benchmark", - testonly = 1, + testonly = True, srcs = ["internal/periodic_sampler_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/random/BUILD.bazel b/absl/random/BUILD.bazel index 80c4f055..bec979d6 100644 --- a/absl/random/BUILD.bazel +++ b/absl/random/BUILD.bazel @@ -132,7 +132,7 @@ cc_library( cc_library( name = "mock_distributions", - testonly = 1, + testonly = True, hdrs = ["mock_distributions.h"], linkopts = ABSL_DEFAULT_LINKOPTS, deps = [ @@ -146,7 +146,7 @@ cc_library( cc_library( name = "mocking_bit_gen", - testonly = 1, + testonly = True, hdrs = [ "mocking_bit_gen.h", ], @@ -521,7 +521,7 @@ cc_test( # Benchmarks for various methods / test utilities cc_binary( name = "benchmarks", - testonly = 1, + testonly = True, srcs = [ "benchmarks.cc", ], diff --git a/absl/random/internal/BUILD.bazel b/absl/random/internal/BUILD.bazel index 71a742ee..69fb5f2b 100644 --- a/absl/random/internal/BUILD.bazel +++ b/absl/random/internal/BUILD.bazel @@ -137,7 +137,7 @@ cc_library( cc_library( name = "explicit_seed_seq", - testonly = 1, + testonly = True, hdrs = [ "explicit_seed_seq.h", ], @@ -151,7 +151,7 @@ cc_library( cc_library( name = "sequence_urbg", - testonly = 1, + testonly = True, hdrs = [ "sequence_urbg.h", ], @@ -375,7 +375,7 @@ cc_binary( cc_library( name = "distribution_test_util", - testonly = 1, + testonly = True, srcs = [ "chi_square.cc", "distribution_test_util.cc", @@ -534,7 +534,7 @@ cc_library( cc_library( name = "mock_overload_set", - testonly = 1, + testonly = True, hdrs = ["mock_overload_set.h"], linkopts = ABSL_DEFAULT_LINKOPTS, deps = [ diff --git a/absl/strings/BUILD.bazel b/absl/strings/BUILD.bazel index 1b6c7e4b..942c5324 100644 --- a/absl/strings/BUILD.bazel +++ b/absl/strings/BUILD.bazel @@ -829,7 +829,7 @@ cc_test( cc_library( name = "cord_test_helpers", - testonly = 1, + testonly = True, hdrs = [ "cord_test_helpers.h", ], @@ -845,7 +845,7 @@ cc_library( cc_library( name = "cord_rep_test_util", - testonly = 1, + testonly = True, hdrs = ["internal/cord_rep_test_util.h"], copts = ABSL_DEFAULT_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -859,7 +859,7 @@ cc_library( cc_library( name = "cordz_test_helpers", - testonly = 1, + testonly = True, hdrs = ["cordz_test_helpers.h"], copts = ABSL_DEFAULT_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -1449,7 +1449,7 @@ cc_test( cc_binary( name = "atod_manual_test", - testonly = 1, + testonly = True, srcs = ["atod_manual_test.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/synchronization/BUILD.bazel b/absl/synchronization/BUILD.bazel index de06ebdd..f747d14a 100644 --- a/absl/synchronization/BUILD.bazel +++ b/absl/synchronization/BUILD.bazel @@ -183,7 +183,7 @@ cc_test( cc_binary( name = "blocking_counter_benchmark", - testonly = 1, + testonly = True, srcs = ["blocking_counter_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -230,7 +230,7 @@ cc_test( cc_library( name = "thread_pool", - testonly = 1, + testonly = True, hdrs = ["internal/thread_pool.h"], linkopts = ABSL_DEFAULT_LINKOPTS, visibility = [ @@ -281,7 +281,7 @@ cc_test( cc_library( name = "mutex_benchmark_common", - testonly = 1, + testonly = True, srcs = ["mutex_benchmark.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, @@ -300,7 +300,7 @@ cc_library( cc_binary( name = "mutex_benchmark", - testonly = 1, + testonly = True, copts = ABSL_DEFAULT_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, deps = [ @@ -326,7 +326,7 @@ cc_test( cc_library( name = "per_thread_sem_test_common", - testonly = 1, + testonly = True, srcs = ["internal/per_thread_sem_test.cc"], copts = ABSL_TEST_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, diff --git a/absl/time/BUILD.bazel b/absl/time/BUILD.bazel index e3fe705b..05f1f2f2 100644 --- a/absl/time/BUILD.bazel +++ b/absl/time/BUILD.bazel @@ -65,7 +65,7 @@ cc_library( cc_library( name = "test_util", - testonly = 1, + testonly = True, srcs = ["internal/test_util.cc"], hdrs = ["internal/test_util.h"], copts = ABSL_DEFAULT_COPTS, -- cgit v1.2.3 From 52a711fc8021b49c869581cb5dfea93580502e2c Mon Sep 17 00:00:00 2001 From: Gennadiy Rozental Date: Fri, 2 Feb 2024 14:12:56 -0800 Subject: Avoid static initializers in case of ABSL_FLAGS_STRIP_NAMES=1 PiperOrigin-RevId: 603784442 Change-Id: I3d57e5f438b276c984f5d5416889b19e7ddb501a --- absl/flags/BUILD.bazel | 10 ++++----- absl/flags/CMakeLists.txt | 7 +++--- absl/flags/commandlineflag_test.cc | 9 ++++++-- absl/flags/flag.h | 2 ++ absl/flags/flag_test.cc | 46 +++++++++++++++++++++++++++++--------- absl/flags/internal/flag.h | 6 ++--- absl/flags/internal/usage_test.cc | 6 +++++ absl/flags/parse_test.cc | 7 ++++++ absl/flags/reflection_test.cc | 11 +++++---- 9 files changed, 77 insertions(+), 27 deletions(-) (limited to 'absl/flags') diff --git a/absl/flags/BUILD.bazel b/absl/flags/BUILD.bazel index 657f8d2c..cc513a09 100644 --- a/absl/flags/BUILD.bazel +++ b/absl/flags/BUILD.bazel @@ -236,10 +236,10 @@ cc_library( copts = ABSL_DEFAULT_COPTS, linkopts = ABSL_DEFAULT_LINKOPTS, deps = [ + ":commandlineflag", ":config", ":flag_internal", ":reflection", - "//absl/base", "//absl/base:config", "//absl/base:core_headers", "//absl/strings", @@ -343,7 +343,6 @@ cc_test( ], deps = [ ":commandlineflag", - ":commandlineflag_internal", ":config", ":flag", ":private_handle_accessor", @@ -394,9 +393,11 @@ cc_test( ":reflection", "//absl/base:core_headers", "//absl/base:malloc_internal", + "//absl/base:raw_logging_internal", "//absl/numeric:int128", "//absl/strings", "//absl/time", + "//absl/types:optional", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], @@ -459,6 +460,7 @@ cc_test( "no_test_wasm", ], deps = [ + ":config", ":flag", ":parse", ":reflection", @@ -520,11 +522,9 @@ cc_test( "no_test_wasm", ], deps = [ - ":commandlineflag_internal", + ":config", ":flag", - ":marshalling", ":reflection", - ":usage_internal", "//absl/memory", "//absl/strings", "@com_google_googletest//:gtest", diff --git a/absl/flags/CMakeLists.txt b/absl/flags/CMakeLists.txt index 44953124..48cc8325 100644 --- a/absl/flags/CMakeLists.txt +++ b/absl/flags/CMakeLists.txt @@ -214,7 +214,6 @@ absl_cc_library( absl::flags_config absl::flags_internal absl::flags_reflection - absl::base absl::core_headers absl::strings ) @@ -307,7 +306,6 @@ absl_cc_test( DEPS absl::flags absl::flags_commandlineflag - absl::flags_commandlineflag_internal absl::flags_config absl::flags_private_handle_accessor absl::flags_reflection @@ -344,6 +342,8 @@ absl_cc_test( absl::flags_marshalling absl::flags_reflection absl::int128 + absl::optional + absl::raw_logging_internal absl::strings absl::time GTest::gtest_main @@ -370,6 +370,7 @@ absl_cc_test( ${ABSL_TEST_COPTS} DEPS absl::flags + absl::flags_config absl::flags_parse absl::flags_reflection absl::flags_usage_internal @@ -413,8 +414,8 @@ absl_cc_test( COPTS ${ABSL_TEST_COPTS} DEPS - absl::flags_commandlineflag_internal absl::flags + absl::flags_config absl::flags_reflection absl::flags_usage absl::memory diff --git a/absl/flags/commandlineflag_test.cc b/absl/flags/commandlineflag_test.cc index 585db4ba..54700cf9 100644 --- a/absl/flags/commandlineflag_test.cc +++ b/absl/flags/commandlineflag_test.cc @@ -19,8 +19,8 @@ #include #include "gtest/gtest.h" +#include "absl/flags/config.h" #include "absl/flags/flag.h" -#include "absl/flags/internal/commandlineflag.h" #include "absl/flags/internal/private_handle_accessor.h" #include "absl/flags/reflection.h" #include "absl/flags/usage_config.h" @@ -51,7 +51,12 @@ class CommandLineFlagTest : public testing::Test { absl::SetFlagsUsageConfig(default_config); } - void SetUp() override { flag_saver_ = absl::make_unique(); } + void SetUp() override { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif + flag_saver_ = absl::make_unique(); + } void TearDown() override { flag_saver_.reset(); } private: diff --git a/absl/flags/flag.h b/absl/flags/flag.h index 06ea6932..a8e0e932 100644 --- a/absl/flags/flag.h +++ b/absl/flags/flag.h @@ -29,12 +29,14 @@ #ifndef ABSL_FLAGS_FLAG_H_ #define ABSL_FLAGS_FLAG_H_ +#include #include #include #include "absl/base/attributes.h" #include "absl/base/config.h" #include "absl/base/optimization.h" +#include "absl/flags/commandlineflag.h" #include "absl/flags/config.h" #include "absl/flags/internal/flag.h" #include "absl/flags/internal/registry.h" diff --git a/absl/flags/flag_test.cc b/absl/flags/flag_test.cc index 53ad4635..8bc7cdfd 100644 --- a/absl/flags/flag_test.cc +++ b/absl/flags/flag_test.cc @@ -19,14 +19,13 @@ #include #include -#include -#include #include #include // NOLINT #include #include "gtest/gtest.h" #include "absl/base/attributes.h" +#include "absl/base/internal/raw_logging.h" #include "absl/base/macros.h" #include "absl/flags/config.h" #include "absl/flags/declare.h" @@ -40,7 +39,9 @@ #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "absl/time/clock.h" #include "absl/time/time.h" +#include "absl/types/optional.h" ABSL_DECLARE_FLAG(int64_t, mistyped_int_flag); ABSL_DECLARE_FLAG(std::vector, mistyped_string_flag); @@ -226,9 +227,10 @@ ABSL_DECLARE_FLAG(absl::uint128, test_flag_14); namespace { -#if !ABSL_FLAGS_STRIP_NAMES - TEST_F(FlagTest, TestFlagDeclaration) { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif // test that we can access flag objects. EXPECT_EQ(absl::GetFlagReflectionHandle(FLAGS_test_flag_01).Name(), "test_flag_01"); @@ -259,12 +261,27 @@ TEST_F(FlagTest, TestFlagDeclaration) { EXPECT_EQ(absl::GetFlagReflectionHandle(FLAGS_test_flag_14).Name(), "test_flag_14"); } -#endif // !ABSL_FLAGS_STRIP_NAMES - -// -------------------------------------------------------------------- } // namespace +#if ABSL_FLAGS_STRIP_NAMES +// The intent of this helper struct and an expression below is to make sure that +// in the configuration where ABSL_FLAGS_STRIP_NAMES=1 registrar construction +// (in cases of of no Tail calls like OnUpdate) is constexpr and thus can and +// should be completely optimized away, thus avoiding the cost/overhead of +// static initializers. +struct VerifyConsteval { + friend consteval flags::FlagRegistrarEmpty operator+( + flags::FlagRegistrarEmpty, VerifyConsteval) { + return {}; + } +}; + +ABSL_FLAG(int, test_registrar_const_init, 0, "") + VerifyConsteval(); +#endif + +// -------------------------------------------------------------------- + ABSL_FLAG(bool, test_flag_01, true, "test flag 01"); ABSL_FLAG(int, test_flag_02, 1234, "test flag 02"); ABSL_FLAG(int16_t, test_flag_03, -34, "test flag 03"); @@ -283,8 +300,10 @@ ABSL_FLAG(absl::uint128, test_flag_14, absl::MakeUint128(0, 0xFFFAAABBBCCCDDD), namespace { -#if !ABSL_FLAGS_STRIP_NAMES TEST_F(FlagTest, TestFlagDefinition) { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif absl::string_view expected_file_name = "absl/flags/flag_test.cc"; EXPECT_EQ(absl::GetFlagReflectionHandle(FLAGS_test_flag_01).Name(), @@ -413,7 +432,6 @@ TEST_F(FlagTest, TestFlagDefinition) { expected_file_name)) << absl::GetFlagReflectionHandle(FLAGS_test_flag_14).Filename(); } -#endif // !ABSL_FLAGS_STRIP_NAMES // -------------------------------------------------------------------- @@ -604,6 +622,9 @@ TEST_F(FlagTest, TestGetSet) { // -------------------------------------------------------------------- TEST_F(FlagTest, TestGetViaReflection) { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif auto* handle = absl::FindCommandLineFlag("test_flag_01"); EXPECT_EQ(*handle->TryGet(), true); handle = absl::FindCommandLineFlag("test_flag_02"); @@ -638,6 +659,9 @@ TEST_F(FlagTest, TestGetViaReflection) { // -------------------------------------------------------------------- TEST_F(FlagTest, ConcurrentSetAndGet) { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif static constexpr int kNumThreads = 8; // Two arbitrary durations. One thread will concurrently flip the flag // between these two values, while the other threads read it and verify @@ -785,10 +809,12 @@ TEST_F(FlagTest, TestCustomUDT) { // MSVC produces link error on the type mismatch. // Linux does not have build errors and validations work as expected. #if !defined(_WIN32) && GTEST_HAS_DEATH_TEST - using FlagDeathTest = FlagTest; TEST_F(FlagDeathTest, TestTypeMismatchValidations) { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif #if !defined(NDEBUG) EXPECT_DEATH_IF_SUPPORTED( static_cast(absl::GetFlag(FLAGS_mistyped_int_flag)), diff --git a/absl/flags/internal/flag.h b/absl/flags/internal/flag.h index 2e6e6b87..7b056382 100644 --- a/absl/flags/internal/flag.h +++ b/absl/flags/internal/flag.h @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include @@ -770,7 +769,8 @@ struct FlagRegistrarEmpty {}; template class FlagRegistrar { public: - explicit FlagRegistrar(Flag& flag, const char* filename) : flag_(flag) { + constexpr explicit FlagRegistrar(Flag& flag, const char* filename) + : flag_(flag) { if (do_register) flags_internal::RegisterCommandLineFlag(flag_.impl_, filename); } @@ -783,7 +783,7 @@ class FlagRegistrar { // Make the registrar "die" gracefully as an empty struct on a line where // registration happens. Registrar objects are intended to live only as // temporary. - operator FlagRegistrarEmpty() const { return {}; } // NOLINT + constexpr operator FlagRegistrarEmpty() const { return {}; } // NOLINT private: Flag& flag_; // Flag being registered (not owned). diff --git a/absl/flags/internal/usage_test.cc b/absl/flags/internal/usage_test.cc index 6847386f..9b6d730c 100644 --- a/absl/flags/internal/usage_test.cc +++ b/absl/flags/internal/usage_test.cc @@ -22,6 +22,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "absl/flags/config.h" #include "absl/flags/flag.h" #include "absl/flags/internal/parse.h" #include "absl/flags/internal/program_name.h" @@ -97,6 +98,11 @@ class UsageReportingTest : public testing::Test { flags::SetFlagsHelpMatchSubstr(""); flags::SetFlagsHelpFormat(flags::HelpFormat::kHumanReadable); } + void SetUp() override { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif + } private: absl::FlagSaver flag_saver_; diff --git a/absl/flags/parse_test.cc b/absl/flags/parse_test.cc index 97b78980..b3d4ce4e 100644 --- a/absl/flags/parse_test.cc +++ b/absl/flags/parse_test.cc @@ -25,6 +25,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/base/internal/scoped_set_env.h" +#include "absl/flags/config.h" #include "absl/flags/flag.h" #include "absl/flags/internal/parse.h" #include "absl/flags/internal/usage.h" @@ -243,6 +244,12 @@ class ParseTest : public testing::Test { public: ~ParseTest() override { flags::SetFlagsHelpMode(flags::HelpMode::kNone); } + void SetUp() override { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif + } + private: absl::FlagSaver flag_saver_; }; diff --git a/absl/flags/reflection_test.cc b/absl/flags/reflection_test.cc index 79cfa90c..68abeda4 100644 --- a/absl/flags/reflection_test.cc +++ b/absl/flags/reflection_test.cc @@ -20,10 +20,8 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -#include "absl/flags/declare.h" +#include "absl/flags/config.h" #include "absl/flags/flag.h" -#include "absl/flags/internal/commandlineflag.h" -#include "absl/flags/marshalling.h" #include "absl/memory/memory.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" @@ -36,7 +34,12 @@ namespace { class ReflectionTest : public testing::Test { protected: - void SetUp() override { flag_saver_ = absl::make_unique(); } + void SetUp() override { +#if ABSL_FLAGS_STRIP_NAMES + GTEST_SKIP() << "This test requires flag names to be present"; +#endif + flag_saver_ = absl::make_unique(); + } void TearDown() override { flag_saver_.reset(); } private: -- cgit v1.2.3 From 1c233c55171af9101962ba9a5c2bda7ace18fcfe Mon Sep 17 00:00:00 2001 From: Adam Gajda <80600850+adgajda@users.noreply.github.com> Date: Wed, 13 Mar 2024 13:19:59 -0700 Subject: PR #1603: Disable -Wnon-virtual-dtor warning for CommandLineFlag implementations Imported from GitHub PR https://github.com/abseil/abseil-cpp/pull/1603 Merge e324303b1f2aaee8e4418cffb838f150a2d4f4e7 into d802708117c6ef6b9783efe499b2a2d0d0536c77 Merging this change closes #1603 COPYBARA_INTEGRATE_REVIEW=https://github.com/abseil/abseil-cpp/pull/1603 from adgajda:master e324303b1f2aaee8e4418cffb838f150a2d4f4e7 PiperOrigin-RevId: 615522811 Change-Id: I46a388ac62ffd42ce175dbfa04e414dd498855f8 --- absl/copts/GENERATED_AbseilCopts.cmake | 2 ++ absl/copts/GENERATED_copts.bzl | 2 ++ absl/copts/copts.py | 1 + absl/flags/commandlineflag.h | 11 +++++++++++ absl/flags/internal/flag.h | 13 +++++++++++-- absl/flags/reflection.cc | 10 ++++++++++ 6 files changed, 37 insertions(+), 2 deletions(-) (limited to 'absl/flags') diff --git a/absl/copts/GENERATED_AbseilCopts.cmake b/absl/copts/GENERATED_AbseilCopts.cmake index f90bff79..0079a719 100644 --- a/absl/copts/GENERATED_AbseilCopts.cmake +++ b/absl/copts/GENERATED_AbseilCopts.cmake @@ -44,6 +44,7 @@ list(APPEND ABSL_GCC_FLAGS "-Wconversion-null" "-Wformat-security" "-Wmissing-declarations" + "-Wnon-virtual-dtor" "-Woverlength-strings" "-Wpointer-arith" "-Wundef" @@ -61,6 +62,7 @@ list(APPEND ABSL_GCC_TEST_FLAGS "-Wcast-qual" "-Wconversion-null" "-Wformat-security" + "-Wnon-virtual-dtor" "-Woverlength-strings" "-Wpointer-arith" "-Wundef" diff --git a/absl/copts/GENERATED_copts.bzl b/absl/copts/GENERATED_copts.bzl index 3a659529..d9a9b375 100644 --- a/absl/copts/GENERATED_copts.bzl +++ b/absl/copts/GENERATED_copts.bzl @@ -45,6 +45,7 @@ ABSL_GCC_FLAGS = [ "-Wconversion-null", "-Wformat-security", "-Wmissing-declarations", + "-Wnon-virtual-dtor", "-Woverlength-strings", "-Wpointer-arith", "-Wundef", @@ -62,6 +63,7 @@ ABSL_GCC_TEST_FLAGS = [ "-Wcast-qual", "-Wconversion-null", "-Wformat-security", + "-Wnon-virtual-dtor", "-Woverlength-strings", "-Wpointer-arith", "-Wundef", diff --git a/absl/copts/copts.py b/absl/copts/copts.py index 946ceb86..1170d005 100644 --- a/absl/copts/copts.py +++ b/absl/copts/copts.py @@ -18,6 +18,7 @@ ABSL_GCC_FLAGS = [ "-Wconversion-null", "-Wformat-security", "-Wmissing-declarations", + "-Wnon-virtual-dtor", "-Woverlength-strings", "-Wpointer-arith", "-Wundef", diff --git a/absl/flags/commandlineflag.h b/absl/flags/commandlineflag.h index c30aa609..26ec0e7d 100644 --- a/absl/flags/commandlineflag.h +++ b/absl/flags/commandlineflag.h @@ -59,6 +59,14 @@ class PrivateHandleAccessor; // // Now you can get flag info from that reflection handle. // std::string flag_location = my_flag_data->Filename(); // ... + +// These are only used as constexpr global objects. +// They do not use a virtual destructor to simplify their implementation. +// They are not destroyed except at program exit, so leaks do not matter. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wnon-virtual-dtor" +#endif class CommandLineFlag { public: constexpr CommandLineFlag() = default; @@ -193,6 +201,9 @@ class CommandLineFlag { // flag's value type. virtual void CheckDefaultValueParsingRoundtrip() const = 0; }; +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic pop +#endif ABSL_NAMESPACE_END } // namespace absl diff --git a/absl/flags/internal/flag.h b/absl/flags/internal/flag.h index 7b056382..602531b1 100644 --- a/absl/flags/internal/flag.h +++ b/absl/flags/internal/flag.h @@ -424,6 +424,13 @@ struct DynValueDeleter { class FlagState; +// These are only used as constexpr global objects. +// They do not use a virtual destructor to simplify their implementation. +// They are not destroyed except at program exit, so leaks do not matter. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wnon-virtual-dtor" +#endif class FlagImpl final : public CommandLineFlag { public: constexpr FlagImpl(const char* name, const char* filename, FlagOpFn op, @@ -623,6 +630,9 @@ class FlagImpl final : public CommandLineFlag { // problems. alignas(absl::Mutex) mutable char data_guard_[sizeof(absl::Mutex)]; }; +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic pop +#endif /////////////////////////////////////////////////////////////////////////////// // The Flag object parameterized by the flag's value type. This class implements @@ -753,8 +763,7 @@ void* FlagOps(FlagOp op, const void* v1, void* v2, void* v3) { // Round sizeof(FlagImp) to a multiple of alignof(FlagValue) to get the // offset of the data. size_t round_to = alignof(FlagValue); - size_t offset = - (sizeof(FlagImpl) + round_to - 1) / round_to * round_to; + size_t offset = (sizeof(FlagImpl) + round_to - 1) / round_to * round_to; return reinterpret_cast(offset); } } diff --git a/absl/flags/reflection.cc b/absl/flags/reflection.cc index 841921a9..ea856ff9 100644 --- a/absl/flags/reflection.cc +++ b/absl/flags/reflection.cc @@ -217,6 +217,13 @@ void FinalizeRegistry() { namespace { +// These are only used as constexpr global objects. +// They do not use a virtual destructor to simplify their implementation. +// They are not destroyed except at program exit, so leaks do not matter. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wnon-virtual-dtor" +#endif class RetiredFlagObj final : public CommandLineFlag { public: constexpr RetiredFlagObj(const char* name, FlagFastTypeId type_id) @@ -276,6 +283,9 @@ class RetiredFlagObj final : public CommandLineFlag { const char* const name_; const FlagFastTypeId type_id_; }; +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic pop +#endif } // namespace -- cgit v1.2.3 From 16e21953352993faeb83d16d4bce99718e6e9d9c Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Fri, 15 Mar 2024 04:17:31 -0700 Subject: Remove redundant semicolons introduced by macros PiperOrigin-RevId: 616083064 Change-Id: I3d69303d32431227c197247682f8dcb70f9a239d --- absl/copts/GENERATED_AbseilCopts.cmake | 2 ++ absl/copts/GENERATED_copts.bzl | 2 ++ absl/copts/copts.py | 1 + absl/flags/flag_benchmark.cc | 2 +- absl/flags/parse_test.cc | 10 +++++----- absl/hash/hash_benchmark.cc | 14 +++++++------- absl/random/benchmarks.cc | 4 ++-- 7 files changed, 20 insertions(+), 15 deletions(-) (limited to 'absl/flags') diff --git a/absl/copts/GENERATED_AbseilCopts.cmake b/absl/copts/GENERATED_AbseilCopts.cmake index 0079a719..24b6175a 100644 --- a/absl/copts/GENERATED_AbseilCopts.cmake +++ b/absl/copts/GENERATED_AbseilCopts.cmake @@ -84,6 +84,7 @@ list(APPEND ABSL_GCC_TEST_FLAGS list(APPEND ABSL_LLVM_FLAGS "-Wall" "-Wextra" + "-Wc++98-compat-extra-semi" "-Wcast-qual" "-Wconversion" "-Wdead-code-aggressive" @@ -123,6 +124,7 @@ list(APPEND ABSL_LLVM_FLAGS list(APPEND ABSL_LLVM_TEST_FLAGS "-Wall" "-Wextra" + "-Wc++98-compat-extra-semi" "-Wcast-qual" "-Wconversion" "-Wdead-code-aggressive" diff --git a/absl/copts/GENERATED_copts.bzl b/absl/copts/GENERATED_copts.bzl index d9a9b375..9f59be4f 100644 --- a/absl/copts/GENERATED_copts.bzl +++ b/absl/copts/GENERATED_copts.bzl @@ -85,6 +85,7 @@ ABSL_GCC_TEST_FLAGS = [ ABSL_LLVM_FLAGS = [ "-Wall", "-Wextra", + "-Wc++98-compat-extra-semi", "-Wcast-qual", "-Wconversion", "-Wdead-code-aggressive", @@ -124,6 +125,7 @@ ABSL_LLVM_FLAGS = [ ABSL_LLVM_TEST_FLAGS = [ "-Wall", "-Wextra", + "-Wc++98-compat-extra-semi", "-Wcast-qual", "-Wconversion", "-Wdead-code-aggressive", diff --git a/absl/copts/copts.py b/absl/copts/copts.py index 1170d005..64e6f8a5 100644 --- a/absl/copts/copts.py +++ b/absl/copts/copts.py @@ -44,6 +44,7 @@ ABSL_GCC_TEST_ADDITIONAL_FLAGS = [ ABSL_LLVM_FLAGS = [ "-Wall", "-Wextra", + "-Wc++98-compat-extra-semi", "-Wcast-qual", "-Wconversion", "-Wdead-code-aggressive", diff --git a/absl/flags/flag_benchmark.cc b/absl/flags/flag_benchmark.cc index 758a6a55..88cc0c56 100644 --- a/absl/flags/flag_benchmark.cc +++ b/absl/flags/flag_benchmark.cc @@ -143,7 +143,7 @@ constexpr size_t kNumFlags = 0 REPLICATE(COUNT, _, _); #pragma clang section data = ".benchmark_flags" #endif #define DEFINE_FLAG(T, name, index) ABSL_FLAG(T, name##_##index, {}, ""); -#define FLAG_DEF(T) REPLICATE(DEFINE_FLAG, T, T##_flag); +#define FLAG_DEF(T) REPLICATE(DEFINE_FLAG, T, T##_flag) BENCHMARKED_TYPES(FLAG_DEF) #if defined(__clang__) && defined(__linux__) #pragma clang section data = "" diff --git a/absl/flags/parse_test.cc b/absl/flags/parse_test.cc index b3d4ce4e..99970692 100644 --- a/absl/flags/parse_test.cc +++ b/absl/flags/parse_test.cc @@ -44,31 +44,31 @@ #define FLAG_MULT(x) F3(x) #define TEST_FLAG_HEADER FLAG_HEADER_ -#define F(name) ABSL_FLAG(int, name, 0, ""); +#define F(name) ABSL_FLAG(int, name, 0, "") #define F1(name) \ F(name##1); \ F(name##2); \ F(name##3); \ F(name##4); \ - F(name##5); + F(name##5) /**/ #define F2(name) \ F1(name##1); \ F1(name##2); \ F1(name##3); \ F1(name##4); \ - F1(name##5); + F1(name##5) /**/ #define F3(name) \ F2(name##1); \ F2(name##2); \ F2(name##3); \ F2(name##4); \ - F2(name##5); + F2(name##5) /**/ -FLAG_MULT(TEST_FLAG_HEADER) +FLAG_MULT(TEST_FLAG_HEADER); namespace { diff --git a/absl/hash/hash_benchmark.cc b/absl/hash/hash_benchmark.cc index d18ea694..9b73f461 100644 --- a/absl/hash/hash_benchmark.cc +++ b/absl/hash/hash_benchmark.cc @@ -160,7 +160,7 @@ absl::flat_hash_set FlatHashSet(size_t count) { return hash{}(arg); \ } \ bool absl_hash_test_odr_use##hash##name = \ - (benchmark::DoNotOptimize(&Codegen##hash##name), false); + (benchmark::DoNotOptimize(&Codegen##hash##name), false) MAKE_BENCHMARK(AbslHash, Int32, int32_t{}); MAKE_BENCHMARK(AbslHash, Int64, int64_t{}); @@ -315,9 +315,9 @@ struct StringRand { BENCHMARK(BM_latency_##hash##_##name); \ } // namespace -MAKE_LATENCY_BENCHMARK(AbslHash, Int32, PodRand); -MAKE_LATENCY_BENCHMARK(AbslHash, Int64, PodRand); -MAKE_LATENCY_BENCHMARK(AbslHash, String9, StringRand<9>); -MAKE_LATENCY_BENCHMARK(AbslHash, String33, StringRand<33>); -MAKE_LATENCY_BENCHMARK(AbslHash, String65, StringRand<65>); -MAKE_LATENCY_BENCHMARK(AbslHash, String257, StringRand<257>); +MAKE_LATENCY_BENCHMARK(AbslHash, Int32, PodRand) +MAKE_LATENCY_BENCHMARK(AbslHash, Int64, PodRand) +MAKE_LATENCY_BENCHMARK(AbslHash, String9, StringRand<9>) +MAKE_LATENCY_BENCHMARK(AbslHash, String33, StringRand<33>) +MAKE_LATENCY_BENCHMARK(AbslHash, String65, StringRand<65>) +MAKE_LATENCY_BENCHMARK(AbslHash, String257, StringRand<257>) diff --git a/absl/random/benchmarks.cc b/absl/random/benchmarks.cc index 0900e818..26bc95e8 100644 --- a/absl/random/benchmarks.cc +++ b/absl/random/benchmarks.cc @@ -291,7 +291,7 @@ void BM_Thread(benchmark::State& state) { BENCHMARK_TEMPLATE(BM_Shuffle, Engine, 100)->ThreadPerCpu(); \ BENCHMARK_TEMPLATE(BM_Shuffle, Engine, 1000)->ThreadPerCpu(); \ BENCHMARK_TEMPLATE(BM_ShuffleReuse, Engine, 100)->ThreadPerCpu(); \ - BENCHMARK_TEMPLATE(BM_ShuffleReuse, Engine, 1000)->ThreadPerCpu(); + BENCHMARK_TEMPLATE(BM_ShuffleReuse, Engine, 1000)->ThreadPerCpu() #define BM_EXTENDED(Engine) \ /* -------------- Extended Uniform -----------------------*/ \ @@ -355,7 +355,7 @@ void BM_Thread(benchmark::State& state) { BENCHMARK_TEMPLATE(BM_Beta, Engine, absl::beta_distribution, 410, \ 580); \ BENCHMARK_TEMPLATE(BM_Gamma, Engine, std::gamma_distribution, 199); \ - BENCHMARK_TEMPLATE(BM_Gamma, Engine, std::gamma_distribution, 199); + BENCHMARK_TEMPLATE(BM_Gamma, Engine, std::gamma_distribution, 199) // ABSL Recommended interfaces. BM_BASIC(absl::InsecureBitGen); // === pcg64_2018_engine -- cgit v1.2.3 From eb46a63d77d7d1c1d30e2c9243099f89c629003c Mon Sep 17 00:00:00 2001 From: Gennadiy Rozental Date: Wed, 3 Jul 2024 14:29:50 -0700 Subject: Optimize the absl::GetFlag cost for most non built-in flag types (including string). PiperOrigin-RevId: 649200175 Change-Id: Ic6741d9fe5e0b1853ed8bb37b585d38b51d15581 --- absl/flags/BUILD.bazel | 1 + absl/flags/CMakeLists.txt | 1 + absl/flags/flag_test.cc | 200 +++++++++++++++++++++++++++++++++++++------ absl/flags/internal/flag.cc | 147 +++++++++++++++++++++++++------- absl/flags/internal/flag.h | 203 ++++++++++++++++++++++++++++++++++++++------ 5 files changed, 470 insertions(+), 82 deletions(-) (limited to 'absl/flags') diff --git a/absl/flags/BUILD.bazel b/absl/flags/BUILD.bazel index cc513a09..7a8ec7e6 100644 --- a/absl/flags/BUILD.bazel +++ b/absl/flags/BUILD.bazel @@ -390,6 +390,7 @@ cc_test( ":flag", ":flag_internal", ":marshalling", + ":parse", ":reflection", "//absl/base:core_headers", "//absl/base:malloc_internal", diff --git a/absl/flags/CMakeLists.txt b/absl/flags/CMakeLists.txt index 48cc8325..7376d117 100644 --- a/absl/flags/CMakeLists.txt +++ b/absl/flags/CMakeLists.txt @@ -340,6 +340,7 @@ absl_cc_test( absl::flags_config absl::flags_internal absl::flags_marshalling + absl::flags_parse absl::flags_reflection absl::int128 absl::optional diff --git a/absl/flags/flag_test.cc b/absl/flags/flag_test.cc index 8bc7cdfd..8af5bf7f 100644 --- a/absl/flags/flag_test.cc +++ b/absl/flags/flag_test.cc @@ -31,6 +31,7 @@ #include "absl/flags/declare.h" #include "absl/flags/internal/flag.h" #include "absl/flags/marshalling.h" +#include "absl/flags/parse.h" #include "absl/flags/reflection.h" #include "absl/flags/usage_config.h" #include "absl/numeric/int128.h" @@ -126,9 +127,9 @@ TEST_F(FlagTest, Traits) { #endif EXPECT_EQ(flags::StorageKind(), - flags::FlagValueStorageKind::kAlignedBuffer); + flags::FlagValueStorageKind::kHeapAllocated); EXPECT_EQ(flags::StorageKind>(), - flags::FlagValueStorageKind::kAlignedBuffer); + flags::FlagValueStorageKind::kHeapAllocated); EXPECT_EQ(flags::StorageKind(), flags::FlagValueStorageKind::kSequenceLocked); @@ -267,7 +268,7 @@ TEST_F(FlagTest, TestFlagDeclaration) { #if ABSL_FLAGS_STRIP_NAMES // The intent of this helper struct and an expression below is to make sure that // in the configuration where ABSL_FLAGS_STRIP_NAMES=1 registrar construction -// (in cases of of no Tail calls like OnUpdate) is constexpr and thus can and +// (in cases of no Tail calls like OnUpdate) is constexpr and thus can and // should be completely optimized away, thus avoiding the cost/overhead of // static initializers. struct VerifyConsteval { @@ -515,8 +516,10 @@ TEST_F(FlagTest, TestDefault) { struct NonTriviallyCopyableAggregate { NonTriviallyCopyableAggregate() = default; + // NOLINTNEXTLINE NonTriviallyCopyableAggregate(const NonTriviallyCopyableAggregate& rhs) : value(rhs.value) {} + // NOLINTNEXTLINE NonTriviallyCopyableAggregate& operator=( const NonTriviallyCopyableAggregate& rhs) { value = rhs.value; @@ -975,51 +978,194 @@ bool AbslParseFlag(absl::string_view, SmallAlignUDT*, std::string*) { } std::string AbslUnparseFlag(const SmallAlignUDT&) { return ""; } -// User-defined type with small size, but not trivially copyable. +} // namespace + +ABSL_FLAG(SmallAlignUDT, test_flag_sa_udt, {}, "help"); + +namespace { + +TEST_F(FlagTest, TestSmallAlignUDT) { + EXPECT_EQ(flags::StorageKind(), + flags::FlagValueStorageKind::kSequenceLocked); + SmallAlignUDT value = absl::GetFlag(FLAGS_test_flag_sa_udt); + EXPECT_EQ(value.c, 'A'); + EXPECT_EQ(value.s, 12); + + value.c = 'B'; + value.s = 45; + absl::SetFlag(&FLAGS_test_flag_sa_udt, value); + value = absl::GetFlag(FLAGS_test_flag_sa_udt); + EXPECT_EQ(value.c, 'B'); + EXPECT_EQ(value.s, 45); +} +} // namespace + +// -------------------------------------------------------------------- + +namespace { + +// User-defined not trivially copyable type. +template struct NonTriviallyCopyableUDT { - NonTriviallyCopyableUDT() : c('A') {} - NonTriviallyCopyableUDT(const NonTriviallyCopyableUDT& rhs) : c(rhs.c) {} + NonTriviallyCopyableUDT() : c('A') { s_num_instance++; } + NonTriviallyCopyableUDT(const NonTriviallyCopyableUDT& rhs) : c(rhs.c) { + s_num_instance++; + } NonTriviallyCopyableUDT& operator=(const NonTriviallyCopyableUDT& rhs) { c = rhs.c; return *this; } + ~NonTriviallyCopyableUDT() { s_num_instance--; } + static uint64_t s_num_instance; char c; }; -bool AbslParseFlag(absl::string_view, NonTriviallyCopyableUDT*, std::string*) { +template +uint64_t NonTriviallyCopyableUDT::s_num_instance = 0; + +template +bool AbslParseFlag(absl::string_view txt, NonTriviallyCopyableUDT* f, + std::string*) { + f->c = txt.empty() ? '\0' : txt[0]; return true; } -std::string AbslUnparseFlag(const NonTriviallyCopyableUDT&) { return ""; } +template +std::string AbslUnparseFlag(const NonTriviallyCopyableUDT&) { + return ""; +} +template +void TestExpectedLeaks( + F&& f, uint64_t num_leaks, + absl::optional num_new_instances = absl::nullopt) { + if (!num_new_instances.has_value()) num_new_instances = num_leaks; + + auto num_leaked_before = flags::NumLeakedFlagValues(); + auto num_instances_before = NonTriviallyCopyableUDT::s_num_instance; + f(); + EXPECT_EQ(num_leaked_before + num_leaks, flags::NumLeakedFlagValues()); + EXPECT_EQ(num_instances_before + num_new_instances.value(), + NonTriviallyCopyableUDT::s_num_instance); +} } // namespace -ABSL_FLAG(SmallAlignUDT, test_flag_sa_udt, {}, "help"); -ABSL_FLAG(NonTriviallyCopyableUDT, test_flag_ntc_udt, {}, "help"); +ABSL_FLAG(NonTriviallyCopyableUDT<1>, test_flag_ntc_udt1, {}, "help"); +ABSL_FLAG(NonTriviallyCopyableUDT<2>, test_flag_ntc_udt2, {}, "help"); +ABSL_FLAG(NonTriviallyCopyableUDT<3>, test_flag_ntc_udt3, {}, "help"); +ABSL_FLAG(NonTriviallyCopyableUDT<4>, test_flag_ntc_udt4, {}, "help"); +ABSL_FLAG(NonTriviallyCopyableUDT<5>, test_flag_ntc_udt5, {}, "help"); namespace { -TEST_F(FlagTest, TestSmallAlignUDT) { - SmallAlignUDT value = absl::GetFlag(FLAGS_test_flag_sa_udt); - EXPECT_EQ(value.c, 'A'); - EXPECT_EQ(value.s, 12); +TEST_F(FlagTest, TestNonTriviallyCopyableGetSetSet) { + EXPECT_EQ(flags::StorageKind>(), + flags::FlagValueStorageKind::kHeapAllocated); + + TestExpectedLeaks<1>( + [&] { + NonTriviallyCopyableUDT<1> value = + absl::GetFlag(FLAGS_test_flag_ntc_udt1); + EXPECT_EQ(value.c, 'A'); + }, + 0); + + TestExpectedLeaks<1>( + [&] { + NonTriviallyCopyableUDT<1> value; + value.c = 'B'; + absl::SetFlag(&FLAGS_test_flag_ntc_udt1, value); + EXPECT_EQ(value.c, 'B'); + }, + 1); + + TestExpectedLeaks<1>( + [&] { + NonTriviallyCopyableUDT<1> value; + value.c = 'C'; + absl::SetFlag(&FLAGS_test_flag_ntc_udt1, value); + }, + 0); +} - value.c = 'B'; - value.s = 45; - absl::SetFlag(&FLAGS_test_flag_sa_udt, value); - value = absl::GetFlag(FLAGS_test_flag_sa_udt); - EXPECT_EQ(value.c, 'B'); - EXPECT_EQ(value.s, 45); +TEST_F(FlagTest, TestNonTriviallyCopyableParseSet) { + TestExpectedLeaks<2>( + [&] { + const char* in_argv[] = {"testbin", "--test_flag_ntc_udt2=A"}; + absl::ParseCommandLine(2, const_cast(in_argv)); + }, + 0); + + TestExpectedLeaks<2>( + [&] { + NonTriviallyCopyableUDT<2> value; + value.c = 'B'; + absl::SetFlag(&FLAGS_test_flag_ntc_udt2, value); + EXPECT_EQ(value.c, 'B'); + }, + 0); } -TEST_F(FlagTest, TestNonTriviallyCopyableUDT) { - NonTriviallyCopyableUDT value = absl::GetFlag(FLAGS_test_flag_ntc_udt); - EXPECT_EQ(value.c, 'A'); +TEST_F(FlagTest, TestNonTriviallyCopyableSet) { + TestExpectedLeaks<3>( + [&] { + NonTriviallyCopyableUDT<3> value; + value.c = 'B'; + absl::SetFlag(&FLAGS_test_flag_ntc_udt3, value); + EXPECT_EQ(value.c, 'B'); + }, + 0); +} - value.c = 'B'; - absl::SetFlag(&FLAGS_test_flag_ntc_udt, value); - value = absl::GetFlag(FLAGS_test_flag_ntc_udt); - EXPECT_EQ(value.c, 'B'); +// One new instance created during initialization and stored in the flag. +auto premain_utd4_get = + (TestExpectedLeaks<4>([] { (void)absl::GetFlag(FLAGS_test_flag_ntc_udt4); }, + 0, 1), + false); + +TEST_F(FlagTest, TestNonTriviallyCopyableGetBeforeMainParseGet) { + TestExpectedLeaks<4>( + [&] { + const char* in_argv[] = {"testbin", "--test_flag_ntc_udt4=C"}; + absl::ParseCommandLine(2, const_cast(in_argv)); + }, + 1); + + TestExpectedLeaks<4>( + [&] { + NonTriviallyCopyableUDT<4> value = + absl::GetFlag(FLAGS_test_flag_ntc_udt4); + EXPECT_EQ(value.c, 'C'); + }, + 0); +} + +// One new instance created during initialization, which is reused since it was +// never read. +auto premain_utd5_set = (TestExpectedLeaks<5>( + [] { + NonTriviallyCopyableUDT<5> value; + value.c = 'B'; + absl::SetFlag(&FLAGS_test_flag_ntc_udt5, value); + }, + 0, 1), + false); + +TEST_F(FlagTest, TestNonTriviallyCopyableSetParseGet) { + TestExpectedLeaks<5>( + [&] { + const char* in_argv[] = {"testbin", "--test_flag_ntc_udt5=C"}; + absl::ParseCommandLine(2, const_cast(in_argv)); + }, + 0); + + TestExpectedLeaks<5>( + [&] { + NonTriviallyCopyableUDT<5> value = + absl::GetFlag(FLAGS_test_flag_ntc_udt5); + EXPECT_EQ(value.c, 'C'); + }, + 0); } } // namespace diff --git a/absl/flags/internal/flag.cc b/absl/flags/internal/flag.cc index 65d0e58f..981f19fd 100644 --- a/absl/flags/internal/flag.cc +++ b/absl/flags/internal/flag.cc @@ -22,14 +22,17 @@ #include #include +#include #include -#include #include #include +#include +#include "absl/base/attributes.h" #include "absl/base/call_once.h" #include "absl/base/casts.h" #include "absl/base/config.h" +#include "absl/base/const_init.h" #include "absl/base/dynamic_annotations.h" #include "absl/base/optimization.h" #include "absl/flags/config.h" @@ -44,10 +47,9 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace flags_internal { -// The help message indicating that the commandline flag has been -// 'stripped'. It will not show up when doing "-help" and its -// variants. The flag is stripped if ABSL_FLAGS_STRIP_HELP is set to 1 -// before including absl/flags/flag.h +// The help message indicating that the commandline flag has been stripped. It +// will not show up when doing "-help" and its variants. The flag is stripped +// if ABSL_FLAGS_STRIP_HELP is set to 1 before including absl/flags/flag.h const char kStrippedFlagHelp[] = "\001\002\003\004 (unknown) \004\003\002\001"; namespace { @@ -78,8 +80,31 @@ class MutexRelock { absl::Mutex& mu_; }; +// This is a freelist of leaked flag values and guard for its access. +// When we can't guarantee it is safe to reuse the memory for flag values, +// we move the memory to the freelist where it lives indefinitely, so it can +// still be safely accessed. This also prevents leak checkers from complaining +// about the leaked memory that can no longer be accessed through any pointer. +ABSL_CONST_INIT absl::Mutex s_freelist_guard(absl::kConstInit); +ABSL_CONST_INIT std::vector* s_freelist = nullptr; + +void AddToFreelist(void* p) { + absl::MutexLock l(&s_freelist_guard); + if (!s_freelist) { + s_freelist = new std::vector; + } + s_freelist->push_back(p); +} + } // namespace +/////////////////////////////////////////////////////////////////////////////// + +uint64_t NumLeakedFlagValues() { + absl::MutexLock l(&s_freelist_guard); + return s_freelist == nullptr ? 0u : s_freelist->size(); +} + /////////////////////////////////////////////////////////////////////////////// // Persistent state of the flag data. @@ -97,7 +122,7 @@ class FlagState : public flags_internal::FlagStateInterface { counter_(counter) {} ~FlagState() override { - if (flag_impl_.ValueStorageKind() != FlagValueStorageKind::kAlignedBuffer && + if (flag_impl_.ValueStorageKind() != FlagValueStorageKind::kHeapAllocated && flag_impl_.ValueStorageKind() != FlagValueStorageKind::kSequenceLocked) return; flags_internal::Delete(flag_impl_.op_, value_.heap_allocated); @@ -140,6 +165,33 @@ void DynValueDeleter::operator()(void* ptr) const { Delete(op, ptr); } +MaskedPointer::MaskedPointer(ptr_t rhs, bool is_candidate) : ptr_(rhs) { + if (is_candidate) { + ApplyMask(kUnprotectedReadCandidate); + } +} + +bool MaskedPointer::IsUnprotectedReadCandidate() const { + return CheckMask(kUnprotectedReadCandidate); +} + +bool MaskedPointer::HasBeenRead() const { return CheckMask(kHasBeenRead); } + +void MaskedPointer::Set(FlagOpFn op, const void* src, bool is_candidate) { + flags_internal::Copy(op, src, Ptr()); + if (is_candidate) { + ApplyMask(kUnprotectedReadCandidate); + } +} +void MaskedPointer::MarkAsRead() { ApplyMask(kHasBeenRead); } + +void MaskedPointer::ApplyMask(mask_t mask) { + ptr_ = reinterpret_cast(reinterpret_cast(ptr_) | mask); +} +bool MaskedPointer::CheckMask(mask_t mask) const { + return (reinterpret_cast(ptr_) & mask) != 0; +} + void FlagImpl::Init() { new (&data_guard_) absl::Mutex; @@ -174,11 +226,16 @@ void FlagImpl::Init() { (*default_value_.gen_func)(AtomicBufferValue()); break; } - case FlagValueStorageKind::kAlignedBuffer: + case FlagValueStorageKind::kHeapAllocated: // For this storage kind the default_value_ always points to gen_func // during initialization. assert(def_kind == FlagDefaultKind::kGenFunc); - (*default_value_.gen_func)(AlignedBufferValue()); + // Flag value initially points to the internal buffer. + MaskedPointer ptr_value = PtrStorage().load(std::memory_order_acquire); + (*default_value_.gen_func)(ptr_value.Ptr()); + // Default value is a candidate for an unprotected read. + PtrStorage().store(MaskedPointer(ptr_value.Ptr(), true), + std::memory_order_release); break; } seq_lock_.MarkInitialized(); @@ -234,7 +291,7 @@ std::unique_ptr FlagImpl::MakeInitValue() const { return {res, DynValueDeleter{op_}}; } -void FlagImpl::StoreValue(const void* src) { +void FlagImpl::StoreValue(const void* src, ValueSource source) { switch (ValueStorageKind()) { case FlagValueStorageKind::kValueAndInitBit: case FlagValueStorageKind::kOneWordAtomic: { @@ -249,8 +306,27 @@ void FlagImpl::StoreValue(const void* src) { seq_lock_.Write(AtomicBufferValue(), src, Sizeof(op_)); break; } - case FlagValueStorageKind::kAlignedBuffer: - Copy(op_, src, AlignedBufferValue()); + case FlagValueStorageKind::kHeapAllocated: + MaskedPointer ptr_value = PtrStorage().load(std::memory_order_acquire); + + if (ptr_value.IsUnprotectedReadCandidate() && ptr_value.HasBeenRead()) { + // If current value is a candidate for an unprotected read and if it was + // already read at least once, follow up reads (if any) are done without + // mutex protection. We can't guarantee it is safe to reuse this memory + // since it may have been accessed by another thread concurrently, so + // instead we move the memory to a freelist so it can still be safely + // accessed, and allocate a new one for the new value. + AddToFreelist(ptr_value.Ptr()); + ptr_value = MaskedPointer(Clone(op_, src), source == kCommandLine); + } else { + // Current value either was set programmatically or was never read. + // We can reuse the memory since all accesses to this value (if any) + // were protected by mutex. That said, if a new value comes from command + // line it now becomes a candidate for an unprotected read. + ptr_value.Set(op_, src, source == kCommandLine); + } + + PtrStorage().store(ptr_value, std::memory_order_release); seq_lock_.IncrementModificationCount(); break; } @@ -305,9 +381,10 @@ std::string FlagImpl::CurrentValue() const { ReadSequenceLockedData(cloned.get()); return flags_internal::Unparse(op_, cloned.get()); } - case FlagValueStorageKind::kAlignedBuffer: { + case FlagValueStorageKind::kHeapAllocated: { absl::MutexLock l(guard); - return flags_internal::Unparse(op_, AlignedBufferValue()); + return flags_internal::Unparse( + op_, PtrStorage().load(std::memory_order_acquire).Ptr()); } } @@ -370,10 +447,12 @@ std::unique_ptr FlagImpl::SaveState() { return absl::make_unique(*this, cloned, modified, on_command_line, ModificationCount()); } - case FlagValueStorageKind::kAlignedBuffer: { + case FlagValueStorageKind::kHeapAllocated: { return absl::make_unique( - *this, flags_internal::Clone(op_, AlignedBufferValue()), modified, - on_command_line, ModificationCount()); + *this, + flags_internal::Clone( + op_, PtrStorage().load(std::memory_order_acquire).Ptr()), + modified, on_command_line, ModificationCount()); } } return nullptr; @@ -388,11 +467,11 @@ bool FlagImpl::RestoreState(const FlagState& flag_state) { switch (ValueStorageKind()) { case FlagValueStorageKind::kValueAndInitBit: case FlagValueStorageKind::kOneWordAtomic: - StoreValue(&flag_state.value_.one_word); + StoreValue(&flag_state.value_.one_word, kProgrammaticChange); break; case FlagValueStorageKind::kSequenceLocked: - case FlagValueStorageKind::kAlignedBuffer: - StoreValue(flag_state.value_.heap_allocated); + case FlagValueStorageKind::kHeapAllocated: + StoreValue(flag_state.value_.heap_allocated, kProgrammaticChange); break; } @@ -411,11 +490,6 @@ StorageT* FlagImpl::OffsetValue() const { return reinterpret_cast(p + offset); } -void* FlagImpl::AlignedBufferValue() const { - assert(ValueStorageKind() == FlagValueStorageKind::kAlignedBuffer); - return OffsetValue(); -} - std::atomic* FlagImpl::AtomicBufferValue() const { assert(ValueStorageKind() == FlagValueStorageKind::kSequenceLocked); return OffsetValue>(); @@ -427,6 +501,11 @@ std::atomic& FlagImpl::OneWordValue() const { return OffsetValue()->value; } +std::atomic& FlagImpl::PtrStorage() const { + assert(ValueStorageKind() == FlagValueStorageKind::kHeapAllocated); + return OffsetValue()->value; +} + // Attempts to parse supplied `value` string using parsing routine in the `flag` // argument. If parsing successful, this function replaces the dst with newly // parsed value. In case if any error is encountered in either step, the error @@ -460,9 +539,17 @@ void FlagImpl::Read(void* dst) const { ReadSequenceLockedData(dst); break; } - case FlagValueStorageKind::kAlignedBuffer: { + case FlagValueStorageKind::kHeapAllocated: { absl::MutexLock l(guard); - flags_internal::CopyConstruct(op_, AlignedBufferValue(), dst); + MaskedPointer ptr_value = PtrStorage().load(std::memory_order_acquire); + + flags_internal::CopyConstruct(op_, ptr_value.Ptr(), dst); + + // For unprotected read candidates, mark that the value as has been read. + if (ptr_value.IsUnprotectedReadCandidate() && !ptr_value.HasBeenRead()) { + ptr_value.MarkAsRead(); + PtrStorage().store(ptr_value, std::memory_order_release); + } break; } } @@ -513,7 +600,7 @@ void FlagImpl::Write(const void* src) { } } - StoreValue(src); + StoreValue(src, kProgrammaticChange); } // Sets the value of the flag based on specified string `value`. If the flag @@ -534,7 +621,7 @@ bool FlagImpl::ParseFrom(absl::string_view value, FlagSettingMode set_mode, auto tentative_value = TryParse(value, err); if (!tentative_value) return false; - StoreValue(tentative_value.get()); + StoreValue(tentative_value.get(), source); if (source == kCommandLine) { on_command_line_ = true; @@ -555,7 +642,7 @@ bool FlagImpl::ParseFrom(absl::string_view value, FlagSettingMode set_mode, auto tentative_value = TryParse(value, err); if (!tentative_value) return false; - StoreValue(tentative_value.get()); + StoreValue(tentative_value.get(), source); break; } case SET_FLAGS_DEFAULT: { @@ -573,7 +660,7 @@ bool FlagImpl::ParseFrom(absl::string_view value, FlagSettingMode set_mode, if (!modified_) { // Need to set both default value *and* current, in this case. - StoreValue(default_value_.dynamic_value); + StoreValue(default_value_.dynamic_value, source); modified_ = false; } break; diff --git a/absl/flags/internal/flag.h b/absl/flags/internal/flag.h index 602531b1..a0be31d9 100644 --- a/absl/flags/internal/flag.h +++ b/absl/flags/internal/flag.h @@ -295,11 +295,8 @@ constexpr FlagDefaultArg DefaultArg(char) { } /////////////////////////////////////////////////////////////////////////////// -// Flag current value auxiliary structs. - -constexpr int64_t UninitializedFlagValue() { - return static_cast(0xababababababababll); -} +// Flag storage selector traits. Each trait indicates what kind of storage kind +// to use for the flag value. template using FlagUseValueAndInitBitStorage = @@ -321,9 +318,11 @@ enum class FlagValueStorageKind : uint8_t { kValueAndInitBit = 0, kOneWordAtomic = 1, kSequenceLocked = 2, - kAlignedBuffer = 3, + kHeapAllocated = 3, }; +// This constexpr function returns the storage kind for the given flag value +// type. template static constexpr FlagValueStorageKind StorageKind() { return FlagUseValueAndInitBitStorage::value @@ -332,14 +331,24 @@ static constexpr FlagValueStorageKind StorageKind() { ? FlagValueStorageKind::kOneWordAtomic : FlagUseSequenceLockStorage::value ? FlagValueStorageKind::kSequenceLocked - : FlagValueStorageKind::kAlignedBuffer; + : FlagValueStorageKind::kHeapAllocated; } +// This is a base class for the storage classes used by kOneWordAtomic and +// kValueAndInitBit storage kinds. It literally just stores the one word value +// as an atomic. By default, it is initialized to a magic value that is unlikely +// a valid value for the flag value type. struct FlagOneWordValue { + constexpr static int64_t Uninitialized() { + return static_cast(0xababababababababll); + } + + constexpr FlagOneWordValue() : value(Uninitialized()) {} constexpr explicit FlagOneWordValue(int64_t v) : value(v) {} std::atomic value; }; +// This class represents a memory layout used by kValueAndInitBit storage kind. template struct alignas(8) FlagValueAndInitBit { T value; @@ -348,16 +357,91 @@ struct alignas(8) FlagValueAndInitBit { uint8_t init; }; +// This class implements an aligned pointer with two options stored via masks +// in unused bits of the pointer value (due to alignment requirement). +// - IsUnprotectedReadCandidate - indicates that the value can be switched to +// unprotected read without a lock. +// - HasBeenRead - indicates that the value has been read at least once. +// - AllowsUnprotectedRead - combination of the two options above and indicates +// that the value can now be read without a lock. +// Further details of these options and their use is covered in the description +// of the FlagValue specialization. +class MaskedPointer { + public: + using mask_t = uintptr_t; + using ptr_t = void*; + + static constexpr int RequiredAlignment() { return 4; } + + constexpr explicit MaskedPointer(ptr_t rhs) : ptr_(rhs) {} + MaskedPointer(ptr_t rhs, bool is_candidate); + + void* Ptr() const { + return reinterpret_cast(reinterpret_cast(ptr_) & + kPtrValueMask); + } + bool AllowsUnprotectedRead() const { + return (reinterpret_cast(ptr_) & kAllowsUnprotectedRead) == + kAllowsUnprotectedRead; + } + bool IsUnprotectedReadCandidate() const; + bool HasBeenRead() const; + + void Set(FlagOpFn op, const void* src, bool is_candidate); + void MarkAsRead(); + + private: + // Masks + // Indicates that the flag value either default or originated from command + // line. + static constexpr mask_t kUnprotectedReadCandidate = 0x1u; + // Indicates that flag has been read. + static constexpr mask_t kHasBeenRead = 0x2u; + static constexpr mask_t kAllowsUnprotectedRead = + kUnprotectedReadCandidate | kHasBeenRead; + static constexpr mask_t kPtrValueMask = ~kAllowsUnprotectedRead; + + void ApplyMask(mask_t mask); + bool CheckMask(mask_t mask) const; + + ptr_t ptr_; +}; + +// This class implements a type erased storage of the heap allocated flag value. +// It is used as a base class for the storage class for kHeapAllocated storage +// kind. The initial_buffer is expected to have an alignment of at least +// MaskedPointer::RequiredAlignment(), so that the bits used by the +// MaskedPointer to store masks are set to 0. This guarantees that value starts +// in an uninitialized state. +struct FlagMaskedPointerValue { + constexpr explicit FlagMaskedPointerValue(MaskedPointer::ptr_t initial_buffer) + : value(MaskedPointer(initial_buffer)) {} + + std::atomic value; +}; + +// This is the forward declaration for the template that represents a storage +// for the flag values. This template is expected to be explicitly specialized +// for each storage kind and it does not have a generic default +// implementation. template ()> struct FlagValue; +// This specialization represents the storage of flag values types with the +// kValueAndInitBit storage kind. It is based on the FlagOneWordValue class +// and relies on memory layout in FlagValueAndInitBit to indicate that the +// value has been initialized or not. template struct FlagValue : FlagOneWordValue { constexpr FlagValue() : FlagOneWordValue(0) {} bool Get(const SequenceLock&, T& dst) const { int64_t storage = value.load(std::memory_order_acquire); if (ABSL_PREDICT_FALSE(storage == 0)) { + // This assert is to ensure that the initialization inside FlagImpl::Init + // is able to set init member correctly. + static_assert(offsetof(FlagValueAndInitBit, init) == sizeof(T), + "Unexpected memory layout of FlagValueAndInitBit"); return false; } dst = absl::bit_cast>(storage).value; @@ -365,12 +449,16 @@ struct FlagValue : FlagOneWordValue { } }; +// This specialization represents the storage of flag values types with the +// kOneWordAtomic storage kind. It is based on the FlagOneWordValue class +// and relies on the magic uninitialized state of default constructed instead of +// FlagOneWordValue to indicate that the value has been initialized or not. template struct FlagValue : FlagOneWordValue { - constexpr FlagValue() : FlagOneWordValue(UninitializedFlagValue()) {} + constexpr FlagValue() : FlagOneWordValue() {} bool Get(const SequenceLock&, T& dst) const { int64_t one_word_val = value.load(std::memory_order_acquire); - if (ABSL_PREDICT_FALSE(one_word_val == UninitializedFlagValue())) { + if (ABSL_PREDICT_FALSE(one_word_val == FlagOneWordValue::Uninitialized())) { return false; } std::memcpy(&dst, static_cast(&one_word_val), sizeof(T)); @@ -378,6 +466,12 @@ struct FlagValue : FlagOneWordValue { } }; +// This specialization represents the storage of flag values types with the +// kSequenceLocked storage kind. This storage is used by trivially copyable +// types with size greater than 8 bytes. This storage relies on uninitialized +// state of the SequenceLock to indicate that the value has been initialized or +// not. This storage also provides lock-free read access to the underlying +// value once it is initialized. template struct FlagValue { bool Get(const SequenceLock& lock, T& dst) const { @@ -391,11 +485,62 @@ struct FlagValue { std::atomic) std::atomic value_words[kNumWords]; }; +// This specialization represents the storage of flag values types with the +// kHeapAllocated storage kind. This is a storage of last resort and is used +// if none of other storage kinds are applicable. +// +// Generally speaking the values with this storage kind can't be accessed +// atomically and thus can't be read without holding a lock. If we would ever +// want to avoid the lock, we'd need to leak the old value every time new flag +// value is being set (since we are in danger of having a race condition +// otherwise). +// +// Instead of doing that, this implementation attempts to cater to some common +// use cases by allowing at most 2 values to be leaked - default value and +// value set from the command line. +// +// This specialization provides an initial buffer for the first flag value. This +// is where the default value is going to be stored. We attempt to reuse this +// buffer if possible, including storing the value set from the command line +// there. +// +// As long as we only read this value, we can access it without a lock (in +// practice we still use the lock for the very first read to be able set +// "has been read" option on this flag). +// +// If flag is specified on the command line we store the parsed value either +// in the internal buffer (if the default value never been read) or we leak the +// default value and allocate the new storage for the parse value. This value is +// also a candidate for an unprotected read. If flag is set programmatically +// after the command line is parsed, the storage for this value is going to be +// leaked. Note that in both scenarios we are not going to have a real leak. +// Instead we'll store the leaked value pointers in the internal freelist to +// avoid triggering the memory leak checker complains. +// +// If the flag is ever set programmatically, it stops being the candidate for an +// unprotected read, and any follow up access to the flag value requires a lock. +// Note that if the value if set programmatically before the command line is +// parsed, we can switch back to enabling unprotected reads for that value. template -struct FlagValue { - bool Get(const SequenceLock&, T&) const { return false; } +struct FlagValue + : FlagMaskedPointerValue { + // We const initialize the value with unmasked pointer to the internal buffer, + // making sure it is not a candidate for unprotected read. This way we can + // ensure Init is done before any access to the flag value. + constexpr FlagValue() : FlagMaskedPointerValue(&buffer[0]) {} + + bool Get(const SequenceLock&, T& dst) const { + MaskedPointer ptr_value = value.load(std::memory_order_acquire); + + if (ABSL_PREDICT_TRUE(ptr_value.AllowsUnprotectedRead())) { + ::new (static_cast(&dst)) T(*static_cast(ptr_value.Ptr())); + return true; + } + return false; + } - alignas(T) char value[sizeof(T)]; + alignas(MaskedPointer::RequiredAlignment()) alignas( + T) char buffer[sizeof(T)]{}; }; /////////////////////////////////////////////////////////////////////////////// @@ -483,7 +628,7 @@ class FlagImpl final : public CommandLineFlag { // Used in read/write operations to validate source/target has correct type. // For example if flag is declared as absl::Flag FLAGS_foo, a call to // absl::GetFlag(FLAGS_foo) validates that the type of FLAGS_foo is indeed - // int. To do that we pass the "assumed" type id (which is deduced from type + // int. To do that we pass the assumed type id (which is deduced from type // int) as an argument `type_id`, which is in turn is validated against the // type id stored in flag object by flag definition statement. void AssertValidType(FlagFastTypeId type_id, @@ -504,17 +649,13 @@ class FlagImpl final : public CommandLineFlag { void Init(); // Offset value access methods. One per storage kind. These methods to not - // respect const correctness, so be very carefull using them. + // respect const correctness, so be very careful using them. // This is a shared helper routine which encapsulates most of the magic. Since // it is only used inside the three routines below, which are defined in // flag.cc, we can define it in that file as well. template StorageT* OffsetValue() const; - // This is an accessor for a value stored in an aligned buffer storage - // used for non-trivially-copyable data types. - // Returns a mutable pointer to the start of a buffer. - void* AlignedBufferValue() const; // The same as above, but used for sequencelock-protected storage. std::atomic* AtomicBufferValue() const; @@ -523,13 +664,16 @@ class FlagImpl final : public CommandLineFlag { // mutable reference to an atomic value. std::atomic& OneWordValue() const; + std::atomic& PtrStorage() const; + // Attempts to parse supplied `value` string. If parsing is successful, // returns new value. Otherwise returns nullptr. std::unique_ptr TryParse(absl::string_view value, std::string& err) const ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard()); // Stores the flag value based on the pointer to the source. - void StoreValue(const void* src) ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard()); + void StoreValue(const void* src, ValueSource source) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(*DataGuard()); // Copy the flag data, protected by `seq_lock_` into `dst`. // @@ -585,7 +729,7 @@ class FlagImpl final : public CommandLineFlag { const char* const name_; // The file name where ABSL_FLAG resides. const char* const filename_; - // Type-specific operations "vtable". + // Type-specific operations vtable. const FlagOpFn op_; // Help message literal or function to generate it. const FlagHelpMsg help_; @@ -720,16 +864,21 @@ class FlagImplPeer { // Implementation of Flag value specific operations routine. template void* FlagOps(FlagOp op, const void* v1, void* v2, void* v3) { + struct AlignedSpace { + alignas(MaskedPointer::RequiredAlignment()) alignas(T) char buf[sizeof(T)]; + }; + using Allocator = std::allocator; switch (op) { case FlagOp::kAlloc: { - std::allocator alloc; - return std::allocator_traits>::allocate(alloc, 1); + Allocator alloc; + return std::allocator_traits::allocate(alloc, 1); } case FlagOp::kDelete: { T* p = static_cast(v2); p->~T(); - std::allocator alloc; - std::allocator_traits>::deallocate(alloc, p, 1); + Allocator alloc; + std::allocator_traits::deallocate( + alloc, reinterpret_cast(p), 1); return nullptr; } case FlagOp::kCopy: @@ -789,7 +938,7 @@ class FlagRegistrar { return *this; } - // Make the registrar "die" gracefully as an empty struct on a line where + // Makes the registrar die gracefully as an empty struct on a line where // registration happens. Registrar objects are intended to live only as // temporary. constexpr operator FlagRegistrarEmpty() const { return {}; } // NOLINT @@ -798,6 +947,10 @@ class FlagRegistrar { Flag& flag_; // Flag being registered (not owned). }; +/////////////////////////////////////////////////////////////////////////////// +// Test only API +uint64_t NumLeakedFlagValues(); + } // namespace flags_internal ABSL_NAMESPACE_END } // namespace absl -- cgit v1.2.3