Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions scripts/build/deps/tinycc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
* TinyCC — small embeddable C compiler. Powers bun:ffi's JIT-compile path,
* where user-provided C gets compiled and linked at runtime.
*
* Disabled on windows-arm64 (tinycc doesn't have an arm64-coff backend).
* Disabled on windows-arm64 (upstream tinycc now has an arm64-pe backend,
* but bun:ffi has never been enabled there — see cfg.tinycc in config.ts).

Check failure on line 6 in scripts/build/deps/tinycc.ts

View check run for this annotation

Claude / Claude Code Review

Windows ARM64 enablement claimed in title/description but absent from the diff

The PR title, description (section 2), and the 18:37 follow-up comment all say this push enables `bun:ffi` on Windows ARM64, but that commit isn't in the diff — only 4 files are touched, `config.ts:869` still gates tinycc off for `(windows && arm64)`, `cc.test.ts:8` still sets `isFFIUnavailable = isWindows && isArm64`, and this very comment is being *updated* to say "Disabled on windows-arm64 … bun:ffi has never been enabled there". Looks like the second commit was never pushed (or got dropped)
Comment on lines +5 to +6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The PR title, description (section 2), and the 18:37 follow-up comment all say this push enables bun:ffi on Windows ARM64, but that commit isn't in the diff — only 4 files are touched, config.ts:869 still gates tinycc off for (windows && arm64), cc.test.ts:8 still sets isFFIUnavailable = isWindows && isArm64, and this very comment is being updated to say "Disabled on windows-arm64 … bun:ffi has never been enabled there". Looks like the second commit was never pushed (or got dropped) — either push it or drop the claim from the title/description before merge.

Extended reasoning...

What the discrepancy is

The PR title is "Update TinyCC to latest upstream; enable bun:ffi on Windows ARM64", the description's section 2 describes concrete enablement changes ("Drops the windows-arm64 exclusion from cfg.tinycc, the generated ENABLE_TINYCC constant, and the tcc_sys link stubs, and un-skips the FFI tests…"), and Jarred's own follow-up comment at 2026-07-07T18:37 says "the current push also enables bun:ffi on Windows ARM64". CodeRabbit's walkthrough likewise lists scripts/build/config.ts, scripts/build/buildOptionsRs.ts, src/tcc_sys/tcc.rs, ffi.test.js, ffi-error-messages.test.ts, cp.test.ts, fs-writeSync-stdio-windows.test.ts, and napi-value-ffi.test.ts as changed. None of that is in the diff being reviewed.

What's actually in the diff

The changed-files list contains exactly four files: scripts/build/deps/tinycc.ts, src/runtime/ffi/libtcc1.c, test/js/bun/ffi/cc.test.ts, and test/js/node/process/process.test.js. git log on the branch shows only one commit (0bf5d067 "Update TinyCC to upstream tip"). Checked against the repo at HEAD:

  • scripts/build/config.ts:869 still reads const tinycc = partial.tinycc ?? !((windows && arm64) || abi === "android" || freebsd); — the windows-arm64 exclusion is intact.
  • test/js/bun/ffi/cc.test.ts:8 still has const isFFIUnavailable = isWindows && isArm64;.
  • No changes to buildOptionsRs.ts, src/tcc_sys/tcc.rs, ffi.test.js, ffi-error-messages.test.ts, ffi-viewSource-non-object.test.ts, cp.test.ts, fs-writeSync-stdio-windows.test.ts, or napi-value-ffi.test.ts.

Most tellingly, the one hunk this PR does apply to tinycc.ts:5-6 updates the doc comment to say "Disabled on windows-arm64 (upstream tinycc now has an arm64-pe backend, but bun:ffi has never been enabled there — see cfg.tinycc in config.ts)" — i.e. the change itself documents that it remains disabled. That's internally consistent with a first-commit-only push and directly contradicts the title.

Why this matters

This is not merely a stale description. The 18:37 comment explicitly says "the current push also enables bun:ffi on Windows ARM64", so the author believes the enablement commit is on the branch. It isn't — the second commit appears to have been lost in a rebase, force-push, or was authored but never pushed. Merging as-is would (a) record in git history that Windows ARM64 FFI was enabled when the code path is still compiled out, and (b) silently drop work the author intended to ship.

Step-by-step proof

  1. PR diff → 4 files; none is config.ts, buildOptionsRs.ts, tcc.rs, or any of the un-skipped test files.
  2. git log on HEAD → only 0bf5d067 "Update TinyCC to upstream tip"; no second commit.
  3. scripts/build/config.ts:869!((windows && arm64) || …) still excludes windows-arm64.
  4. scripts/build/deps/tinycc.ts:5-6 in the diff → new comment literally says "Disabled on windows-arm64 … bun:ffi has never been enabled there".
  5. test/js/bun/ffi/cc.test.ts:8isFFIUnavailable = isWindows && isArm64 unchanged.
  6. Therefore: on a windows-arm64 build with this PR applied, cfg.tinycc is still false, TinyCC is not built, and every bun:ffi test is still skipped — the PR does not do what the title claims.

Fix

Either push the missing second commit (the one touching config.ts / buildOptionsRs.ts / tcc_sys/tcc.rs / the FFI + fs test skips), or — if it's intentionally being deferred to a follow-up — retitle the PR and drop section 2 from the description so the merge commit accurately reflects what shipped.

*
* Built via DirectBuild — no cmake sub-process. The old overlay
* CMakeLists.txt had two recurring ASAN workarounds for the c2str host
Expand All @@ -12,7 +13,7 @@

import type { Dependency, DirectBuild } from "../source.ts";

const TINYCC_COMMIT = "12882eee073cfe5c7621bcfadf679e1372d4537b";
const TINYCC_COMMIT = "c22ca054d270c9e25630590fe0e2c867793ab82d";

export const tinycc: Dependency = {
name: "tinycc",
Expand Down
53 changes: 53 additions & 0 deletions src/runtime/ffi/libtcc1.c
Original file line number Diff line number Diff line change
Expand Up @@ -604,3 +604,56 @@ unsigned long long __fixunsxfdi (long double a1)
else
return 0;
}

/* TinyCC's lib/va_list.c: __va_arg is no longer inlined by tccdefs.h, and Bun
replaces libtcc1 with this file. Only referenced by x86_64 SysV codegen.
Deviation from upstream: no extern abort() — Bun never injects that symbol,
so referencing it would make every cc() fail with an unresolved reference. */
Comment on lines +608 to +611

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Comment exceeds 3-line guideline.

This block comment spans 4 lines. As per coding guidelines: "Keep code comments to 3 lines max - Comments must be concise. If the code needs more explanation than that, it belongs in docs."

✏️ Proposed trim
-/* TinyCC's lib/va_list.c: __va_arg is no longer inlined by tccdefs.h, and Bun
-   replaces libtcc1 with this file. Only referenced by x86_64 SysV codegen.
-   Deviation from upstream: no extern abort() — Bun never injects that symbol,
-   so referencing it would make every cc() fail with an unresolved reference. */
+/* TinyCC's lib/va_list.c, referenced only by x86_64 SysV codegen. Deviation
+   from upstream: no extern abort() (Bun never injects that symbol, so any
+   reference would make every cc() fail to link). */
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/* TinyCC's lib/va_list.c: __va_arg is no longer inlined by tccdefs.h, and Bun
replaces libtcc1 with this file. Only referenced by x86_64 SysV codegen.
Deviation from upstream: no extern abort() — Bun never injects that symbol,
so referencing it would make every cc() fail with an unresolved reference. */
/* TinyCC's lib/va_list.c, referenced only by x86_64 SysV codegen. Deviation
from upstream: no extern abort() (Bun never injects that symbol, so any
reference would make every cc() fail to link). */
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/ffi/libtcc1.c` around lines 608 - 611, The block comment in
libtcc1.c is too long and needs to be trimmed to 3 lines or fewer. Condense the
explanation around __va_arg, Bun’s replacement of libtcc1, and the
no-extern-abort deviation into a shorter comment near the existing
libtcc1/va_arg logic, keeping only the essential context and moving any extra
detail to docs if needed.

Source: Coding guidelines

#if defined(__x86_64__) && !defined(_WIN32)

enum __va_arg_type {
__va_gen_reg, __va_float_reg, __va_stack
};

void *__va_arg(__builtin_va_list ap,
int arg_type,
int size, int align)
{
size = (size + 7) & ~7;
align = (align + 7) & ~7;
switch ((enum __va_arg_type)arg_type) {
case __va_gen_reg:
if (ap->gp_offset + size <= 48) {
ap->gp_offset += size;
return ap->reg_save_area + ap->gp_offset - size;
}
goto use_overflow_area;

case __va_float_reg:
if (ap->fp_offset < 128 + 48) {
ap->fp_offset += 16;
if (size == 8)
return ap->reg_save_area + ap->fp_offset - 16;
if (ap->fp_offset < 128 + 48) {
double *p = (double *)(ap->reg_save_area + ap->fp_offset);
p[-1] = p[0];
ap->fp_offset += 16;
return ap->reg_save_area + ap->fp_offset - 32;
}
}
goto use_overflow_area;

case __va_stack:
use_overflow_area:
ap->overflow_arg_area += size;
ap->overflow_arg_area = (char*)((long long)(ap->overflow_arg_area + align - 1) & -align);
return ap->overflow_arg_area - size;

default:
/* unreachable: the compiler only emits the three classes above.
Trap with a null write like TinyCC's old inline __va_arg did. */
*(volatile char *)0 = 0;
return 0;
}
}
#endif /* __x86_64__ && !_WIN32 */
183 changes: 183 additions & 0 deletions test/js/bun/ffi/cc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,189 @@ describe.skipIf(isASAN || isFFIUnavailable)("GC liveness of compiled symbols and
});
});

// va_arg on x86_64 SysV lowers to a call to __va_arg, which TinyCC expects
// libtcc1 to provide; Bun replaces libtcc1 with src/runtime/ffi/libtcc1.c.
// TinyCC's setjmp/longjmp error handling conflicts with ASan.
describe.skipIf(isASAN || isFFIUnavailable)("variadic functions inside cc()-compiled C", () => {
it("va_arg over ints, doubles, and the stack overflow area", async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider it.concurrent for subprocess-spawning tests.

These tests spawn a Bun.spawn subprocess and are independent of each other (unique temp dirs). As per path instructions: "Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it's very difficult to make them concurrent."

Also applies to: 549-556, 579-579, 607-614

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/bun/ffi/cc.test.ts` at line 464, The subprocess-spawning tests in
cc.test.ts should be made concurrent instead of sequential. Update the affected
`it(...)` cases (including the `va_arg over ints, doubles, and the stack
overflow area` test and the other listed `it` blocks) to use `it.concurrent` or
group them under `describe.concurrent`, since they are independent and each uses
its own temp directory. Keep the test bodies unchanged and only switch the test
declarations to the concurrent variant.

Source: Path instructions

using dir = tempDir("bun-ffi-cc-varargs", {
"varargs.c": /* c */ `
#include <stdarg.h>

static long long sum_ints(int count, ...) {
va_list ap;
va_start(ap, count);
long long total = 0;
for (int i = 0; i < count; i++) total += va_arg(ap, int);
va_end(ap);
return total;
}

static double sum_doubles(int count, ...) {
va_list ap;
va_start(ap, count);
double total = 0;
for (int i = 0; i < count; i++) total += va_arg(ap, double);
va_end(ap);
return total;
}

/* alternating int/double reads from one va_list: gp_offset and
fp_offset must advance independently */
static double sum_pairs(int count, ...) {
va_list ap;
va_start(ap, count);
double total = 0;
for (int i = 0; i < count; i++) {
total += va_arg(ap, int);
total += va_arg(ap, double);
}
va_end(ap);
return total;
}

/* a 16-byte all-double struct occupies two SSE register save slots */
struct dd { double a, b; };
static double sum_dd(int count, ...) {
va_list ap;
va_start(ap, count);
double total = 0;
for (int i = 0; i < count; i++) {
struct dd v = va_arg(ap, struct dd);
total += v.a + v.b;
}
va_end(ap);
return total;
}

/* 10 ints: exhausts the 6 integer registers and spills to the stack. */
long long ten_ints(void) { return sum_ints(10, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10); }
/* 10 doubles: exhausts the 8 SSE registers and spills to the stack. */
double ten_doubles(void) { return sum_doubles(10, 0.5, 1.5, 2.5, 3.5, 4.5, 5.5, 6.5, 7.5, 8.5, 9.5); }
double interleaved(void) { return sum_pairs(9, 1,0.5, 2,0.5, 3,0.5, 4,0.5, 5,0.5, 6,0.5, 7,0.5, 8,0.5, 9,0.5); }
double double_pairs(void) {
struct dd x = { 1.5, 2.5 }, y = { 3.0, 4.0 };
return sum_dd(2, x, y);
}
`,
"fixture.js": /* js */ `
import { cc } from "bun:ffi";
import path from "path";

const { symbols } = cc({
source: path.join(import.meta.dir, "varargs.c"),
symbols: {
ten_ints: { args: [], returns: "i64" },
ten_doubles: { args: [], returns: "f64" },
interleaved: { args: [], returns: "f64" },
double_pairs: { args: [], returns: "f64" },
},
});
console.log(
JSON.stringify({
ten_ints: Number(symbols.ten_ints()),
ten_doubles: symbols.ten_doubles(),
interleaved: symbols.interleaved(),
double_pairs: symbols.double_pairs(),
}),
);
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "fixture.js"],
env: bunEnv,
cwd: String(dir),
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

// stderr is included in the received object so failures show it, but is not
// asserted empty: debug builds emit benign startup warnings.
const results = stdout.startsWith("{") ? JSON.parse(stdout) : stdout;
expect({ results, stderr, exitCode }).toMatchObject({
results: {
ten_ints: 55,
ten_doubles: 50,
interleaved: 49.5,
double_pairs: 11,
},
exitCode: 0,
});
});
});

// long double is 16 bytes on x86_64 and always va_arg'd through the stack; on
// aarch64 it is binary128 and its arithmetic needs soft-float helpers
// (__addtf3, ...) that Bun's TCC states do not provide, so x64 only.
describe.skipIf(isASAN || isFFIUnavailable || process.arch !== "x64")(
"long double varargs inside cc()-compiled C",
() => {
it("va_arg over long double", async () => {
using dir = tempDir("bun-ffi-cc-varargs-ld", {
"ld.c": /* c */ `
#include <stdarg.h>

static double sum_long_doubles(int count, ...) {
va_list ap;
va_start(ap, count);
long double total = 0;
for (int i = 0; i < count; i++) total += va_arg(ap, long double);
va_end(ap);
return (double)total;
}

double long_doubles(void) { return sum_long_doubles(3, 1.5L, 2.25L, 3.25L); }
`,
"fixture.js": /* js */ `
import { cc } from "bun:ffi";
import path from "path";

const { symbols } = cc({
source: path.join(import.meta.dir, "ld.c"),
symbols: { long_doubles: { args: [], returns: "f64" } },
});
console.log(JSON.stringify({ long_doubles: symbols.long_doubles() }));
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "fixture.js"],
env: bunEnv,
cwd: String(dir),
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

const results = stdout.startsWith("{") ? JSON.parse(stdout) : stdout;
expect({ results, stderr, exitCode }).toMatchObject({
results: { long_doubles: 7 },
exitCode: 0,
});
});
},
);

