Invalid getproperty/setproperty! for struct records

[serenity4]

Currently, getproperty/setproperty! are not defined correctly for struct records:

• getproperty(::Ptr, ...) may return a pointer that aliases memory used by other fields. This happens because struct fields may have less bits allocated than the type they represent. For example, a UInt32 may be allocated 3 bytes instead of 4 in a record.
• setproperty!(::Ptr, ...) assumes that what is returned by getproperty(::Ptr, ...) is a pointer which can be safely stored to. This assumption is false when getproperty(::Ptr, ...) aliases memory since a naive store operation would override other fields. Furthermore, when the field is not byte-aligned, it returns instead a tuple consisting of a base pointer, an offset and a width (see this example).

The case with non-byte-aligned fields seems tricky to deal with, and I am open to suggestions. For the case where fields are byte-aligned I believe we need to:

• Handle the case where a pointer aliases memory in getproperty(::MyType, ...) to get a value reconstructed from what's in the pointer for the bytes that fit in the struct and pad with null bytes for the overlapping part.
• Handle the case in setproperty!(::Ptr, ...) where a pointer that aliases memory is returned from getproperty(::Ptr, ...). Basically, instead of unsafe_store! which stores all bytes at once, we can use unsafe_copyto! to copy only the bytes that fit into the struct.

Taking this example from VulkanCore to illustrate the byte-aligned case, I think the generated code should look like

struct VkAccelerationStructureInstanceKHR
    data::NTuple{64, UInt8}
end

# no changes
function Base.getproperty(x::Ptr{VkAccelerationStructureInstanceKHR}, f::Symbol)
    f === :transform && return Ptr{VkTransformMatrixKHR}(x + 0)
    f === :instanceCustomIndex && return Ptr{UInt32}(x + 48)
    f === :mask && return Ptr{UInt32}(x + 51)
    f === :instanceShaderBindingTableRecordOffset && return Ptr{UInt32}(x + 52)
    f === :flags && return Ptr{VkGeometryInstanceFlagsKHR}(x + 55)
    f === :accelerationStructureReference && return Ptr{UInt64}(x + 56)
    return getfield(x, f)
end

function unsafe_load_overlapping(ptr, nbytes)
    T = eltype(ptr)
    bytes = Base.unsafe_convert(Ptr{UInt8}, ptr)
    arr = zeros(UInt8, sizeof(T))
    arr[1:nbytes] .= unsafe_wrap(Array, bytes, nbytes)
    Base.reinterpret(T, arr)[]
end

function Base.getproperty(x::VkAccelerationStructureInstanceKHR, f::Symbol)
    r = Ref{VkAccelerationStructureInstanceKHR}(x)
    ptr = Base.unsafe_convert(Ptr{VkAccelerationStructureInstanceKHR}, r)
    fptr = getproperty(ptr, f)
    begin
        # change here; I didn't do the other branch but we could employ a similar logic
        if fptr isa Ptr
            GC.@preserve r begin
                # use unsafe_load_overlapping only for fields that overlap with others
                # (all the following fields are UInt32, so 4 bytes in size)
                if f === :instanceCustomIndex
                    unsafe_load_overlapping(fptr, 3)
                elseif f === :mask
                    unsafe_load_overlapping(fptr, 1)
                elseif f === :instanceShaderBindingTableRecordOffset
                    unsafe_load_overlapping(fptr, 3)
                elseif f === :flags
                    unsafe_load_overlapping(fptr, 1)
                else
                    unsafe_load(fptr)
                end
            end
        else
            (baseptr, offset, width) = fptr
            ty = eltype(baseptr)
            i8 = GC.@preserve(r, unsafe_load(baseptr))
            bitstr = bitstring(i8)
            sig = bitstr[(end - offset) - (width - 1):end - offset]
            zexted = lpad(sig, 8 * sizeof(ty), '0')
            return parse(ty, zexted; base = 2)
        end
    end
end

# this one was reworked a bit. Instead of using `unsafe_store!` (which stores all the bytes at once)
# we use `unsafe_copyto!` to store just the number of bytes we need (truncating in case of overflow)
# behavior is modified only for fields that overlap with others
function Base.setproperty!(x::Ptr{VkAccelerationStructureInstanceKHR}, f::Symbol, v)
    vref = Ref(v)
    GC.@preserve vref begin
        vptr = Base.unsafe_convert(Ptr{eltype(vref)}, vref)
        vbytes = Base.unsafe_convert(Ptr{UInt8}, vptr)
        xbytes = Base.unsafe_convert(Ptr{UInt8}, getproperty(x, f))
        if f === :transform
            unsafe_copyto!(xbytes, vbytes, 48)
        elseif f === :instanceCustomIndex
            unsafe_copyto!(xbytes, vbytes, 3)
        elseif f === :mask
            unsafe_copyto!(xbytes, vbytes, 1)
        elseif f === :instanceShaderBindingTableRecordOffset
            unsafe_copyto!(xbytes, vbytes, 3)
        elseif f === :flags
            unsafe_copyto!(xbytes, vbytes, 1)
        elseif f === :accelerationStructureReference
            unsafe_copyto!(xbytes, vbytes, 8)
        end
    end
end

I would welcome any better way to use e.g. 3 bytes to construct a UInt32 with an implicit padding instead of defining an array of null bytes to be reinterpret in the target type. Also, for the case with non-byte-aligned fields I am essentially missing a way to store/load specific bits (and not whole bytes).

Originates from #305, which will require this issue to be solved to work properly on struct records.

[Gnimuc]

This is the definition of VkAccelerationStructureInstanceKHR:

// Provided by VK_KHR_acceleration_structure
typedef struct VkAccelerationStructureInstanceKHR {
    VkTransformMatrixKHR          transform;
    uint32_t                      instanceCustomIndex:24;
    uint32_t                      mask:8;
    uint32_t                      instanceShaderBindingTableRecordOffset:24;
    VkGeometryInstanceFlagsKHR    flags:8;
    uint64_t                      accelerationStructureReference;
} VkAccelerationStructureInstanceKHR;

For now, bit field structs support is very experimental. I think we should reimplement both getproperty and setproperty! based on what @jpsamaroo suggested in #228 (comment).

[Gnimuc]

fixed by #312