builtin: fix -gc boehm_leak freeing live arrays with the Windows tcc libgc, and print GC aborts to stderr on Windows - #28917
medvednikov wants to merge 4 commits into
Conversation
…pointers V array data points one pointer-width past the allocation header, and builtin registers that offset with GC_register_displacement. With -gc boehm_leak, GC_DEBUG gives every allocation Boehm's debug header too, so the data pointer sits sizeof(oh) + 8 bytes into the object. That offset was never registered. This only matters for a libgc that does not recognize all interior pointers. The bundled Windows tcc libgc.a is built without ALL_INTERIOR_POINTERS (GC_all_interior_pointers is 0 in it), while the other bundled builds have it. With that library, the leak detector treats array buffers that are only referenced from the heap (like the closure lifetime `records` and `frames`) as garbage. It frees them while they are still used, and releases whole blocks when all their objects look dead. closure.Lifetime.dispose() then frees such a buffer again, and GC_debug_free aborts with "Invalid pointer passed to free()" (#28896). Use GC_REGISTER_DISPLACEMENT, which is GC_debug_register_displacement under GC_DEBUG, and registers both offsets. Add a test that turns off interior pointer recognition before GC_INIT, and collects while the lifetime bookkeeping is live. Without the fix it reports smashed objects or hangs in the collector on macOS.
On Windows, Boehm's default abort handler shows a modal "Fatal error in GC" message box, and waits for a click before it calls DebugBreak(). Unattended runs hang on it; closure_lifetime_api_test.v blocked `v test vlib/v/tests` that way in #28896. Boehm writes its own error output to <exe>.gc.log there, so stderr did not show the reason either. For Windows -gc boehm* builds, install a handler with GC_set_abort_func after GC_INIT. It writes the message to stderr without allocating and without the CRT stdio locks, prints a backtrace for tcc builds (like tcc's own handler did after the click), and exits with code 1, without running the at-exit handlers. With a debugger attached, it returns, so Boehm's DebugBreak() still stops there. The call with a nil message before exit(1) shows no message box, and still goes to Boehm's default handler.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: dc8f276067
ℹ️ 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".
| C.GC_REGISTER_DISPLACEMENT(sizeof(voidptr)) | ||
| $if windows { | ||
| g_gc_default_abort_func = C.v_gc_get_abort_func() | ||
| C.v_gc_set_abort_func(gc_abort_without_message_box) |
There was a problem hiding this comment.
For a Windows V DLL linked to a shared/system collector (-shared -d dynamic_boehm), _vinit runs this initializer and replaces libgc's process-global abort callback with a function located inside the DLL. dl.close can subsequently unload that DLL through FreeLibrary, but no cleanup restores g_gc_default_abort_func, leaving the host collector with a dangling callback that it will call on its next fatal error. Avoid installing the callback from shared builds or restore it during library cleanup.
Useful? React with 👍 / 👎.
| for gc_error in ['Invalid pointer', 'smashed', 'deallocated', 'Fatal error in GC'] { | ||
| assert !res.output.contains(gc_error), res.output | ||
| } | ||
| assert res.output.contains('lifetime done'), res.output |
There was a problem hiding this comment.
After the missing-libgc case is excluded, this test never asserts res.exit_code == 0; a child that prints lifetime done and then crashes or aborts with output not matching the four selected strings is therefore accepted. This is particularly relevant for platform-dependent libgc failures during shutdown, so assert a successful exit in addition to checking the marker.
Useful? React with 👍 / 👎.
A V DLL runs the Boehm startup code from `_vinit` too. When it shares the collector with its host (`-d dynamic_boehm`), replacing the process wide abort handler with a function inside of the DLL leaves the collector with a dangling handler, once the DLL is unloaded with `FreeLibrary`. V DLLs have no unload hook on Windows, so the old handler can not be restored reliably. Install the handler only when builtin is part of the executable image, which `VirtualQuery` reports for the address of one of its globals. Also check the exit status of the child process in the regression test without interior pointers, so that a crash after its last output fails it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40ba2b95ad
ℹ️ 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".
| // corrupted, and stopped threads may hold those locks. | ||
| prefix := 'Fatal error in GC: ' | ||
| newline := '\n' | ||
| write_buf_to_fd_kernel32(2, prefix.str, prefix.len) |
There was a problem hiding this comment.
When stderr is attached to a console and Boehm aborts while another thread is suspended holding the Windows process-heap lock, this handler can deadlock instead of terminating: write_buf_to_fd_kernel32() first calls write_buf_to_console_kernel32(), which allocates with HeapAlloc at builtin_windows.c.v:173. This contradicts the handler's no-allocation requirement and is especially unsafe while the collector has stopped other threads; use a direct, allocation-free WriteFile path for these fatal messages.
Useful? React with 👍 / 👎.
`write_buf_to_fd_kernel32` first tries `write_buf_to_console_kernel32`, which converts the text to UTF-16 in a `HeapAlloc` buffer. Boehm aborts while other threads may be stopped, and one of them may hold the lock of the process heap, so the abort handler could deadlock on a console. Move the `WriteFile` loop into `write_buf_to_std_handle_kernel32`, and use that for the abort message. The messages are ASCII, so a console shows them the same way.
This branch has not been deployed
1 participant
Summary
Root cause. V array data points one pointer-width past the allocation header.
v3_gcboehm_runtime_initregisters that offset withGC_register_displacement(sizeof(voidptr)). With-gc boehm_leak,GC_DEBUGputs Boehm's debug header in front of every object, so the data pointer issizeof(oh) + 8bytes into the object, and that offset was never registered.This only matters for a libgc that does not recognize all interior pointers, and that is the one Windows + tcc links.
thirdparty/tcc/lib/libgc.aon thethirdparty-windows-amd64tccbin branch is built bybuild.ps1astcc gc.c -DGC_NOT_DLL -DGC_WIN32_THREADS -DGC_THREADS -DGC_BUILTIN_ATOMIC, without-DALL_INTERIOR_POINTERS. In that archive,GC_all_interior_pointersis initialized to 0. The gcc/msvc builds compilegc.cwith-DALL_INTERIOR_POINTERS=1, and the macOS/Linux libgc is built by configure, which enables it. So only Windows + tcc +boehm_leakis affected, which matches the report.With that library, the leak detector treats array buffers that are referenced only from the heap as garbage. The closure lifetime's
recordsandframesare such buffers: they live inside theClosureLifetimeState. Boehm frees them while they are still in use (reported leaks areGC_freed, the rest are reclaimed). When every object in a block looks dead, the whole block is released.Lifetime.dispose()then callsarray.free()on such a buffer.GC_base()returns NULL for a released block, soGC_debug_freeaborts with "Invalid pointer passed to free()".Fix 1 (
builtin: keep array buffers alive ...): useGC_REGISTER_DISPLACEMENT. UnderGC_DEBUGthat isGC_debug_register_displacement, which registers bothoffsetandsizeof(oh) + offset(gc.hsays to use the debugging variant when debug allocation is used). WithoutGC_DEBUGit is the sameGC_register_displacementcall as before.Fix 2 (
builtin: print Boehm GC fatal errors to stderr on Windows): on Windows, Boehm's default abort handler shows a modal "Fatal error in GC" message box (MSGBOX_ON_ERROR) and waits for a click beforeDebugBreak(). That is whyclosure_lifetime_api_test.vhungv test. Boehm also writes its own messages to<exe>.gc.logthere, not to stderr. For Windows-gc boehm*builds, builtin now installs a handler withGC_set_abort_funcafterGC_INIT. The handler:Fatal error in GC: <msg>to stderr withWriteFile, without allocating (not even through the UTF-16 console path, whoseHeapAlloccould wait for a stopped thread that holds the process heap lock) or taking CRT stdio locks;Only an executable installs the handler (
builtin: install the Windows GC abort handler only from executables). A V DLL runs the same startup code from_vinit. If it shares the collector with its host (-d dynamic_boehm), the host would keep a dangling handler afterFreeLibrary, and V DLLs have no unload hook to restore it.VirtualQueryon one of builtin's globals shows whether builtin is in the executable image (AllocationBase == GetModuleHandleW(NULL)).With a debugger attached, it returns instead, so
DebugBreak()still stops in the debugger. Themsg == NULLcall beforeexit(1)never shows a box, so it still goes to Boehm's default handler.GC_set_abort_func,GC_get_abort_funcandGC_debug_register_displacementare all exported by the Windows tcclibgc.a(checked withnm), andgc.hdeclares them.Test:
test_closure_lifetime_boehm_leak_runtime_without_interior_pointersmakes this failure reproducible on every platform. Its child program includes a C header whose constructor callsGC_set_all_interior_pointers(0)before V'sGC_INIT(). The child runs the issue's lifetime loop under-gc boehm_leakand callsgc_collect()every 16 frames. The test checks that the child exits with status 0, that its output has no GC corruption messages, and that the program finishes. On MSVC the header is a no-op.What is verified and what is reasoned: I had no Windows machine. The libgc configuration above was read from the actual Windows tcc archive. The mechanism was reproduced on macOS by building the bundled
gc.cwith the Windows tcc flags. There, a GC mark-end hook shows theframesbuffer unmarked while itsClosureLifetimeStateis marked. The macOS symptom is different: the double free leaves a free-list cycle, and the next collection spins inGC_set_fl_marks, or reports "found smashed location ... sz= 40". I did not get the exact "Invalid pointer passed to free()" abort, because that depends on the whole block being released, which depends on heap layout. On Windows, the reported abort following from this is reasoned from Boehm's source. The new abort handler has not been run on Windows.Fixes #28896
Validation
Everything was run on macOS arm64. Nothing was run on Windows. This checkout has no
thirdparty/tcc, so boehm builds usedVFLAGS='-d use_bundled_libgc', or the generated C was compiled by hand. For tcc, thetcc.exeandlibgc.dylibcame from another checkout.gc.cwithoutALL_INTERIOR_POINTERSand ran the issue repro against it, 6 runs each. On master every run hangs at exit inGC_finish_collection. With the fix every run printsdoneand the same leak report as an interior-pointer build. Registering the debug displacement in master's generated C alone also fixes it.GC_set_fl_marksat exit; I sampled the process to confirm. The same child program against the prebuilt macOSlibgc.a: master prints "GC_debug_free: found smashed location ... sz= 40" and hangs. With the fix it passes. With the macOS tcclibgc.dylib, master hangs and the fix passes.-gc boehm_leakand-gc boehm, and tcc with-gc boehm_leakand-gc boehm, all printdone.-os windowsstill generates C for-gc boehmandboehm_leak. V3 drops the collector for cross builds, so that C has no GC calls. To inspect the GC path, I built a temporary compiler with that check disabled and generated Windows C with it. I compiled that C with MinGW-w64 gcc 16.2 for-gc boehmand-gc boehm_leak, against the bundledgc.cbuilt with the Windows tcc flags. Both link, and the new code has no warnings. The tcc-targeted C contains the handler with theprint_backtrace()call../v -silent vlib/v/compiler_errors_test.v: 1724 passed, 1 skipped. Same on master.VJOBS=6 ./v -silent test vlib/builtin/: 33 passed, 5 skipped, 2 failed. Same with-gc boehm_leak, and the same on master. The failures arebuiltin_stdout_flushed_by_default_test.vandbuiltin_print_write_error_test.v: their child builds look for the missingthirdparty/tcc/lib/libgc.a. Both pass when run directly with the bundled libgc.VJOBS=6 ./v -silent test vlib/v/tests/fns/: 163 passed, 5 failed. The same 5 fail on master:closure_context_skip_unused_test.v,closure_lifetime_api_test.v,multiline_fn_signature_omitted_comma_test.v.fn_call_mut_ref_args_test.v(segfault) andfn_with_opt_or_res_of_multi_return_test.v(checker error).vlib/v/tests/fns/, with-gc boehm_leakand with-gc boehm: the only failures are the missing-libgc child builds above.thirdparty/tccpresent:./v -silent vlib/v/compiler_errors_test.v: 1720 passed, 5 skipped.VJOBS=6 ./v -silent test vlib/builtin/: 35 passed, 5 skipped.VJOBS=6 ./v -silent test vlib/v/tests/fns/: 166 passed, 2 failed. The failures arefn_call_mut_ref_args_test.vandfn_with_opt_or_res_of_multi_return_test.v, which fail the same way on master.closure_lifetime_api_test.v: 16/16. The new test's child really runs and exits 0.-gc boehmand-gc boehm_leakexecutables, and for a-gc boehm -sharedlibrary, all with-arch amd64. All three compile with MinGW-w64 gcc 16.2 (-Wall), with no warnings in the new code. Not run on Windows.builtin: write GC abort messages without allocating on Windows):./v -silent vlib/v/compiler_errors_test.v: 1720 passed, 5 skipped.VJOBS=6 ./v -silent test vlib/builtin/ vlib/v/tests/fns/closure_lifetime_api_test.v: 36 passed, 5 skipped.-arch amd64) for-gc boehm,-gc boehm_leak,-gc noneand-gc boehm -sharedcompiles with MinGW-w64 gcc 16.2 (-Wall), with no warnings in the changed code.