Fix debug build warnings: uninitialized intent(out) args, unused variables#1340
Fix debug build warnings: uninitialized intent(out) args, unused variables#1340sbryngelson wants to merge 11 commits intoMFlowCode:masterfrom
Conversation
- m_mpi_common.fpp: Initialize icfl_max_glb/vcfl_max_glb/Rc_min_glb before #ifdef MFC_SIMULATION. Use block/end block for ierr declaration to avoid declaration-after-executable-statement error. Change intent(out) to intent(inout) in s_mpi_allreduce_vectors_sum for aliased calls. - m_variables_conversion.fpp: Initialize rho_K/gamma_K/pi_inf_K/qv_K/Re_K/G_K before #ifdef MFC_SIMULATION in s_convert_species_to_mixture_variables_acc.
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
d792134 to
1eaee91
Compare
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
1eaee91 to
6439d2f
Compare
|
Claude Code Review Head SHA: d792134 Files changed:
Findings:
|
|
Claude Code Review Head SHA: 1eaee91 Files changed:
Findings:
|
|
Claude Code Review Incremental review from: 6439d2f New findings since last Claude review:
|
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
6439d2f to
0415ca1
Compare
|
Claude Code Review Incremental review from: 4fff2da New findings since last Claude review:
|
📝 WalkthroughWalkthroughThis pull request introduces a new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
264-281: Please route the GitHub job through the shared warning collector.This is already a second implementation of the same collector, and it has drifted from
.github/workflows/common/collect-warnings.sh(-B2here vs-B3there,/tmp/warnings_*.txtonly on this path). If you move the file-writing into the shared script and call it here, fixes to the warning logic only need to land once.
302-308: Please confirm dropping the draft guard is intentional.Removing
github.event.pull_request.draft != truemeans the Phoenix/Frontier matrix will now run on every draft PR update as well. Sincecase-optimizationbelow is still draft-gated, this looks easy to do accidentally and can burn a lot of queue time if it was not deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e48f2af-f7ba-41d4-a254-11482fc3b634
📒 Files selected for processing (16)
.github/workflows/common/collect-warnings.sh.github/workflows/test.ymlCMakeLists.txtsrc/common/m_helper.fppsrc/common/m_mpi_common.fppsrc/common/m_variables_conversion.fppsrc/post_process/m_data_output.fppsrc/pre_process/m_icpp_patches.fppsrc/simulation/m_bubbles_EL.fppsrc/simulation/m_compute_cbc.fppsrc/simulation/m_compute_levelset.fppsrc/simulation/m_start_up.fpptoolchain/mfc/build.pytoolchain/mfc/cli/argparse_gen.pytoolchain/mfc/cli/docs_gen.pytoolchain/mfc/state.py
💤 Files with no reviewable changes (1)
- src/simulation/m_compute_cbc.fpp
| # Touch all generated .f90 to force recompile | ||
| find fypp/ -name '*.f90' -exec touch {} + 2>/dev/null | ||
| # Rebuild single-threaded to get clean warning output with file/line context | ||
| make "$target" -j1 2>&1 | grep -E "Warning:|warning:" -B3 || echo "(no warnings)" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the script for pipefail and understand its context
cat -n .github/workflows/common/collect-warnings.sh | head -30Repository: MFlowCode/MFC
Length of output: 1243
🏁 Script executed:
# Look for HANDLE_SOURCES in CMakeLists.txt
rg -A 10 "HANDLE_SOURCES" --type cmakeRepository: MFlowCode/MFC
Length of output: 2254
🏁 Script executed:
# Check if there are any checked-in .f90 files (not generated)
find . -name "*.f90" -type f | grep -v fypp | grep -v build | head -20Repository: MFlowCode/MFC
Length of output: 330
🏁 Script executed:
# Understand the build directory structure and where fypp outputs go
find . -name "CMakeLists.txt" -type f | head -5Repository: MFlowCode/MFC
Length of output: 113
🏁 Script executed:
# Check how other workflows/scripts rebuild targets
rg "cmake --build|make.*target" --type bashRepository: MFlowCode/MFC
Length of output: 85
🏁 Script executed:
# Look at how mfc.sh handles test/build operations
head -100 ./mfc.shRepository: MFlowCode/MFC
Length of output: 3461
🏁 Script executed:
# Check if there's existing warning collection or CI logic
rg -A 5 "collect-warnings" .github/workflows/Repository: MFlowCode/MFC
Length of output: 902
Use cmake --build with --clean-first instead of selective touch + make | grep.
This script touches only generated .f90 files in fypp/, but HANDLE_SOURCES in CMakeLists.txt compiles both generated and checked-in .f90 sources (e.g., src/common/m_compile_specific.f90). Checked-in files retain their original timestamps and won't be recompiled in the rebuild, so their warnings never reappear. Also, under set -o pipefail (line 5), a failing make triggers the || echo "(no warnings)" fallback, masking build failures as clean runs. A cmake --build . --target "$target" --parallel 1 --clean-first with proper error handling avoids both problems.
🛠️ Suggested change
- # Touch all generated .f90 to force recompile
- find fypp/ -name '*.f90' -exec touch {} + 2>/dev/null
- # Rebuild single-threaded to get clean warning output with file/line context
- make "$target" -j1 2>&1 | grep -E "Warning:|warning:" -B3 || echo "(no warnings)"
+ tmp_log=$(mktemp)
+ if ! cmake --build . --target "$target" --parallel 1 --clean-first >"$tmp_log" 2>&1; then
+ echo "(warning collection rebuild failed)"
+ cat "$tmp_log"
+ else
+ grep -E "Warning:|warning:" -B3 "$tmp_log" || echo "(no warnings)"
+ fi
+ rm -f "$tmp_log"| integer, intent(in) :: num_vectors, vector_length | ||
| real(wp), dimension(:,:), intent(in) :: var_loc | ||
| real(wp), dimension(:,:), intent(inout) :: var_glb |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
import re
pat = re.compile(r'call\s+s_mpi_allreduce_vectors_sum\s*\(', re.I)
for pattern in ("*.fpp", "*.f90"):
for path in Path("src").rglob(pattern):
text = path.read_text(encoding="utf-8", errors="ignore")
idx = 0
while True:
m = pat.search(text, idx)
if not m:
break
i = m.end()
depth = 1
while i < len(text) and depth:
if text[i] == '(':
depth += 1
elif text[i] == ')':
depth -= 1
i += 1
call = text[m.start():i]
args = re.sub(r'&\s*\n\s*&?', ' ', call[call.find('(') + 1:-1])
parts, cur, depth = [], [], 0
for ch in args:
if ch == '(':
depth += 1
elif ch == ')':
depth -= 1
if ch == ',' and depth == 0:
parts.append(''.join(cur).strip())
cur = []
else:
cur.append(ch)
parts.append(''.join(cur).strip())
if len(parts) >= 2 and parts[0].lower() == parts[1].lower():
line = text.count('\n', 0, m.start()) + 1
print(f"{path}:{line}: {parts[0]}")
idx = i
PYRepository: MFlowCode/MFC
Length of output: 133
🏁 Script executed:
head -346 src/common/m_mpi_common.fpp | tail -10Repository: MFlowCode/MFC
Length of output: 474
🏁 Script executed:
sed -n '340,350p' src/common/m_mpi_common.fppRepository: MFlowCode/MFC
Length of output: 524
🏁 Script executed:
sed -n '1000,1015p' src/simulation/m_ibm.fppRepository: MFlowCode/MFC
Length of output: 610
🏁 Script executed:
sed -n '344,380p' src/common/m_mpi_common.fppRepository: MFlowCode/MFC
Length of output: 1447
Split the in-place and two-buffer allreduce APIs.
Passing the same actual array as both intent(in) (var_loc) and intent(inout) (var_glb) arguments violates Fortran standard conformance constraints at the interface level. Although the implementation includes a runtime check using loc() and MPI_IN_PLACE, this does not resolve the constraint violation—different compilers may reject, warn, or silently mishandle this pattern. Per the coding guidelines, all code must compile with GNU gfortran, NVIDIA nvfortran, Cray ftn, and Intel ifx compilers.
The codebase currently has two call sites with this issue:
src/simulation/m_ibm.fpp:1007:call s_mpi_allreduce_vectors_sum(forces, forces, num_ibs, 3)src/simulation/m_ibm.fpp:1008:call s_mpi_allreduce_vectors_sum(torques, torques, num_ibs, 3)
Use a dedicated in-place entry point or a generic overload to separate the two-buffer API from the in-place API.
| real(wp) :: normals(1:3) !< Boundary normal buffer | ||
| integer :: boundary_vertex_count, boundary_edge_count, total_vertices !< Boundary vertex | ||
| real(wp) :: normals(1:3) !< Boundary normal buffer | ||
| integer :: boundary_vertex_count, boundary_edge_count, total_vertices !< Boundary vertex |
There was a problem hiding this comment.
Remove still-unused total_vertices from s_icpp_model.
Line 1272 declares total_vertices, but it is not used later in the routine, so this likely keeps an unused-local warning alive.
Suggested diff
- integer :: boundary_vertex_count, boundary_edge_count, total_vertices !< Boundary vertex
+ integer :: boundary_vertex_count, boundary_edge_count !< Boundary vertex📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| integer :: boundary_vertex_count, boundary_edge_count, total_vertices !< Boundary vertex | |
| integer :: boundary_vertex_count, boundary_edge_count !< Boundary vertex |
| mpi: bool = True | ||
| gpu: str = gpuConfigOptions.NONE.value | ||
| debug: bool = False | ||
| reldebug: bool = False |
There was a problem hiding this comment.
reldebug shouldn't be another independent boolean.
Line 18 makes debug=true, reldebug=true a valid config. toolchain/mfc/build.py then silently collapses that to Debug, while make_slug() and make_options() still preserve both flags, so one effective build mode can hash to multiple staging dirs. Please enforce mutual exclusivity or model build type as a single field.
| cli_k = k.replace("_", "-") | ||
| if k == "gpu": | ||
| options.append(f"--{v}-{k}") | ||
| options.append(f"--{v}-{cli_k}") | ||
| else: | ||
| options.append(f"--{'no-' if not v else ''}{k}") | ||
| options.append(f"--{'no-' if not v else ''}{cli_k}") |
There was a problem hiding this comment.
make_options() now emits a GPU flag the parser doesn't accept.
For gpu="acc" or gpu="mp", this returns --acc-gpu / --mp-gpu, but _add_mfc_config_arguments() only defines --gpu [acc/mp] and --no-gpu. Any round-trip that reconstructs CLI args from MFCConfig will break on GPU configs.
🛠️ Suggested change
for k, v in self.items():
cli_k = k.replace("_", "-")
if k == "gpu":
- options.append(f"--{v}-{cli_k}")
+ if v == gpuConfigOptions.NONE.value:
+ options.append(f"--no-{cli_k}")
+ else:
+ options.append(f"--{cli_k}={v}")
else:
options.append(f"--{'no-' if not v else ''}{cli_k}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cli_k = k.replace("_", "-") | |
| if k == "gpu": | |
| options.append(f"--{v}-{k}") | |
| options.append(f"--{v}-{cli_k}") | |
| else: | |
| options.append(f"--{'no-' if not v else ''}{k}") | |
| options.append(f"--{'no-' if not v else ''}{cli_k}") | |
| cli_k = k.replace("_", "-") | |
| if k == "gpu": | |
| if v == gpuConfigOptions.NONE.value: | |
| options.append(f"--no-{cli_k}") | |
| else: | |
| options.append(f"--{cli_k}={v}") | |
| else: | |
| options.append(f"--{'no-' if not v else ''}{cli_k}") |
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
0415ca1 to
b0bccde
Compare
sbryngelson
force-pushed
the
fix-debug-warnings
branch
3 times, most recently
from
4356909 to
a9ffd4e
Compare
Build self-hosted targets with --reldebug to enable warning flags. Skip test step on self-hosted (only need build output for warnings). Skip rebuild-cache entirely (not needed for warning collection). Add compile-time warning flags to RelDebug for all compilers: - Cray ftn: -h msgs -h msglevel_0 -h style - LLVMFlang: -Wall -Wextra -Wunused - Intel ifx: -warn all - NVHPC: -Minform=inform -Mstandard This CI change is temporary and will be reverted before merge.
sbryngelson
force-pushed
the
fix-debug-warnings
branch
from
a9ffd4e to
21b5405
Compare
1 participant
Summary
intent(out)arguments unconditionally before#ifdef MFC_SIMULATIONblocks to fix "INTENT(OUT) but was not set" warnings in non-simulation buildsintent(out)tointent(inout)ons_mpi_allreduce_vectors_sumto fix aliasing violation when same array passed as both input and outputCI note
This draft includes a temporary CI step ("Collect Compiler Warnings") that rebuilds each target with
-j1and captures all compiler warnings as artifacts. This is to collect warning lists from gfortran and ifx before making further removals. The CI step will be removed before merging.Test plan
./mfc.sh precheck -j 8passes./mfc.sh build -j 8passes (gfortran, CPU, MPI)./mfc.sh test --reldebug -j 8 -% 5passes (27/27)