Alter FailFast behaviour of memcpy (#526)

This commit changes the codegen for error messages for failed memcpys.
This no longer generates a stack frame and correctly tail calls the
error messages generator.

It also turns the error messages on in Release builds.  This will lead
to better adoption experience.

src/snmalloc/global/bounds_checks.h

@@ -22,15 +22,16 @@ namespace snmalloc
    * error. With it set to false we print a helpful error message and then crash
    * the process.  The process may be in an undefined state by the time the
    * check fails, so there are potentially security implications to turning this
-   * off. It defaults to true for debug builds, false for release builds and
-   * can be overridden by defining the macro `SNMALLOC_FAIL_FAST` to true or
-   * false.
+   * off. It defaults to false and can be overridden by defining the macro
+   * `SNMALLOC_FAIL_FAST` to true.
+   *
+   * Current default to true will help with adoption experience.
    */
   static constexpr bool FailFast =
 #ifdef SNMALLOC_FAIL_FAST
     SNMALLOC_FAIL_FAST
 #else
-    !DEBUG
+    false
 #endif
     ;
 
@@ -40,71 +41,63 @@ namespace snmalloc
    * `p` is the input pointer and `len` is the offset from this pointer of the
    * bounds.  `msg` is the message that will be reported along with the
    * start and end of the real object's bounds.
+   *
+   * Note that this function never returns.  We do not mark it [[NoReturn]]
+   * so as to generate better code, because [[NoReturn]] prevents tailcails
+   * in GCC and Clang.
+   *
+   * The function claims to return a FakeReturn, this is so it can be tail
+   * called where the bound checked function returns a value, for instance, in
+   * memcpy it is specialised to void*.
    */
-  SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline void
-    report_fatal_bounds_error [[noreturn]] (
-      void* p, size_t len, const char* msg, decltype(ThreadAlloc::get())& alloc)
-  {
-    report_fatal_error(
-      "{}: {} is in allocation {}--{}, offset {} is past the end\n",
-      msg,
-      p,
-      alloc.template external_pointer<Start>(p),
-      alloc.template external_pointer<OnePastEnd>(p),
-      len);
-  }
-
-  /**
-   * The direction for a bounds check.
-   */
-  enum class CheckDirection
+  template<typename FakeReturn = void*>
+  SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline FakeReturn
+  report_fatal_bounds_error(const void* ptr, size_t len, const char* msg)
   {
-    /**
-     * A read bounds check, performed only when read checks are enabled.
-     */
-    Read,
+    if constexpr (FailFast)
+    {
+      UNUSED(ptr, len, msg);
+      SNMALLOC_FAST_FAIL();
+    }
+    else
+    {
+      auto& alloc = ThreadAlloc::get();
+      void* p = const_cast<void*>(ptr);
 
-    /**
-     * A write bounds check, performed unconditionally.
-     */
-    Write
-  };
+      auto range_end = pointer_offset(p, len);
+      auto object_end = alloc.template external_pointer<OnePastEnd>(p);
+      report_fatal_error(
+        "Fatal Error!\n{}: \n\trange [{}, {})\n\tallocation [{}, "
+        "{})\nrange goes beyond allocation by {} bytes \n",
+        msg,
+        p,
+        range_end,
+        alloc.template external_pointer<Start>(p),
+        object_end,
+        pointer_diff(object_end, range_end));
+    }
+  }
 
   /**
    * Check whether a pointer + length is in the same object as the pointer.
-   * Fail with the error message from the third argument if not.
    *
-   * The template parameter indicates whether this is a read.  If so, this
-   * function is a no-op when `CheckReads` is false.
+   * Returns true if the checks succeeds.
+   *
+   * The template parameter indicates whether the check should be performed.  It
+   * defaults to true. If it is false, the check will always succeed.
    */
-  template<
-    CheckDirection Direction = CheckDirection::Write,
-    bool CheckBoth = CheckReads>
-  SNMALLOC_FAST_PATH_INLINE void
-  check_bounds(const void* ptr, size_t len, const char* msg = "")
+  template<bool PerformCheck = true>
+  SNMALLOC_FAST_PATH_INLINE bool check_bounds(const void* ptr, size_t len)
   {
-    if constexpr ((Direction == CheckDirection::Write) || CheckBoth)
+    if constexpr (PerformCheck)
     {
       auto& alloc = ThreadAlloc::get();
-      void* p = const_cast<void*>(ptr);
-
-      if (SNMALLOC_UNLIKELY(!alloc.check_bounds(ptr, len)))
-      {
-        if constexpr (FailFast)
-        {
-          UNUSED(p, len, msg);
-          SNMALLOC_FAST_FAIL();
-        }
-        else
-        {
-          report_fatal_bounds_error(p, len, msg, alloc);
-        }
-      }
+      return alloc.check_bounds(ptr, len);
     }
     else
     {
-      UNUSED(ptr, len, msg);
+      UNUSED(ptr, len);
+      return true;
     }
   }
-
 } // namespace snmalloc

src/snmalloc/global/memcpy.h

@@ -317,14 +317,13 @@ namespace snmalloc
       return dst;
     }
 
-    if constexpr (Checked)
-    {
-      // Check the bounds of the arguments.
-      check_bounds(
-        dst, len, "memcpy with destination out of bounds of heap allocation");
-      check_bounds<CheckDirection::Read, CheckReads>(
+    // Check the bounds of the arguments.
+    if (SNMALLOC_UNLIKELY(!check_bounds<(Checked && ReadsChecked)>(src, len)))
+      return report_fatal_bounds_error(
         src, len, "memcpy with source out of bounds of heap allocation");
-    }
+    if (SNMALLOC_UNLIKELY(!check_bounds<Checked>(dst, len)))
+      return report_fatal_bounds_error(
+        dst, len, "memcpy with destination out of bounds of heap allocation");
 
     Arch::copy(dst, src, len);
     return orig_dst;

src/test/perf/memcpy/memcpy.cc

@@ -90,7 +90,12 @@ void memcpy_unchecked(void* dst, const void* src, size_t size)
 NOINLINE
 void memcpy_platform_checked(void* dst, const void* src, size_t size)
 {
-  check_bounds(dst, size, "");
+  if (SNMALLOC_UNLIKELY(!check_bounds(dst, size)))
+  {
+    report_fatal_bounds_error(dst, size, "");
+    return;
+  }
+
   memcpy(dst, src, size);
 }