Sundials.jl icon indicating copy to clipboard operation
Sundials.jl copied to clipboard

[WIP] Refreshing the wrappers for Sundials 6.6

Open ViralBShah opened this issue 1 year ago • 6 comments

This PR now has the newly generated C wrappers for Sundials 6.5. The package needs to be updated to match the changes in the wrappers.

There's some new ARKRhsFn_wrapper thing that needs to be dealt with (among other things). But at least the clang generators are now running through.

cc @ChrisRackauckas

ViralBShah avatar Jul 13 '23 03:07 ViralBShah

Why is >v1.6 required?

ChrisRackauckas avatar Aug 22 '23 20:08 ChrisRackauckas

Never mind. I think this is generating for Sundials 5.2 still. I think I just need Julia 1.10 for generating the headers with the latest Clang.jl. Let me see if I can get it working first and then will worry about bumping julia_compat down to 1.6, which should just work.

ViralBShah avatar Aug 22 '23 21:08 ViralBShah

This is the error running Clang. The clang wrappers need updating:

ERROR: LoadError: BoundsError: attempt to access 2-element Vector{Any} at index [3]
Stacktrace:
  [1] getindex(A::Vector{Any}, i1::Int64)
    @ Base ./essentials.jl:13
  [2] wrap_sundials_api(expr::Expr)
    @ Main ~/.julia/dev/Sundials/gen/generate.jl:93
  [3] rewrite(ex::Expr)

ViralBShah avatar Aug 22 '23 21:08 ViralBShah

Minimum change that I think would be needed to the generated code in libsundials_api.jl to make the NVector memory management work as in the current release (ie as it does following fixes in release v4.12.0 https://github.com/SciML/Sundials.jl/pull/380 ):

The generated ccall wrappers need to take NVector arguments and pass through to ccall so that ccall then uses the cconvert and unsafe_convert machinery to preserve the lifetime of the NVector (which may well be a temporary generated by the calling code).

So eg the generated wrapper

function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0::N_Vector)
    ccall((:CVodeInit, libsundials_cvodes), Cint, (Ptr{Cvoid}, CVRhsFn, realtype, N_Vector), cvode_mem, f, t0, y0)
end

would need to be instead:

function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0::NVector)
    ccall((:CVodeInit, libsundials_cvodes), Cint, (Ptr{Cvoid}, CVRhsFn, realtype, N_Vector), cvode_mem, f, t0, y0)
end

I'm guessing it might also be necessary to accept other types for y0 as in the current released code:

  • to avoid extra conversions where the caller already has an N_Vector and is responsible for managing the lifetime, allow
function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0::Union{NVector, N_Vector})
  • to allow y0::Vector , provide
function CVodeInit(cvode_mem, f::CVRhsFn, t0::realtype, y0)
    return CVodeInit(cvode_mem, f, t0, convert(NVector, y0))
end

(or perhaps this is overcomplicated and just removing the type signature to vector arguments (y0 etc) would be enough to let ccall apply whatever is needed ? Not sure if it would chain together convert and then cconvert and unsafe_convert to handle a Julia Vector though ?)

sjdaines avatar Aug 29 '23 08:08 sjdaines

Can this be rebased on master to see if it works now?

oscardssmith avatar Feb 07 '24 14:02 oscardssmith

I thought this PR needs a substantial amount of work. I really doubt it will work that easily.

ViralBShah avatar Feb 07 '24 15:02 ViralBShah