Fix IB Periodicity Movement#1405
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Claude Code ReviewHead SHA: 05fbe8f Files changed:
FindingsDouble-decode of
|
📝 WalkthroughWalkthroughThe file 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
| patch_id = ghost_points(i)%ib_patch_id | ||
| call s_decode_patch_periodicity(patch_id, patch_id) |
There was a problem hiding this comment.
🧩 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 2Repository: 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_idThis 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)
1 participant
Ib patch periodicity was never decoded before state correction. This causes errors specifically when the IB is moving across the boundary periodically. One processor reads normal values. The other reads uninstantiated garbage, and this causes conflict.
This one-line change resolves the issue.