Skip to content

Commit

Permalink
Revert "replace many unsafe_convert methods with safer cconvert ones (#…
Browse files Browse the repository at this point in the history
…51764)"

This reverts commit df39cee.
  • Loading branch information
Keno authored Oct 25, 2023
1 parent a1ccf53 commit 552a04f
Show file tree
Hide file tree
Showing 18 changed files with 44 additions and 61 deletions.
4 changes: 2 additions & 2 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1250,10 +1250,10 @@ end
# note: the following type definitions don't mean any AbstractArray is convertible to
# a data Ref. they just map the array element type to the pointer type for
# convenience in cases that work.
pointer(x::AbstractArray{T}) where {T} = unsafe_convert(Ptr{T}, cconvert(Ptr{T}, x))
pointer(x::AbstractArray{T}) where {T} = unsafe_convert(Ptr{T}, x)
function pointer(x::AbstractArray{T}, i::Integer) where T
@inline
pointer(x) + Int(_memory_offset(x, i))::Int
unsafe_convert(Ptr{T}, x) + Int(_memory_offset(x, i))::Int
end

# The distance from pointer(x) to the element at x[I...] in bytes
Expand Down
11 changes: 4 additions & 7 deletions base/c.jl
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ cconvert(::Type{Cstring}, s::AbstractString) =
function cconvert(::Type{Cwstring}, s::AbstractString)
v = transcode(Cwchar_t, String(s))
push!(v, 0)
return cconvert(Cwstring, v)
return v
end

eltype(::Type{Cstring}) = Cchar
Expand All @@ -218,19 +218,16 @@ function unsafe_convert(::Type{Cstring}, s::String)
return Cstring(p)
end

unsafe_convert(::Type{Cstring}, s::Union{Vector{UInt8},Vector{Int8}}) = Cstring(unsafe_convert(Ptr{Cvoid}, s))

function cconvert(::Type{Cwstring}, v::Vector{Cwchar_t})
function unsafe_convert(::Type{Cwstring}, v::Vector{Cwchar_t})
for i = 1:length(v)-1
v[i] == 0 &&
throw(ArgumentError("embedded NULs are not allowed in C strings: $(repr(v))"))
end
v[end] == 0 ||
throw(ArgumentError("C string data must be NUL terminated: $(repr(v))"))
return cconvert(Ptr{Cwchar_t}, v)
p = unsafe_convert(Ptr{Cwchar_t}, v)
return Cwstring(p)
end
unsafe_convert(::Type{Cwstring}, s) = Cwstring(unsafe_convert(Ptr{Cwchar_t}, s))
unsafe_convert(::Type{Cwstring}, s::Cwstring) = s

# symbols are guaranteed not to contain embedded NUL
cconvert(::Type{Cstring}, s::Symbol) = s
Expand Down
4 changes: 3 additions & 1 deletion base/permuteddimsarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,10 @@ Base.parent(A::PermutedDimsArray) = A.parent
Base.size(A::PermutedDimsArray{T,N,perm}) where {T,N,perm} = genperm(size(parent(A)), perm)
Base.axes(A::PermutedDimsArray{T,N,perm}) where {T,N,perm} = genperm(axes(parent(A)), perm)
Base.has_offset_axes(A::PermutedDimsArray) = Base.has_offset_axes(A.parent)

Base.similar(A::PermutedDimsArray, T::Type, dims::Base.Dims) = similar(parent(A), T, dims)
Base.cconvert(::Type{Ptr{T}}, A::PermutedDimsArray{T}) where {T} = Base.cconvert(Ptr{T}, parent(A))

Base.unsafe_convert(::Type{Ptr{T}}, A::PermutedDimsArray{T}) where {T} = Base.unsafe_convert(Ptr{T}, parent(A))

