yeesian / ArchGDAL.jl

A high level API for GDAL - Geospatial Data Abstraction Library
https://yeesian.github.io/ArchGDAL.jl/stable/
Other
137 stars 25 forks source link

Crash on PkgEval #347

Closed maleadt closed 1 year ago

maleadt commented 1 year ago

ArchGDAL.jl fails tests on PkgEval, specifically, it segfaults:

[355] signal (11.1): Segmentation fault
in expression starting at /home/pkgeval/.julia/packages/ArchGDAL/AZONk/test/test_tables.jl:5
OGR_FD_GetFieldIndex at /home/pkgeval/.julia/artifacts/131cab42093e41cb0d9c0382948d9aa9998b33cb/lib/libgdal.so (unknown line)
#createlayer#236 at /home/pkgeval/.julia/packages/ArchGDAL/AZONk/src/context.jl:267
createlayer at /home/pkgeval/.julia/packages/ArchGDAL/AZONk/src/context.jl:264 [inlined]
#14 at /home/pkgeval/.julia/packages/ArchGDAL/AZONk/test/test_tables.jl:268 [inlined]
#create#226 at /home/pkgeval/.julia/packages/ArchGDAL/AZONk/src/context.jl:267

This doesn't seem like a Julia-related crash, as it crashes during libgdal.so. As this crash has been on the top of PkgEval reports for a while, it would be nice to see this fixed. You can easily reproduce the PkgEval environment (on Linux) by installing PkgEval from https://github.com/JuliaCI/PkgEval.jl and doing:

julia> using PkgEval

julia> config = Configuration(julia="nightly")
PkgEval configuration 'unnamed' (
  - julia: nightly
...
)

julia> PkgEval.sandboxed_julia(config)

# this spawns a sandbox where you can install and test ArchGDAL

(@v1.10) pkg> add ArchGDAL

(@v1.10) pkg> test ArchGDAL

[355] signal (11.1): Segmentation fault
maleadt commented 1 year ago

I see there's a PR already, great! I've blacklisted testing of ArchGDAL for now, just to reduce the noise on PkgEval reports; so could you ping (or create a PR to remove the blacklist) when the fix is part of a tagged release? Thanks!

visr commented 1 year ago

Will do! Turns out it what is really needed is lots of f(obj.ptr) to GC.@preserve obj f(obj.ptr) all over the package, which I added now in #348.

maleadt commented 1 year ago

Ah yes, the GC has become quite a bit more aggressive lately so it's likely that any missing lifetime annotations will cause corruption now.

maleadt commented 1 year ago

Note that generally though, you don't need these, only if you're explicitly deriving a raw resource (e.g. a pointer) from a tracked object. Normally, that conversion should happen by Base.unsafe_convert as called by ccall, in which case the Julia compiler will track lifetimes accordingly.

visr commented 1 year ago

Interesting. ArchGDAL.jl defines structs (say Dataset) that wrap pointers, to create a convenient typed interface on top of GDAL.jl, which has the ccalls. So at the point that ArchGDAL calls into GDAL it always has to take out the pointer. Would it be ok to just define Base.unsafe_convert(::Type{Ptr{Void}, x::Dataset) = x.ptr, such that ccall knows` how to take out the pointer whilst tracking lifetimes? That would be an easier solution.

maleadt commented 1 year ago

If the contained pointers' lifetime is tracked by the Dataset object, then yes, having unsafe_convert return the pointer would be sufficient.

visr commented 1 year ago

Should be all fixed now on ArchGDAL v0.9.4, I put up https://github.com/JuliaCI/PkgEval.jl/pull/203 to remove it from the skip list.