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 <[email protected]>
Reviewed-by: Wyatt Hepler <[email protected]>
Pigweed-Auto-Submit: Armando Montanez <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>

.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" ]
 

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(

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 <cstddef> (std::byte) and <iterator> (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 = [ ":*" ]
 }

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
 )

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 <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
 -------------

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)

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:
 //

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>
 

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
     }