JuliaInterop / Clang.jl

C binding generator and Julia interface to libclang
https://juliainterop.github.io/Clang.jl/
MIT License
225 stars 68 forks source link

`field_access_method_list` is not architecture-independent #512

Open JamesWrigley opened 2 weeks ago

JamesWrigley commented 2 weeks ago

I came across a bug when testing some code on 32bit CI:

c_attrs = LibSSH.lib.sftp_attributes_struct(Ptr{Int8} @0x00000000, Ptr{Int8} @0x00000000, 0x0000000f, 0x01, 0x0000000000000000, 0x000003e9, 0x0000007f, Ptr{Int8} @0x00000000, Ptr{Int8} @0x00000000, 0x00008180, 0x0000000000000000, 0x671c0a45, 0x00000000, 0x0000000000000000, 0x00000000, 0x0000000000000000, 0x671c0a45, 0x00000000, Ptr{LibSSH.lib.ssh_string_struct} @0x00000000, 0x00000000, Ptr{LibSSH.lib.ssh_string_struct} @0x00000000, Ptr{LibSSH.lib.ssh_string_struct} @0x00000000)
c_attrs.owner = Ptr{Int8} @0x00000000
x = Ptr{Int8} @0x00008180

[2065] signal (11.1): Segmentation fault
in expression starting at none:1
unknown function (ip: 0xe7ea162a)

c_attrs is a struct that has been unsafe_load()'d from a pointer. c_attrs.owner is a pointer field of c_attrs, and x is the result of calling unsafe_load(getproperty(::Ptr{sftp_attributes_struct}, :owner)). The problem is that c_attrs.owner and x do not have the same value, and I believe this is because our getproperty() implementations don't take into account the fact that sizeof(Ptr) is different on 32bit/64bit platforms.

This is the struct definition generated by Clang.jl:

Code ```julia mutable struct sftp_attributes_struct name::Ptr{Cchar} longname::Ptr{Cchar} flags::UInt32 type::UInt8 size::UInt64 uid::UInt32 gid::UInt32 owner::Ptr{Cchar} group::Ptr{Cchar} permissions::UInt32 atime64::UInt64 atime::UInt32 atime_nseconds::UInt32 createtime::UInt64 createtime_nseconds::UInt32 mtime64::UInt64 mtime::UInt32 mtime_nseconds::UInt32 acl::ssh_string extended_count::UInt32 extended_type::ssh_string extended_data::ssh_string end function Base.getproperty(x::Ptr{sftp_attributes_struct}, f::Symbol) f === :name && return Ptr{Ptr{Cchar}}(x + 0) f === :longname && return Ptr{Ptr{Cchar}}(x + 8) f === :flags && return Ptr{UInt32}(x + 16) f === :type && return Ptr{UInt8}(x + 20) f === :size && return Ptr{UInt64}(x + 24) f === :uid && return Ptr{UInt32}(x + 32) f === :gid && return Ptr{UInt32}(x + 36) f === :owner && return Ptr{Ptr{Cchar}}(x + 40) f === :group && return Ptr{Ptr{Cchar}}(x + 48) f === :permissions && return Ptr{UInt32}(x + 56) f === :atime64 && return Ptr{UInt64}(x + 64) f === :atime && return Ptr{UInt32}(x + 72) f === :atime_nseconds && return Ptr{UInt32}(x + 76) f === :createtime && return Ptr{UInt64}(x + 80) f === :createtime_nseconds && return Ptr{UInt32}(x + 88) f === :mtime64 && return Ptr{UInt64}(x + 96) f === :mtime && return Ptr{UInt32}(x + 104) f === :mtime_nseconds && return Ptr{UInt32}(x + 108) f === :acl && return Ptr{ssh_string}(x + 112) f === :extended_count && return Ptr{UInt32}(x + 120) f === :extended_type && return Ptr{ssh_string}(x + 128) f === :extended_data && return Ptr{ssh_string}(x + 136) return getfield(x, f) end function Base.setproperty!(x::Ptr{sftp_attributes_struct}, f::Symbol, v) unsafe_store!(getproperty(x, f), v) end ```

The first field is a pointer and the offset for the second field is hardcoded to 8 bytes, but that's only valid on 64bit systems (which the bindings were generated on). The problem is coming from codegen where we just use the field offset from Clang, which presumably returns the field offset on the host system: https://github.com/JuliaInterop/Clang.jl/blob/8ed2dd214fd89748b92a13b656b5f00b3fd71a18/src/generator/codegen.jl#L311-L320

It's slightly annoying to fix since we'll need to keep track of how many pointer fields there are and take into account all of their offsets. But I'm guessing not many people use 32bit machines anymore, especially in scientific computing, so this probably isn't very high-priority.

Gnimuc commented 2 weeks ago

The problem is coming from codegen where we just use the field offset from Clang, which presumably returns the field offset on the host system:

Does this happen when a multiplatform setup is used? e.g. https://github.com/JuliaGPU/VulkanCore.jl/tree/master/lib

JamesWrigley commented 2 weeks ago

Ah hah, no it doesn't :) Maybe we should warn users that if they're not generating code for all targets they must at least generate a 64bit and 32bit target?

Gnimuc commented 2 weeks ago

Yes, we should issue a warning whenever _emit_getproperty_ptr! is called. However, we only provide two options: generating a single target(unsafe) or generating all targets(extremely safe) that Julia currently runs on.