Skip to content

fix(arpack): pass Fortran hidden character-length arguments in ARPACK calls#990

Merged
pcchen merged 1 commit into
masterfrom
fix/arpack-fortran-strlen
Jul 7, 2026
Merged

fix(arpack): pass Fortran hidden character-length arguments in ARPACK calls#990
pcchen merged 1 commit into
masterfrom
fix/arpack-fortran-strlen

Conversation

@yingjerkao

Copy link
Copy Markdown
Collaborator

Summary

The extern "C" prototypes in include/backend/arpack_wrapper.hpp omitted the hidden length argument that Fortran compilers append for every CHARACTER dummy (bmat, which, howmny). Calling through such prototypes is undefined behavior. It happens to be benign for ARPACK's fixed-length dummies under today's gfortran ABI — and it was explicitly not the cause of the macOS segfaults investigated in #974 (those were environment-induced; see that issue) — but it is exactly the class of ABI mismatch that surfaces as platform-specific corruption, and was flagged as a cleanup follow-up when closing #974.

Changes

  • include/backend/arpack_wrapper.hpp — all twelve x{s,n}aupd_ / x{s,n}eupd_ prototypes gain trailing std::size_t length parameters, one per CHARACTER argument, with a comment documenting the convention. std::size_t matches gfortran ≥ 8 and LAPACK's modern FORTRAN_STRLEN convention; on LP64 little-endian targets it is also register/stack-slot compatible with the int lengths used by older compilers.
  • src/linalg/Lanczos.cpp, src/linalg/Arnoldi.cpp — the std::function dispatch signatures extended to match, and every call site now passes the actual lengths (bmat: 1, which: 2, howmny: 1).

No functional change is expected on any platform where the code currently works: the callee already expected these arguments; we now actually pass them.

Verification

On macOS arm64 (AppleClang, conda-forge arpack-ng 2.1.0 + OpenBLAS), with the #972 test compile fixes applied locally (not part of this branch):

Linux/GCC CI exercises the same call paths and should show no change.

Related: #974 (root-cause investigation that flagged this), #626 (original ARPACK integration), #893 (previous wrapper cleanup, exonerated in #974).

🤖 Generated with Claude Code

… calls

The extern "C" prototypes for the ARPACK drivers omitted the hidden
length argument that Fortran compilers append for every CHARACTER dummy
(bmat, which, howmny). Calling through such prototypes is undefined
behavior: it happens to be benign for ARPACK's fixed-length dummies with
today's gfortran ABI, but is not guaranteed by any standard and is the
kind of mismatch that surfaces as platform-specific corruption.

Add trailing std::size_t lengths to all twelve x{s,n}aupd_/x{s,n}eupd_
prototypes (size_t matches gfortran >= 8 and LAPACK's FORTRAN_STRLEN
convention; on LP64 little-endian targets it is also register-compatible
with the int lengths of older compilers), extend the std::function
dispatch signatures in Lanczos.cpp/Arnoldi.cpp accordingly, and pass the
actual lengths (bmat: 1, which: 2, howmny: 1) at every call site.

Cleanup noted when closing #974 (whose segfault was environmental, not
caused by this — see the issue for the root cause).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the ARPACK C declarations in arpack_wrapper.hpp and their corresponding function signatures and calls in Arnoldi.cpp and Lanczos.cpp to include hidden string length arguments (std::size_t) for CHARACTER arguments. This aligns with Fortran's calling convention to prevent undefined behavior. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 30.88%. Comparing base (d02cb29) to head (3d83087).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #990   +/-   ##
=======================================
  Coverage   30.88%   30.88%           
=======================================
  Files         229      229           
  Lines       34758    34758           
  Branches    14409    14409           
=======================================
  Hits        10734    10734           
  Misses      16720    16720           
  Partials     7304     7304           
Flag Coverage Δ
cpp 30.39% <ø> (ø)
python 59.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
C++ backend 32.21% <ø> (ø)
Python bindings 17.28% <ø> (ø)
Python package 59.41% <ø> (ø)
Files with missing lines Coverage Δ
src/linalg/Arnoldi.cpp 58.15% <ø> (ø)
src/linalg/Lanczos.cpp 51.90% <ø> (ø)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d02cb29...3d83087. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcchen

pcchen commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Adds the Fortran hidden CHARACTER-length arguments (which every Fortran compiler appends for CHARACTER dummies) to the 12 ARPACK extern "C" prototypes and their call sites, fixing a genuine undefined-behavior class — calling through prototypes that omit the hidden lengths reads uninitialized register/stack slots. Correct and clean.

Verified correct

  • Arg count & order per function*aupd_ (chars bmat, which) gets 2 trailing lengths; *eupd_ (chars howmny, bmat, which, in that source order) gets 3, ordered howmny_len, bmat_len, which_len to match the order the CHARACTER dummies appear. That's the correct gfortran convention. ✅
  • Values passedbmat=1, which=2, howmny=1 at every call site ((..., 1, 2) for aupd, (..., 1, 1, 2) for eupd). ✅
  • std::function dispatch signatures in Lanczos.cpp/Arnoldi.cpp extended consistently so func_xsaupd = dsaupd_ etc. stay assignable — confirmed by the green compile. ✅
  • std::size_t is ABI-safe — on LP64 little-endian it's compatible with both the modern (gfortran ≥ 8 size_t) and legacy (int) hidden-length conventions; the small values fit in the low bytes. Verified on macOS arm64 (81 Lanczos/Arnoldi tests) and now green on Linux + macOS CI. ✅
  • Strictly a safety improvement, no behavior change — passing the args is defined whether or not the linked ARPACK reads them, whereas the old code was UB when it did. The PR is honest that this was not the macOS: ARPACK-backed linalg::Lanczos / Arnoldi tests segfault in dsaupd_ (null dereference) #974 macOS-segfault cause (environment-induced), just the cleanup flagged when closing it.

Notes (non-blocking)

  • No new test — reasonable, since this is a benign-under-current-ABI UB cleanup with no observable behavior change; the existing Lanczos/Arnoldi suite (green in CI) covers the affected call paths.
  • Behind master but conflict-free; a fresh CI run is optional (the current green is valid — not the stale-ci: set git committer identity before merging the target branch #1007 pattern).

No findings requiring changes — a well-reasoned, well-documented ABI-correctness fix. LGTM.

Posted by Claude Code on behalf of @pcchen

@pcchen pcchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving. Correct, clean Fortran/C ABI fix: the 12 ARPACK extern "C" prototypes and their call sites now carry the hidden CHARACTER-length arguments — right count and order per function (aupd: bmat,which1,2; eupd: howmny,bmat,which1,1,2), consistent std::function dispatch signatures, and std::size_t chosen for ABI-compatibility on LP64 little-endian with both the modern (gfortran ≥8) and legacy (int) hidden-length conventions. Strictly a UB-safety improvement with no behavior change, and CI is green on all platforms including the Arnoldi/Lanczos suites that exercise these calls.

Posted by Claude Code on behalf of @pcchen

@pcchen
pcchen merged commit 4c17ceb into master Jul 7, 2026
19 checks passed
@pcchen
pcchen deleted the fix/arpack-fortran-strlen branch July 7, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants