fix(as370): restore the -Werror build on modern GNU gcc, and add CI to keep it - #47
Merged
Conversation
Two separate diagnostics broke the project's documented build line on current GNU gcc, while Apple clang never saw either -- so 'warning-clean under -Werror' only held on one toolchain and a contributor on gcc could not build at all. -Wdangling-pointer (#11): expr_val() parked its cursor argument in the file static xp_, and callers pass stack buffers, so on return xp_ pointed at storage that had gone out of scope. Nothing outside the expression evaluator ever reads xp_ -- the only uses are lines 228..283 -- so this was latent rather than live, but the store is genuinely invalid and gcc is right to reject it. Clear xp_ before returning; the early-return path is folded into the same exit so there is one place to do it. -Wformat-overflow (#33): the deck sequence number was printed through a format string built at run time (sprintf(fmt, "%%0%dld", nd)), which gcc cannot bound. Take the width from the argument instead -- snprintf(d, sizeof d, "%0*ld", nd, ...) -- which is the same output with no constructed format and no fmt[] at all. Verified with gcc 16.1 at -O2 -Wall -Wextra -Werror (stricter than the CI runner: it reports #11, and does not happen to reproduce #33) and with clang. Output is unchanged where it matters: 'make test-as370' still reports ALL SAMPLES BYTE-IDENTICAL TO IFOX00, and the libc370 corpus check produces output identical byte-for-byte before and after this commit. Closes #11 Closes #33 Claude-Session: https://claude.ai/code/session_018SbqQkXrAyDhVgcF8X8cdz
cc370 had no CI, while mbt's reusable workflow builds this repo's main for every downstream project -- so a broken main went out to the whole ecosystem and came back as a red build in mvsmf. That is the wrong direction for the signal to travel. Scope is the five standalone tools and their suites, not the GCC fork, since that is where the regressions have been and 'make compiler' takes minutes. Both compilers build the tools on purpose: the -Wformat-overflow break that prompted this reached main because Apple clang does not implement that warning, so a clang-only check cannot defend -Werror. libc370 is cloned as a sibling because four as370 samples assemble against its macro libraries; nothing is built from it. test-corpus stays out (its SHA manifest has drifted from libc370's current sources and it is red regardless of any change here) and so does test-cc370 (needs the GCC fork). Claude-Session: https://claude.ai/code/session_018SbqQkXrAyDhVgcF8X8cdz
…rness 61b4e99 renamed the crent370 references but left the shell variable itself called CRENT in tests/run.sh and tests/listref/check.sh, while the Makefile and tests/corpus/check.sh had already moved to LIBC370. Two names for one thing, and the stale one points at a libc that is frozen and no longer checked out. Rename to LIBC370 everywhere so the whole harness reads the same. The default (../../libc370) is unchanged; anyone passing CRENT= explicitly now falls back to that default, which fails loudly with 'Undefined operation code ... PDPPRLG' rather than silently, if their checkout lives elsewhere. Verified all three paths: default, LIBC370= override, and a bad path. Claude-Session: https://claude.ai/code/session_018SbqQkXrAyDhVgcF8X8cdz
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
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.
Closes #11
Closes #33
The two build breaks
Both diagnostics broke the project's documented build line on current GNU gcc while Apple clang saw neither — so "warning-clean under
-Werror" only ever held on one toolchain, and a contributor on gcc could not build at all.-Wdangling-pointer(#11).expr_val()parked its cursor argument in the file-staticxp_, and callers pass stack buffers, so on returnxp_pointed at storage that had gone out of scope. Nothing outside the expression evaluator readsxp_— its only uses are lines 228..283 — so this was latent rather than live, but the store is genuinely invalid and gcc is right to reject it. The cursor is now cleared before returning, with the early-return path folded into the same exit so there is one place to do it.-Wformat-overflow(#33). The deck sequence number went through a format string built at run time,sprintf(fmt, "%%0%dld", nd), which gcc cannot bound. The width now comes from the argument instead —snprintf(d, sizeof d, "%0*ld", nd, ...)— same output, no constructed format, andfmt[]disappears entirely.Verification
Built with gcc 16.1 at
-O2 -Wall -Wextra -Werror, which is stricter than the CI runner: it reports #11, and as it happens does not reproduce #33 (gcc's range propagation has moved on). #33 is fixed on the merits — the construct the warning is about is gone — rather than against a reproduction, and that is worth stating plainly.Output is unchanged where it matters:
make test-as370→ALL SAMPLES BYTE-IDENTICAL TO IFOX00On that:
test-corpusis currently red, and was already red before this branch. Its committed SHA manifest has drifted from libc370's current sources (7 modules assemble that the manifest does not list). Nothing here touches that, but it means the regression gate is not gating anything at the moment — worth its own issue.The CI
cc370 had none, while mbt's reusable workflow builds this repo's
mainfor every downstream project. So a brokenmainwent out to the whole ecosystem and came back as a red build in mvsmf — the signal travelling in exactly the wrong direction..github/workflows/build.ymlbuilds the five standalone tools and runs the as370, ld370 and xmit370 suites. Scope is deliberately the tools and not the GCC fork: that is where the regressions have been, andmake compilertakes minutes.Both gcc and clang build the tools, on purpose. The
-Wformat-overflowbreak that prompted all this reachedmainbecause Apple clang does not implement that warning — a clang-only check cannot defend-Werror.libc370 is cloned as a sibling because four as370 samples assemble against its macro libraries; nothing is built from it.
test-corpusstays out (red for the reason above) and so doestest-cc370(needs the GCC fork).Dry-run in the exact layout the workflow creates, all four steps green, including the skip paths for xmit370's two external fixtures.
One thing found on the way
61b4e99was titled "complete the crent370 → libc370 rename in test harness + docs" but left the shell variable itself calledCRENTintests/run.shandtests/listref/check.sh, while the Makefile andtests/corpus/check.shhad already moved toLIBC370— two names for one thing, the stale one naming a libc that is frozen and no longer checked out. Renamed toLIBC370throughout; default unchanged, and all three paths (default, override, bad path) verified.https://claude.ai/code/session_018SbqQkXrAyDhVgcF8X8cdz