diff --git a/CLAUDE.md b/CLAUDE.md index 72abf10f..0e3d28a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,16 +124,19 @@ This project uses a **copying garbage collector**. ANY JS allocation (`JS_NewObj JS_FRAME(js); JS_ROOT(obj, JS_NewObject(js)); JS_SetPropertyStr(js, obj.val, "x", JS_NewInt32(js, 42)); -JS_SetPropertyStr(js, obj.val, "name", JS_NewString(js, "hello")); +JSValue name = JS_NewString(js, "hello"); +JS_SetPropertyStr(js, obj.val, "name", name); JS_RETURN(obj.val); ``` -**Pattern — array with loop:** +**Pattern — array with loop (declare root BEFORE the loop):** ```c JS_FRAME(js); JS_ROOT(arr, JS_NewArray(js)); +JSGCRef item = { .val = JS_NULL, .prev = NULL }; +JS_PushGCRef(js, &item); for (int i = 0; i < count; i++) { - JS_ROOT(item, JS_NewObject(js)); + item.val = JS_NewObject(js); JS_SetPropertyStr(js, item.val, "v", JS_NewInt32(js, i)); JS_SetPropertyNumber(js, arr.val, i, item.val); } @@ -142,18 +145,28 @@ JS_RETURN(arr.val); **Rules:** - Access rooted values via `.val` (e.g., `obj.val`, not `obj`) +- NEVER put `JS_ROOT` inside a loop — it pushes the same stack address twice, corrupting the GC chain - Error returns before `JS_FRAME` use plain `return` - Error returns after `JS_FRAME` must use `JS_RETURN_EX()` or `JS_RETURN_NULL()` -- When calling a helper that itself returns a JSValue, that return value is safe to pass directly into `JS_SetPropertyStr` — no need to root temporaries that aren't stored in a local -**Common mistake — UNSAFE (will crash under GC pressure):** +**CRITICAL — C argument evaluation order bug:** + +Allocating functions (`JS_NewString`, `JS_NewFloat64`, `js_new_blob_stoned_copy`, etc.) used as arguments to `JS_SetPropertyStr` can crash because C evaluates arguments in unspecified order. The compiler may read `obj.val` BEFORE the allocating call, then GC moves the object, leaving a stale pointer. + ```c -JSValue obj = JS_NewObject(js); // NOT rooted -JS_SetPropertyStr(js, obj, "pixels", js_new_blob_stoned_copy(js, data, len)); -// ^^^ blob allocation can GC, invalidating obj -return obj; // obj may be a dangling pointer +// UNSAFE — intermittent crash: +JS_SetPropertyStr(js, obj.val, "format", JS_NewString(js, "rgba32")); +JS_SetPropertyStr(js, obj.val, "pixels", js_new_blob_stoned_copy(js, data, len)); + +// SAFE — separate the allocation: +JSValue fmt = JS_NewString(js, "rgba32"); +JS_SetPropertyStr(js, obj.val, "format", fmt); +JSValue pixels = js_new_blob_stoned_copy(js, data, len); +JS_SetPropertyStr(js, obj.val, "pixels", pixels); ``` +`JS_NewInt32`, `JS_NewUint32`, and `JS_NewBool` do NOT allocate and are safe inline. + See `docs/c-modules.md` for the full GC safety reference. ## Project Layout @@ -167,17 +180,19 @@ See `docs/c-modules.md` for the full GC safety reference. ## Package Management (Shop CLI) -When running locally with `./cell --dev`, these commands manage packages: +**Two shops:** `cell ` uses the global shop at `~/.cell/packages/`. `cell --dev ` uses the local shop at `.cell/packages/`. Linked packages (via `cell link`) are symlinked into the shop — edit the source directory directly. ``` -./cell --dev add # add a package (local path or remote) -./cell --dev remove # remove a package (cleans lock, symlink, dylibs) -./cell --dev build # build C modules for a package -./cell --dev test package # run tests for a package -./cell --dev list # list installed packages +cell add # add a package (local path or remote) +cell remove # remove a package (cleans lock, symlink, dylibs) +cell build # build C modules for a package +cell build --force # force rebuild (ignore stat cache) +cell test package # run tests for a package +cell list # list installed packages +cell link # list linked packages ``` -Local paths are symlinked into `.cell/packages/`. The build step compiles C files to content-addressed dylibs in `~/.cell/build/` and writes a per-package manifest so the runtime can find them. C files in `src/` are support files linked into module dylibs, not standalone modules. +The build step compiles C files to content-addressed dylibs in `~/.cell/build/` and writes a per-package manifest so the runtime can find them. C files in `src/` are support files linked into module dylibs, not standalone modules. ## Debugging Compiler Issues diff --git a/docs/c-modules.md b/docs/c-modules.md index 006cf18f..f09e58fa 100644 --- a/docs/c-modules.md +++ b/docs/c-modules.md @@ -453,6 +453,53 @@ JSC_CCALL(mymod_make, ) ``` +### C Argument Evaluation Order (critical) + +In C, the **order of evaluation of function arguments is unspecified**. This interacts with the copying GC to create intermittent crashes that are extremely difficult to diagnose. + +```c +// UNSAFE — crashes intermittently: +JS_FRAME(js); +JS_ROOT(obj, JS_NewObject(js)); +JS_SetPropertyStr(js, obj.val, "format", JS_NewString(js, "rgba32")); +// ^^^^^^^ may be evaluated BEFORE JS_NewString runs +// If JS_NewString triggers GC, the already-read obj.val is a dangling pointer. +``` + +The compiler is free to evaluate `obj.val` into a register, *then* call `JS_NewString`. If `JS_NewString` triggers GC, the object moves to a new address. The rooted `obj` is updated by GC, but the **register copy** is not — it still holds the old address. `JS_SetPropertyStr` then writes to freed memory. + +**Fix:** always separate the allocating call into a local variable: + +```c +// SAFE: +JS_FRAME(js); +JS_ROOT(obj, JS_NewObject(js)); +JSValue fmt = JS_NewString(js, "rgba32"); +JS_SetPropertyStr(js, obj.val, "format", fmt); +// obj.val is read AFTER JS_NewString completes — guaranteed correct. +``` + +This applies to **any** allocating function used as an argument when another argument references a rooted `.val`: + +```c +// ALL of these are UNSAFE: +JS_SetPropertyStr(js, obj.val, "pixels", js_new_blob_stoned_copy(js, data, len)); +JS_SetPropertyStr(js, obj.val, "x", JS_NewFloat64(js, 3.14)); +JS_SetPropertyStr(js, obj.val, "name", JS_NewString(js, name)); + +// SAFE versions — separate the allocation: +JSValue pixels = js_new_blob_stoned_copy(js, data, len); +JS_SetPropertyStr(js, obj.val, "pixels", pixels); +JSValue x = JS_NewFloat64(js, 3.14); +JS_SetPropertyStr(js, obj.val, "x", x); +JSValue s = JS_NewString(js, name); +JS_SetPropertyStr(js, obj.val, "name", s); +``` + +**Functions that allocate** (must be separated): `JS_NewString`, `JS_NewFloat64`, `JS_NewInt64`, `JS_NewObject`, `JS_NewArray`, `JS_NewCFunction`, `js_new_blob_stoned_copy` + +**Functions that do NOT allocate** (safe inline): `JS_NewInt32`, `JS_NewUint32`, `JS_NewBool`, `JS_NULL`, `JS_TRUE`, `JS_FALSE` + ### Macros | Macro | Purpose |