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 <tommycraig@gmail.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
Pigweed-Auto-Submit: Armando Montanez <amontanez@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
diff --git a/.gn b/.gn
index 4dcf1b7..3fe130b 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 61efc4a..d1a800d 100644
--- a/pw_span/BUILD.bazel
+++ b/pw_span/BUILD.bazel
@@ -25,9 +25,15 @@
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 3fe2c06..e9f1901 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("common") {
public_configs = [ ":public_config" ]
- public_deps = [ dir_pw_polyfill ]
+ public_deps = [
+ ":config",
+ "$dir_pw_polyfill",
+ ]
# Polyfill <cstddef> (std::byte) and <iterator> (std::size(), std::data) if
# C++17 is not supported.
@@ -43,6 +88,11 @@
]
}
+ # 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 a63ab66..4e30377 100644
--- a/pw_span/CMakeLists.txt
+++ b/pw_span/CMakeLists.txt
@@ -18,10 +18,12 @@
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 2a7dc98..8a2d1b6 100644
--- a/pw_span/docs.rst
+++ b/pw_span/docs.rst
@@ -65,6 +65,26 @@
``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 <module-structure-compile-time-configuration>` 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 0000000..89e6fe3
--- /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 a347ffc..ac68adc 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 <utility>
#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 cd4dce5..69a118c 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(<version>)
#include <version>
#endif // __has_include(<version>)
-// If the C++ library fully supports <span>, 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 <span>, 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 <span>
diff --git a/targets/host/target_toolchains.gni b/targets/host/target_toolchains.gni
index e657c30..a50fe73 100644
--- a/targets/host/target_toolchains.gni
+++ b/targets/host/target_toolchains.gni
@@ -422,6 +422,11 @@
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
}
