Skip to content

Fix IB Periodicity Movement #1405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
danieljvickers wants to merge 1 commit into from
+1 −0
Closed

Fix IB Periodicity Movement #1405

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/simulation/m_ibm.fpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ contains
k = gp%loc(2)
l = gp%loc(3)
patch_id = ghost_points(i)%ib_patch_id
call s_decode_patch_periodicity(patch_id, patch_id)
Comment on lines 203 to +204
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate the implementation of s_decode_patch_periodicity
ast-grep --pattern 'subroutine s_decode_patch_periodicity($$$)'

# Show the dummy argument declarations inside it
rg -n -A 20 'subroutine s_decode_patch_periodicity' --type=fortran
rg -n -A 20 'subroutine s_decode_patch_periodicity' -g '*.fpp' -g '*.f90'

Repository: MFlowCode/MFC

Length of output: 3208

🏁 Script executed:

#!/bin/bash
# Verify all assignment sites of ib_patch_id to see whether encoded or decoded values are stored
rg -n 'ib_patch_id\s*=' -g '*.fpp' -g '*.f90' -C 2

Repository: MFlowCode/MFC

Length of output: 4173

Aliasing violation and logic error: s_decode_patch_periodicity called with same variable for encoded input and decoded output.

The call s_decode_patch_periodicity(patch_id, patch_id) at line 204 passes the same variable as both the intent(in) argument (encoded patch ID) and the intent(out) argument (decoded patch ID). The subroutine signature confirms this:

subroutine s_decode_patch_periodicity(encoded_patch_id, patch_id, ...)
    integer, intent(in)  :: encoded_patch_id
    integer, intent(out) :: patch_id

This is a non-conforming Fortran argument alias. The Fortran standard prohibits the same actual argument from being associated with both intent(in) and intent(out) dummy arguments in a single call. gfortran's -Waliasing flag specifically warns about this pattern. Under optimization on any of the four required compilers (gfortran, nvfortran, Cray ftn, ifx), the compiler may reorder reads and writes, causing the intent(in) value to be read after the intent(out) write, producing a decoded value derived from garbage instead of the original encoded ID.

Additionally, this introduces a logic error: ghost_points(i)%ib_patch_id is populated with an already-decoded value (line 576: ghost_points_in(local_idx)%ib_patch_id = patch_id where patch_id results from a decode call). Decoding an already-decoded value will produce incorrect results.

Fix: pass the encoded struct field directly as the first argument and write the decoded result to patch_id:

Proposed fix 
-               patch_id = ghost_points(i)%ib_patch_id
-               call s_decode_patch_periodicity(patch_id, patch_id)
+               call s_decode_patch_periodicity(ghost_points(i)%ib_patch_id, patch_id)


! Calculate physical location of GP
if (p > 0) then
Expand Down
Loading