Work around aarch64+ASan clang preserve_none miscompile

clang miscompiles preserve_none on aarch64: a preserve_none callee uses
x19 as its frame base without saving it, so a caller that also uses x19
as its frame base resumes with a clobbered frame
(llvm/llvm-project#70577, llvm/llvm-project#177519). ASan's larger
frames make it reliable: n_string2() is left with nulled locals and the
parser segfaults on the valid document `""`.

preserve_none is only an optimization for the musttail chain, so fall
back to the default calling convention where it is not trustworthy:

- Define PRESERVE_NONE only when musttail is available (without it the
  parse() trampoline calls every continuation with an ordinary call, and
  those calls returned WeaselJson_REJECT for `""`) and we are not
  building for aarch64 with ASan.
- Stop annotating scan_string()/scan_string_impl() with it: n_string2()
  calls them with an ordinary call and keeps buf and bufEnd live across
  it, so they must preserve their caller's callee-saved registers.

Add a regression test for the minimal valid documents, parsing through
parser3::Parser3 directly so the continuations run under the flags of
the test binary; it segfaults in the aarch64+ASan build without this
change.

Closes #78.

src/parser3.h

@@ -284,11 +284,15 @@ inline PRESERVE_NONE ContinuationStatus n_number(Parser3 *self, char *buf,
 }
 
 // Advance buf until double quote, backslash, invalid utf8, or codepoint <
-// 0x20
+// 0x20.
+//
+// Not PRESERVE_NONE, unlike the continuations: n_string2() calls this with an
+// ordinary (not tail) call and keeps buf and bufEnd live across it, so it must
+// preserve its caller's callee-saved registers.
+// https://git.weaselab.dev/weaselab/weaseljson/issues/78
 template <class V>
-inline PRESERVE_NONE ContinuationStatus scan_string_impl(Parser3 *self,
-                                                         char *&buf,
-                                                         char *bufEnd) {
+inline ContinuationStatus scan_string_impl(Parser3 *self, char *&buf,
+                                           char *bufEnd) {
   const auto before = buf;
 
   // Advance buf past characters that transition the accept state to itself
@@ -343,20 +347,19 @@ template __attribute__((target("avx2"))) ContinuationStatus
 scan_string_impl<simd<int8_t, kLanes, sse::Simd_x86_AVX2>>(Parser3 *, char *&,
                                                            char *);
 
-__attribute__((target("default"))) inline PRESERVE_NONE ContinuationStatus
+__attribute__((target("default"))) inline ContinuationStatus
 scan_string(Parser3 *self, char *&buf, char *bufEnd) {
   MUSTTAIL return scan_string_impl<simd<int8_t, kLanes, sse::Simd_x86_SSE>>(
       self, buf, bufEnd);
 }
 
-__attribute__((target("avx2"))) inline PRESERVE_NONE ContinuationStatus
+__attribute__((target("avx2"))) inline ContinuationStatus
 scan_string(Parser3 *self, char *&buf, char *bufEnd) {
   MUSTTAIL return scan_string_impl<simd<int8_t, kLanes, sse::Simd_x86_AVX2>>(
       self, buf, bufEnd);
 }
 #else
-inline PRESERVE_NONE ContinuationStatus scan_string(Parser3 *self, char *&buf,
-                                                    char *bufEnd) {
+inline ContinuationStatus scan_string(Parser3 *self, char *&buf, char *bufEnd) {
   MUSTTAIL return scan_string_impl<simd<int8_t, 32>>(self, buf, bufEnd);
 }
 #endif

src/preserve_none.h

@@ -1,10 +1,37 @@
 #pragma once
 
+#include "musttail.h"
+
 #ifndef __has_attribute
 #define __has_attribute(x) 0
 #endif
 
-#if __has_attribute(preserve_none)
+// Clang reports AddressSanitizer through __has_feature, GCC through
+// __SANITIZE_ADDRESS__. Normalize so the check below has one spelling.
+#ifndef __SANITIZE_ADDRESS__
+#if defined(__has_feature)
+#if __has_feature(address_sanitizer)
+#define __SANITIZE_ADDRESS__
+#endif
+#endif
+#endif
+
+// preserve_none lets a continuation hand off to its successor without saving
+// callee-saved registers, so it is only worth something when the hand-off is a
+// tail call. clang also miscompiles it on aarch64: a preserve_none callee uses
+// x19 as its frame base without saving it, so a caller that also uses x19 as
+// its frame base resumes with a clobbered frame. ASan's larger frames make that
+// reliable (the parser segfaults on the valid document `""`), and without
+// musttail the parse() trampoline calls every continuation with an ordinary
+// call. Use the default calling convention in both of those configurations.
+//
+// This is a workaround for the compiler bug, not a requirement of
+// preserve_none.
+// https://github.com/llvm/llvm-project/issues/70577
+// https://github.com/llvm/llvm-project/issues/177519
+// https://git.weaselab.dev/weaselab/weaseljson/issues/78
+#if __has_attribute(preserve_none) && defined(HAS_MUSTTAIL) &&                 \
+    !(defined(__aarch64__) && defined(__SANITIZE_ADDRESS__))
 #define PRESERVE_NONE __attribute__((preserve_none))
 #else
 #define PRESERVE_NONE

src/test.cpp

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <limits>
+#include <new>
 #include <string>
 #include <vector>
 
@@ -195,6 +196,30 @@ std::vector<size_t> stridedCuts(std::string const &doc, size_t stride) {
   return cuts;
 }
 
+// Like parseChunked(), but uses parser3::Parser3 directly instead of the public
+// API, so that the continuations are compiled into this translation unit and
+// run under the flags this binary was built with. A `stride` of 0 feeds the
+// whole document at once.
+WeaselJsonStatus parseDirect(std::string const &doc, size_t stride) {
+  auto callbacks = noopCallbacks();
+  void *storage = ::operator new(parser3::Parser3::allocationSize(1024));
+  auto *parser = new (storage) parser3::Parser3{&callbacks, nullptr, 1024, 0};
+  std::string copy = doc;
+  size_t chunk = stride == 0 ? copy.size() : stride;
+  WeaselJsonStatus status = WeaselJson_AGAIN;
+  for (size_t i = 0; i < copy.size() && status == WeaselJson_AGAIN;
+       i += chunk) {
+    status =
+        parser->parse(copy.data() + i, int(std::min(chunk, copy.size() - i)));
+  }
+  if (status == WeaselJson_AGAIN) {
+    status = parser->parse(nullptr, 0);
+  }
+  parser->~Parser3();
+  ::operator delete(storage);
+  return status;
+}
+
 } // namespace
 
 TEST_CASE("parser3") {
@@ -245,6 +270,20 @@ TEST_CASE("parser3") {
   }
 }
 
+// The shortest valid documents, which must parse the same however they are
+// chunked. This doubles as a regression test for
+// https://git.weaselab.dev/weaselab/weaseljson/issues/78: `""` is the smallest
+// input that runs the n_string2() -> scan_string() -> scan_string_impl()
+// chain, which segfaulted under clang + AddressSanitizer on aarch64 when
+// scan_string() was annotated preserve_none and clobbered n_string2()'s frame.
+TEST_CASE("minimal valid documents") {
+  for (std::string doc : {"\"\"", "[]", "{}"}) {
+    for (size_t stride : {size_t(0), size_t(1), size_t(2)}) {
+      CHECK(parseDirect(doc, stride) == WeaselJson_OK);
+    }
+  }
+}
+
 TEST_CASE("overflow state is sticky") {
   auto c = noopCallbacks();
   // stackSize 3 is exactly big enough to hold reset()'s bootstrap, but too