From fbdfbc1200c37d7260b4a118dd0648617c80800f Mon Sep 17 00:00:00 2001 From: John Alanbrook Date: Tue, 17 Feb 2026 01:54:25 -0600 Subject: [PATCH] add audit --- CLAUDE.md | 49 +++++++++++++++++++++- add.ce | 5 ++- audit.ce | 79 ++++++++++++++++++++++++++++++++++ build.ce | 6 +-- cell.toml | 4 +- docs/c-modules.md | 105 ++++++++++++++++++++++++++++++++++------------ docs/cli.md | 10 +++++ help.ce | 1 + internal/shop.cm | 15 ++++++- 9 files changed, 240 insertions(+), 34 deletions(-) create mode 100644 audit.ce diff --git a/CLAUDE.md b/CLAUDE.md index 4bff8a87..c984f4ad 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,6 +20,7 @@ All code uses 2 spaces for indentation. K&R style for C and Javascript. - `==` and `!=` are strict (no `===` or `!==`) - No `undefined` — only `null` - No classes — only objects and prototypes (`meme()`, `proto()`, `isa()`) +- No `switch`/`case` — use record dispatch (a record keyed by case, values are functions or results) instead of if/else chains - No `for...in`, `for...of`, spread (`...`), rest params, or default params - No named function declarations — use `var fn = function() {}` or arrow functions - Functions have a maximum of 4 parameters — use a record for more @@ -108,7 +109,53 @@ var v = a[] // pop: v is 3, a is [1, 2] - Core is the `core` package — its symbols follow the same `js_core__use` pattern as all other packages - Package directories should contain only source files (no `.mach`/`.mcode` alongside source) - Build cache files in `build/` are bare hashes (no extensions) -- Use `JS_FRAME`/`JS_ROOT`/`JS_RETURN` macros for any C function that allocates multiple heap objects. Any `JS_New*`/`JS_SetProperty*` call can trigger GC. + +### MANDATORY: GC Rooting for C Functions + +This project uses a **copying garbage collector**. ANY JS allocation (`JS_NewObject`, `JS_NewString`, `JS_NewArray`, `JS_NewInt32`, `JS_SetPropertyStr`, `js_new_blob_stoned_copy`, etc.) can trigger GC, which **invalidates all unrooted JSValue locals**. This is not theoretical — it causes real crashes. + +**Before writing or modifying ANY C function**, apply this checklist: + +1. Count the number of `JS_New*`, `JS_SetProperty*`, and `js_new_blob*` calls in the function +2. If there are 2 or more, the function MUST use `JS_FRAME`/`JS_ROOT`/`JS_RETURN` +3. Every JSValue that is held across an allocating call must be rooted + +**Pattern — object with properties:** +```c +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")); +JS_RETURN(obj.val); +``` + +**Pattern — array with loop:** +```c +JS_FRAME(js); +JS_ROOT(arr, JS_NewArray(js)); +for (int i = 0; i < count; i++) { + JS_ROOT(item, JS_NewObject(js)); + JS_SetPropertyStr(js, item.val, "v", JS_NewInt32(js, i)); + JS_SetPropertyNumber(js, arr.val, i, item.val); +} +JS_RETURN(arr.val); +``` + +**Rules:** +- Access rooted values via `.val` (e.g., `obj.val`, not `obj`) +- 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):** +```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 +``` + +See `docs/c-modules.md` for the full GC safety reference. ## Project Layout diff --git a/add.ce b/add.ce index 2b12bef4..7a8a866d 100644 --- a/add.ce +++ b/add.ce @@ -87,7 +87,10 @@ var _install = function() { shop.extract(locator) // Build scripts - shop.build_package_scripts(locator) + var script_result = shop.build_package_scripts(locator) + if (length(script_result.errors) > 0) { + log.console(" Warning: " + text(length(script_result.errors)) + " script(s) failed to compile") + } // Build C code if any var _build_c = function() { diff --git a/audit.ce b/audit.ce new file mode 100644 index 00000000..8b228098 --- /dev/null +++ b/audit.ce @@ -0,0 +1,79 @@ +// cell audit [] - Test-compile all .ce and .cm scripts +// +// Usage: +// cell audit Audit all packages +// cell audit Audit specific package +// cell audit . Audit current directory package +// +// Compiles every script in the package(s) to check for errors. +// Continues past failures and reports all issues at the end. + +var shop = use('internal/shop') +var pkg = use('package') +var fd = use('fd') + +var target_package = null +var i = 0 +var resolved = null + +for (i = 0; i < length(args); i++) { + if (args[i] == '--help' || args[i] == '-h') { + log.console("Usage: cell audit []") + log.console("") + log.console("Test-compile all .ce and .cm scripts in package(s).") + log.console("Reports all errors without stopping at the first failure.") + $stop() + } else if (!starts_with(args[i], '-')) { + target_package = args[i] + } +} + +// Resolve local paths +if (target_package) { + if (target_package == '.' || starts_with(target_package, './') || starts_with(target_package, '../') || fd.is_dir(target_package)) { + resolved = fd.realpath(target_package) + if (resolved) { + target_package = resolved + } + } +} + +var packages = null +var total_ok = 0 +var total_errors = 0 +var total_scripts = 0 +var all_failures = [] + +if (target_package) { + packages = [target_package] +} else { + packages = shop.list_packages() +} + +arrfor(packages, function(p) { + var scripts = shop.get_package_scripts(p) + if (length(scripts) == 0) return + + log.console("Auditing " + p + " (" + text(length(scripts)) + " scripts)...") + var result = shop.build_package_scripts(p) + total_ok = total_ok + result.ok + total_errors = total_errors + length(result.errors) + total_scripts = total_scripts + result.total + + arrfor(result.errors, function(e) { + push(all_failures, p + ": " + e) + }) +}) + +log.console("") +if (length(all_failures) > 0) { + log.console("Failed scripts:") + arrfor(all_failures, function(f) { + log.console(" " + f) + }) + log.console("") +} + +log.console("Audit complete: " + text(total_ok) + "/" + text(total_scripts) + " scripts compiled" + (total_errors > 0 ? ", " + text(total_errors) + " failed" : "")) + +$stop() diff --git a/build.ce b/build.ce index 842c0178..0bc20318 100644 --- a/build.ce +++ b/build.ce @@ -121,10 +121,8 @@ if (target_package) { success = 0 failed = 0 for (i = 0; i < length(results); i++) { - if (results[i].library) { - success++ - } else if (results[i].error) { - failed++ + if (results[i].modules) { + success = success + length(results[i].modules) } } diff --git a/cell.toml b/cell.toml index fd89a789..dc36b553 100644 --- a/cell.toml +++ b/cell.toml @@ -1,6 +1,8 @@ [dependencies] cell-steam = "/Users/johnalanbrook/work/cell-steam" +cell-image = "/Users/johnalanbrook/work/cell-image" cell-sdl3 = "/Users/johnalanbrook/work/cell-sdl3" [compilation] +[compilation] [compilation.playdate] -CFLAGS = "-DMINIZ_NO_TIME -DTARGET_EXTENSION -DTARGET_PLAYDATE -I$LOCAL/PlaydateSDK/C_API" +CFLAGS = "-DMINIZ_NO_TIME -DTARGET_EXTENSION -DTARGET_PLAYDATE -I$LOCAL/PlaydateSDK/C_API" \ No newline at end of file diff --git a/docs/c-modules.md b/docs/c-modules.md index d43b69a6..e74a8c25 100644 --- a/docs/c-modules.md +++ b/docs/c-modules.md @@ -188,11 +188,11 @@ JSC_CCALL(vector_normalize, double len = sqrt(x*x + y*y); if (len > 0) { JS_FRAME(js); - JS_LOCAL(result, JS_NewObject(js)); - JS_SetPropertyStr(js, result, "x", number2js(js, x/len)); - JS_SetPropertyStr(js, result, "y", number2js(js, y/len)); + JS_ROOT(result, JS_NewObject(js)); + JS_SetPropertyStr(js, result.val, "x", number2js(js, x/len)); + JS_SetPropertyStr(js, result.val, "y", number2js(js, y/len)); JS_RestoreFrame(_js_ctx, _js_gc_frame, _js_local_frame); - ret = result; + ret = result.val; } ) @@ -262,7 +262,13 @@ The module file (`rtree.c`) includes the library header and uses `cell.h` as usu ## GC Safety -ƿit uses a Cheney copying garbage collector. Any JS allocation — `JS_NewObject`, `JS_NewString`, `JS_SetPropertyStr`, etc. — can trigger GC, which **moves** heap objects to new addresses. C locals holding JSValue become stale after any allocating call. +ƿit uses a **Cheney copying garbage collector**. Any JS allocation — `JS_NewObject`, `JS_NewString`, `JS_NewInt32`, `JS_SetPropertyStr`, `js_new_blob_stoned_copy`, etc. — can trigger GC, which **moves** heap objects to new addresses. Bare C locals holding `JSValue` become **dangling pointers** after any allocating call. This is not a theoretical concern — it causes real crashes that are difficult to reproduce because they depend on heap pressure. + +### Checklist (apply to EVERY C function you write or modify) + +1. Count the `JS_New*`, `JS_SetProperty*`, and `js_new_blob*` calls in the function +2. If there are **2 or more**, the function **MUST** use `JS_FRAME` / `JS_ROOT` / `JS_RETURN` +3. Every `JSValue` held in a C local across an allocating call must be rooted ### When you need rooting @@ -274,22 +280,70 @@ JSC_CCALL(mymod_name, ) ``` -If a function holds a heap object across further allocating calls, you must root it: +If a function creates an object and then sets properties on it, you **must** root it — each `JS_SetPropertyStr` call is an allocating call that can trigger GC: ```c -JSC_CCALL(vector_normalize, - double x = js2number(js, argv[0]); - double y = js2number(js, argv[1]); - double len = sqrt(x*x + y*y); - if (len > 0) { - JS_FRAME(js); - JS_LOCAL(result, JS_NewObject(js)); - // result is rooted — GC can update it through these calls: - JS_SetPropertyStr(js, result, "x", number2js(js, x/len)); - JS_SetPropertyStr(js, result, "y", number2js(js, y/len)); - JS_RestoreFrame(_js_ctx, _js_gc_frame, _js_local_frame); - ret = result; - } +// UNSAFE — will crash under GC pressure: +JSValue obj = JS_NewObject(js); +JS_SetPropertyStr(js, obj, "x", JS_NewInt32(js, 1)); // can GC → obj is stale +JS_SetPropertyStr(js, obj, "y", JS_NewInt32(js, 2)); // obj may be garbage +return obj; + +// SAFE: +JS_FRAME(js); +JS_ROOT(obj, JS_NewObject(js)); +JS_SetPropertyStr(js, obj.val, "x", JS_NewInt32(js, 1)); +JS_SetPropertyStr(js, obj.val, "y", JS_NewInt32(js, 2)); +JS_RETURN(obj.val); +``` + +### Patterns + +**Object with properties** — the most common pattern in this codebase: + +```c +JS_FRAME(js); +JS_ROOT(result, JS_NewObject(js)); +JS_SetPropertyStr(js, result.val, "width", JS_NewInt32(js, w)); +JS_SetPropertyStr(js, result.val, "height", JS_NewInt32(js, h)); +JS_SetPropertyStr(js, result.val, "pixels", js_new_blob_stoned_copy(js, data, len)); +JS_RETURN(result.val); +``` + +**Array with loop** — root both the array and each element created in the loop: + +```c +JS_FRAME(js); +JS_ROOT(arr, JS_NewArray(js)); +for (int i = 0; i < count; i++) { + JS_ROOT(item, JS_NewObject(js)); + JS_SetPropertyStr(js, item.val, "index", JS_NewInt32(js, i)); + JS_SetPropertyStr(js, item.val, "data", js_new_blob_stoned_copy(js, ptr, sz)); + JS_SetPropertyNumber(js, arr.val, i, item.val); +} +JS_RETURN(arr.val); +``` + +**Nested objects** — root every object that persists across an allocating call: + +```c +JS_FRAME(js); +JS_ROOT(outer, JS_NewObject(js)); +JS_ROOT(inner, JS_NewArray(js)); +// ... populate inner ... +JS_SetPropertyStr(js, outer.val, "items", inner.val); +JS_RETURN(outer.val); +``` + +**Inside `JSC_CCALL`** — use `JS_RestoreFrame` and assign to `ret`: + +```c +JSC_CCALL(mymod_make, + JS_FRAME(js); + JS_ROOT(obj, JS_NewObject(js)); + JS_SetPropertyStr(js, obj.val, "x", number2js(js, 42)); + JS_RestoreFrame(_js_ctx, _js_gc_frame, _js_local_frame); + ret = obj.val; ) ``` @@ -297,19 +351,18 @@ JSC_CCALL(vector_normalize, | Macro | Purpose | |-------|---------| -| `JS_FRAME(js)` | Save the GC and local frames. Required before any `JS_LOCAL`. | -| `JS_LOCAL(name, init)` | Declare and root a JSValue. GC updates it through its address. | +| `JS_FRAME(js)` | Save the GC frame. Required before any `JS_ROOT`. | +| `JS_ROOT(name, init)` | Declare a `JSGCRef` and root its value. Access via `name.val`. | +| `JS_LOCAL(name, init)` | Declare a rooted `JSValue` (GC updates it through its address). | | `JS_RETURN(val)` | Restore the frame and return a value. | | `JS_RETURN_NULL()` | Restore the frame and return `JS_NULL`. | | `JS_RETURN_EX()` | Restore the frame and return `JS_EXCEPTION`. | | `JS_RestoreFrame(...)` | Manual frame restore (for `JSC_CCALL` bodies that use `ret =`). | -### Rules of thumb +### Error return rules -1. **One allocation, immediate return** — no rooting needed. -2. **Object + property sets** — root the object with `JS_LOCAL`. -3. **Array + loop** — root the array; if loop body creates objects, root the loop variable too with a manual `JSLocalRef`. -4. **`CELL_USE_INIT` bodies** — always use `JS_FRAME` / `JS_LOCAL` / `JS_RETURN`. +- Error returns **before** `JS_FRAME` can use plain `return JS_ThrowTypeError(...)` etc. +- Error returns **after** `JS_FRAME` must use `JS_RETURN_EX()` or `JS_RETURN_NULL()` — never plain `return`, which would leak the GC frame. ### Migrating from gc_mark diff --git a/docs/cli.md b/docs/cli.md index dc69033d..c4ac7d88 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -220,6 +220,16 @@ pit verify --deep # traverse full dependency closure pit verify --target ``` +### pit audit + +Test-compile all `.ce` and `.cm` scripts in package(s). Continues past failures and reports all errors at the end. + +```bash +pit audit # audit all installed packages +pit audit # audit specific package +pit audit . # audit current directory +``` + ### pit pack Build a statically linked binary from a package and all its dependencies. diff --git a/help.ce b/help.ce index e167b999..f950c6ea 100644 --- a/help.ce +++ b/help.ce @@ -58,6 +58,7 @@ if (stat && stat.isFile) { log.console(" resolve [locator] Print fully resolved dependency closure") log.console(" graph [locator] Emit dependency graph (tree, dot, json)") log.console(" verify [scope] Verify integrity and consistency") + log.console(" audit [locator] Test-compile all scripts in package(s)") log.console("") log.console("Other:") log.console(" help [command] Show help for a command") diff --git a/internal/shop.cm b/internal/shop.cm index 6c4ea826..dbde2841 100644 --- a/internal/shop.cm +++ b/internal/shop.cm @@ -1428,14 +1428,27 @@ function get_package_scripts(package) Shop.build_package_scripts = function(package) { // compiles all .ce and .cm files in a package + // continues past failures and returns results var scripts = get_package_scripts(package) var pkg_dir = get_package_abs_dir(package) + var errors = [] + var ok = 0 arrfor(scripts, function(script, i) { - resolve_mod_fn(pkg_dir + '/' + script, package) + var _try = function() { + resolve_mod_fn(pkg_dir + '/' + script, package) + ok = ok + 1 + } disruption { + push(errors, script) + } + _try() }) + + return {ok: ok, errors: errors, total: length(scripts)} } +Shop.get_package_scripts = get_package_scripts + Shop.list_packages = function() { var lock = Shop.load_lock()