ReviewOS
Reload

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
packages/zig/src/database.zigmodified+77-62
@@ -5,8 +5,6 @@ const std = ;
55/// and migration support. Uses real SQLite on all platforms — the vendored
66/// amalgamation (vendor/sqlite/sqlite3.c) is compiled by Zig's C compiler,
77/// so no system SQLite dependency is required.
8const builtin = ;
9
108const sqlite3 = opaque {};
119const sqlite3_stmt = opaque {};
1210const sqlite3_backup = opaque {};
@@ -60,64 +58,33 @@ extern fn sqlite3_backup_init(destDb: ?*sqlite3, destName: [*:0]const u8, source
6058extern fn sqlite3_backup_step(backup: ?*sqlite3_backup, pages: c_int) c_int;
6159extern fn sqlite3_backup_finish(backup: ?*sqlite3_backup) c_int;
6260
63// SQLITE_TRANSIENT tells SQLite to make its own copy of the bound data.
64// The C definition is ((sqlite3_destructor_type)-1), i.e. a function pointer with value -1.
65// We call SQLite through thin asm stubs that pass the correct destructor
66// value (-1), avoiding a function-pointer cast in Zig source.
67extern fn sqlite3_bind_text_transient(stmt: ?*sqlite3_stmt, idx: c_int, text: [*]const u8, len: c_int) c_int;
68extern fn sqlite3_bind_blob_transient(stmt: ?*sqlite3_stmt, idx: c_int, blob: [*]const u8, len: c_int) c_int;
69
70comptime {
71 // Asm stubs that tail-call sqlite3_bind_text/sqlite3_bind_blob with the 5th argument
72 // (destructor) set to -1 (SQLITE_TRANSIENT). Platform-specific calling conventions:
73 // arm64: 5th arg in x4
74 // x86_64 SysV: 5th arg in r8 (Linux)
75 // x86_64 Win64: 5th arg on stack (Windows uses 4-register fastcall)
76 if (builtin.cpu.arch == .aarch64) {
77 asm (
78 \\.globl _sqlite3_bind_text_transient
79 \\_sqlite3_bind_text_transient:
80 \\ mov x4, #-1
81 \\ b _sqlite3_bind_text
82 );
83 asm (
84 \\.globl _sqlite3_bind_blob_transient
85 \\_sqlite3_bind_blob_transient:
86 \\ mov x4, #-1
87 \\ b _sqlite3_bind_blob
88 );
89 } else if (builtin.cpu.arch == .x86_64) {
90 if (builtin.os.tag == .windows) {
91 // Win64 ABI: first 4 args in rcx,rdx,r8,r9; 5th arg at [rsp+40]
92 asm (
93 \\.globl sqlite3_bind_text_transient
94 \\sqlite3_bind_text_transient:
95 \\ mov qword ptr [rsp+32], -1
96 \\ jmp sqlite3_bind_text
97 );
98 asm (
99 \\.globl sqlite3_bind_blob_transient
100 \\sqlite3_bind_blob_transient:
101 \\ mov qword ptr [rsp+32], -1
102 \\ jmp sqlite3_bind_blob
103 );
104 } else {
105 // SysV ABI (Linux/macOS): first 6 args in rdi,rsi,rdx,rcx,r8,r9
106 asm (
107 \\.globl sqlite3_bind_text_transient
108 \\sqlite3_bind_text_transient:
109 \\ mov r8, -1
110 \\ jmp sqlite3_bind_text
111 );
112 asm (
113 \\.globl sqlite3_bind_blob_transient
114 \\sqlite3_bind_blob_transient:
115 \\ mov r8, -1
116 \\ jmp sqlite3_bind_blob
117 );
118 }
119 }
120}
61// SQLITE_TRANSIENT is `((sqlite3_destructor_type)-1)`: a destructor pointer
62// whose bits are all ones, which tells SQLite to copy the bound data rather
63// than borrow it. `bindText`/`bindBlob` hand out slices the caller may free
64// the moment they return, so it is not optional.
65//
66// This used to be six hand-written module-level asm stubs — one pair per
67// calling convention — that tail-called sqlite3_bind_text/blob with the fifth
68// argument preset to -1, so that Zig never had to name a function pointer with
69// an impossible value. Both halves of that were broken on x86_64 Linux:
70//
71// - Zig's self-hosted x86_64 backend, which is the default for Debug builds,
72// drops module-level `asm` on the floor. It emits no symbol and no
73// diagnostic, so the link failed with `undefined symbol:
74// sqlite3_bind_text_transient` and every Linux build of the database tests
75// died there.
76// - The stubs were written in Intel syntax, which the LLVM backend's
77// assembler rejects outright on x86 ELF ("unknown use of instruction
78// mnemonic without a size suffix"). So `-fllvm` was not an escape either.
79//
80// None of it was necessary. The destructor is declared as an opaque pointer
81// instead of a typed function pointer — identical ABI, and it sidesteps the
82// alignment requirement a `*const fn` carries, which an all-ones address
83// cannot satisfy.
84const SQLITE_TRANSIENT: ?*const anyopaque = @ptrFromInt(std.math.maxInt(usize));
85
86extern fn sqlite3_bind_text(stmt: ?*sqlite3_stmt, idx: c_int, text: [*]const u8, len: c_int, destructor: ?*const anyopaque) c_int;
87extern fn sqlite3_bind_blob(stmt: ?*sqlite3_stmt, idx: c_int, blob: [*]const u8, len: c_int, destructor: ?*const anyopaque) c_int;
12188
12289pub const DatabaseError = error{
12390 ConnectionFailed,
@@ -386,11 +353,12 @@ pub const Statement = struct {
386353 try self.ensureParamCapacity(index);
387354 self.bound_params.items[index - 1] = .{ .text = value };
388355 if (self.stmt_handle) |handle| {
389 const rc = sqlite3_bind_text_transient(
356 const rc = sqlite3_bind_text(
390357 handle,
391358 @intCast(index),
392359 value.ptr,
393360 @intCast(value.len),
361 SQLITE_TRANSIENT,
394362 );
395363 if (rc != SQLITE_OK) return DatabaseError.BindFailed;
396364 }
@@ -401,11 +369,12 @@ pub const Statement = struct {
401369 try self.ensureParamCapacity(index);
402370 self.bound_params.items[index - 1] = .{ .blob = value };
403371 if (self.stmt_handle) |handle| {
404 const rc = sqlite3_bind_blob_transient(
372 const rc = sqlite3_bind_blob(
405373 handle,
406374 @intCast(index),
407375 value.ptr,
408376 @intCast(value.len),
377 SQLITE_TRANSIENT,
409378 );
410379 if (rc != SQLITE_OK) return DatabaseError.BindFailed;
411380 }
@@ -1394,6 +1363,52 @@ test "prepared statement binding" {
13941363 try std.testing.expectEqual(@as(f64, 95.5), stmt.bound_params.items[2].real);
13951364}
13961365
1366test "bound text and blob are copied at bind time, not borrowed" {
1367 // This is what SQLITE_TRANSIENT buys: `bindText` and `bindBlob` take a
1368 // slice the caller owns and may reuse the moment they return. Both buffers
1369 // here are overwritten between bind and step, so a borrowing destructor
1370 // (SQLITE_STATIC, i.e. a null pointer) would store the overwritten bytes
1371 // instead. Nothing else in this file reads back what SQLite actually
1372 // stored — the other binding test asserts only against our own mirror of
1373 // the parameters — so this is the only check on the destructor value.
1374 const allocator = std.testing.allocator;
1375
1376 var db = try Database.init(allocator, .{ .path = ":memory:" });
1377 defer db.deinit();
1378
1379 try db.executeRaw("CREATE TABLE t (name TEXT, data BLOB)");
1380
1381 var name = "Alice".*;
1382 var data = [_]u8{ 0xDE, 0xAD, 0xBE, 0xEF };
1383
1384 {
1385 var stmt = try db.prepare("INSERT INTO t (name, data) VALUES (?, ?)");
1386 defer {
1387 stmt.deinit();
1388 allocator.destroy(stmt);
1389 }
1390 try stmt.bindText(1, &name);
1391 try stmt.bindBlob(2, &data);
1392 name = "Zzzzz".*;
1393 data = @splat(0);
1394 try stmt.execute();
1395 }
1396
1397 // hex() rather than a blob getter, which Row does not expose.
1398 const rows = try db.query("SELECT name, hex(data) AS data FROM t", .{});
1399 defer {
1400 for (rows) |*row| {
1401 var r = row.*;
1402 r.deinit();
1403 }
1404 allocator.free(rows);
1405 }
1406
1407 try std.testing.expectEqual(@as(usize, 1), rows.len);
1408 try std.testing.expectEqualStrings("Alice", rows[0].getText("name").?);
1409 try std.testing.expectEqualStrings("DEADBEEF", rows[0].getText("data").?);
1410}
1411
13971412test "prepared statement reset" {
13981413 const allocator = std.testing.allocator;
13991414