Merge pull request #137 from tcbrindle/pr/static_bounds_checks

Use compile-time bounds checking when possible
tcbrindle · Nov 28, 2023 · fbd10d1 · fbd10d1
2 parents 9c3c618 + 78cef5f
commit fbd10d1
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 7 deletions.
diff --git a/include/flux/core/assert.hpp b/include/flux/core/assert.hpp
@@ -47,6 +47,11 @@ struct runtime_error_fn {
 
 FLUX_EXPORT inline constexpr auto runtime_error = detail::runtime_error_fn{};
 
+#ifdef FLUX_HAVE_GCC_STATIC_BOUNDS_CHECKING
+[[gnu::error("out-of-bounds sequence access detected")]]
+void static_bounds_check_failed(); // not defined
+#endif
+
 namespace detail {
 
 struct assert_fn {
@@ -70,10 +75,30 @@ struct bounds_check_fn {
     }
 };
 
+struct indexed_bounds_check_fn {
+    template <typename T>
+    inline constexpr void operator()(T idx, T limit,
+                                     std::source_location loc = std::source_location::current()) const
+    {
+        if (!std::is_constant_evaluated()) {
+#ifdef FLUX_HAVE_GCC_STATIC_BOUNDS_CHECKING
+            if (__builtin_constant_p(idx) && __builtin_constant_p(limit)) {
+                if (idx < T{0} || idx >= limit) {
+                    static_bounds_check_failed();
+                }
+            }
+#endif
+            assert_fn{}(idx >= T{0}, "index cannot be negative", std::move(loc));
+            assert_fn{}(idx < limit, "out-of-bounds sequence access", std::move(loc));
+        }
+    }
+};
+
 } // namespace detail
 
 FLUX_EXPORT inline constexpr auto assert_ = detail::assert_fn{};
 FLUX_EXPORT inline constexpr auto bounds_check = detail::bounds_check_fn{};
+FLUX_EXPORT inline constexpr auto indexed_bounds_check = detail::indexed_bounds_check_fn{};
 
 #define FLUX_ASSERT(cond) (::flux::assert_(cond, "assertion '" #cond "' failed"))
 

diff --git a/include/flux/core/config.hpp b/include/flux/core/config.hpp
@@ -82,6 +82,15 @@
 #  define FLUX_DIVIDE_BY_ZERO_POLICY FLUX_DEFAULT_DIVIDE_BY_ZERO_POLICY
 #endif // FLUX_ERROR_ON_DIVIDE_BY_ZERO
 
+// Should we try to use static bounds checking?
+#if !defined(FLUX_DISABLE_STATIC_BOUNDS_CHECKING)
+#  if defined(__has_cpp_attribute) && defined(__has_builtin)
+#    if __has_builtin(__builtin_constant_p) && __has_cpp_attribute(gnu::error)
+#      define FLUX_HAVE_GCC_STATIC_BOUNDS_CHECKING 1
+#    endif
+#  endif
+#endif // FLUX_DISABLE_STATIC_BOUNDS_CHECKING
+
 // Default int_t is ptrdiff_t
 #define FLUX_DEFAULT_INT_TYPE std::ptrdiff_t
 

diff --git a/include/flux/core/default_impls.hpp b/include/flux/core/default_impls.hpp
@@ -25,8 +25,7 @@ struct sequence_traits<T[N]> {
 
     static constexpr auto read_at(auto& self, index_t idx) -> decltype(auto)
     {
-        bounds_check(idx >= 0);
-        bounds_check(idx < N);
+        indexed_bounds_check(idx, N);
         return self[idx];
     }
 
@@ -191,8 +190,7 @@ struct sequence_traits<R> {
 
     static constexpr auto read_at(auto& self, index_t idx) -> decltype(auto)
     {
-        bounds_check(idx >= 0);
-        bounds_check(idx < size(self));
+        indexed_bounds_check(idx, size(self));
         return data(self)[idx];
     }
 

diff --git a/include/flux/source/array_ptr.hpp b/include/flux/source/array_ptr.hpp
@@ -84,8 +84,7 @@ struct array_ptr : inline_sequence_base<array_ptr<T>> {
 
         static constexpr auto read_at(array_ptr const& self, index_t idx) -> T&
         {
-            bounds_check(idx >= 0);
-            bounds_check(idx < self.sz_);
+            indexed_bounds_check(idx, self.sz_);
             return self.data_[idx];
         }
 

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
@@ -77,7 +77,11 @@ add_executable(test-libflux
     test_unfold.cpp
 )
 target_link_libraries(test-libflux flux catch-main)
-target_compile_definitions(test-libflux PUBLIC FLUX_UNWIND_ON_ERROR FLUX_ERROR_ON_OVERFLOW)
+target_compile_definitions(test-libflux PUBLIC
+    FLUX_UNWIND_ON_ERROR
+    FLUX_ERROR_ON_OVERFLOW
+    FLUX_DISABLE_STATIC_BOUNDS_CHECKING
+)
 
 if(${FLUX_ENABLE_ASAN})
     target_compile_options(test-libflux PRIVATE -fsanitize=address)