Eliminating allocation of ccall root in simple cases

Also fix and test an invalid `unsafe_convert` definition in Base.
The result of `pointer_from_objref` on isbits types is never valid to use
across safepoint.

Author: yuyichao

base/inference.jl

@@ -5227,6 +5227,27 @@ function occurs_outside_getfield(@nospecialize(e), @nospecialize(sym),
         if head === :(=)
             return occurs_outside_getfield(e.args[2], sym, sv,
                                            field_count, field_names)
+        elseif head === :foreigncall
+            args = e.args
+            nccallargs = args[5]::Int
+            # Only arguments escape the structure/layout of the object,
+            # GC root arguments do not.
+            # Also note that only being used in the root slot for this ccall itself
+            # does **not** mean that the object is not needed during the ccall.
+            # However, if its address is never taken
+            # and the object is never used in a way that escapes its layout, we can be sure
+            # that there's no way the user code can rely on the heap allocation of this object.
+            for i in 1:length(args)
+                a = args[i]
+                if i > 5 + nccallargs && symequal(a, sym)
+                    # No need to verify indices, uninitialized members can be
+                    # ignored in root slot.
+                    continue
+                end
+                if occurs_outside_getfield(a, sym, sv, field_count, field_names)
+                    return true
+                end
+            end
         else
             if (head === :block && isa(sym, Slot) &&
                 sv.src.slotflags[slot_id(sym)] & Slot_UsedUndef == 0)
@@ -5810,8 +5831,11 @@ end
 function replace_getfield!(e::Expr, tupname, vals, field_names, sv::InferenceState)
     for i = 1:length(e.args)
         a = e.args[i]
-        if isa(a,Expr) && is_known_call(a, getfield, sv.src, sv.mod) &&
-            symequal(a.args[2],tupname)
+        if !isa(a, Expr)
+            continue
+        end
+        a = a::Expr
+        if is_known_call(a, getfield, sv.src, sv.mod) && symequal(a.args[2], tupname)
             idx = if isa(a.args[3], Int)
                 a.args[3]
             else
@@ -5840,8 +5864,23 @@ function replace_getfield!(e::Expr, tupname, vals, field_names, sv::InferenceSta
                 end
             end
             e.args[i] = val
-        elseif isa(a, Expr)
-            replace_getfield!(a::Expr, tupname, vals, field_names, sv)
+        else
+            if a.head === :foreigncall
+                args = a.args
+                nccallargs = args[5]::Int
+                le = length(args)
+                next_i = 6 + nccallargs
+                while next_i <= le
+                    i = next_i
+                    next_i += 1
+
+                    symequal(args[i], tupname) || continue
+                    # Replace the gc root argument with its fields
+                    splice!(args, i, vals)
+                    next_i += length(vals) - 1
+                end
+            end
+            replace_getfield!(a, tupname, vals, field_names, sv)
         end
     end
 end

base/refpointer.jl

@@ -54,9 +54,16 @@ convert(::Type{Ref{T}}, x) where {T} = RefValue{T}(x)
 
 function unsafe_convert(P::Type{Ptr{T}}, b::RefValue{T}) where T
     if isbits(T)
-        return convert(P, data_pointer_from_objref(b))
+        return convert(P, pointer_from_objref(b))
+    elseif isleaftype(T)
+        return convert(P, pointer_from_objref(b.x))
     else
-        return convert(P, data_pointer_from_objref(b.x))
+        # If the slot is not leaf type, it could be either isbits or not.
+        # If it is actually an isbits type and the type inference can infer that
+        # it can rebox the `b.x` if we simply call `pointer_from_objref(b.x)` on it.
+        # Instead, explicitly load the pointer from the `RefValue` so that the pointer
+        # is the same as the one rooted in the `RefValue` object.
+        return convert(P, pointerref(Ptr{Ptr{Void}}(pointer_from_objref(b)), 1, 0))
     end
 end
 function unsafe_convert(P::Type{Ptr{Any}}, b::RefValue{Any})

test/ccall.jl

@@ -1285,3 +1285,21 @@ evalf_callback_c_19805{FUNC_FT}(ci::callinfos_19805{FUNC_FT}) = cfunction(
 
 @test_throws(ErrorException("ccall: the type of argument 1 doesn't correspond to a C type"),
              evalf_callback_c_19805( callinfos_19805(sin) ))
+
+# test Ref{abstract_type} calling parameter passes a heap box
+abstract type Abstract22734 end
+struct Bits22734 <: Abstract22734
+    x::Int
+    y::Float64
+end
+function cb22734(ptr::Ptr{Void})
+    gc()
+    obj = unsafe_pointer_to_objref(ptr)::Bits22734
+    obj.x + obj.y
+end
+ptr22734 = cfunction(cb22734, Float64, Tuple{Ptr{Void}})
+function caller22734(ptr)
+    obj = Bits22734(12, 20)
+    ccall(ptr, Float64, (Ref{Abstract22734},), obj)
+end
+@test caller22734(ptr22734) === 32.0

test/codegen.jl

@@ -128,10 +128,37 @@ function compare_large_struct(a)
     end
 end
 
+mutable struct MutableStruct
+    a::Int
+    MutableStruct() = new()
+end
+
+breakpoint_mutable(a::MutableStruct) = ccall(:jl_breakpoint, Void, (Ref{MutableStruct},), a)
+
+# Allocation with uninitialized field as gcroot
+mutable struct BadRef
+    x::MutableStruct
+    y::MutableStruct
+    BadRef(x) = new(x)
+end
+Base.cconvert(::Type{Ptr{BadRef}}, a::MutableStruct) = BadRef(a)
+Base.unsafe_convert(::Type{Ptr{BadRef}}, ar::BadRef) = Ptr{BadRef}(pointer_from_objref(ar.x))
+
+breakpoint_badref(a::MutableStruct) = ccall(:jl_breakpoint, Void, (Ptr{BadRef},), a)
+
 if opt_level > 0
     @test !contains(get_llvm(isequal, Tuple{Nullable{BigFloat}, Nullable{BigFloat}}), "%gcframe")
     @test !contains(get_llvm(pointer_not_safepoint, Tuple{}), "%gcframe")
     compare_large_struct_ir = get_llvm(compare_large_struct, Tuple{typeof(create_ref_struct())})
     @test contains(compare_large_struct_ir, "call i32 @memcmp")
     @test !contains(compare_large_struct_ir, "%gcframe")
+
+    @test contains(get_llvm(MutableStruct, Tuple{}), "jl_gc_pool_alloc")
+    breakpoint_mutable_ir = get_llvm(breakpoint_mutable, Tuple{MutableStruct})
+    @test !contains(breakpoint_mutable_ir, "%gcframe")
+    @test !contains(breakpoint_mutable_ir, "jl_gc_pool_alloc")
+
+    breakpoint_badref_ir = get_llvm(breakpoint_badref, Tuple{MutableStruct})
+    @test !contains(breakpoint_badref_ir, "%gcframe")
+    @test !contains(breakpoint_badref_ir, "jl_gc_pool_alloc")
 end