v3: keep refined C extern redeclarations in the signature tables - #27978
Conversation
579ec07 added a first-registration-wins guard to register_fn_name_alias so cross-module V name collisions cannot clobber signatures. C externs live in one global namespace, though, and modules routinely redeclare them with refined types: builtin declares C.pthread_join(thread voidptr, ...) while v3.workers refines it to (thread C.pthread_t, ...). The guard silently dropped the refinement, so cgen's module-blind parameter lookup saw voidptr and passed struct-typed pthread handles by address; every second-generation compiler (v4 built with -building-v, and v5 built by v4) then panicked with 'failed to join compiler worker 0' in the close_workers defer after every otherwise-successful compile. Exempt C.-prefixed names from the guard, restoring last-write-wins for extern redeclarations. Verified: the generated pool code passes the handle by value again, the v1 -> v3 -> v4 -> v5 bootstrap chain exits 0 with no join panics, and the fixture-conformance tests that pass on master still pass (markused_test and post_merge_review_fixes_test fail identically on pristine master).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c992636369
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // module-blind parameter lookup would then see `voidptr` and take the | ||
| // address of struct-typed handles (gen-2 compilers panicked with | ||
| // `failed to join compiler worker 0`). | ||
| if !name.starts_with('C.') { |
There was a problem hiding this comment.
When a program imports a module that redeclares a builtin memory/string extern—for example, json redeclares C.malloc and C.memcpy—this exemption lets register_fn_name_alias overwrite fn_type_modules[name] with that module. is_builtin_unsafe_c_call only requires an unsafe block when this owner remains builtin, so merely importing such a module suppresses the unsafe-call warning for subsequent C.malloc, C.memcpy, or C.memset calls. Refine the global signature without replacing the builtin ownership provenance, or make the unsafe check consult the module-specific declaration tables.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e98f3ea — took the first suggested route: the exemption now refines only the signature tables, while fn_type_files/fn_type_modules keep the first declarer for C.-prefixed names, so builtin ownership (and with it the is_builtin_unsafe_c_call gate) is preserved. Verified both directions: with import json, a bare C.memcpy call now produces function \C.memcpy` must be called from an `unsafe` blockagain (it was silently suppressed before this fix, exactly as you described), stays silent insideunsafe {}, and the original regression stays fixed — the emitted pool code still passes pthread_join(thread_id, …)` by value and the v1 → v3 → v4 → v5 bootstrap exits 0 with no join panics.
Review feedback on #27978: the C-extern exemption let a module's refined redeclaration overwrite fn_type_modules for the name, and is_builtin_unsafe_c_call requires the owner to stay builtin — so merely importing json (which redeclares C.malloc/C.memcpy) suppressed the 'must be called from an unsafe block' diagnostic program-wide. Refine the signature tables but keep the first owner's provenance for C.-prefixed names. Verified: with 'import json' the C.memcpy diagnostic fires again (and stays silent inside unsafe blocks); the pthread_join refinement still reaches cgen and the v1 -> v3 -> v4 -> v5 bootstrap remains green with no join panics.
1 participant
Fixes the second-generation bootstrap regression introduced by `579ec07454` (bisected: `73fa3211d6` good → `579ec07454` bad, also reported on #27974): v4 (built with `-building-v`) and v5 panic with `V panic: failed to join compiler worker 0` in the `close_workers` defer after every otherwise-successful compile.
Root cause. `579ec07454` added a first-registration-wins guard to `register_fn_name_alias` so cross-module V-name collisions cannot clobber signatures. C externs live in one global namespace, though, and modules routinely redeclare them with refined types: builtin declares `fn C.pthread_join(thread voidptr, retval voidptr)` while `v3.workers` refines it to `(thread C.pthread_t, retval voidptr)`. The guard silently dropped the refinement from the global `fn_param_types` table. The checker's own call path is module-aware (`c_fn_module_param_types`) and unaffected — but cgen's `param_types_for` reads the module-blind global table, saw `voidptr`, and `voidptr_value_arg_needs_address` then emitted `pthread_join(&thread_id, …)` instead of `pthread_join(thread_id, …)`, passing the address of the stack slot rather than the handle. It compiles silently (the emitted prototype is `pthread_join(void*, …)`) and only fails at join time.
Fix. Exempt `C.`-prefixed names from the guard, restoring last-write-wins for extern redeclarations (the pre-`579ec07454` behavior). The guard's protection for V functions is unchanged.
Validation.