Fix a small number of invalid unsafe code #23914

yuyichao · 2017-09-28T02:56:38Z

Started as trying to fix #23520 but as the title said, this end up fixing only a small number of them -- basically all use of pointer were wrong. Also marked a few even more invalid places where managed pointer is passed to unsafe_wrap (the fix for those will be much more involved).

The only unrelated change is https://github.com/JuliaLang/julia/compare/yyc/gc/safe_unsafe_write?expand=1#diff-b103e3ad474d354f14374e435955adcbL516 which is what I happened to notice when fixing the function.

Should in principle have no effect on performance but inlining may still be sensitive in some cases so

@nanosoldier runbenchmarks(ALL, vs=":master")

quinnj · 2017-09-28T03:01:25Z

Could we get a little documentation/explanation on gc_preserve for those of us who haven't been following this development very closely? This seems like a pretty significant change, i.e. any time I use pointer in my code is probably wrong, correct? Are there cases where I use pointer that I don't need gc_preserve?

nanosoldier · 2017-09-28T06:48:30Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

mschauer · 2017-09-28T09:48:50Z

The infrastructure was introduced in #23610, related devdoc entry: https://docs.julialang.org/en/latest/devdocs/llvm/#Keeping-values-alive-in-the-absence-of-uses-1

yuyichao · 2017-09-28T12:47:03Z

Could we get a little documentation/explanation on gc_preserve for those of us who haven't been following this development very closely?

The macro has doc.

This seems like a pretty significant change, i.e. any time I use pointer in my code is probably wrong, correct? Are there cases where I use pointer that I don't need gc_preserve?

There has been no change in the semantic, those code has always been wrong. It's just that the compiler wasn't smart enough to use them in a lot of the cases. The main issue is that local variables do not create GC roots, only their use does. This is consistent with the property that extra local variable assignments has absolutely no semantic significance, including the arguments for inlined functions.

There are cases where @gc_preserve is not necessary, basic when there are other defined way an object is rooted. Other defined way includes,

Globals.

Note that simply stroing the object to a global is not good enough. Ref JuliaPy/PyCall.jl@0e3bf0e
ccall

If the pointer value is only used in ccall and the same object is being preserved by another ccall argument then you don't have to preserve that object explicitly, i.e. ccall(...., pointer(array) + 1, array) is fine, though could be a little confusing/misleading for the reader.
If the caller is supposed to do so for you

When you define unsafe_convert and pointer methods, it's obviously fine to call these without preserving the arguments since the caller must do that for you, if the result needs to be used. If you are using the pointer yourself in those methods (not just pointer arithmetic, but loading/storing on the pointer) though, those use must be preserved since the caller only need to preserve the argument if the result will be used.

Fix #23520

rfourquet · 2017-09-28T13:21:28Z

base/random/RNGs.jl