# It's OK to return a pointer to the first element, and indeed quite
# useful for wrapping C routines that require a different storage
Expand Down
9 changes: 3 additions & 6 deletions base/pointer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,17 @@ See also [`cconvert`](@ref)
"""
function unsafe_convert end

# convert strings to String etc. to pass as pointers
cconvert(::Type{Ptr{UInt8}}, s::AbstractString) = String(s)
cconvert(::Type{Ptr{Int8}}, s::AbstractString) = String(s)
unsafe_convert(::Type{Ptr{UInt8}}, x::Symbol) = ccall(:jl_symbol_name, Ptr{UInt8}, (Any,), x)
unsafe_convert(::Type{Ptr{Int8}}, x::Symbol) = ccall(:jl_symbol_name, Ptr{Int8}, (Any,), x)
unsafe_convert(::Type{Ptr{UInt8}}, s::String) = ccall(:jl_string_ptr, Ptr{UInt8}, (Any,), s)
unsafe_convert(::Type{Ptr{Int8}}, s::String) = ccall(:jl_string_ptr, Ptr{Int8}, (Any,), s)
# convert strings to String etc. to pass as pointers
cconvert(::Type{Ptr{UInt8}}, s::AbstractString) = String(s)
cconvert(::Type{Ptr{Int8}}, s::AbstractString) = String(s)

unsafe_convert(::Type{Ptr{T}}, a::Array{T}) where {T} = ccall(:jl_array_ptr, Ptr{T}, (Any,), a)
unsafe_convert(::Type{Ptr{S}}, a::AbstractArray{T}) where {S,T} = convert(Ptr{S}, unsafe_convert(Ptr{T}, a))
unsafe_convert(::Type{Ptr{T}}, a::AbstractArray{T}) where {T} = error("conversion to pointer not defined for $(typeof(a))")
# TODO: add this deprecation to give a better error:
# cconvert(::Type{<:Ptr}, a::AbstractArray) = error("conversion to pointer not defined for $(typeof(a))")
# unsafe_convert(::Type{Ptr{T}}, a::AbstractArray{T}) where {T} = error("missing call to cconvert for call to unsafe_convert for AbstractArray")

# unsafe pointer to array conversions
"""
Expand Down
15 changes: 8 additions & 7 deletions base/refpointer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -148,12 +148,13 @@ if is_primary_base_module
Ref(x::Ptr{T}, i::Integer) where {T} = x + (i - 1) * Core.sizeof(T)

# convert Arrays to pointer arrays for ccall
# For example `["a", "b"]` to Ptr{Cstring} for `char **argv`
function Ref{P}(a::Array{<:Union{Ptr,Cwstring,Cstring}}) where P<:Union{Ptr,Cwstring,Cstring}
return RefArray(a) # effectively a no-op
end
function Ref{P}(a::Array{T}) where P<:Union{Ptr,Cwstring,Cstring} where T
if (isbitstype(T) ? T <: Ptr || T <: Union{Cwstring,Cstring} : T <: eltype(P))
if (!isbitstype(T) && T <: eltype(P))
# this Array already has the right memory layout for the requested Ref
# but the wrong eltype for the constructor
return RefArray{P,typeof(a),Nothing}(a, 1, nothing) # effectively a no-op
return RefArray(a,1,false) # root something, so that this function is type-stable
else
ptrs = Vector{P}(undef, length(a)+1)
roots = Vector{Any}(undef, length(a))
Expand All @@ -163,14 +164,14 @@ if is_primary_base_module
roots[i] = root
end
ptrs[length(a)+1] = C_NULL
return RefArray{P,typeof(ptrs),typeof(roots)}(ptrs, 1, roots)
return RefArray(ptrs,1,roots)
end
end
Ref(x::AbstractArray, i::Integer) = RefArray(x, i)
end

cconvert(::Type{Ptr{P}}, a::Array{P}) where {P<:Union{Ptr,Cwstring,Cstring}} = a
cconvert(::Type{Ref{P}}, a::Array{P}) where {P<:Union{Ptr,Cwstring,Cstring}} = a
cconvert(::Type{Ptr{P}}, a::Array{<:Ptr}) where {P<:Ptr} = a
cconvert(::Type{Ref{P}}, a::Array{<:Ptr}) where {P<:Ptr} = a
cconvert(::Type{Ptr{P}}, a::Array) where {P<:Union{Ptr,Cwstring,Cstring}} = Ref{P}(a)
cconvert(::Type{Ref{P}}, a::Array) where {P<:Union{Ptr,Cwstring,Cstring}} = Ref{P}(a)

