codegen: fix gc rooting bug (#51744)

ccall was not creating roots for the contents of struct values which contained roots on the stack, as expected to align with `GC.@preserve`, and causing many segfaults for #51319
JuliaLang · Oct 18, 2023 · e36f65f · e36f65f
1 parent 0b31e8b
commit e36f65f
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 25 deletions.
diff --git a/src/ccall.cpp b/src/ccall.cpp
@@ -1393,7 +1393,7 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs)
         if (jl_is_long(argi_root))
             continue;
         jl_cgval_t arg_root = emit_expr(ctx, argi_root);
-        Value *gc_root = get_gc_root_for(arg_root);
+        Value *gc_root = get_gc_root_for(ctx, arg_root);
         if (gc_root)
             gc_uses.push_back(gc_root);
     }

diff --git a/src/cgutils.cpp b/src/cgutils.cpp
@@ -314,19 +314,34 @@ static Value *emit_pointer_from_objref(jl_codectx_t &ctx, Value *V)
     return Call;
 }
 
-static Value *get_gc_root_for(const jl_cgval_t &x)
+static Value *emit_unbox(jl_codectx_t &ctx, Type *to, const jl_cgval_t &x, jl_value_t *jt);
+static void emit_unbox_store(jl_codectx_t &ctx, const jl_cgval_t &x, Value* dest, MDNode *tbaa_dest, unsigned alignment, bool isVolatile=false);
+
+static Value *get_gc_root_for(jl_codectx_t &ctx, const jl_cgval_t &x)
 {
-    if (x.Vboxed)
+    if (x.constant || x.typ == jl_bottom_type)
+        return nullptr;
+    if (x.Vboxed) // superset of x.isboxed
         return x.Vboxed;
-    if (x.ispointer() && !x.constant) {
+    assert(!x.isboxed);
+#ifndef NDEBUG
+    if (x.ispointer()) {
         assert(x.V);
         if (PointerType *T = dyn_cast<PointerType>(x.V->getType())) {
-            if (T->getAddressSpace() == AddressSpace::Tracked ||
-                T->getAddressSpace() == AddressSpace::Derived) {
-                return x.V;
+            assert(T->getAddressSpace() != AddressSpace::Tracked);
+            if (T->getAddressSpace() == AddressSpace::Derived) {
+                // n.b. this IR would not be valid after LLVM-level inlining,
+                // since codegen does not have a way to determine the whether
+                // this argument value needs to be re-rooted
             }
         }
     }
+#endif
+    if (jl_is_concrete_immutable(x.typ) && !jl_is_pointerfree(x.typ)) {
+        Type *T = julia_type_to_llvm(ctx, x.typ);
+        return emit_unbox(ctx, T, x, x.typ);
+    }
+    // nothing here to root, move along
     return nullptr;
 }
 
@@ -1786,9 +1801,6 @@ static Value *emit_bounds_check(jl_codectx_t &ctx, const jl_cgval_t &ainfo, jl_v
     return im1;
 }
 
-static Value *emit_unbox(jl_codectx_t &ctx, Type *to, const jl_cgval_t &x, jl_value_t *jt);
-static void emit_unbox_store(jl_codectx_t &ctx, const jl_cgval_t &x, Value* dest, MDNode *tbaa_dest, unsigned alignment, bool isVolatile=false);
-
 static void emit_write_barrier(jl_codectx_t&, Value*, ArrayRef<Value*>);
 static void emit_write_barrier(jl_codectx_t&, Value*, Value*);
 static void emit_write_multibarrier(jl_codectx_t&, Value*, Value*, jl_value_t*);

diff --git a/src/codegen.cpp b/src/codegen.cpp
@@ -3064,9 +3064,12 @@ static Value *emit_bits_compare(jl_codectx_t &ctx, jl_cgval_t arg1, jl_cgval_t a
             varg2 = emit_pointer_from_objref(ctx, varg2);
             Value *gc_uses[2];
             int nroots = 0;
-            if ((gc_uses[nroots] = get_gc_root_for(arg1)))
+            // these roots may seem a bit overkill, but we want to make sure
+            // that a!=b implies (a,)!=(b,) even if a and b are unused and
+            // therefore could be freed and then the memory for a reused for b
+            if ((gc_uses[nroots] = get_gc_root_for(ctx, arg1)))
                 nroots++;
-            if ((gc_uses[nroots] = get_gc_root_for(arg2)))
+            if ((gc_uses[nroots] = get_gc_root_for(ctx, arg2)))
                 nroots++;
             OperandBundleDef OpBundle("jl_roots", makeArrayRef(gc_uses, nroots));
             auto answer = ctx.builder.CreateCall(prepare_call(memcmp_func), {
@@ -5863,16 +5866,9 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_
         }
         SmallVector<Value*, 0> vals;
         for (size_t i = 0; i < nargs; ++i) {
-            const jl_cgval_t &ai = argv[i];
-            if (ai.constant || ai.typ == jl_bottom_type)
-                continue;
-            if (ai.isboxed) {
-                vals.push_back(ai.Vboxed);
-            }
-            else if (jl_is_concrete_immutable(ai.typ) && !jl_is_pointerfree(ai.typ)) {
-                Type *at = julia_type_to_llvm(ctx, ai.typ);
-                vals.push_back(emit_unbox(ctx, at, ai, ai.typ));
-            }
+            Value *gc_root = get_gc_root_for(ctx, argv[i]);
+            if (gc_root)
+                vals.push_back(gc_root);
         }
         Value *token = vals.empty()
             ? (Value*)ConstantTokenNone::get(ctx.builder.getContext())

diff --git a/test/compiler/codegen.jl b/test/compiler/codegen.jl
@@ -581,13 +581,13 @@ end
     end
     @test occursin("llvm.julia.gc_preserve_begin", get_llvm(f3, Tuple{Bool}, true, false, false))
 
-    # unions of immutables (JuliaLang/julia#39501)
+    # PhiNode of unions of immutables (JuliaLang/julia#39501)
     function f2(cond)
-        val = cond ? 1 : 1f0
+        val = cond ? 1 : ""
         GC.@preserve val begin end
         return cond
     end
-    @test !occursin("llvm.julia.gc_preserve_begin", get_llvm(f2, Tuple{Bool}, true, false, false))
+    @test occursin("llvm.julia.gc_preserve_begin", get_llvm(f2, Tuple{Bool}, true, false, false))
     # make sure the fix for the above doesn't regress #34241
     function f4(cond)
         val = cond ? ([1],) : ([1f0],)