Address review: return an error instead of aborting when the parser throws

The standalone module aborted on any query that the parser rejects by throwing
rather than by returning `false` - `SELECT sum(x) OVER (ROWS BETWEEN UNBOUNDED
FOLLOWING AND CURRENT ROW)`, for one - because `-fignore-exceptions` leaves
nothing to catch. Stopping the module on an ordinary user mistake is not
acceptable, so `__cxa_throw` now jumps back to a `setjmp` boundary armed by
`ch_format`, which reports the message as a parse failure; the module stays
usable afterwards, which the test now checks.

This costs 570 bytes and `-lsetjmp`, against the 262 KB and the
exception-handling proposal that `-fwasm-exceptions` would cost. `longjmp` runs
no destructors, so such a query leaks what the parser allocated below the
boundary - documented at `__cxa_throw`.

utils/wasm-parser/README.md

@@ -27,8 +27,10 @@ The module is a WASI reactor and exports a C interface (see `wasm_parser.cpp`):
 It runs on a plain engine: the build is compiled with `-fignore-exceptions`, so no unwinding is
 emitted and the WebAssembly exception-handling proposal is not needed. A parser that does not
 match returns `false` rather than throwing (see check 19 in
-`ci/jobs/scripts/check_style/check_cpp.sh`), and the one exception left - the syntax error itself
-- is reported through `ch_format` returning 0.
+`ci/jobs/scripts/check_style/check_cpp.sh`), and a syntax error is reported through `ch_format`
+returning 0. The handful of parser checks that still report an invalid query by throwing - `Frame
+start cannot be UNBOUNDED FOLLOWING`, for one - reach the same place through a `setjmp` boundary
+that `wasm_runtime.cpp` jumps to instead of aborting.
 
 ## What it costs
 

utils/wasm-parser/build.sh

@@ -51,6 +51,8 @@ CXXFLAGS=(
   -D_WASI_EMULATED_PROCESS_CLOCKS -D_WASI_EMULATED_GETPID
   -DCLICKHOUSE_PARSER_MINIMAL_BUILD
   -fignore-exceptions
+  # `setjmp`/`longjmp` is the error boundary around the parser - see wasm_runtime.cpp
+  -mllvm -wasm-enable-sjlj
   -fno-asynchronous-unwind-tables
   -Wno-everything
 )
@@ -156,7 +158,7 @@ echo "Linking..."
     -Wl,--no-entry -mexec-model=reactor -Wl,--strip-all \
     -Wl,--export=ch_format,--export=ch_alloc,--export=ch_free \
     -Wl,--export=ch_result_data,--export=ch_result_size \
