Repository navigation
cgen: preserve standalone poll and native-cache helpers - #32297
Conversation
Codex Review Summary: ❌ FailedThis 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
medvednikov
left a comment
medvednikov
left a comment
There was a problem hiding this comment.
Reviewed d6fec3c4beb844d9e53307d066c02d26baaa6f2d for correctness. CI status and logs were excluded.
One C++ linkage issue in the new headerless native-cache helper is noted inline.
Rebuilt the candidate compiler and passed fd_set_poll_test.c.v and native_object_cache_noreplace_test.c.v on Linux. Native C probes also passed publication, no-replacement, fallback, and error-propagation checks. The SDK-backed include retains the original header's directory, so its relative implementation include resolves correctly.
For the C++ finding, a paired native test of the exact helper block links with the baseline SDK header, fails with the candidate's raw declarations, and links after adding C linkage to those two declarations. This is a reduced reproduction: the ordinary driver unit-test fixture selected SDK headers, and a full V driver C++ build was not completed.
| g.writeln('int link(char*, char*);') | ||
| g.writeln('#if defined(__linux__)') | ||
| g.writeln('long syscall(long, ...);') |
There was a problem hiding this comment.
[P2] Preserve C linkage for the standalone libc prototypes
When a driver/compiler unit takes this headerless path with the C++ backend (-cc g++), these raw declarations acquire C++ linkage. The preceding headerless_libc_preamble does not declare either function, and collect_preserved_c_fns below suppresses their normal declarations through cpp_c_linkage. The embedded publication helper consequently refers to mangled link(char*, char*) and syscall(long, ...) symbols instead of libc's C symbols.
A native C++ comparison using this exact helper block links with the baseline SDK-backed header, fails on the candidate with those two undefined references, and links after adding extern "C" to just these declarations. This is a reduced helper reproduction, not a completed end-to-end V C++ build. Please emit these prototypes through g.cpp_c_linkage or an equivalent C-linkage guard.
medvednikov
left a comment
medvednikov
left a comment
There was a problem hiding this comment.
Re-reviewed d6fec3c4beb844d9e53307d066c02d26baaa6f2d, ignoring CI status and logs.
Two P2 issues remain:
- The existing C++ linkage finding is confirmed on an actual generated compiler translation unit. With this exact-head compiler,
./v -cc g++ -gc none -nocache -o compiler.c cmd/vselects the new headerless native-cache helper. G++ rejects the generated compiler because its rawsyscall(long, ...)declaration has C++ linkage and conflicts with the laterextern "C"declaration used by the gettid compatibility helper. The same generation and syntax check passes at the exact base. Wrapping only the two new libc prototypes inextern "C"in a scratch copy also makes the candidate pass. Please useg.cpp_c_linkage; the existing thread is #32297 (comment). - The newly embedded implementation is missing from the compiler runtime-input fingerprint. The focused existing coverage test fails on the new embed, and runtime-reading backend consumers need this input in the cache salt. See the new inline finding.
Validation: rebuilt the exact candidate compiler; compared complete candidate/base generated cmd/v translation units with G++ 14.2.0 (-std=gnu++20 -fpermissive -w -fsyntax-only -x c++); ran test_module_cache_compiler_runtime_inputs_cover_compiler_embedded_files with the exact-head compiler and fallback disabled. The generated C++ was syntax-checked, not linked or executed. No stale cached binary reproduction is claimed.
| g.writeln('long syscall(long, ...);') | ||
| g.writeln('#endif') | ||
| g.writeln('#endif') | ||
| g.write($embed_file('@VEXEROOT/vlib/v/driver/native_object_cache_impl.c').to_string()) |
There was a problem hiding this comment.
[P2] Fingerprint the newly embedded native-cache implementation
This new $embed_file target is absent from v3_cache_compiler_runtime_inputs in driver.v. That list contributes the running backend's external inputs to the cache salt for development/older-bootstrap consumers that read embedded files at runtime; changing this implementation would leave that part of the cache identity unchanged. Please add vlib/v/driver/native_object_cache_impl.c to the list alongside the other embedded runtime inputs. The existing focused test test_module_cache_compiler_runtime_inputs_cover_compiler_embedded_files reproducibly fails at cache_prune_test.v:206 on this exact head and names this new embed. Modern compiler self-builds that embed the bytes are separately covered by executable identity, but that does not cover the runtime-reading cases.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
CI-repair follow-up on current head Native validation still needed on this exact head before a compiler/runtime squash merge: |
The shipped os poll header forced otherwise standalone programs into SDK-backed declarations, breaking poll bindings and cache reuse. The native-cache publication insert separately introduced headers into portable compiler snapshots.
Recognize these exact shipped interfaces and supply their standalone implementations in the generated preamble. Share cache-publication implementation with the SDK-backed header, preserving atomic hard-link publication and Linux RENAME_NOREPLACE fallback. No C headers are scanned or expanded by V.
Validation: poll coexistence and warm-cache regressions, mmap ownership, cache builtin reuse, full headerless compiler tests (including cross-target Clang checks), and full vself tests pass. The native publication tests and a headerless mock-Linux fallback/error-routing harness pass. The aggregate compiler-error suite passed 1,886 cases with one skip, and all 104 output fixtures completed without errors. Validation is limited to these and the focused checks above; the full compiler suite and paired instruction comparison remain incomplete. No test expectations are relaxed.