@@ -137,7 +137,7 @@ hash(r::MersenneTwister, h::UInt) = foldr(hash, h, (r.seed, r.state, r.vals, r.i
 @inline mt_pop!(r::MersenneTwister) = @inbounds return r.vals[r.idx+=1]

 function gen_rand(r::MersenneTwister)
-    dsfmt_fill_array_close1_open2!(r.state, pointer(r.vals), length(r.vals))
+    Base.@gc_preserve r dsfmt_fill_array_close1_open2!(r.state, pointer(r.vals), length(r.vals))


Here (also in other places), the "preserved" object is again used on the line after, isn't it enough to preserve it? (I guess not as you updated the code) Is it that the compiler can reorder instructions?

No it's not.

To clarify, after inlining any uses after may be dead code and eliminated.

This is the detail I don't want to go into since I don't want people to rely on it but another way this can be invalid is that the compiler can see you are using only some fields and then discards all other fields.

rfourquet · 2017-09-28T13:27:32Z

base/random/RNGs.jl

-          2*n128, Close1Open2())
+    Base.@gc_preserve A rand!(r, unsafe_wrap(Array, convert(Ptr{Float64}, pointer(A)), 2*n128),
+                              2*n128, Close1Open2())
+    # FIXME: This code is completely invalid!!!
    A128 = unsafe_wrap(Array, convert(Ptr{UInt128}, pointer(A)), n128)


I had plans to change this to avoid using unsafe_wrap for performance reasons (cf. #9174), but maybe I need to understand what is invalid here? Preserving A wouldn't be enough?

You can't pass a managed pointer to unsafe_wrap.

Ok thanks for the precision (this limitation is not documented in the docstring). I will work on fixing those cases.

yuyichao · 2017-09-28T13:58:10Z

There are of course cases where the code will practically be fine, for now. But I don't want to go into detail to document the limitation of the compiler/optimizer especially since people don't have to rely on these anymore with the introduction of @gc_preserve. We have a few guaranteed rooting as mentioned above (ccall roots (of cconvert return value), @gc_preserve and globals) their rooting behavior is documented in their own docstrings (not for global I guess so we can add that). In general, if it is not explicitly documented that something will guarantee the lifetime of an object then there's no such guarantee.

vtjnash · 2017-09-28T19:56:47Z

base/iostream.jl

@@ -264,7 +264,7 @@ end
 function readbytes_all!(s::IOStream, b::Array{UInt8}, nb)
    olb = lb = length(b)
    nr = 0
-    while nr < nb
+    @gc_preserve b while nr < nb


should we just use Ref(b, nr + 1) instead?

There's a few different places that uses pointer(ary, i) that can be changed, yes.

vtjnash · 2017-09-28T20:02:25Z

src/cgutils.cpp

@@ -2108,6 +2108,9 @@ static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &vinfo)
        return UndefValue::get(T_prjlvalue);
    if (vinfo.constant)
        return maybe_decay_untracked(literal_pointer_val(ctx, vinfo.constant));
+    // This can happen in early bootstrap for `gc_preserve_begin` return value.
+    if (jt == (jl_value_t*)jl_void_type)
+        return maybe_decay_untracked(literal_pointer_val(ctx, jl_nothing));


If vinfo.typ == jl_void_type then vinfo.constant == jl_nothing and we shouldn't get here. What's violating that?

julia/src/codegen.cpp

Line 3926 in 056b374

jl_cgval_t tok(token, NULL, false, (jl_value_t*)jl_void_type, NULL);

It's not just a nothing but also an LLVM token though so I'm avoiding touching that.

Ah, ok. Right, this was an intentionally malformed value.

StefanKarpinski · 2017-09-29T01:29:48Z

There's a bunch of cases here where you use @gc_preserve_begin and @gc_preserve_end pairs instead of the @gc_preserve macro even though the latter seems applicable. Was there some reason for that? Too early in bootstrap? Macro lowering having some undesirable effect?

yuyichao · 2017-09-29T01:35:22Z

They are exactly the same. The @_gc_preserve_begin and @_gc_preserve_end are used just for bootstrap, which is why it's defined right after the inline/noinline macros that serves the same purpose..................

StefanKarpinski · 2017-09-29T01:48:17Z

I figured it was bootstrap-related.

More cases similar to those identified in #23914 fix #33899

More cases similar to those identified in #23914 fix #33899 (cherry picked from commit e1086fe)

More cases similar to those identified in #23914 fix #33899

yuyichao force-pushed the yyc/gc/safe_unsafe_write branch 2 times, most recently from dd7a115 to ed296e9 Compare September 28, 2017 13:05

Fix a small number of invalid unsafe code

15ca594

Fix #23520

yuyichao force-pushed the yyc/gc/safe_unsafe_write branch from ed296e9 to 15ca594 Compare September 28, 2017 13:06

rfourquet reviewed Sep 28, 2017

View reviewed changes

vtjnash approved these changes Sep 28, 2017

View reviewed changes

Keno approved these changes Sep 28, 2017

View reviewed changes

yuyichao merged commit 3776fce into master Sep 29, 2017

yuyichao deleted the yyc/gc/safe_unsafe_write branch September 29, 2017 00:32

danielwe mentioned this pull request Nov 26, 2024

Refactor Linear Algebra's matmul to use a safer scratch buffer JuliaLang/LinearAlgebra.jl#500

Closed

vtjnash added a commit that referenced this pull request Nov 25, 2019

fix some more invalid unsafe code

e1086fe

More cases similar to those identified in #23914 fix #33899

KristofferC pushed a commit that referenced this pull request Nov 29, 2019

fix some more invalid unsafe code

f41937b

More cases similar to those identified in #23914 fix #33899 (cherry picked from commit e1086fe)

KristofferC pushed a commit that referenced this pull request Dec 4, 2019

fix some more invalid unsafe code

efae1fb

More cases similar to those identified in #23914 fix #33899 (cherry picked from commit e1086fe)

KristofferC pushed a commit that referenced this pull request Apr 11, 2020

fix some more invalid unsafe code

9c0a9df

More cases similar to those identified in #23914 fix #33899

mbauman mentioned this pull request May 4, 2020

FIXME: Invalid matmul code #35731

Closed

tkf mentioned this pull request Sep 19, 2021

Remove buffer pool for tiled GEMM #42309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a small number of invalid unsafe code #23914

Fix a small number of invalid unsafe code #23914

yuyichao commented Sep 28, 2017

quinnj commented Sep 28, 2017

nanosoldier commented Sep 28, 2017

mschauer commented Sep 28, 2017

yuyichao commented Sep 28, 2017

rfourquet Sep 28, 2017

yuyichao Sep 28, 2017

Keno Sep 28, 2017

yuyichao Sep 28, 2017

rfourquet Sep 28, 2017

yuyichao Sep 28, 2017

rfourquet Sep 28, 2017

yuyichao commented Sep 28, 2017

vtjnash Sep 28, 2017

yuyichao Sep 28, 2017

vtjnash Sep 28, 2017

yuyichao Sep 28, 2017

vtjnash Sep 28, 2017

StefanKarpinski commented Sep 29, 2017

yuyichao commented Sep 29, 2017

StefanKarpinski commented Sep 29, 2017

Fix a small number of invalid unsafe code #23914

Fix a small number of invalid unsafe code #23914

Conversation

yuyichao commented Sep 28, 2017

quinnj commented Sep 28, 2017

nanosoldier commented Sep 28, 2017

mschauer commented Sep 28, 2017

yuyichao commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Sep 29, 2017

yuyichao commented Sep 29, 2017

StefanKarpinski commented Sep 29, 2017