Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base.unsafe_convert(Ptr, ::Vector} started throwing on master #51996

Closed
Drvi opened this issue Nov 2, 2023 · 9 comments · Fixed by #53589
Closed

Base.unsafe_convert(Ptr, ::Vector} started throwing on master #51996

Drvi opened this issue Nov 2, 2023 · 9 comments · Fixed by #53589
Labels
arrays [a, r, r, a, y, s]

Comments

@Drvi
Copy link
Contributor

Drvi commented Nov 2, 2023

Hey folks,

This used to work prior to 1.11:

julia> Base.unsafe_convert(Ptr{Float64}, Int[0]) 
Ptr{Float64} @0x00000002992d5d00

julia> Base.unsafe_convert(Ptr{Int}, Int[0])
Ptr{Int64} @0x00000002998d5b40

But now it throws:

julia> Base.unsafe_convert(Ptr{Float64}, Int[0])
ERROR: conversion to pointer not defined for Vector{Int64}

julia> Base.unsafe_convert(Ptr{Int}, Int[0])
ERROR: conversion to pointer not defined for Vector{Int64}

To give more context, we do something like:

struct W{Int}
    x::Int
end

SRC_T = Tuple{String,W{Int}}
DST_T = Tuple{String,Int}

x = SRC_T[("a", W(1)), ("b", W(2))]
ptr = Base.unsafe_convert(Ptr{DST_T}, x)
Base.unsafe_wrap(Vector{DST_T}, ptr, length(x))

Which would on 1.9 / 1.10 yield:

julia> ptr = Base.unsafe_convert(Ptr{DST_T}, x)
Ptr{Tuple{String, Int64}} @0x0000000160d55060

julia> Base.unsafe_wrap(Vector{DST_T}, ptr, length(x))
2-element Vector{Tuple{String, Int64}}:
 ("a", 1)
 ("b", 2)

But throws in the unsafe_convert step on master.

Is this expected? This broke our master-tracking CI.

Would a reasonable fix for this be rewriting theunsafe_convert step to

ptr = Base.unsafe_convert(Ptr{DST_T}, pointer(x))

(adding the pointer(...) call)?

Is this fundamentally impossible to make safe and there is a better idiom for this kind of unsafe conversion?

Thanks!

@KristofferC
Copy link
Member

Dup of #51962 I think

@vtjnash
Copy link
Member

vtjnash commented Nov 2, 2023

It seems like this is some sort of reinterpret call? It is not well-defined what happens if you call unsafe_wrap on an pointer derived from any existing Julia object. In particular for the example here, any gc_wb would hit the wrong object and lead to memory corruption.

In the particular unsafe_convert example, the docs for ccall state that you must call cconvert first and GC.@preserve the result of that.

@Drvi
Copy link
Contributor Author

Drvi commented Nov 2, 2023

@vtjnash Thanks for explaining! To answer your question, yes, what we try to do here is to have a reinterpret that would work on non-isbits types (tuples very similar to the one I used as an example). The actual code does try to ensure the source object is not outlived by the reinterpreted one -- is the approach fundamentally flawed even then? Is there a better way to achieve that (really just getting from SRC_T[("a", W(1)), ("b", W(2))] to DST_T[("a", 1), ("b", 2)])?

@vtjnash
Copy link
Member

vtjnash commented Nov 2, 2023

The operation is illegal in most programming languages (for example, in C it is undefined behavior), so Julia is no different in that respect. That is why reinterpret only works with cases where it can legally do so, and errors for cases that are invalid. Even just "try to ensure the source object is not outlived" is very complicated, and would require you to implement your own garbage collector (a simple RC one is fine) such that each unsafe call incremented the counter and registered a finalizer which decremented the counter.

@vtjnash
Copy link
Member

vtjnash commented Nov 2, 2023

Note that the GC property you must ensure is not "the source object is not outlived by the reinterpreted one" but "the lifetime of the source object is exactly identical to the lifetime of the reinterpreted one"

@oscardssmith
Copy link
Member

specifically because if the reinterpreted one dies first, gc might try to free the reinterpreted one.

@vtjnash
Copy link
Member

vtjnash commented Nov 2, 2023

There is no free on the reinterpreted one. But if the reinterpreted one dies first, the gc may forget to traverse the source one, resulting in part of it getting freed earlier than you expected

@nsajko
Copy link
Contributor

nsajko commented May 1, 2024

Can't repro anymore. Though I guess the idea is still to deprecate at some point?

@giordano
Copy link
Contributor

giordano commented May 1, 2024

I think this was fixed by #53589

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants