ReviewOS

also looking at this

craft-native/craft

fix(db): bind SQLITE_TRANSIENT directly instead of through asm stubs

#38
Merged glennmichael123 wants to merge fix/sqlite-transient-without-asm into main
1 file +77 -62
11 days ago

Found while checking CI for the menu PRs. Every Linux build of the database tests has been failing to link:

error: undefined symbol: sqlite3_bind_text_transient
    note: referenced by database.o:database.Statement.bindText

It is one of the two things keeping the Benchmark job red on main (the other is zig-utils/zig-js#625).

What was there

bindText/bindBlob reached SQLite through six hand-written module-level asm stubs — one pair per calling convention — that tail-called sqlite3_bind_text/_blob with the fifth argument preset to -1, so Zig would never have to name a function pointer with an impossible value.

Both halves of that are broken on x86_64. I checked each against a three-line reproduction rather than inferring it from the failure:

extern fn helper_stub() c_int;
comptime { asm (
    \\.globl helper_stub
    \\helper_stub:
    \\  mov eax, 7
    \\  ret
); }
backendresult
self-hosted x86_64 (the Debug default)error: undefined symbol: helper_stub — the asm is dropped, no symbol and no diagnostic
LLVM (-fllvm)unknown use of instruction mnemonic without a size suffix — the stubs are Intel syntax, and x86 ELF wants AT&T

So the definition was sitting right there in the file and the linker could not find it, and forcing LLVM would not have helped either. aarch64-macOS was fine, which is why nobody local ever saw it.

What replaces it

None of the assembly was necessary:

const SQLITE_TRANSIENT: ?*const anyopaque = @ptrFromInt(std.math.maxInt(usize));

extern fn sqlite3_bind_text(stmt: ?*sqlite3_stmt, idx: c_int, text: [*]const u8, len: c_int, destructor: ?*const anyopaque) c_int;
extern fn sqlite3_bind_blob(stmt: ?*sqlite3_stmt, idx: c_int, blob: [*]const u8, len: c_int, destructor: ?*const anyopaque) c_int;

The destructor is an opaque pointer rather than a typed function pointer — identical ABI, and it sidesteps the alignment requirement *const fn carries, which an all-ones address cannot satisfy. 62 lines of platform-specific assembly become two declarations and a constant.

The test that was missing

prepared statement binding asserts only against our own mirror of the parameters and never steps the statement, so nothing in this file read back what SQLite actually stored. A wrong destructor value was invisible.

The new test overwrites both the text and the blob buffer between bind and step, then reads the row back. That is the case SQLITE_TRANSIENT exists for: a borrowing destructor reads the caller's memory at step time and would store the overwritten bytes.

I confirmed it discriminates by setting the destructor to null (SQLITE_STATIC) and watching it fail — 18 pass / 1 fail — rather than by trusting a green run.

Verification

On the pinned CI compiler (0.17.0-dev.1509+bb296ab9b):

  • zig build test:database -Dtarget=x86_64-linux-gnu — links, where before it was undefined symbol. It then stops at "the host system is unable to execute binaries from the target", which is the expected macOS-can't-run-Linux error and not a compile failure.
  • zig build test:database natively — 19 tests pass
  • zig build test — full suite green
  • zig fmt --check clean

1 changed file on the files tab, with 0 review threads.

Sign in to comment on this pull request.