// TinyCC compiles thread-local variables to Local-Exec TLS, which has no
// meaning inside an in-memory relocation (there is no PT_TLS segment): the
// generated loads/stores alias the host process's own thread block. TinyCC
// must reject it instead of silently corrupting Bun's thread-locals.
describe.skipIf(isASAN || isFFIUnavailable)("thread-local storage inside cc()-compiled C", () => {
it.each(["_Thread_local", "__thread"])("%s is a compile error", keyword => {
const dir = tempDirWithFiles(`bun-ffi-cc-tls`, {
"tls.c": `${keyword} int bun_test_tls_counter = 0;\nint bump(void) { return ++bun_test_tls_counter; }\n`,
});
expect(() => {
cc({
source: path.join(dir, "tls.c"),
symbols: { bump: { args: [], returns: "int" } },
});
}).toThrow(/thread-local storage is not supported/);
});
});

describe.skipIf(isFFIUnavailable)("double <-> JSValue conversions", () => {
// JSC NaN-boxes doubles, so a NaN whose payload collides with the tag space
// ("impure NaN", see JSC's PureNaN.h) must never be encoded as-is: it would
Expand Down
2 changes: 1 addition & 1 deletion test/js/node/process/process.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ it("process.versions", () => {
mimalloc: "afb41757285694f832e7a2f164d35f5717457f96",
picohttpparser: "066d2b1e9ab820703db0837a7255d92d30f0c9f5",
zlib: "12731092979c6d07f42da27da673a9f6c7b13586",
tinycc: "12882eee073cfe5c7621bcfadf679e1372d4537b",
tinycc: "c22ca054d270c9e25630590fe0e2c867793ab82d",
lolhtml: "77127cd2b8545998756e8d64e36ee2313c4bb312",
ares: "3ac47ee46edd8ea40370222f91613fc16c434853",
libdeflate: "c8c56a20f8f621e6a966b716b31f1dedab6a41e3",
Expand Down
Loading