From 424785a0670b910a166b60968be59491350ebdce Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Thu, 24 Mar 2022 10:13:25 -0400
Subject: [PATCH] Add missing gc root in codegen (#44724)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In #44635, we observe that occasionally a call to
`view(::SubArray, ::Colon, ...)` dispatches to the
wrong function. The post-inlining IR is in relevant part:

```
│   │ %8   = (isa)(I, Tuple{Colon, UnitRange{Int64}, SubArray{Int64, 2, UnitRange{Int64}, Tuple{Matrix{Int64}}, false}})::Bool
└───│        goto #3 if not %8
2 ──│ %10  = π (I, Tuple{Colon, UnitRange{Int64}, SubArray{Int64, 2, UnitRange{Int64}, Tuple{Matrix{Int64}}, false}})
│   │ @ indices.jl:324 within `to_indices` @ multidimensional.jl:859
│   │┌ @ multidimensional.jl:864 within `uncolon`
│   ││┌ @ indices.jl:351 within `Slice` @ indices.jl:351
│   │││ %11  = %new(Base.Slice{Base.OneTo{Int64}}, %7)::Base.Slice{Base.OneTo{Int64}}
│   │└└
│   │┌ @ essentials.jl:251 within `tail`
│   ││ %12  = Core.getfield(%10, 2)::UnitRange{Int64}
│   ││ %13  = Core.getfield(%10, 3)::SubArray{Int64, 2, UnitRange{Int64}, Tuple{Matrix{Int64}}, false}
│   │└
│   │ @ indices.jl:324 within `to_indices`
└───│        goto #5
    │ @ indices.jl:324 within `to_indices` @ indices.jl:333
    │┌ @ tuple.jl:29 within `getindex`
3 ──││ %15  = Base.getfield(I, 1, true)::Function
│   │└
│   │        invoke Base.to_index(A::SubArray{Int64, 3, Array{Int64, 3}, Tuple{Vector{Int64}, Base.Slice{Base.OneTo{Int64}}, UnitRange{Int64}}, false}, %15::Function)::Union{}
```

Here we expect the `isa` at `%8` to always be [1]. However,
we seemingly observe the result that the branch is not taken
and we instead end up in the fallback `to_index`, which (correctly)
complains that the colon should have been dereferenced to
an index.

After some investigation of the relevant rr trace, what turns out
to happen here is that the va tuple we compute in codegen gets
garbage collected before the call to `emit_isa`, causing a use-after-free
read, which happens to make `emit_isa` think that the isa condition
is impossible, causing it to fold the branch away.

The fix is to simply add the relevant GC root. It's a bit unfortunate that this
wasn't caught by the GC verifier. It would have in principle been capable of doing
so, but it is currently disabled for C++ sources. It would be worth revisiting
this in the future to see if it can't be made to work.

Fixes #44635.

[1] The specialization heuristics decided to widen `Colon` to `Function`,
which doesn't make much sense here, but regardless, it shouldn't
crash.
---
 src/codegen.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/codegen.cpp b/src/codegen.cpp
index b7483d1e88504..ca51b8dcaf5d2 100644
--- a/src/codegen.cpp
+++ b/src/codegen.cpp
@@ -6346,7 +6346,8 @@ static std::pair<std::unique_ptr<Module>, jl_llvm_functions_t>
     // step 1. unpack AST and allocate codegen context for this function
     jl_llvm_functions_t declarations;
     jl_codectx_t ctx(ctxt, params);
-    JL_GC_PUSH2(&ctx.code, &ctx.roots);
+    jl_datatype_t *vatyp = NULL;
+    JL_GC_PUSH3(&ctx.code, &ctx.roots, &vatyp);
     ctx.code = src->code;
 
     std::map<int, BasicBlock*> labels;
@@ -6451,7 +6452,7 @@ static std::pair<std::unique_ptr<Module>, jl_llvm_functions_t>
     if (va && ctx.vaSlot != -1) {
         jl_varinfo_t &varinfo = ctx.slots[ctx.vaSlot];
         varinfo.isArgument = true;
-        jl_datatype_t *vatyp = specsig ? compute_va_type(lam, nreq) : (jl_tuple_type);
+        vatyp = specsig ? compute_va_type(lam, nreq) : (jl_tuple_type);
         varinfo.value = mark_julia_type(ctx, (Value*)NULL, false, vatyp);
     }