Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The regression test does not deterministically exercise V3’s scoped path and may pass through fallback compilation.
Pull request overview
This pull request fixes V3 monomorphization cache lifetime issues by preserving specialization data across worker merges.
Changes:
- Deep-copy specialization arguments during merges.
- Re-record cache entries with parent-owned strings.
- Add regression coverage for generic closure compilation.
File summaries
| File | Summary |
|---|---|
vlib/v/transform/transform_parallel_notd_v3_no_parallel.v |
Safely merges specialization data from workers. |
vlib/v/transform/monomorphize.v |
Owns cache strings and refreshes duplicate entries. |
vlib/v/compiler_tests/scoped_monomorphize_closure_test.v |
Adds scoped monomorphization regression coverage. |
Review details
Suppressed comments (3)
vlib/v/compiler_tests/scoped_monomorphize_closure_test.v:89
- This invocation does not force V3. On the regression described here, V3 can emit the bogus diagnostics and then transparently fall back to V1, which still returns exit code 0 and produces the binary, so all three assertions pass without exercising the fix. Run the generated compiler with
-new-compilerso the test fails on the stale-argument path.
compile := os.execute('${v3_bin} -nocache -o ${out} ${dir}')
vlib/v/compiler_tests/scoped_monomorphize_closure_test.v:22
- Calling
scoped_monomorph_v3_bin()here builds the compiler when it is absent and then immediately deletes it, so every test run performs the expensive self-build twice. Remove the path directly in the suite hook instead of invoking the build helper.
os.rm(scoped_monomorph_v3_bin()) or {}
vlib/v/compiler_tests/scoped_monomorphize_closure_test.v:16
-prealloconly enablesscope_parallel_workers;run_scoped_monomorphize_specs()is selected separately when the AST reachesscoped_monomorph_node_threshold(currently 1,000,000). The small program compiled at line 89 does not force that gate, so this test can pass while the changed scoped merge/re-recording path is never executed. Please make the regression deterministically select the bounded path, for example by providing a test-only threshold override or a fixture large enough to cross it.
// `-prealloc` is what enables `scope_parallel_workers` and the scoped
// monomorphize path, matching how the distributed compiler is built.
build := os.execute('${scoped_monomorph_vexe} -gc none -prealloc -path "${scoped_monomorph_vlib_dir}|@vlib|@vmodules" -o ${bin} ${scoped_monomorph_v3_src}')
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a9a45f2 to
5c4151a
Compare
|
request for review |
The monomorphize worker merge copied its `[]string` tables shallowly: the array was copied, but every element still pointed into the worker scratch arena that is released right after the merge. A later pass (the driver compiles twice on this path) re-seeds its specializations from `generic_specialization_args` / `monomorph_cache_specs`, so it read freed type arguments: they turn into NUL bytes, and V3 then reports bogus `unknown function` diagnostics for perfectly valid calls (`ctrl.use` / `ctrl.route_use` in veb apps) and silently falls back to the compatibility compiler. Deep-copy the merged arguments into the parent arena, re-record every emitted specification from the master (instead of skipping keys a worker already recorded) so the last, parent-owned copy wins, and copy the emitted specialization keys/arguments into the parent arena while the worker scope is still alive but suspended. Needed to compile RuoQi-v (https://github.com/RuoQi-DoDo/RuoQi-v) with `-new-compiler`. Fixes vlang#28489.
5c4151a to
e7ac3a1
Compare
medvednikov
left a comment
There was a problem hiding this comment.
Reviewed the current code changes only; CI status intentionally not considered. The deep-copy changes themselves look coherent, but the new regression does not actually force the scoped merge path this PR also changes.
…ize-released-args # Conflicts: # vlib/v/transform/monomorphize.v # vlib/v/transform/monomorphize_test.v
The fixture compiles its sample with the compiler built from cmd/v, but without -new-compiler a V3 failure silently retries with the V 0.5.2 compatibility compiler, which can still produce a binary and exit 0, so the assertions would pass without covering the scoped merge path. Pass -new-compiler so a regression fails the test instead.
|
fixed |
…ize-released-args
…ize-released-args
…ize-released-args
Monomorph workers resolve and lift specialization declarations inside their own scratch arena, which is released right after the batch merges. The merges published the worker's own `GenericFnDecl` (module, key, file and the `flat.Node` text), so the master's signature tables and the unresolved-type cache ended up holding strings from released arenas: after a later batch released its block, `same_transform_text()` compared `cache.module` and read unmapped memory, which made the veb program from vlang#28489 abort with a segfault instead of compiling. Publish a copy the master arena owns when a worker's queued or emitted specialization is merged, in both the scoped and the chunk-worker paths, like the argument lists and queue keys already are. The merged upstream state could not be built by V3 at all, which hid this: `transform_late_used_fn_bodies` still dereferenced `(*names)` after the parameter became a value, and `transform_selector_expr` shadowed `iface_name`. Fix both so `./v -new-compiler` can compile the transform module again. Fixes vlang#28489.
…ize-released-args
…nomorphize-released-args
…ize-released-args # Conflicts: # vlib/v/transform/transform_parallel_notd_v3_no_parallel.v
…ize-released-args
…ize-released-args
Why
The monomorphize worker merge copied its
[]stringtables shallowly: the arraywas copied, but every element still pointed into the worker scratch arena that
transform_worker_scope_leave()releases right after the merge. A later pass(the driver compiles twice on this path) re-seeds its specializations from
generic_specialization_args/monomorph_cache_specs, so it read freed typearguments. The strings turn into NUL bytes, and V3 then reports bogus
diagnostics for perfectly valid calls:
...and silently falls back to the compatibility compiler - exactly the
diagnostic family of #28489.
What changed
run_parallel_monomorphize_specs(): deep-copy the mergedgeneric_specialization_argsentry into the parent arena instead ofspec_args.clone()(which copies the slice header only).run_scoped_monomorphize_specs(): re-record every emitted specializationthrough
record_monomorph_cache_spec()from the master, so theparent-owned copy wins over one a worker recorded for the same key.
record_monomorph_cache_spec(): clone the key, declaration key, module andevery argument string, and re-record existing keys instead of skipping them
(the worker's copy dies with its arena).
Test plan
vlib/v/compiler_tests/scoped_monomorphize_closure_test.vbuilds a V3 compilerwith
-preallocand compiles the smallvebprogram from #28489 (embeddedMiddleware[Ctx]/Controllerstructs, a generic controller helper that callsctrl.use()/ctrl.route_use(), and lifted closures). The fixture fails on ashallow merge (freed arguments -> bogus diagnostics/SIGSEGV); the follow-up PR
for #28640 keeps it as the shared regression fixture for the bounded path.
v -g -keepc -o vnew cmd/v ./vnew -silent test vlib/v/compiler_tests/scoped_monomorphize_closure_test.vNotes
(https://github.com/RuoQi-DoDo/RuoQi-v) compile with
-new-compiler; thebogus
unknown functiondiagnostics appeared there after ~100s ofcompilation followed by a silent fallback.
v3 scoped monomorphize: SIGSEGV or wrong C when the memory-bounded path runs#28640 and V3:monomorphizememory grows without bound until the compiler is OOM-killed #28564 build on this branch (the bounded path has tobe correct before it is used for every batch).
Fixes #28489.