Hello,
I work on pgrx, a Rust crate (really, a set of them) that allows
people to use Rust to write extensions against Postgres, exporting
what Postgres sees as ordinary "C" dynamic libraries. Unfortunately,
the build system for this is a touch complicated, as it cannot simply
run pgxs.mk, and as-of Postgres 16 it has been periodically failing on
platforms it used to do fine on, due to troubles involved with the
SIMD extension headers.
I have root-caused the exact problem, but the bug is... social, rather
than technical in nature: people with inadequate options at their
disposal have implemented increasingly clever educated guesses that
are increasingly prone to going wrong, rather than just asking anyone
to help them increase their options. Rather than continuing this
trend, I figured I would simply start doing things to hopefully draw
the line here. I will be looking to follow up with the bindgen tools
that fail to handle this correctly, but it would be nice if this
stopped shipping in Postgres 16."${PG_NEXT_MINOR}", as pgrx does need
the definitions in pg_wchar.h to have enough data to correctly
determine database encoding and preserve certain Rust library
invariants ("all Rust strings are correctly-formed UTF-8, anything
else is just a sequence of bytes") without also obliterating
performance.
On the off-chance that everyone agrees with me without reserve, the
attached patch moves the relevant code around (and includes the gory
details). This seems to be unlikely to be the only mildly-exotic build
system failure caused by such an overexposed implementation detail, so
while I'm not married to this particular code motion, it seems best to
improve this some way.
From f23b3675e38d32e7d52450573237f206286a9d61 Mon Sep 17 00:00:00 2001
From: Jubilee Young <workingjubi...@gmail.com>
Date: Mon, 13 Nov 2023 11:59:27 -0800
Subject: [PATCH] Keep simd.h out of unrelated headers
The PGRX ("PostGres Rust eXtensions") build system has trouble inferring
where the appropriate compiler headers would be, due to the Clang-C APIs
it relies on not exposing an API to locate them. Because of this, it uses
an educated guess. When multiple clang installations are on the system,
this guess has a possibility of being wrong in disastrous ways.
Compiler builtin headers cannot be meaningfully "swapped" between
compiler versions, and we are lucky if they only type error on build.
Because of this, it is useful if pg_wchar.h does not expose simd.h details
to satisfy only one caller in wchar.c, as the header is still required by
PGRX (and many other users besides) in order to maintain Rust invariants.
Move is_valid_ascii into w_char.c in order to keep these implementation
details private to Postgres, and pray that other upstreams improve their
tooling so this problem doesn't trouble Postgres 17.
---
src/common/wchar.c | 68 ++++++++++++++++++++++++++++++++++++++
src/include/mb/pg_wchar.h | 69 ---------------------------------------
2 files changed, 68 insertions(+), 69 deletions(-)
diff --git a/src/common/wchar.c b/src/common/wchar.c
index fb9d9f5c85..ea3c327fa5 100644
--- a/src/common/wchar.c
+++ b/src/common/wchar.c
@@ -14,6 +14,8 @@
#include "mb/pg_wchar.h"
+#include "port/simd.h"
+
/*
* Operations on multi-byte encodings are driven by a table of helper
@@ -1911,6 +1913,72 @@ utf8_advance(const unsigned char *s, uint32 *state, int len)
*state &= 31;
}
+/*
+ * Verify a chunk of bytes for valid ASCII.
+ *
+ * Returns false if the input contains any zero bytes or bytes with the
+ * high-bit set. Input len must be a multiple of the chunk size (8 or 16).
+ */
+static inline bool
+is_valid_ascii(const unsigned char *s, int len)
+{
+ const unsigned char *const s_end = s + len;
+ Vector8 chunk;
+ Vector8 highbit_acc = vector8_broadcast(0);
+#ifdef USE_NO_SIMD
+ Vector8 zero_acc = vector8_broadcast(0x80);
+#endif
+
+ Assert(len % sizeof(chunk) == 0);
+
+ while (s < s_end)
+ {
+ vector8_load(&chunk, s);
+
+ /* Capture any zero bytes in this chunk. */
+#ifdef USE_NO_SIMD
+
+ /*
+ * First, add 0x7f to each byte. This sets the high bit in each byte,
+ * unless it was a zero. If any resulting high bits are zero, the
+ * corresponding high bits in the zero accumulator will be cleared.
+ *
+ * If none of the bytes in the chunk had the high bit set, the max
+ * value each byte can have after the addition is 0x7f + 0x7f = 0xfe,
+ * and we don't need to worry about carrying over to the next byte. If
+ * any input bytes did have the high bit set, it doesn't matter
+ * because we check for those separately.
+ */
+ zero_acc &= (chunk + vector8_broadcast(0x7F));
+#else
+
+ /*
+ * Set all bits in each lane of the highbit accumulator where input
+ * bytes are zero.
+ */
+ highbit_acc = vector8_or(highbit_acc,
+ vector8_eq(chunk, vector8_broadcast(0)));
+#endif
+
+ /* Capture all set bits in this chunk. */
+ highbit_acc = vector8_or(highbit_acc, chunk);
+
+ s += sizeof(chunk);
+ }
+
+ /* Check if any high bits in the high bit accumulator got set. */
+ if (vector8_is_highbit_set(highbit_acc))
+ return false;
+
+#ifdef USE_NO_SIMD
+ /* Check if any high bits in the zero accumulator got cleared. */
+ if (zero_acc != vector8_broadcast(0x80))
+ return false;
+#endif
+
+ return true;
+}
+
static int
pg_utf8_verifystr(const unsigned char *s, int len)
{
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index 29cd5732f1..80676d9e02 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -22,8 +22,6 @@
#ifndef PG_WCHAR_H
#define PG_WCHAR_H
-#include "port/simd.h"
-
/*
* The pg_wchar type
*/
@@ -722,71 +720,4 @@ extern int mic2latin_with_table(const unsigned char *mic, unsigned char *p,
extern WCHAR *pgwin32_message_to_UTF16(const char *str, int len, int *utf16len);
#endif
-
-/*
- * Verify a chunk of bytes for valid ASCII.
- *
- * Returns false if the input contains any zero bytes or bytes with the
- * high-bit set. Input len must be a multiple of the chunk size (8 or 16).
- */
-static inline bool
-is_valid_ascii(const unsigned char *s, int len)
-{
- const unsigned char *const s_end = s + len;
- Vector8 chunk;
- Vector8 highbit_cum = vector8_broadcast(0);
-#ifdef USE_NO_SIMD
- Vector8 zero_cum = vector8_broadcast(0x80);
-#endif
-
- Assert(len % sizeof(chunk) == 0);
-
- while (s < s_end)
- {
- vector8_load(&chunk, s);
-
- /* Capture any zero bytes in this chunk. */
-#ifdef USE_NO_SIMD
-
- /*
- * First, add 0x7f to each byte. This sets the high bit in each byte,
- * unless it was a zero. If any resulting high bits are zero, the
- * corresponding high bits in the zero accumulator will be cleared.
- *
- * If none of the bytes in the chunk had the high bit set, the max
- * value each byte can have after the addition is 0x7f + 0x7f = 0xfe,
- * and we don't need to worry about carrying over to the next byte. If
- * any input bytes did have the high bit set, it doesn't matter
- * because we check for those separately.
- */
- zero_cum &= (chunk + vector8_broadcast(0x7F));
-#else
-
- /*
- * Set all bits in each lane of the highbit accumulator where input
- * bytes are zero.
- */
- highbit_cum = vector8_or(highbit_cum,
- vector8_eq(chunk, vector8_broadcast(0)));
-#endif
-
- /* Capture all set bits in this chunk. */
- highbit_cum = vector8_or(highbit_cum, chunk);
-
- s += sizeof(chunk);
- }
-
- /* Check if any high bits in the high bit accumulator got set. */
- if (vector8_is_highbit_set(highbit_cum))
- return false;
-
-#ifdef USE_NO_SIMD
- /* Check if any high bits in the zero accumulator got cleared. */
- if (zero_cum != vector8_broadcast(0x80))
- return false;
-#endif
-
- return true;
-}
-
#endif /* PG_WCHAR_H */
--
2.42.1
