From 16116aee67cefbd7963d443117fbcb4df5bab1db Mon Sep 17 00:00:00 2001 From: Armando Montanez Date: Fri, 9 Sep 2022 23:48:57 +0000 Subject: [PATCH] pw_span: Add flag to enable asserts Optionally backs Pigweed's std::span polyfill with PW_ASSERT() rather than always disabling assert checks. Bug: b/234873709 Change-Id: I2c0a316a9c7f0753fb3f0d81d93715512989d420 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/59496 Reviewed-by: Tom Craig Reviewed-by: Wyatt Hepler Pigweed-Auto-Submit: Armando Montanez Commit-Queue: Auto-Submit --- .gn | 3 ++ pw_span/BUILD.bazel | 10 +++- pw_span/BUILD.gn | 52 ++++++++++++++++++- pw_span/CMakeLists.txt | 2 + pw_span/docs.rst | 20 +++++++ pw_span/public/pw_span/internal/config.h | 26 ++++++++++ .../public/pw_span/internal/span_common.inc | 10 +++- pw_span/public/pw_span/span.h | 10 ++-- targets/host/target_toolchains.gni | 5 ++ 9 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 pw_span/public/pw_span/internal/config.h diff --git a/.gn b/.gn index 4dcf1b761e..3fe130bfc2 100644 --- a/.gn +++ b/.gn @@ -15,6 +15,9 @@ buildconfig = "//BUILDCONFIG.gn" default_args = { + # Default all upstream Pigweed toolchains to enable pw::span asserts. + pw_span_ENABLE_ASSERTS = true + pw_build_PIP_CONSTRAINTS = [ "//pw_env_setup/py/pw_env_setup/virtualenv_setup/constraint.list" ] diff --git a/pw_span/BUILD.bazel b/pw_span/BUILD.bazel index 61efc4a80e..d1a800d1ef 100644 --- a/pw_span/BUILD.bazel +++ b/pw_span/BUILD.bazel @@ -25,9 +25,15 @@ licenses(["notice"]) pw_cc_library( name = "pw_span", srcs = ["public/pw_span/internal/span_common.inc"], - hdrs = ["public/pw_span/span.h"], + hdrs = [ + "public/pw_span/internal/config.h", + "public/pw_span/span.h", + ], includes = ["public"], - deps = ["//pw_polyfill"], + deps = [ + # TODO(b/243851191): Depending on pw_assert causes a dependency cycle. + "//pw_polyfill", + ], ) pw_cc_test( diff --git a/pw_span/BUILD.gn b/pw_span/BUILD.gn index 3fe2c062f2..e9f1901053 100644 --- a/pw_span/BUILD.gn +++ b/pw_span/BUILD.gn @@ -14,16 +14,58 @@ import("//build_overrides/pigweed.gni") +import("$dir_pw_build/module_config.gni") import("$dir_pw_build/target_types.gni") import("$dir_pw_docgen/docs.gni") import("$dir_pw_toolchain/traits.gni") import("$dir_pw_unit_test/test.gni") +declare_args() { + # Whether or not to enable bounds-checking asserts in pw::span. Enabling this + # may significantly increase binary size, and can introduce dependency cycles + # if your pw_assert backend's headers depends directly or indirectly on + # pw_span. It's recommended to enable this for debug builds if possible. + pw_span_ENABLE_ASSERTS = false + + # The build target that overrides the default configuration options for this + # module. This should point to a source set that provides defines through a + # public config (which may -include a file or add defines directly). + # + # Most modules depend on pw_build_DEFAULT_MODULE_CONFIG as the default config, + # but since this module's config options require interaction with the build + # system, this defaults to an internal config to properly support + # pw_span_ENABLE_ASSERTS. + pw_span_CONFIG = "$dir_pw_span:span_asserts" +} + +config("public_include_path") { + include_dirs = [ "public" ] + visibility = [ ":*" ] +} + +pw_source_set("config") { + public = [ "public/pw_span/internal/config.h" ] + public_configs = [ ":public_include_path" ] + public_deps = [ pw_span_CONFIG ] + remove_public_deps = [ "*" ] + visibility = [ ":*" ] +} + config("public_config") { include_dirs = [ "public" ] visibility = [ ":*" ] } +config("span_asserts_config") { + defines = [ "PW_SPAN_ENABLE_ASSERTS=${pw_span_ENABLE_ASSERTS}" ] + visibility = [ ":span_asserts" ] +} + +pw_source_set("span_asserts") { + public_configs = [ ":span_asserts_config" ] + visibility = [ ":config" ] +} + # Provides "pw_span/span.h" and pw::span. pw_source_set("pw_span") { public = [ "public/pw_span/span.h" ] @@ -32,7 +74,10 @@ pw_source_set("pw_span") { pw_source_set("common") { public_configs = [ ":public_config" ] - public_deps = [ dir_pw_polyfill ] + public_deps = [ + ":config", + "$dir_pw_polyfill", + ] # Polyfill (std::byte) and (std::size(), std::data) if # C++17 is not supported. @@ -43,6 +88,11 @@ pw_source_set("common") { ] } + # Only add a dependency on pw_assert if the flag is explicitly enabled. + if (pw_span_ENABLE_ASSERTS) { + public_deps += [ "$dir_pw_assert:assert" ] + } + sources = [ "public/pw_span/internal/span_common.inc" ] visibility = [ ":*" ] } diff --git a/pw_span/CMakeLists.txt b/pw_span/CMakeLists.txt index a63ab66cfc..4e30377c27 100644 --- a/pw_span/CMakeLists.txt +++ b/pw_span/CMakeLists.txt @@ -18,10 +18,12 @@ include($ENV{PW_ROOT}/pw_build/pigweed.cmake) pw_add_module_library(pw_span HEADERS public/pw_span/span.h + public/pw_span/internal/config.h public/pw_span/internal/span_common.inc PUBLIC_INCLUDES public PUBLIC_DEPS + pw_assert pw_polyfill pw_polyfill.standard_library ) diff --git a/pw_span/docs.rst b/pw_span/docs.rst index 2a7dc984f3..8a2d1b62a7 100644 --- a/pw_span/docs.rst +++ b/pw_span/docs.rst @@ -65,6 +65,26 @@ Pointer and size arguments can be replaced with a :cpp:class:`pw::span`: ``pw_bytes/span.h`` provides ``ByteSpan`` and ``ConstByteSpan`` aliases for these types. +---------------------------- +Module Configuration Options +---------------------------- +The following configurations can be adjusted via compile-time configuration of +this module, see the +:ref:`module documentation ` for +more details. + +.. c:macro:: PW_SPAN_ENABLE_ASSERTS + + PW_SPAN_ENABLE_ASSERTS controls whether pw_span's implementation includes + asserts for detecting disallowed span operations at runtime. For C++20 and + later, this replaces std::span with the custom implementation in pw_span to + ensure bounds-checking asserts have been enabled. + + This defaults to disabled because of the significant increase in code size + caused by enabling this feature. It's strongly recommended to enable this + in debug and testing builds. This can be done by setting + ``pw_span_ENABLE_ASSERTS`` to ``true`` in the GN build. + ------------- Compatibility ------------- diff --git a/pw_span/public/pw_span/internal/config.h b/pw_span/public/pw_span/internal/config.h new file mode 100644 index 0000000000..89e6fe38c3 --- /dev/null +++ b/pw_span/public/pw_span/internal/config.h @@ -0,0 +1,26 @@ +// Copyright 2022 The Pigweed Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); you may not +// use this file except in compliance with the License. You may obtain a copy of +// the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +// License for the specific language governing permissions and limitations under +// the License. +#pragma once + +// PW_SPAN_ENABLE_ASSERTS controls whether pw_span's implementation includes +// asserts for detecting disallowed span operations at runtime. For C++20 and +// later, this replaces std::span with the custom implementation in pw_span to +// ensure bounds-checking asserts have been enabled. +// +// This defaults to disabled because of the significant increase in code size +// caused by enabling this feature. It's strongly recommended to enable this +// in debug and testing builds. +#if !defined(PW_SPAN_ENABLE_ASSERTS) +#define PW_SPAN_ENABLE_ASSERTS 0 +#endif // !defined(PW_SPAN_ENABLE_ASSERTS) diff --git a/pw_span/public/pw_span/internal/span_common.inc b/pw_span/public/pw_span/internal/span_common.inc index a347ffcfb3..ac68adc3c0 100644 --- a/pw_span/public/pw_span/internal/span_common.inc +++ b/pw_span/public/pw_span/internal/span_common.inc @@ -45,9 +45,17 @@ #include #include "pw_polyfill/language_feature_macros.h" +#include "pw_span/internal/config.h" -// Pigweed: Disable the asserts from Chromium for now. +#if PW_SPAN_ENABLE_ASSERTS + +#include "pw_assert/assert.h" + +#define _PW_SPAN_ASSERT(arg) PW_ASSERT(arg) + +#else #define _PW_SPAN_ASSERT(arg) +#endif // PW_SPAN_ENABLE_ASSERTS // The file that includes this .inc file must define the following macros: // diff --git a/pw_span/public/pw_span/span.h b/pw_span/public/pw_span/span.h index cd4dce5e5d..69a118c590 100644 --- a/pw_span/public/pw_span/span.h +++ b/pw_span/public/pw_span/span.h @@ -16,13 +16,17 @@ // implementation is shared with the std::span polyfill class. #pragma once +#include "pw_span/internal/config.h" + #if __has_include() #include #endif // __has_include() -// If the C++ library fully supports , pw::span is an alias of std::span. -#if defined(__cpp_lib_span) && __cpp_lib_span >= 202002L || \ - defined(_PW_SPAN_POLYFILL_ENABLED) +// If the C++ library fully supports , pw::span is an alias of std::span, +// but only if PW_SPAN_ENABLE_ASSERTS is not enabled. +#if !PW_SPAN_ENABLE_ASSERTS && \ + (defined(__cpp_lib_span) && __cpp_lib_span >= 202002L || \ + defined(_PW_SPAN_POLYFILL_ENABLED)) #include diff --git a/targets/host/target_toolchains.gni b/targets/host/target_toolchains.gni index e657c30746..a50fe73178 100644 --- a/targets/host/target_toolchains.gni +++ b/targets/host/target_toolchains.gni @@ -422,6 +422,11 @@ pw_internal_host_toolchains = [ forward_variables_from(_os_specific_config, "*") default_configs += _internal_clang_default_configs + # Don't enable span asserts since C++20 provides the implementation for + # pw::span, and there's no way to ensure asserts are enabled for the C++ + # standard library's std::span implementation. + pw_span_ENABLE_ASSERTS = false + # Set the C++ standard to C++20 instead of the default. pw_toolchain_CXX_STANDARD = pw_toolchain_STANDARD.CXX20 }