From 4f348ac617d353f609ca206bf19b2c56f1fe4547 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Fri, 11 Sep 2026 22:29:59 -0500 Subject: [PATCH 1/4] docs: fix doc-reference lint failures from #1855 Three checks in lint_docs.py fail on master. Two single-backtick spans contain a single quote, which Doxygen treats as ending the span, so they need double backticks. The testing.md cross-reference to the new pitfalls section needs a matching {#selection-and-execution-pitfalls} anchor on that header. Written with assistance from Claude Code. --- docs/documentation/contributing.md | 2 +- docs/documentation/gpuParallelization.md | 2 +- docs/documentation/testing.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md index d42dc09e8..dd3c0f33c 100644 --- a/docs/documentation/contributing.md +++ b/docs/documentation/contributing.md @@ -217,7 +217,7 @@ Both human reviewers and AI code reviewers reference this section. ### Parameter Plumbing -- **Derived-type parameters are not auto-broadcast.** `generated_bcast.fpp` covers namelist *scalars* only. Each derived type (`chem_params`, `lag_params`, `rburn`) needs a hand-written `_emit_` in `toolchain/mfc/params/generators/fortran_gen.py` plus its call site in that generator's simulation branch, and, if it is read on device, an explicit `$:GPU_UPDATE(device='[name]')` in both the target's `m_global_parameters.fpp` and `src/simulation/m_start_up.fpp` — `GPU_DECLARE` alone does not make it device-resident. Regrouping existing scalars into a derived type silently drops their broadcast, leaving every non-root rank holding the `dflt_real` sentinel. Single-rank golden files cannot catch this, so pair such a change with a `ppn=2` test and confirm it fails without the emitter. +- **Derived-type parameters are not auto-broadcast.** `generated_bcast.fpp` covers namelist *scalars* only. Each derived type (`chem_params`, `lag_params`, `rburn`) needs a hand-written `_emit_` in `toolchain/mfc/params/generators/fortran_gen.py` plus its call site in that generator's simulation branch, and, if it is read on device, an explicit ``$:GPU_UPDATE(device='[name]')`` in both the target's `m_global_parameters.fpp` and `src/simulation/m_start_up.fpp` — `GPU_DECLARE` alone does not make it device-resident. Regrouping existing scalars into a derived type silently drops their broadcast, leaving every non-root rank holding the `dflt_real` sentinel. Single-rank golden files cannot catch this, so pair such a change with a `ppn=2` test and confirm it fails without the emitter. - **A `patch_ib` member that immersed-boundary ghost-point code reads must also be set in `s_add_cloud_particle`** (`src/simulation/m_particle_cloud.fpp`). `particle_cloud_ibs` is allocated without default initialization, and `s_reduce_ib_patch_array` copies the whole struct into `patch_ib`, overwriting the defaults assigned in `s_assign_default_values_to_user_inputs`. Anything left unset reaches the solver as uninitialized memory, and only where the allocation is not already zero-filled. A platform-only NaN is the signature of this class: a garbage `v_blow` once failed an AMD lane with `ICFL is NaN` while every NVIDIA lane and all local runs passed. - **Runtime checks go where they run.** Shared constraints belong in `src/common/m_checker_common.fpp`, simulation-only ones in `src/simulation/m_checker.fpp`, and pre- and post-process ones in their own `m_checker.fpp`. Those two `s_check_inputs` are currently empty; that is still the correct home for their checks, not `m_checker_common`. - **Analytic initial conditions are compiled into the binary** and their expressions are AST-validated at case load, so syntax errors and unknown variables surface immediately and by name. Each IC variable maps to an `eqn_idx` expression in `QPVF_IDX_VARS` (`toolchain/mfc/case.py`); adding a patch-settable conserved variable means updating that map and the Fortran `eqn_idx` builder together, because a mismatch is a silent wrong index. diff --git a/docs/documentation/gpuParallelization.md b/docs/documentation/gpuParallelization.md index 8b205715d..b559c17fe 100644 --- a/docs/documentation/gpuParallelization.md +++ b/docs/documentation/gpuParallelization.md @@ -874,7 +874,7 @@ answer is wrong, or one backend diverges from all the others. always use `GPU_PARALLEL_LOOP` / `END_GPU_PARALLEL_LOOP`. - **An array whose bound is a device global** (`dimension(num_fluids)`, `dimension(num_species)`) may be passed to a device routine from a parallel-loop body, - but **not from inside another `GPU_ROUTINE(parallelism='[seq]')`**. Cray OpenACC rejects + but **not from inside another ``GPU_ROUTINE(parallelism='[seq]')``**. Cray OpenACC rejects the second form with `ftn-7066 ... Global in accelerator routine without declare`, and reports it at whatever line it gave up on: remove one trigger and the message walks forward to the next call, so the reported line is not the cause. Only the plain lanes diff --git a/docs/documentation/testing.md b/docs/documentation/testing.md index be50883f7..b23e15231 100644 --- a/docs/documentation/testing.md +++ b/docs/documentation/testing.md @@ -92,7 +92,7 @@ If a trace is empty (that is, the empty string `""`), it will not appear in the Finally, the case is appended to the `cases` list, which will be returned by the `list_cases` function. -### Selection and Execution Pitfalls +### Selection and Execution Pitfalls {#selection-and-execution-pitfalls} Each of these fails quietly rather than loudly. From 9555b04a4ff1b9e2ea21cdea212a081a227adb27 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Fri, 11 Sep 2026 22:51:49 -0500 Subject: [PATCH 2/4] docs: address review follow-ups from #1855 Point the device-routine lint at documentation that exists. The error message now names the Silent-Failure Traps section of gpuParallelization.md, which is repo-local and survives changes to the external reproducer repo, and that section gains the {#silent-failure-traps} anchor Doxygen needs. The docstring keeps the full context: the docs section, MFC issue 1815, and a full URL to the compiler-bugs reproducer, whose path was verified to exist. Also fix trailing whitespace and three grammar slips in .claude/CLAUDE.md. Written with assistance from Claude Code. --- .claude/CLAUDE.md | 8 ++++---- docs/documentation/gpuParallelization.md | 2 +- toolchain/mfc/lint_source.py | 8 +++++--- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index a6913c6d0..7547db8b0 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -1,8 +1,8 @@ * New branches cannot be made on MFlowCode/MFC, they are made on forks * PRs: - * made using AI tools like Claude Code and Codex should say so. + * made using AI tools like Claude Code and Codex should say so. * are made from those MFC forks - * that change CFD result need verification PR is correct + * that change CFD results need verification that the PR is correct * follow template * that break a feature but promise a followup PR to fix it are rejected * Commands: @@ -11,5 +11,5 @@ * Programming and Design: * New code should follow the DRY principle and also make side-effect code DRY as well * Comments should be as short as possible without sacrificing value - * GPU macros should follow existing the source's GPU macro principles and patterns - * Functions/subroutines/modules shorter is better while being correctness, fast, and separating concerns + * GPU macros should follow the source's existing GPU macro principles and patterns + * Functions/subroutines/modules shorter is better while being correct, fast, and separating concerns diff --git a/docs/documentation/gpuParallelization.md b/docs/documentation/gpuParallelization.md index b559c17fe..85b0d7719 100644 --- a/docs/documentation/gpuParallelization.md +++ b/docs/documentation/gpuParallelization.md @@ -864,7 +864,7 @@ while the host still registers it. The first launch aborts with followed by a segmentation fault. Never place a GPU kernel inside a `block` construct; hoist it into its own (module) subroutine with the locals passed as arguments. -## Silent-Failure Traps +## Silent-Failure Traps {#silent-failure-traps} Every entry here was measured. They share a failure mode: the build stays green and the answer is wrong, or one backend diverges from all the others. diff --git a/toolchain/mfc/lint_source.py b/toolchain/mfc/lint_source.py index 7dca078f0..4093fcf7d 100644 --- a/toolchain/mfc/lint_source.py +++ b/toolchain/mfc/lint_source.py @@ -629,8 +629,10 @@ def check_device_routine_element_args(repo_root: Path) -> list[str]: routine containing any `GPU_LOOP`, called with an array element as an actual argument, reads the element as garbage and never writes it back. Either alone is fine, every `routine` level is affected, and the loop counts when it sits in anything the routine calls. Copy the element - to a scalar before the call and receive results into a scalar. See - sbryngelson/compiler-bugs cce/acc-routine-element-by-reference. + to a scalar before the call and receive results into a scalar. See the Silent-Failure + Traps section of docs/documentation/gpuParallelization.md, MFC issue + https://github.com/MFlowCode/MFC/issues/1815, and the reproducer at + https://github.com/sbryngelson/compiler-bugs/tree/main/cce/acc-routine-element-by-reference. """ src_dir = repo_root / SRC_DIR files = {src: src.read_text(encoding="utf-8").splitlines() for src in _fortran_fpp_files(src_dir)} @@ -683,7 +685,7 @@ def check_device_routine_element_args(repo_root: Path) -> list[str]: for arg in _split_top_level(stmt[m.end() : j - 1]): e = _ELEMENT_ARG.match(arg) if e and ":" not in arg and not _VALUE_CALL_NAMES.match(e.group(1)): - errors.append(f" {rel}:{line_no} `{arg}` into `{name}` (a device routine with a seq loop): pass a scalar, see sbryngelson/compiler-bugs cce/acc-routine-element-by-reference") + errors.append(f" {rel}:{line_no} `{arg}` into `{name}` (a device routine with a seq loop): pass a scalar, see docs/documentation/gpuParallelization.md (Silent-Failure Traps)") return errors From 213f101786ab71396f28869f3ecad5b40c6e384f Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Fri, 11 Sep 2026 23:06:31 -0500 Subject: [PATCH 3/4] fix: make the preflight node probe portable to BSD find and bash 3.2 Two portability bugs made preflight.sh silently skip the node probe outside Linux, which is the failure mode the script exists to prevent. find -printf is GNU-only, so on a BSD find the discovery pipeline failed into 2>/dev/null, returned nothing, and the script reported 'no syscheck binary' and exited healthy; it now uses -exec ls -t {} + instead. Under set -u, bash 3.2 treats the empty array expansion "${launcher[@]}" as an unbound variable, so the probe aborted before running; it now uses ${launcher[@]+"${launcher[@]}"}. The toolchain lint reported 13 failures across the five preflight test files on macOS, and two further tests passed only because a skipped probe returns the same healthy status the assertions expected. All 29 now pass, and ./mfc.sh precheck is green end to end. Verified against both GNU findutils 4.11.0 with bash 5.3.15 and the system BSD find with bash 3.2.57. Written with assistance from Claude Code. --- .github/scripts/preflight.sh | 10 +++++++--- toolchain/mfc/test_preflight.py | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/.github/scripts/preflight.sh b/.github/scripts/preflight.sh index ba0244f37..ec07f7d52 100755 --- a/.github/scripts/preflight.sh +++ b/.github/scripts/preflight.sh @@ -57,8 +57,10 @@ fi # microarchitecture dies with SIGILL, which would be reported as a bad node and # get a perfectly healthy one excluded. newest_syscheck() { - find "$@" -name syscheck -type f -printf '%T@ %p\n' 2>/dev/null \ - | sort -rn | head -1 | cut -d' ' -f2- + # ls -t rather than find -printf: -printf is GNU-only, and on a BSD find it + # fails into 2>/dev/null, so discovery silently returns nothing and every + # probe is skipped as "no syscheck binary". + find "$@" -name syscheck -type f -exec ls -t {} + 2>/dev/null | head -1 } syscheck_bin=$(newest_syscheck build/install -path "*${device}*") @@ -132,7 +134,9 @@ run_probe() { fi } -run_probe "${launcher[@]}" +# ${arr[@]+"${arr[@]}"} rather than "${arr[@]}": under set -u, bash 3.2 (which is +# what macOS ships) treats an empty array expansion as an unbound variable. +run_probe ${launcher[@]+"${launcher[@]}"} # If this launcher does not take the flags we added, drop them and probe again # rather than reporting a verdict about the node. Otherwise a launcher that diff --git a/toolchain/mfc/test_preflight.py b/toolchain/mfc/test_preflight.py index 9125fb667..f94e9e76e 100644 --- a/toolchain/mfc/test_preflight.py +++ b/toolchain/mfc/test_preflight.py @@ -44,7 +44,7 @@ def workspace(tmp_path): # stand in for a launcher the test meant to be absent. sysbin = tmp_path / "sysbin" sysbin.mkdir() - for tool in ("bash", "find", "head", "tail", "cat", "sed", "grep", "tr", "cut", "date", "mkdir", "mv", "rm", "hostname", "env", "sort", "wc", "dirname", "basename"): + for tool in ("bash", "find", "head", "tail", "cat", "sed", "grep", "tr", "cut", "date", "mkdir", "mv", "rm", "hostname", "env", "sort", "wc", "dirname", "basename", "ls"): for root in ("/usr/bin", "/bin"): src = Path(root) / tool if src.exists(): From 64f2635f5fe087fcd0a8a71da8116f0988da6f34 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Fri, 11 Sep 2026 23:18:41 -0500 Subject: [PATCH 4/4] ci: trigger the homebrew formula update on release, not tag push GitHub skips push-triggered workflows when the head commit message carries [skip ci], and MFC tags land on the coverage-map commit, which does. v5.6.0 and v5.7.0 therefore published without a formula update: v5.6.0 was rescued by a manual workflow_dispatch three minutes later, and v5.7.0 has had none. v5.6.1 fired normally because its tag pointed at an ordinary commit. docker.yml was unaffected throughout because a release event is not a push event, so this moves homebrew-release.yml to the same trigger. The version now comes from github.event.release.tag_name rather than GITHUB_REF, and prereleases are skipped rather than failing the X.Y.Z check. Written with assistance from Claude Code. --- .github/workflows/homebrew-release.yml | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/.github/workflows/homebrew-release.yml b/.github/workflows/homebrew-release.yml index fe616e96c..7cb31b12f 100644 --- a/.github/workflows/homebrew-release.yml +++ b/.github/workflows/homebrew-release.yml @@ -1,10 +1,12 @@ name: Update Homebrew Formula on Release -# Triggers when a new version tag is pushed +# Triggers when a release is published. Not `push: tags`, which GitHub skips +# when the tagged commit message carries [skip ci] -- that silently dropped the +# v5.6.0 and v5.7.0 formula updates, while docker.yml fired for both because a +# release event is not a push event. on: - push: - tags: - - 'v*' + release: + types: [published] pull_request: branches: [master] paths: @@ -28,6 +30,9 @@ permissions: jobs: update-homebrew-tap: name: Update homebrew-mfc tap + # A prerelease tag (v5.8.0-rc1) would fail the X.Y.Z check below, so skip it + # rather than reporting a red release job. + if: ${{ github.event_name != 'release' || !github.event.release.prerelease }} runs-on: ubuntu-latest environment: name: homebrew @@ -44,8 +49,9 @@ jobs: VERSION="5.2.0" echo "::notice::PR test mode - using version $VERSION" else - # Extract version from tag (remove 'v' prefix) - VERSION="${GITHUB_REF#refs/tags/v}" + # Extract version from the released tag (remove 'v' prefix) + VERSION="${{ github.event.release.tag_name }}" + VERSION="${VERSION#v}" fi if [[ ! "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then @@ -97,7 +103,7 @@ jobs: echo "- Update formula" >> $GITHUB_STEP_SUMMARY echo "- Push to tap" >> $GITHUB_STEP_SUMMARY echo "" >> $GITHUB_STEP_SUMMARY - echo "The full workflow will run when a \`v*\` tag is pushed after merge." >> $GITHUB_STEP_SUMMARY + echo "The full workflow will run when a release is published after merge." >> $GITHUB_STEP_SUMMARY - name: Checkout homebrew-mfc tap if: ${{ github.event_name != 'pull_request' }}