Expand Down
2 changes: 1 addition & 1 deletion base/reinterpretarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ axes(a::NonReshapedReinterpretArray{T,0}) where {T} = ()
has_offset_axes(a::ReinterpretArray) = has_offset_axes(a.parent)

elsize(::Type{<:ReinterpretArray{T}}) where {T} = sizeof(T)
cconvert(::Type{Ptr{T}}, a::ReinterpretArray{T,N,S} where N) where {T,S} = cconvert(Ptr{S}, a.parent)
unsafe_convert(::Type{Ptr{T}}, a::ReinterpretArray{T,N,S} where N) where {T,S} = Ptr{T}(unsafe_convert(Ptr{S},a.parent))

@inline @propagate_inbounds function getindex(a::NonReshapedReinterpretArray{T,0,S}) where {T,S}
if isprimitivetype(T) && isprimitivetype(S)
Expand Down
10 changes: 3 additions & 7 deletions base/reshapedarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ setindex!(A::ReshapedRange, val, index::ReshapedIndex) = _rs_setindex!_err()

@noinline _rs_setindex!_err() = error("indexed assignment fails for a reshaped range; consider calling collect")

cconvert(::Type{Ptr{T}}, a::ReshapedArray{T}) where {T} = cconvert(Ptr{T}, parent(a))
unsafe_convert(::Type{Ptr{T}}, a::ReshapedArray{T}) where {T} = unsafe_convert(Ptr{T}, parent(a))

