ci: adopt shared tccbin conformance test suite (macos-amd64) - #74
Merged
GGRei merged 30 commits intoJul 27, 2026
Conversation
Wires this branch into the shared cross-platform conformance suite (thirdparty/tccbin_tests in vlang/v) already running on thirdparty-windows-amd64/linux-amd64/macos-arm64's CI - same shared/hello.c, gc_alloc.c, crash.c tests. No rebuild here - this validates the already-committed tcc.exe/lib/libgc.a as-is. GC linked via the static libgc.a this branch ships (no .dylib on amd64, unlike arm64 - see vlib/builtin/builtin_d_gcboehm.c.v's macos+tinyc+amd64 branch in vlang/v, which uses the same static path). Unverified locally (no Intel Mac access here) - relying on GitHub's hosted macos-13 runner for real verification.
quaesitor-scientiam
changed the title
|
@codex review |
![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: f8164cdadf
ℹ️ 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".
macos-13 no longer exists as a hosted runner image, so this job would sit unassigned/queued indefinitely rather than actually running - matches update_tccbin.yml's own already-established macos-amd64 config (os: macos-15-intel), which should have been used from the start instead of guessing a new label. Found by Codex review (pullrequestreview-4779652865).
…MPROTECT_VDB) to isolate the trigger
…ch trace with -B)
…stale bundled binary This branch's committed tcc.exe is version 0.9.27 with no build_source_hash.txt/build_version.txt provenance at all (unlike every other actively-maintained platform branch), and its bundled lib/libc.dylib is a symlink to /System/DriverKit/usr/lib/libSystem.dylib - an intentional workaround for macOS Big Sur (per the official build script's own "needed for Big Sur" comment), but that path no longer exists on current macOS (confirmed: otool reports "No such file or directory" on macos-15-intel). No CI invocation flag can fix a broken symlink baked into the committed binary. Rebuilds from current tinycc (mob) using vlang/v's own official thirdparty-macos-amd64_tcc.sh (the same script update_tccbin.yml already uses for this exact platform), reusing this branch's already-committed libgc.a (the script's own rsync step preserves it - no GC rebuild needed). Also fixes the macos-13 -> macos-15-intel runner label (Codex review pullrequestreview-4779652865) and adds artifact upload of the rebuilt binaries.
…rward rsync doesn't fail on this branch's first rebuild
The freshly-rebuilt tcc.exe can't parse the branch's old bundled
libgc.a at all ("unrecognized file type") - a real archive/object
format mismatch between the ancient libgc.a and the new tcc, the same
class of old-lib-vs-new-compiler incompatibility as windows-amd64's
original tinyc_getbp issue, just without an existing community fix to
lean on here. Rebuilds libgc.a from current bdwgc master, adapting
thirdparty-macos-arm64_bdwgc.sh (there's no official amd64 variant in
vlang/v - the script is otherwise architecture-generic).
Also repoints the official build script's stale "## needed for Big
Sur" symlink (-> /System/DriverKit/usr/lib/libSystem.dylib, confirmed
nonexistent on current macOS) at /usr/lib/libSystem.B.dylib - the one
system library Apple has kept as a real on-disk compatibility shim.
…ssue and fix as freebsd-amd64)
…sing limitation, matches macos-arm64)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ebc417599
ℹ️ 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".
…en XFAIL, test checked-in binaries) Fixes 3 of 4 findings from pullrequestreview-4780178390 on vlang#74: - P1: prefer HTTPS (with retries for known transient flakiness) over the unauthenticated git:// transport when fetching tinycc source; only fall back to git:// - loudly - if HTTPS is genuinely unreachable - P2: harden the libgc.a XFAIL lane to also assert the actual captured tcc stderr mentions the specific unresolved GC_init reference, not just the run.sh summary-line shape (which the pinned pre-#27935 run.sh can't distinguish from an unrelated static-link regression) - P1: add a lane testing the checked-in tcc.exe/libgc.a as distributed, before the rebuild steps replace them - reports the result without blocking for now (this PR doesn't itself update the committed binaries, so the exact known-broken signature isn't hardened yet); will harden into a signature-matched XFAIL once real CI output is in The 4th finding (macOS amd64 V builds still select the static libgc.a, which this workflow's own XFAIL lane proves can't link) needs a coordinated follow-up: ship a working libgc.dylib to this branch, then update vlib/builtin/builtin_d_gcboehm.c.v to select it - in that order, since flipping the V builder first would break every current macOS amd64 tinyc+boehm build against the still-static-only binaries this branch distributes today. Tracked as a follow-up, not done in this CI-only PR.
quaesitor-scientiam
pushed a commit
to quaesitor-scientiam/tccbin
that referenced
this pull request
Jul 25, 2026
…st the run.sh summary shape Fixes P2 finding from pullrequestreview-4780219278 on vlang#75: the pinned run.sh's summary-only match couldn't distinguish the known etext/end bug from a different static-link regression producing the same PASS-crash/FAIL-gc_alloc/FAIL-hello shape (e.g. a PR replacing libgc.a with a valid-but-incomplete archive). Now compiles hello.c directly to capture real tcc stderr and requires it specifically mention an unresolved etext/end symbol before accepting the XFAIL. Mirrors the identical fix just applied to macos-amd64's libgc.a XFAIL lane for the same class of finding (pullrequestreview-4780178390 on vlang#74).
…apture needs || true, tcc's failure is expected)
…xact XFAIL signature
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd41c2c453
ℹ️ 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".
…cure fallback, exact SIGSEGV check, refresh provenance) Fixes all 5 findings from pullrequestreview-4780817355 on vlang#74: - P1: harden "verify checked-in tcc.exe" into a real signature-matched XFAIL (previously reported without validating anything at all) - asserts the known "0 passed, 3 failed" summary, confirmed via a real CI run - P1: remove the git:// fallback entirely instead of using it after HTTPS retries - an active attacker could just block HTTPS to force the fallback, so a warning-then-downgrade doesn't actually mitigate the supply-chain risk the earlier fix (pullrequestreview-4780178390) was meant to address; fail the job for real instead - P2: require exit code exactly 139 (SIGSEGV) in the no-GC fallback check, not just nonzero, so a binary that can't even start (missing loader/library) can't be mistaken for "the expected crash" - P2: validate BOTH gc_alloc.c's and hello.c's direct compile errors in the libgc.a XFAIL, not just gc_alloc.c's, so a regression isolated to hello.c alone can't hide behind the shared summary shape (same class of finding as pullrequestreview-4780815399 on the sibling freebsd-amd64 workflow, vlang#75) - P2: overwrite the stale lib/libgc_build_cmd.txt (which described the OLD universal x86_64/arm64 build) with the actual amd64-only configure invocation this step uses, so uploaded artifacts carry accurate provenance
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b43268972
ℹ️ 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".
…, warn on packaged libgc.a, pin libatomic_ops provenance, deployment target, exclusive symbol match) Fixes all 5 findings from pullrequestreview-4780918023 on vlang#74: - P1: harden the checked-in-tcc.exe XFAIL beyond the aggregate "0 passed, 3 failed" count - now compiles crash.c directly and requires the known "library 'c' not found" signature (the broken libc.dylib symlink already root-caused earlier this session) - P1: warn against publishing the unlinkable static libgc.a as a ready distribution - adds a KNOWN_ISSUES.txt into the packaged artifact explaining the dylib is the verified-working link path - P2: record libatomic_ops's resolved commit alongside bdwgc's in provenance (it was cloned from a moving branch but untracked) - P1: pin MACOSX_DEPLOYMENT_TARGET=10.13 for both the tcc and libgc rebuilds, and surface the checked-in binary's actual load-command minimum via otool for future refinement - without it, clang stamps rebuilt binaries with the CI runner's own macOS 15 minimum - P2: the libgc.a XFAIL's GC_init check now also rejects any unresolved-reference symbol that isn't GC_-prefixed, not just requiring GC_init's presence - closing the same class of gap as pullrequestreview-4780907186 on the sibling freebsd-amd64 workflow
… real outcomes, positive no-GC check, broader exclusivity check, deployment target in provenance) Fixes 2 findings from pullrequestreview-4781470066 on vlang#74: - P2: package/upload steps stay `!cancelled()` (still publish a debug artifact on failure, matching macos-arm64's own rationale), but the bundled KNOWN_ISSUES.txt's claim that the dylib link path is "verified working" is now conditional on the dylib_test/nogc_test steps' actual outcomes, read via new step `id`s - verified against both success and failure outcome combinations - P2: lib/libgc_build_cmd.txt now records MACOSX_DEPLOYMENT_TARGET alongside the configure invocation, so reproducing this build from the recorded command actually matches its compatibility promise Fixes 2 findings from pullrequestreview-4781468264 on the sibling freebsd-amd64 workflow, vlang#75, same class of gap present here: - P2: the libgc.a XFAIL's exclusivity check now matches on tcc's `tcc: error:` line prefix instead of only "unresolved reference to" lines, so a wholly different KIND of tcc error (not just a differently-named symbol) riding alongside the known GC_init error can no longer hide behind the check - verified empirically: the old narrower check accepted a mocked known-symbol-plus-unrelated-error case, the new one correctly rejects it - P2: added a positive no-GC check (compile+run a trivial non-crashing program, require exit 0) alongside the existing crash.c-must- SIGSEGV check, so a broken startup path that crashes EVERY program can't hide behind "the crash happened as expected" - verified empirically against a mocked always-crashing tcc: old logic would have passed, new logic correctly fails
quaesitor-scientiam
pushed a commit
to quaesitor-scientiam/tccbin
that referenced
this pull request
Jul 26, 2026
…-GC check) Fixes both findings from pullrequestreview-4781468264 on vlang#75: - P2: the libgc.a XFAIL's exclusivity check now matches on tcc's `tcc: error:` line prefix instead of only "undefined symbol" lines, so a wholly different KIND of tcc error riding alongside the known etext/end error can no longer hide behind the check - verified empirically: the old narrower check accepted a mocked known-symbol- plus-unrelated-error case, the new one correctly rejects it - P2: added a positive no-GC check (compile+run a trivial non-crashing program, require exit 0) alongside the existing crash.c-must-SIGSEGV check, so a broken startup path that crashes EVERY program can't hide behind "the crash happened as expected" - verified empirically against a mocked always-crashing tcc: old logic would have passed, new logic correctly fails Same class of gap also fixed on the sibling macos-amd64 workflow (pullrequestreview-4781470066 on vlang#74).
…IL probe exit codes Fixes a real CI failure (run 30200458603): the HTTPS-reachability probe (git ls-remote) succeeded on retry, but the official build script's own git clone then failed anyway - "Failed to connect to repo.or.cz port 443... Couldn't connect to server" - and with the insecure git:// fallback already removed for security reasons, there was nothing left to recover with. A lightweight probe succeeding doesn't guarantee the much heavier clone moments later will also succeed. Now retries the ACTUAL build script invocation (not a decoupled proxy check) up to 3 times, but only when the failure specifically matches a network-connection signature (Failed to connect, Couldn't connect to server, Could not resolve host, Connection reset by peer, Recv failure, Connection timed out) - a genuine build bug (a real gmake or configure error) still fails fast instead of wasting 3 attempts on something retrying can't fix. Safe to retry: the script itself does `rm -rf tinycc/` unconditionally as its first action, and the network operation happens before any step that mutates $TCC_FOLDER, so no partial state survives across attempts. Verified against 3 mocked scenarios: network-failure-then-success, unrelated-bug-fails-fast, and network-failure-exhausts-all-retries. Also fixes 2 findings from pullrequestreview-4781865117 on the sibling freebsd-amd64 workflow, vlang#75 (same class of gap, applied proactively here): the libgc.a XFAIL's direct-error probes and the checked-in-binary probe used `|| true`, discarding the probe's own exit code entirely - a regression that made tcc print the expected error text and then crash/abort (SIGSEGV, SIGABRT) would still pass. Now captures the real exit code alongside the text and rejects any signal-terminated exit (128+signal) even when the expected text is present. Verified empirically: a mocked "expected text + SIGSEGV exit" case passed under the old logic, and is correctly rejected now.
|
@codex review |
GGRei
pushed a commit
that referenced
this pull request
Jul 26, 2026
* ci: adopt shared tccbin conformance test suite Wires this branch into the shared cross-platform conformance suite (thirdparty/tccbin_tests in vlang/v) via a real FreeBSD VM (cross-platform-actions/action, same tooling vlang/v's own update_tccbin.yml uses to rebuild this branch). No rebuild here - validates the already-committed tcc.exe/lib/libgc.a as-is. Unverified locally (no FreeBSD access here) - V's own builder also references -lgc-threaded for this platform+tcc combo (vlib/builtin/builtin_d_gcboehm.c.v), which isn't in this branch's bundle; starting without it since the bundled libgc.a should be self-sufficient, and will adjust based on real CI results if not. * ci: add temporary diagnostic step to surface the compile error * ci: add temporary job to verify candidate tinycc etext/end fix * ci: retry (repo.or.cz connection reset on prior attempt) * ci: clone+patch tinycc on the runner, not inside the VM (repo.or.cz connection reset there twice) * ci: fix patch file path (git -C changes cwd, relative path was wrong) * ci: use git:// protocol for repo.or.cz (https is currently broken there, confirmed independently) * ci: use system cc (clang) instead of gcc, matching update_tccbin.yml's freebsd config * ci: run gmake install so the configured --prefix path actually exists * ci: finalize with XFAIL lane for the tcc etext/end limitation tcc on FreeBSD cannot currently link the bundled libgc.a (undefined symbol 'etext'/'end' - see tccelf.c's tcc_add_linker_symbols, which only defines the glibc-style _etext/_edata/_end, never the plain BSD-style names BDWGC's FreeBSD code references). Root-caused and a fix verified against a real FreeBSD VM, being submitted upstream to tinycc-devel - but it isn't in any shipped tcc.exe yet. crash.c (no GC) is checked directly as the real blocking regression check. The GC-dependent tests (hello.c, gc_alloc.c) run via the full shared suite as an explicit, temporary XFAIL lane - visible in CI logs so a regression that fails *differently* is still noticed, but not blocking. Remove continue-on-error once the tcc fix ships in a rebuilt tcc.exe here. Removes the temporary etext-fix verification job now that the fix is confirmed and the real CI reflects reality. * ci: test the PR's own revision, and require SIGSEGV specifically Two issues found by Codex review (pullrequestreview-4779654227): - The hardcoded `git clone --branch thirdparty-freebsd-amd64 .../vlang/tccbin.git` always tested the already-merged target branch, regardless of what a PR actually proposed - a PR updating tcc.exe/libgc.a could pass CI while testing the old, unrelated binaries. Now resolves the PR's own head repo+ref for pull_request events, falling back to the triggering ref for push/workflow_dispatch (which have no PR context). - The no-GC crash check only verified "nonzero exit", which an exec-format error, missing dynamic loader, or an unrelated miscompilation exiting 1 would also satisfy - silently masking a real regression, since the GC-backed suite is explicitly non-blocking. Now requires exit 139 specifically (killed by SIGSEGV), matching crash.c's actual intended fault. * ci: fix environment_variables forwarding (needs job-level env, not step-level) * ci: fix job-level env referencing steps context (schema violation - GH Actions doesn't allow it) * ci: move env to job level (step-level env on a uses: step wasn't forwarded by environment_variables) * ci: fix Codex-flagged CI hardening issues (pin SHA, pin action, fetch pinned suite commit, harden XFAIL) Fixes 3 P2 + 1 P1 findings from pullrequestreview-4780048339 on #75: - P1: fetch the pinned vlang/v suite commit directly instead of a plain --depth=1 clone-then-checkout, which only ever has the current tip's objects and would start failing the moment vlang/v advances past the pinned commit - P2: clone the exact triggering commit (SHA) instead of a mutable ref name, which was racy against a push landing while this run was queued - P2: fall back to the triggering event's own repository for non-PR runs instead of a hardcoded vlang/tccbin.git - P2: assert the exact known etext/end failure signature in the libgc.a XFAIL lane instead of a blanket continue-on-error, so an unrelated regression (corrupted archive, bad checkout) can't silently pass as the known XFAIL Also pins cross-platform-actions/action to a commit SHA instead of the mutable v1.3.0 tag (same class of finding as the sibling openbsd-amd64 workflow, pullrequestreview-4780049532 on #76). * ci: fix mkdir (needs -p, thirdparty/ parent dir doesn't exist yet in a fresh VM) * ci: harden libgc.a XFAIL to assert the actual etext/end error, not just the run.sh summary shape Fixes P2 finding from pullrequestreview-4780219278 on #75: the pinned run.sh's summary-only match couldn't distinguish the known etext/end bug from a different static-link regression producing the same PASS-crash/FAIL-gc_alloc/FAIL-hello shape (e.g. a PR replacing libgc.a with a valid-but-incomplete archive). Now compiles hello.c directly to capture real tcc stderr and requires it specifically mention an unresolved etext/end symbol before accepting the XFAIL. Mirrors the identical fix just applied to macos-amd64's libgc.a XFAIL lane for the same class of finding (pullrequestreview-4780178390 on #74). * ci: fix set -e aborting before the XFAIL case statement (direct_err capture needs || true, tcc's failure is expected) * ci: validate both gc_alloc.c and hello.c's direct errors in the libgc.a XFAIL, not just one Fixes P1 finding from pullrequestreview-4780815399 on #75: checking only hello.c's direct compiler error let a regression isolated to gc_alloc.c alone hide behind the same "PASS crash, FAIL gc_alloc, FAIL hello" summary shape and pass unnoticed. Now captures and asserts the known etext/end signature for both failing tests independently. * ci: reject additional linker errors alongside the known etext/end signature in the XFAIL check Fixes P1 finding from pullrequestreview-4780907186 on #75: the etext/end substring check accepted the XFAIL even if a regression added a NEW unresolved symbol alongside the known ones. This bug is narrow (only etext/end, unlike macos-amd64's whole-archive-unparseable case) so an exact match is correct: reject if any undefined-symbol line names something other than etext/end. * ci: fix 2 more Codex findings (broader exclusivity check, positive no-GC check) Fixes both findings from pullrequestreview-4781468264 on #75: - P2: the libgc.a XFAIL's exclusivity check now matches on tcc's `tcc: error:` line prefix instead of only "undefined symbol" lines, so a wholly different KIND of tcc error riding alongside the known etext/end error can no longer hide behind the check - verified empirically: the old narrower check accepted a mocked known-symbol- plus-unrelated-error case, the new one correctly rejects it - P2: added a positive no-GC check (compile+run a trivial non-crashing program, require exit 0) alongside the existing crash.c-must-SIGSEGV check, so a broken startup path that crashes EVERY program can't hide behind "the crash happened as expected" - verified empirically against a mocked always-crashing tcc: old logic would have passed, new logic correctly fails Same class of gap also fixed on the sibling macos-amd64 workflow (pullrequestreview-4781470066 on #74). * ci: preserve XFAIL probe exit codes, not just the captured text Fixes P2 finding from pullrequestreview-4781865117 on #75: the libgc.a XFAIL's direct-error probes used `|| true`, discarding the probe's own exit code entirely - a regression that made tcc print the expected etext/end diagnostic and then crash/abort (SIGSEGV, SIGABRT) rather than exiting cleanly would still pass, since only the captured text was checked. Now captures the real exit code alongside the text and rejects any signal-terminated exit (128+signal) even when the expected text is present. Verified empirically: a mocked "expected text + SIGSEGV exit" case passed under the old logic and is correctly rejected now. * ci: test the actual PR merge commit, not just the head branch tip in isolation Fixes P2 finding from pullrequestreview-4781865117 on #75: cloning pull_request.head.sha tested the PR's own branch tree in isolation, never the merge result GitHub would actually produce against the current target branch. A PR replacing tcc.exe could pass against its own tree while the actual merge (combined with whatever the target branch has since gained, e.g. a different libtcc1.a from a merged sibling PR) was never tested at all - the required check stays green for a combination that was never verified. Now uses github.sha + github.event.repository.clone_url uniformly for all 3 trigger types: for pull_request events, github.sha is GitHub's own synthesized merge commit, which exists only in the base repo (never a contributor's fork) - so the clone URL switches from the fork to the base repo accordingly. github.sha stays fixed for a run's entire lifetime once the triggering event fires, so this remains exactly as race-safe against a mid-queue push as the previous head.sha-based approach (the original P1 this replaced, from pullrequestreview-4780048339), while also testing what will actually be merged instead of the head tree alone. macos-amd64 doesn't need this fix: it already uses plain actions/checkout@v4 with no explicit ref, which already defaults to checking out the pull_request merge commit automatically. --------- Co-authored-by: Richard Wheeler <18647491+PythonWillRule@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af46fa6aa7
ℹ️ 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".
… errors in the checked-in XFAIL Fixes both findings from pullrequestreview-4782525602 on vlang#74: - P2: the packaged KNOWN_ISSUES.txt unconditionally claimed "lib/libgc.a CANNOT be linked... confirmed via a real CI run" even when the static XFAIL step failed with an unexpected signature or unexpectedly passed in THIS run - the claim didn't reflect what this specific run actually verified, same class of gap already fixed for the dylib/no-GC claims. Added `id: static_xfail_test` and now derives the static-link text from its real outcome the same way. Verified both branches render correctly (all-success, and static-XFAIL-failed). - P2: the checked-in-binary XFAIL accepted the known "library 'c' not found" diagnostic even if a wholly different, unrelated tcc error was ALSO present alongside it - a bare substring-present check never looked at what else came back. Now rejects unless every tcc error line is specifically the known libc diagnostic, matching the same exclusivity pattern already applied to the libgc.a XFAIL probes. Verified empirically: a mocked "known error + unrelated error" case is now correctly rejected.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f08211f30f
ℹ️ 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".
…L on the expected-good path Fixes P1 finding from pullrequestreview-4782839244 on vlang#74, a real CI failure this exact bug caused (run 30225228663): the previous commit's `other_checkedin=$(... | grep -v ...)` was a standalone assignment, not a function invoked from an if-condition. In the EXPECTED case (no other error present), the final `grep -v` legitimately finds nothing to filter and exits 1 (grep's normal "no match" status) - and since GitHub's bash runs with pipefail, that nonzero pipeline status killed the assignment statement itself under `set -e`, silently aborting the whole step with zero output, in exactly the case that should have reached "known XFAIL". Reproduced directly: `bash -c "set -e; x=\$(printf ... | grep -v ...); echo after"` never prints "after". Refactored into is_known_checkedin_error(), a function invoked as an if-condition (`if is_known_checkedin_error ...; then`) - bash suspends errexit for the entire dynamic extent of evaluating an if/while/until condition, including anything a called function does internally, which is exactly why the sibling is_known_gc_init/is_known_etext_end functions never had this problem despite the identical internal grep -v pattern. Verified: reproduced the silent-death bug in isolation first, confirmed the function-based refactor survives both the expected-good case and the known-error-plus-unrelated-error case, and confirmed the already-working is_known_gc_init's `&&`-chained if-condition usage is genuinely errexit-safe too.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c65c6cf9d
ℹ️ 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".
…oth checked-in and rebuilt libgc.a via ar/nm Fixes all 3 findings from pullrequestreview-4783389496 on vlang#74: - P2: is_known_checkedin_error and is_known_gc_init only rejected signal-terminated exits (>128), never exit 0 - a changed tcc.exe reporting the expected diagnostic text as noise while still succeeding overall would still be accepted as the known XFAIL. Verified empirically: exit=0 with the known text is now rejected, exit=1 with the known text is still accepted. - P1: crash.c (the checked-in-binary signature probe) never references any GC symbol, so it never actually exercises thirdparty/tcc/lib/libgc.a - a PR replacing the checked-in libgc.a with a corrupt/empty/wrong-architecture archive would produce the exact same "0 passed, 3 failed" + "library 'c' not found" signature and pass CI undetected. Added a direct ar t / nm -g check on the checked-in archive (confirming it's well-formed and defines GC_init) before the rebuild step overwrites it. - P2: the static libgc.a XFAIL accepted "unresolved reference to '_GC_init'" as proof of tcc's known archive-parsing limitation without ever confirming the REBUILT archive actually defines that symbol - an empty/incomplete bdwgc build would produce the identical error text and get misclassified as the harmless known limitation. Added the same ar t / nm -g check on the rebuilt archive before accepting the XFAIL, so KNOWN_ISSUES.txt's claim that the symbol is "present and well-formed" is now actually backed by verification. Verified the nm -g grep pattern against both the real observed output format from earlier this session (T = defined) and a mocked broken archive (U = undefined only), confirming it distinguishes them correctly. Same exit-0 rejection also applied to the sibling freebsd-amd64 workflow's is_known_etext_end for parity.
quaesitor-scientiam
pushed a commit
to quaesitor-scientiam/tccbin
that referenced
this pull request
Jul 27, 2026
Fixes the same class of gap as pullrequestreview-4783389496 on the sibling macos-amd64 workflow, vlang#74 (applied here proactively): is_known_etext_end only rejected signal-terminated exits (>128), never exit 0 - a changed tcc.exe reporting the expected etext/end diagnostic as noise while still succeeding overall would still be accepted as the known XFAIL.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb7beb0d19
ℹ️ 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".
…ked-in archive architecture; assert tcc.exe's committed file mode Fixes all 3 findings from pullrequestreview-4783470598 on vlang#74, the first of which broke the CI run immediately after the previous commit (run 30234854073): - P1: `nm -g archive | grep -qE ...` is unsafe under GitHub's pipefail bash - grep -q stops reading as soon as it finds a match, which can make nm receive SIGPIPE while still writing the rest of a large archive's symbol list, and pipefail then reports the WHOLE pipeline as failed even though grep found the match. Reproduced directly: a large mocked producer piped through grep -qE under set -eo pipefail reported "not found" despite matching on the very first line. Fixed by capturing nm's full output into a variable first (command substitution fully drains the producer, no early exit possible) and grepping the captured text without -q. Applied to both the checked-in and rebuilt archive checks, as flagged. - P1: a well-formed, correctly-symboled archive for the WRONG architecture (e.g. an accidentally-committed arm64 build) would have passed both the ar t and nm checks, since neither validated architecture, and the crash.c signature probe never touches libgc.a at all. Added a `lipo -info` check requiring x86_64 before trusting anything else about the checked-in archive. - P2: the unconditional `chmod +x thirdparty/tcc/tcc.exe` silently repaired a potentially-wrong committed file mode before the "as distributed" verification ran against it - a PR that accidentally committed tcc.exe as non-executable would still pass CI, even though a real consumer checking out that exact commit gets the actual (broken) mode. Replaced with an assertion that the committed mode is already correct. Also fixed a second occurrence of the same "standalone grep assignment under set -e" bug this exact PR already hit once (pullrequestreview- 4782839244): the new `grep -E` (without -q) capture still needs its own `|| true`, since a genuinely missing symbol makes grep exit 1 with nothing to catch it. Verified the complete fix against 3 scenarios: symbol present (small output), symbol genuinely missing, and symbol present with a large trailing output (the SIGPIPE-prone case) - all three now report correctly and the script completes without dying.
…y swallowed the error text)
…alidating it with ar/nm Root-caused via the diagnostic pushed in commit 442b25b: the checked-in libgc.a is a FAT/universal binary containing BOTH x86_64 and arm64 slices ("Mach-O universal binary with 2 architectures"), not a plain single-arch archive. macOS's `ar` explicitly refuses to read a fat archive directly ("ar: ... is a fat file (use libtool(1) or lipo(1) and ar(1) on it)"), which is why every previous version of this check failed on every run regardless of the archive's actual (valid) contents - it wasn't a code bug or a real regression, just an untested assumption about this specific file's format. Replaced the separate `ar t` + `lipo -info` substring-match approach (which could also have been fooled by an arm64-only archive that still contains the literal text "x86_64" somewhere in a multi-arch `lipo -info` listing) with `lipo -thin x86_64 ... -output ...`: this extracts the x86_64 slice explicitly - failing if it doesn't exist, closing the architecture-verification gap - and produces a normal thin archive that `ar t`/`nm -g` can then read directly, closing the well-formed-archive and GC-symbol-defined checks the same way.
…n a binary was actually produced
Root-caused via a real CI run: this ancient checked-in v0.9.27
tcc.exe exits 0 while still printing "tcc: error: library 'c' not
found" and producing NO runnable binary - a genuine quirk/bug of this
specific old build, not a new regression. The previous commit's blanket
"reject exit 0" was too broad and made this check permanently fail on
this branch's own known-good state.
The original finding's actual concern (Codex pullrequestreview-
4783389496) was specifically "a changed tcc.exe... while still
reporting overall success... without producing the requested
executable" - i.e. exit 0 AND an actual working binary is the real
problem, not exit 0 alone. Now checks whether the probe's output file
is executable and only rejects the exit-0 case when combined with
that:
if [ "$2" -eq 0 ] && [ "$3" -eq 1 ]; then
return 1 # genuinely succeeded despite the noise - real problem
fi
Verified against all 3 scenarios: exit=0 with no binary produced (the
observed old-tcc behavior) is now accepted, exit=0 with a binary
produced is still rejected, and the normal exit=1 case is unaffected.
Left is_known_gc_init (the rebuilt/modern tcc's equivalent check)
unchanged for now - no evidence yet that the actively-maintained mob
branch has the same exit-code quirk as this 15+-year-old checked-in
binary, and speculative changes without a reproduced failure aren't
worth the added risk on an already-long fix chain.
…'s exit-code/output-file behavior is unreliable, not a meaningful signal Root-caused via a second real CI run: this ancient, checked-in v0.9.27 tcc.exe not only exits 0 while printing "tcc: error: library 'c' not found" (already observed once), it can ALSO leave behind an executable-permission output file in that same state - so neither "exit 0" nor "exit 0 AND a file exists at -o" reliably distinguishes "genuinely still broken as expected" from "secretly still working" for this specific 15+-year-old build's link-error handling. The exit-0 concern (Codex pullrequestreview-4783389496) is legitimate in principle - a CHANGED tcc.exe reporting false success is a real risk - but the outer `case "$output" in *"0 passed, 3 failed"*)` already independently confirms via run.sh's own compile+run cycle that all 3 shared tests genuinely failed; a real "tcc got un-broken" scenario would show up there as passing tests, not hidden in this one probe's exit code. This function's actual job is narrower: given the aggregate already failed, confirm the SPECIFIC cause via the error text, which two consecutive real CI runs now show is a reliable signal even when this old binary's exit code and file-production behavior for the same error are not. is_known_gc_init (the rebuilt/modern tcc's equivalent check) keeps its exit-0 rejection - the actively-maintained mob branch has shown no evidence of this quirk, unlike this fixed historical artifact.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
GGRei
pushed a commit
that referenced
this pull request
Jul 27, 2026
Fixes the same class of gap as pullrequestreview-4783389496 on the sibling macos-amd64 workflow, #74 (applied here proactively): is_known_etext_end only rejected signal-terminated exits (>128), never exit 0 - a changed tcc.exe reporting the expected etext/end diagnostic as noise while still succeeding overall would still be accepted as the known XFAIL. Co-authored-by: Richard Wheeler <18647491+PythonWillRule@users.noreply.github.com>
quaesitor-scientiam
pushed a commit
to quaesitor-scientiam/tccbin
that referenced
this pull request
Jul 28, 2026
…ked mismatch as non-blocking Addresses two Codex findings on vlang#82: P1: the previous version of this fix downgraded ANY checked-in-binary failure to a warning whenever build_source_hash.txt was present, with no positive evidence the binary actually works. That let a genuinely broken bot-rebuilt tcc.exe pass CI silently, since the later rebuild-then-test lanes exercise a freshly-built binary from current mob, not the one this commit actually ships - reintroducing the exact gap the original "verify checked-in tcc.exe" step (vlang#74) was written to close. Now require the checked-in binary to compile, link, AND run a trivial no-GC program to completion (the same bar the "verify no-GC fallback path" step already holds the rebuilt binary to) before a signature mismatch is treated as informational; a binary that fails this sanity check stays a hard failure regardless of provenance. P3: a provenance-tracked binary that fully passes (code 0) logged success but then fell through into the failure-classification case statement anyway, which - since a passing run's output doesn't match "0 passed, 3 failed" - hit the default branch and emitted a contradictory ::warning:: claiming failure. Guard the case statement to skip entirely once a provenance-tracked pass is already accepted. Verified both fixes directly: 6 scenarios (provenance pass, provenance failure+sane, provenance failure+broken, legacy-match, legacy-mismatch, legacy-pass) each produce the intended result, with the 3 legacy-binary cases behaving byte-for-byte as before this change.
quaesitor-scientiam
pushed a commit
to quaesitor-scientiam/tccbin
that referenced
this pull request
Jul 28, 2026
…ary failures unconditionally blocking Three successive attempts on this PR (58f7e17, 29d5da8, 17dfad9) tried to let a provenance-tracked tcc.exe's conformance mismatch be downgraded to a warning via progressively narrower proxy checks - a no-GC compile, then also a GC-allocation test (circular: the same test run.sh had already run against the same artifacts), then a deferred test against a freshly-rebuilt libgc.dylib. Codex correctly rejected each one on the same grounds: none of them test what a real consumer checking out this exact commit actually receives (this checked-in tcc.exe paired with this checked-in libgc.a, not some other libgc nobody ships), so none of them can license treating a real, accurate failure signal as informational. There is no proxy that resolves this without changing what's actually being verified. Revert to the simple, original vlang#74 design: any checked- in tcc.exe/libgc.a failure that doesn't match the ancient binary's exact known signature is a hard failure, full stop - whether or not build_source_hash.txt is present. That file is now used only to annotate failure messages with a diagnostic hint (update_tccbin.yml rebuilds tcc.exe without ever rebuilding libgc.a to match, which is the likely root cause), never to relax the check itself. This means d712d0f - the current tip of this branch, and the reason this PR exists - now correctly fails CI again, honestly. That failure reflects a genuine defect in the distributed pair, not a false alarm: fixing it requires update_tccbin.yml (in vlang/v) to also rebuild libgc.a whenever it rebuilds tcc.exe, or a human to do so manually - not a CI workaround. Verified the reverted logic end-to-end (extracted from the real YAML, mocked tools) across 5 scenarios: legacy-match, legacy-mismatch, provenance-mismatch (now correctly hard-fails), provenance-full-pass (still demands XFAIL removal, unchanged), and provenance non-aggregate shape. No dangling references to the removed sanity-check/deferred- verification machinery remain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wires this branch into the shared cross-platform conformance suite
(
thirdparty/tccbin_testsin vlang/v) already running onthirdparty-windows-amd64/linux-amd64/macos-arm64's CI — same
shared/hello.c,gc_alloc.c,crash.ctests.No rebuild here — validates the already-committed
tcc.exe/lib/libgc.aas-is. GC linked via the static
libgc.athis branch ships (macOSamd64 doesn't have arm64's static-link limitation — see
vlib/builtin/builtin_d_gcboehm.c.v's macos+tinyc+amd64 branch, whichuses this same static path).
I don't have Intel Mac access to verify this locally — relying on
GitHub's hosted
macos-13runner for real verification instead.TODO: same as the other platforms — this points at
vlang/vat apinned commit since sparse-checkout can't target an unmerged PR
anymore now that vlang/v#27924 has merged (already using the real repo
here, no fork needed for this one).