Make tests pass under -c opt

.github/workflows/bazel.yml

@@ -10,20 +10,34 @@ env:
 
 jobs:
  check_lints:
+ name: Check Lints
  runs-on: ubuntu-latest
  steps:
- - uses: actions/checkout@v2
+  - uses: actions/checkout@v2
 
- - name: Check format
- run: ./format.sh
+  - name: Check Format
+  run: "./format.sh"
 
  build_and_test:
+ name: Build and Test (fastbuild)
  runs-on: ubuntu-latest
  steps:
- - uses: actions/checkout@v2
+  - uses: actions/checkout@v2
 
- - name: Build everything
- run: ./bazel build //...
+  - name: Build Everything
+  run: "./bazel build //... -c fastbuild"
 
- - name: Run tests
- run: ./bazel test //...
+ - name: Test Everything
+ run: "./bazel test //... -c fastbuild"
+
+ build_and_test_opt:
+ name: Build and Test (opt)
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v2
+
+ - name: Build Everything
+ run: "./bazel build //... -c opt"
+
+ - name: Test Everything
+ run: "./bazel test //... -c opt"

best/container/span_test.cc

@@ -207,6 +207,9 @@ best::test FrontAndBack = [](auto& t) {
 
  best::vec<int> ints = {1, 2, 3, 4, 5};
  best::span sp = ints;
+ t.expect_eq(ints.size(), 5);
+ t.expect_eq(ints[4], 5);
+ t.expect_eq(ints, {1, 2, 3, 4, 5});
 
  t.expect_eq(sp.first(), 1);
  t.expect_eq(sp.last(), 5);
@@ -366,11 +369,16 @@ struct NonPod {
  ~NonPod() {}
 
  bool operator==(int y) const { return x == y; }
- friend auto& operator<<(auto& os, NonPod np) { return os << np.x; }
+ friend void BestFmt(auto& fmt, const NonPod& np) { fmt.format(np.x); }
 };
 static_assert(!best::relocatable<NonPod, best::trivially>);
 
 best::test Shift = [](auto& t) {
+ // This test doesn't work correctly in opt mode, because it relies on doing
+ // UAF reads of destroyed values.
+ // TODO: Make this test sound?
+ if (!best::is_debug()) return;
+
  unsafe u("test");
 
  int a[] = {1, 2, 3, 4, 5, 6, 7, 8};

best/container/vec.h

@@ -108,8 +108,7 @@ class vec final {
  ///
  /// Constructs an empty vector using the given allocator.
  vec() : vec(alloc{}) {}
- explicit vec(alloc alloc)
- : size_(0), alloc_(best::in_place, std::move(alloc)) {}
+ explicit vec(alloc alloc) : alloc_(best::in_place, std::move(alloc)) {}
 
  /// # `vec::vec(range)`
  ///
@@ -598,7 +597,7 @@ class vec final {
  void splice_within(size_t idx, size_t start, size_t count);
 
  best::option<best::span<T>> on_heap() const {
- if (best::to_signed(size_) < 0) {
+ if (best::to_signed(load_size()) < 0) {
  return raw_;
  }
  return best::none;
@@ -641,9 +640,18 @@ class vec final {
 
  // How big the area where inlined values live is, including the size.
  size_t inlined_region_size() const {
- return reinterpret_cast<const char*>(&size_) -
+ return reinterpret_cast<const char*>(
+ &size_do_not_use_directly_or_else_ub_) -
  reinterpret_cast<const char*>(this) + best::size_of<size_t>;
  }
+ size_t load_size() const {
+ size_t size;
+ std::memcpy(&size, &size_do_not_use_directly_or_else_ub_, sizeof(size));
+ return size;
+ }
+ void store_size(size_t size) {
+ std::memcpy(&size_do_not_use_directly_or_else_ub_, &size, sizeof(size));
+ }
 
  static constexpr size_t InternalAlignment =
  max_inline > 0 && best::align_of<T> > best::align_of<best::span<T>>
@@ -652,7 +660,9 @@ class vec final {
 
  alignas(InternalAlignment) best::span<T> raw_;
  [[no_unique_address]] padding padding_;
- ssize_t size_; // Signed so we get "reasonable" debugging prints in gdb.
+ ssize_t // Signed so we get "reasonable"
+ // debugging prints in gdb.
+ size_do_not_use_directly_or_else_ub_ = 0;
  [[no_unique_address]] best::object<alloc> alloc_;
 };
 
@@ -703,7 +713,7 @@ void vec<T, max_inline, A>::move_construct(vec&& that, bool assign) {
 
  // construct_at because raw_ may contain garbage.
  std::construct_at(&raw_, *heap);
- size_ = ~that.size(); // Need to explicitly flip to on-heap mode here.
+ store_size(~that.size()); // Need to explicitly flip to on-heap mode here.
  } else if (best::relocatable<T, trivially>) {
  destroy();
 
@@ -729,7 +739,7 @@ void vec<T, max_inline, A>::move_construct(vec&& that, bool assign) {
  alloc_ = std::move(that.alloc_);
  }
 
- that.size_ = 0; // This resets `that` to being empty and inlined.
+ that.store_size(0); // This resets `that` to being empty and inlined.
 }
 
 template <best::relocatable T, size_t max_inline, best::allocator A>
@@ -739,7 +749,7 @@ void vec<T, max_inline, A>::destroy() {
  alloc_->dealloc(heap->data(), layout::array<T>(capacity()));
  }
 
- size_ = 0; // This returns `this` to being empty and inlined.
+ store_size(0); // This returns `this` to being empty and inlined.
 }
 
 template <best::relocatable T, size_t max_inline, best::allocator A>
@@ -760,11 +770,12 @@ best::object_ptr<T> vec<T, max_inline, A>::data() {
 
 template <best::relocatable T, size_t max_inline, best::allocator A>
 size_t vec<T, max_inline, A>::size() const {
+ auto size = load_size();
  if constexpr (max_inline == 0) {
  // size_ is implicitly always zero if off-heap.
- return on_heap() ? ~size_ : 0;
+ return on_heap() ? ~size : 0;
  } else {
- return on_heap() ? ~size_ : size_ >> (bits_of<size_t> - SizeBytes * 8);
+ return on_heap() ? ~size : size >> (bits_of<size_t> - SizeBytes * 8);
  }
 }
 template <best::relocatable T, size_t max_inline, best::allocator A>
@@ -775,7 +786,7 @@ void vec<T, max_inline, A>::set_size(unsafe, size_t new_size) {
  }
 
  if (on_heap()) {
- size_ = ~new_size;
+ store_size(~new_size);
  } else {
  if constexpr (max_inline == 0) {
  // size_ is implicitly always zero if off-heap, so we don't have to do
@@ -784,9 +795,16 @@ void vec<T, max_inline, A>::set_size(unsafe, size_t new_size) {
  auto offset = new_size << (bits_of<size_t> - SizeBytes * 8);
  auto mask = ~size_t{0} << (bits_of<size_t> - SizeBytes * 8);
 
- // TODO(mcyoung): This is a strict aliasing violation...
- size_ &= ~mask;
- size_ |= offset;
+ // XXX: we cannot touch size_ directly; it may have been overwritten
+ // partly via the data() pointer, and thus reading through size_ directly
+ // is a strict aliasing violation.
+ //
+ // There are tests that will fail with optimizations turned on if this we
+ // are not careful here.
+ size_t size = load_size();
+ size &= ~mask;
+ size |= offset;
+ store_size(size);
  }
  }
 }
@@ -990,7 +1008,7 @@ void vec<T, max_inline, A>::spill_to_heap(best::option<size_t> capacity_hint) {
  // calling its operator= is UB.
  std::construct_at(&raw_, new_data, new_size);
 
- size_ = ~old_size; // Update the size to the "on heap" form.
+ store_size(~old_size); // Update the size to the "on heap" form.
 }
 } // namespace best
 

best/func/fnref_test.cc

@@ -30,11 +30,17 @@ best::test FromFnptr = [](auto& t) {
 
  f = nullptr;
  t.expect_eq(f, nullptr);
+
+ f = [](int x) { return x - 42; };
+ t.expect_eq(f(8), -34);
 };
 
 best::test FromLambda = [](auto& t) {
  int total = 0;
- best::fnref<int(int) const> f = [&](int x) { return total += x; };
+ auto f0 = [&](int x) { return total += x; };
+ best::fnref<int(int) const> f = f0; // Need to hoist this, since fnref
+ // doesn't trigger RLE. Only causes a
+ // failure in opt mode.
 
  t.expect_eq(f(5), 5);
  t.expect_eq(total, 5);

best/func/internal/fnref.h

@@ -26,6 +26,11 @@
 #include "best/meta/taxonomy.h"
 
 namespace best::fnref_internal {
+template <typename F>
+concept no_captures = requires(F f) {
+ { +f } -> best::is_func_ptr;
+};
+
 template <typename Func, typename R, typename... Args>
 class impl {
  public:
@@ -37,7 +42,8 @@ class impl {
  constexpr impl(R (*fn)(Args...)) : ptr_(nullptr), fnptr_(fn) {}
 
  constexpr impl(const auto& fn)
- requires best::callable<decltype(fn), R(Args...)>
+ requires best::callable<decltype(fn), R(Args...)> &&
+ (!no_captures<decltype(fn)>)
  : ptr_(best::addr(fn)),
  lambda_(+[](const void* captures, Args... args) -> R {
  if constexpr (best::is_void<R>) {
@@ -52,8 +58,7 @@ class impl {
  }) {}
 
  constexpr impl(best::callable<R(Args...)> auto&& fn)
- requires best::callable<decltype(fn), R(Args...)> &&
- (!best::is_const_func<Func>)
+ requires(!best::is_const_func<Func>) && (!no_captures<decltype(fn)>)
  : ptr_(best::addr(fn)),
  lambda_(+[](const void* captures, Args... args) -> R {
  if constexpr (best::is_void<R>) {
@@ -67,6 +72,10 @@ class impl {
  }
  }) {}
 
+ constexpr impl(best::callable<R(Args...)> auto&& fn)
+ requires no_captures<decltype(fn)>
+ : impl(+fn) {}
+
  constexpr R operator()(Args... args) const {
  if (ptr_) {
  return lambda_(ptr_, BEST_FWD(args)...);

best/meta/taxonomy.h

@@ -232,6 +232,13 @@ concept is_ptr = std::is_pointer_v<T>;
 template <typename T>
 concept is_member_ptr = std::is_member_pointer_v<T>;
 
+/// # `best::is_func_ptr`
+///
+/// Whether this is a function pointer type.
+template <typename T>
+concept is_func_ptr =
+ best::is_ptr<T> && best::is_func<std::remove_pointer_t<T>>;
+
 /// # `best::as_ptr<T>`
 ///
 /// If `T` is an object, void, or tame function type, returns a pointer to it.