-    "$OUT"/obj/*.o -lwasi-emulated-signal -lwasi-emulated-mman \
+    "$OUT"/obj/*.o -lsetjmp -lwasi-emulated-signal -lwasi-emulated-mman \
     -o "$OUT/parser.wasm"
 
 printf '%s: %s bytes (%s gzipped)\n' "$OUT/parser.wasm" \

utils/wasm-parser/test.mjs

@@ -35,12 +35,24 @@ const cases = [
     "GRANT SELECT(a, b) ON db.tbl TO u WITH GRANT OPTION",
     'SELECT 1 +',                     // expected to fail
     'SELCT 1',                        // expected to fail
+    // Reported by throwing from the parser, which has no unwinding here: it must come back as an
+    // error, and the module must stay usable afterwards - see __cxa_throw in wasm_runtime.cpp.
+    'SELECT sum(x) OVER (ROWS BETWEEN UNBOUNDED FOLLOWING AND CURRENT ROW) FROM t',
+    'SELECT sum(x) OVER (ROWS BETWEEN CURRENT ROW AND UNBOUNDED PRECEDING) FROM t',
+    'SELECT 1 + 2',                   // the parse after a throw must still work
 ];
 
+const expectedToFail = new Set([
+    'SELECT 1 +',
+    'SELCT 1',
+    'SELECT sum(x) OVER (ROWS BETWEEN UNBOUNDED FOLLOWING AND CURRENT ROW) FROM t',
+    'SELECT sum(x) OVER (ROWS BETWEEN CURRENT ROW AND UNBOUNDED PRECEDING) FROM t',
+]);
+
 let pass = 0;
 for (const sql of cases) {
     const r = format(sql, 1);
-    const expectFail = sql === 'SELECT 1 +' || sql === 'SELCT 1';
+    const expectFail = expectedToFail.has(sql);
     const good = expectFail ? !r.ok : r.ok;
     pass += good ? 1 : 0;
     console.log(`${good ? 'ok  ' : 'FAIL'} ${r.ok ? '' : '[error] '}${r.out.replace(/\n/g, ' ').slice(0, 110)}`);

utils/wasm-parser/wasm_parser.cpp

@@ -14,10 +14,19 @@
 #include <Parsers/ParserQuery.h>
 #include <Parsers/parseQuery.h>
 
+#include <csetjmp>
 #include <cstdint>
 #include <string>
 #include <new>
 
+extern "C"
+{
+    /// The error boundary; defined in `wasm_runtime.cpp`.
+    jmp_buf * chParserRecoveryPoint();
+    void chParserArmRecovery(bool armed);
+    const char * chParserRecoveryMessage();
+}
+
 namespace
 {
 
@@ -48,9 +57,17 @@ void ch_free(uint8_t * ptr)
 
 int ch_format(const char * query, uint32_t size, int one_line)
 {
-    /// `tryParseQuery` reports a syntax error by returning null and filling in the message; it does
-    /// not throw, and nothing in `src/Parsers` catches. So there is nothing to catch here either,
-    /// which is what lets this build with `-fno-exceptions`.
+    /// `tryParseQuery` reports a syntax error by returning null and filling in the message. A few
+    /// checks in the parser still report an invalid query by throwing, and this build has no
+    /// unwinding, so `wasm_runtime.cpp` turns such a throw into a jump back here - see the comment
+    /// on `__cxa_throw` there.
+    if (setjmp(*chParserRecoveryPoint()) != 0)
+    {
+        result() = chParserRecoveryMessage();
+        return 0;
+    }
+    chParserArmRecovery(true);
+
     std::string error;
     const char * end = query + size;
     DB::ParserQuery parser(end);
@@ -60,11 +77,15 @@ int ch_format(const char * query, uint32_t size, int one_line)
 
     if (!ast)
     {
+        chParserArmRecovery(false);
         result() = std::move(error);
         return 0;
     }
 
+    /// Formatting throws for an AST the parser accepted but cannot print, so it stays inside the
+    /// boundary too.
     result() = one_line ? ast->formatWithSecretsOneLine() : ast->formatWithSecretsMultiLine();
+    chParserArmRecovery(false);
     return 1;
 }
 

utils/wasm-parser/wasm_runtime.cpp

@@ -17,6 +17,7 @@
 #include <Common/CurrentMemoryTracker.h>
 #include <Core/Settings.h>
 
+#include <csetjmp>
 #include <cstddef>
 #include <cstdio>
 #include <cstdlib>
@@ -281,16 +282,49 @@ const SettingFieldTimezone & Settings::operator[](SettingsTimezone) const
 /// Exceptions.
 ///
 /// The build uses `-fignore-exceptions`: `throw`, `try` and `catch` still compile, but no landing
-/// pads or unwind tables are emitted, so nothing can be caught. That is sound here because an
-/// exception can only mean a bug or a resource limit - a syntax error is reported by
-/// `tryParseQuery` returning null, and `src/Parsers` contains no `catch` at all, which a style
-/// check enforces. Defining `__cxa_throw` here keeps libc++abi's exception machinery out of the
-/// bundle entirely; the exception object is constructed and then we stop.
+/// pads or unwind tables are emitted, so nothing can be caught, and defining `__cxa_throw` here
+/// keeps libc++abi's exception machinery out of the bundle entirely.
+///
+/// A syntax error does not come this way - `tryParseQuery` returns null and fills in the message,
+/// and `src/Parsers` contains no `catch` at all, which a style check enforces. But a handful of
+/// checks in the parser still report an invalid query by throwing (`Frame start cannot be
+/// UNBOUNDED FOLLOWING`, for one), and stopping the module on an ordinary user mistake is not
+/// acceptable. So `ch_format` arms a `setjmp` boundary and a throw returns to it, with the
+/// message, as a parse failure.
+///
+/// The unwinding this replaces would have run destructors; `longjmp` does not, so a throw leaks
+/// whatever the parser allocated below the boundary. The alternative is `-fwasm-exceptions`, which
+/// costs 262 KB and an engine implementing the exception-handling proposal - too much to pay for
+/// tidiness on a path that a browser hits only on an invalid query.
 /// ---------------------------------------------------------------------------------------------
 
+namespace
+{
+
+jmp_buf recovery_point;
+bool recovery_armed = false;
+std::string recovery_message;
+
+}
+
 extern "C"
 {
 
+jmp_buf * chParserRecoveryPoint()
+{
+    return &recovery_point;
+}
+
+void chParserArmRecovery(bool armed)
+{
+    recovery_armed = armed;
+}
+
+const char * chParserRecoveryMessage()
+{
+    return recovery_message.c_str();
+}
+
 void * __cxa_allocate_exception(size_t size) noexcept
 {
     static char buffer[512];
@@ -305,6 +339,15 @@ void __cxa_free_exception(void *) noexcept
 {
     /// `DB::Exception` derives from `Poco::Exception`, whose `what()` is the message.
     const auto * as_std_exception = static_cast<const std::exception *>(static_cast<const Poco::Exception *>(thrown));
+
+    if (recovery_armed)
+    {
+        /// Disarm first: assigning the message allocates, and a throw from there would loop.
+        recovery_armed = false;
+        recovery_message = as_std_exception->what();
+        longjmp(recovery_point, 1);
+    }
+
     std::fprintf(stderr, "ClickHouse parser: unrecoverable error: %s\n", as_std_exception->what());
     std::abort();
 }