# Add a few handy specializations to further speed up views of reshaped ranges
const ReshapedUnitRange{T,N,A<:AbstractUnitRange} = ReshapedArray{T,N,A,Tuple{}}
Expand All @@ -304,13 +304,9 @@ compute_offset1(parent::AbstractVector, stride1::Integer, I::Tuple{ReshapedRange
(@inline; first(I[1]) - first(axes1(I[1]))*stride1)
substrides(strds::NTuple{N,Int}, I::Tuple{ReshapedUnitRange, Vararg{Any}}) where N =
(size_to_strides(strds[1], size(I[1])...)..., substrides(tail(strds), tail(I))...)
unsafe_convert(::Type{Ptr{T}}, V::SubArray{T,N,P,<:Tuple{Vararg{Union{RangeIndex,ReshapedUnitRange}}}}) where {T,N,P} =
unsafe_convert(Ptr{T}, V.parent) + (first_index(V)-1)*sizeof(T)

# cconvert(::Type{<:Ptr}, V::SubArray{T,N,P,<:Tuple{Vararg{Union{RangeIndex,ReshapedUnitRange}}}}) where {T,N,P} = V
function unsafe_convert(::Type{Ptr{S}}, V::SubArray{T,N,P,<:Tuple{Vararg{Union{RangeIndex,ReshapedUnitRange}}}}) where {S,T,N,P}
parent = V.parent
p = cconvert(Ptr{T}, parent) # XXX: this should occur in cconvert, the result is not GC-rooted
return Ptr{S}(unsafe_convert(Ptr{T}, p) + (first_index(V)-1)*sizeof(T))
end

_checkcontiguous(::Type{Bool}, A::AbstractArray) = false
# `strides(A::DenseArray)` calls `size_to_strides` by default.
Expand Down
7 changes: 3 additions & 4 deletions base/secretbuffer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ function write(io::IO, s::SecretBuffer)
return nb
end

function cconvert(::Type{Cstring}, s::SecretBuffer)
cconvert(::Type{Cstring}, s::SecretBuffer) = unsafe_convert(Cstring, s)
function unsafe_convert(::Type{Cstring}, s::SecretBuffer)
# Ensure that no nuls appear in the valid region
if any(==(0x00), s.data[i] for i in 1:s.size)
throw(ArgumentError("`SecretBuffers` containing nul bytes cannot be converted to C strings"))
Expand All @@ -151,10 +152,8 @@ function cconvert(::Type{Cstring}, s::SecretBuffer)
write(s, '\0')
s.ptr = p
s.size -= 1
return s.data
return Cstring(unsafe_convert(Ptr{Cchar}, s.data))
end
# optional shim for manual calls to unsafe_convert:
# unsafe_convert(::Type{Cstring}, s::SecretBuffer) = unsafe_convert(Cstring, cconvert(Cstring, s))

seek(io::SecretBuffer, n::Integer) = (io.ptr = max(min(n+1, io.size+1), 1); io)
seekend(io::SecretBuffer) = seek(io, io.size+1)
Expand Down
4 changes: 2 additions & 2 deletions base/strings/basic.jl
Original file line number Diff line number Diff line change
Expand Up @@ -800,8 +800,8 @@ IndexStyle(::Type{<:CodeUnits}) = IndexLinear()

write(io::IO, s::CodeUnits) = write(io, s.s)

cconvert(::Type{Ptr{T}}, s::CodeUnits{T}) where {T} = cconvert(Ptr{T}, s.s)
cconvert(::Type{Ptr{Int8}}, s::CodeUnits{UInt8}) = cconvert(Ptr{Int8}, s.s)
unsafe_convert(::Type{Ptr{T}}, s::CodeUnits{T}) where {T} = unsafe_convert(Ptr{T}, s.s)
unsafe_convert(::Type{Ptr{Int8}}, s::CodeUnits{UInt8}) = unsafe_convert(Ptr{Int8}, s.s)

"""
codeunits(s::AbstractString)
Expand Down
7 changes: 2 additions & 5 deletions base/subarray.jl
Original file line number Diff line number Diff line change
Expand Up @@ -466,11 +466,8 @@ find_extended_inds(::ScalarIndex, I...) = (@inline; find_extended_inds(I...))
find_extended_inds(i1, I...) = (@inline; (i1, find_extended_inds(I...)...))
find_extended_inds() = ()

# cconvert(::Type{<:Ptr}, V::SubArray{T,N,P,<:Tuple{Vararg{RangeIndex}}}) where {T,N,P} = V
function unsafe_convert(::Type{Ptr{S}}, V::SubArray{T,N,P,<:Tuple{Vararg{RangeIndex}}}) where {S,T,N,P}
parent = V.parent
p = cconvert(Ptr{T}, parent) # XXX: this should occur in cconvert, the result is not GC-rooted
return Ptr{S}(unsafe_convert(Ptr{T}, p) + _memory_offset(parent, map(first, V.indices)...))
function unsafe_convert(::Type{Ptr{T}}, V::SubArray{T,N,P,<:Tuple{Vararg{RangeIndex}}}) where {T,N,P}
return unsafe_convert(Ptr{T}, V.parent) + _memory_offset(V.parent, map(first, V.indices)...)
end

pointer(V::FastSubArray, i::Int) = pointer(V.parent, V.offset1 + V.stride1*i)
Expand Down
2 changes: 1 addition & 1 deletion stdlib/InteractiveUtils/src/clipboard.jl
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ elseif Sys.iswindows()
pdata == C_NULL && return cleanup(:GlobalAlloc)
plock = ccall((:GlobalLock, "kernel32"), stdcall, Ptr{UInt16}, (Ptr{UInt16},), pdata)
plock == C_NULL && return cleanup(:GlobalLock)
GC.@preserve x_u16 memcpy(plock, Base.unsafe_convert(Ptr{UInt16}, Base.cconvert(Ptr{UInt16}, x_u16)), sizeof(x_u16))
GC.@preserve x_u16 memcpy(plock, Base.unsafe_convert(Ptr{UInt16}, x_u16), sizeof(x_u16))
unlock = ccall((:GlobalUnlock, "kernel32"), stdcall, Cint, (Ptr{UInt16},), pdata)
(unlock == 0 && Libc.GetLastError() == 0) || return cleanup(:GlobalUnlock) # this should never fail
pset = ccall((:SetClipboardData, "user32"), stdcall, Ptr{UInt16}, (Cuint, Ptr{UInt16}), 13, pdata) # CF_UNICODETEXT
Expand Down
4 changes: 2 additions & 2 deletions stdlib/LinearAlgebra/src/adjtrans.jl
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,8 @@ Base.strides(A::Transpose{<:Any, <:AbstractVector}) = (stride(A.parent, 2), stri
Base.strides(A::Adjoint{<:Real, <:AbstractMatrix}) = reverse(strides(A.parent))
Base.strides(A::Transpose{<:Any, <:AbstractMatrix}) = reverse(strides(A.parent))

Base.cconvert(::Type{Ptr{T}}, A::Adjoint{<:Real, <:AbstractVecOrMat}) where {T} = Base.cconvert(Ptr{T}, A.parent)
Base.cconvert(::Type{Ptr{T}}, A::Transpose{<:Any, <:AbstractVecOrMat}) where {T} = Base.cconvert(Ptr{T}, A.parent)
Base.unsafe_convert(::Type{Ptr{T}}, A::Adjoint{<:Real, <:AbstractVecOrMat}) where {T} = Base.unsafe_convert(Ptr{T}, A.parent)
Base.unsafe_convert(::Type{Ptr{T}}, A::Transpose{<:Any, <:AbstractVecOrMat}) where {T} = Base.unsafe_convert(Ptr{T}, A.parent)

Base.elsize(::Type{<:Adjoint{<:Real, P}}) where {P<:AbstractVecOrMat} = Base.elsize(P)
Base.elsize(::Type{<:Transpose{<:Any, P}}) where {P<:AbstractVecOrMat} = Base.elsize(P)
Expand Down
2 changes: 1 addition & 1 deletion stdlib/LinearAlgebra/test/blas.jl
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ Base.getindex(A::WrappedArray, i::Int) = A.A[i]
Base.getindex(A::WrappedArray{T, N}, I::Vararg{Int, N}) where {T, N} = A.A[I...]
Base.setindex!(A::WrappedArray, v, i::Int) = setindex!(A.A, v, i)
Base.setindex!(A::WrappedArray{T, N}, v, I::Vararg{Int, N}) where {T, N} = setindex!(A.A, v, I...)
Base.cconvert(::Type{Ptr{T}}, A::WrappedArray{T}) where T = Base.cconvert(Ptr{T}, A.A)
Base.unsafe_convert(::Type{Ptr{T}}, A::WrappedArray{T}) where T = Base.unsafe_convert(Ptr{T}, A.A)

Base.strides(A::WrappedArray) = strides(A.A)
Base.elsize(::Type{WrappedArray{T,N}}) where {T,N} = Base.elsize(Array{T,N})
Expand Down
8 changes: 3 additions & 5 deletions stdlib/Random/src/DSFMT.jl
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,9 @@ function dsfmt_jump(s::DSFMT_state, jp::GF2X)
work = zeros(Int32, JN32)
rwork = reinterpret(UInt64, work)
dsfmt = Vector{UInt64}(undef, nval >> 1)
dsfmtref = Base.cconvert(Ptr{Cvoid}, dsfmt)
valref = Base.cconvert(Ptr{Cvoid}, val)
GC.@preserve dsfmtref valref begin
pdsfmt = Base.unsafe_convert(Ptr{Cvoid}, dsfmtref)
pval = Base.unsafe_convert(Ptr{Cvoid}, valref)
GC.@preserve dsfmt val begin
pdsfmt = Base.unsafe_convert(Ptr{Cvoid}, dsfmt)
pval = Base.unsafe_convert(Ptr{Cvoid}, val)
Base.Libc.memcpy(pdsfmt, pval, (nval - 1) * sizeof(Int32))
end
dsfmt[end] = UInt64(N*2)
Expand Down
6 changes: 3 additions & 3 deletions stdlib/SharedArrays/src/SharedArrays.jl
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module SharedArrays
using Mmap, Distributed, Random

import Base: length, size, elsize, ndims, IndexStyle, reshape, convert, deepcopy_internal,
show, getindex, setindex!, fill!, similar, reduce, map!, copyto!, cconvert
show, getindex, setindex!, fill!, similar, reduce, map!, copyto!, unsafe_convert
import Random
using Serialization
using Serialization: serialize_cycle_header, serialize_type, writetag, UNDEFREF_TAG, serialize, deserialize
Expand Down Expand Up @@ -358,8 +358,8 @@ for each worker process.
"""
localindices(S::SharedArray) = S.pidx > 0 ? range_1dim(S, S.pidx) : 1:0

cconvert(::Type{Ptr{T}}, S::SharedArray{T}) where {T} = cconvert(Ptr{T}, sdata(S))
cconvert(::Type{Ptr{T}}, S::SharedArray ) where {T} = cconvert(Ptr{T}, sdata(S))
unsafe_convert(::Type{Ptr{T}}, S::SharedArray{T}) where {T} = unsafe_convert(Ptr{T}, sdata(S))
unsafe_convert(::Type{Ptr{T}}, S::SharedArray ) where {T} = unsafe_convert(Ptr{T}, sdata(S))

function SharedArray(A::Array)
S = SharedArray{eltype(A),ndims(A)}(size(A))
Expand Down
6 changes: 1 addition & 5 deletions test/arrayops.jl
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,7 @@ end
@test convert(Array{Int,1}, r) == [2,3,4]
@test_throws MethodError convert(Array{Int,2}, r)
@test convert(Array{Int}, r) == [2,3,4]
let rc = Base.cconvert(Ptr{Int}, r), rs = Base.cconvert(Ptr{Int}, s)
@test rc == rs
@test Base.unsafe_convert(Ptr{Int}, rc) == Base.unsafe_convert(Ptr{Int}, rs)
end
@test Base.unsafe_convert(Ptr{Int}, r) == Base.unsafe_convert(Ptr{Int}, s)
@test isa(r, StridedArray) # issue #22411
end
@testset "linearslow" begin
Expand All @@ -134,7 +131,6 @@ end
@test convert(Array{Int,1}, r) == [2,3,5]
@test_throws MethodError convert(Array{Int,2}, r)
@test convert(Array{Int}, r) == [2,3,5]
# @test_throws ErrorException Base.cconvert(Ptr{Int}, r) broken=true
@test_throws ErrorException Base.unsafe_convert(Ptr{Int}, r)
r[2] = -1
@test a[3] == -1
Expand Down
2 changes: 1 addition & 1 deletion test/strings/basic.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1164,7 +1164,7 @@ end
code_units = Base.CodeUnits("abc")
@test Base.IndexStyle(Base.CodeUnits) == IndexLinear()
@test Base.elsize(code_units) == sizeof(UInt8)
@test Base.unsafe_convert(Ptr{Int8}, Base.cconvert(Ptr{UInt8}, code_units)) == Base.unsafe_convert(Ptr{Int8}, Base.cconvert(Ptr{Int8}, code_units.s))
@test Base.unsafe_convert(Ptr{Int8}, code_units) == Base.unsafe_convert(Ptr{Int8}, code_units.s)
end

@testset "LazyString" begin
Expand Down
2 changes: 1 addition & 1 deletion test/testhelpers/OffsetArrays.jl
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ Base.copy(A::OffsetArray) = parent_call(copy, A)

Base.strides(A::OffsetArray) = strides(parent(A))
Base.elsize(::Type{OffsetArray{T,N,A}}) where {T,N,A} = Base.elsize(A)
Base.cconvert(::Type{Ptr{T}}, A::OffsetArray{T}) where {T} = Base.cconvert(Ptr{T}, parent(A))
@inline Base.unsafe_convert(::Type{Ptr{T}}, A::OffsetArray{T}) where {T} = Base.unsafe_convert(Ptr{T}, parent(A))

# For fast broadcasting: ref https://discourse.julialang.org/t/why-is-there-a-performance-hit-on-broadcasting-with-offsetarrays/32194
Base.dataids(A::OffsetArray) = Base.dataids(parent(A))
Expand Down

0 comments on commit 552a04f

Please sign in to comment.