Skip to content

Commit

Permalink
Add missing gc root in codegen (#44724)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Keno authored Mar 24, 2022
1 parent 7189c81 commit 424785a
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit 424785a

Please sign in to comment.