Add GC.at-preserve for some pointer calls, fix #529.

[fredrikekre]

#484 caused #529 (cc @chethega ) and these GC.@preserve fixes it.

[c42f]

This looks good to me. Is there a MWE of #529 we can add as a regression test?

[fredrikekre]

I could not find an MWE, it does not trigger in simple cases.

[andyferris]

Cool.

Any performance impacts?

[ronisbr]

Btw, I have been trying to find a MWE to reproduce what I am seeing in SatelliteToolbox without success. However, it is indeed related to the garbage collector.

[c42f]

I had a go trying to make an MWE for this, but the optimizer outsmarted me every time so far.

What we need is something like the following:

# Use side effects to encourage allocation of v
@noinline do_alloc(x) = (v = MVector((x,)); @show v; v)

function foo(x)
    a = do_alloc(x)
    # Due to the bug, `a` may be considered unused after this point (maybe?)
    # *** We attempt to garbage collect it:
    GC.gc()
    # Now allocate a new object of the same size - likely to reuse the memory for `a`.
    b = do_alloc(x)
    a[1] = 0 # Attempt to clobber memory in `b`
    return b
end

However, this doesn't trigger the bug. At least one reason is that the GC root placement pass is putting the roots for a and b together into the same GC frame so they will be preserved together:

julia> @code_native foo(1)
	.text
; Function foo {
; Location: REPL[3]:2
	pushq	%r15
	pushq	%r14
	pushq	%r12
	pushq	%rbx
	subq	$24, %rsp
	movq	%rdi, %r14
	vxorps	%xmm0, %xmm0, %xmm0
	vmovaps	%xmm0, (%rsp)
	movq	$0, 16(%rsp)
	movq	%fs:0, %r15
	movq	$2, (%rsp)   ; * $2  =>  two roots next to each other
	movq	-10920(%r15), %rax
	movq	%rax, 8(%rsp)
	movq	%rsp, %rax
	movq	%rax, -10920(%r15) ; * Push GC frame
	movabsq	$julia_do_alloc_35139, %r12
	callq	*%r12
	movq	%rax, %rbx
	movq	%rbx, 16(%rsp)
...

To trigger the bug, we'd need to trick the GC root placement pass into placing these roots separately.

[fredrikekre]

Any performance impacts?

Nope, same code generated. While I appreciate that it would be nice with a test I think it would be good to merge this as is for now, since it fix a pretty critical bug.

[c42f]

Agreed, please go ahead and merge it, the fix looks good to me.

Having tried as far as the above, I think it's quite fair to say the optimizer has defeated me for now :-)

[andyferris]

Thanks, @fredrikekre

[andyferris]

A tag would be nice for this one, I think.

[andyferris]

I just realized... GC preservation should let us do the non-isbits types safely as pointers, like we used to. Right?

[ronisbr]

A tag would be nice for this one, I think.

Hi guys! Thanks for the quick fix! I would really appreciate if a new tag is released, since SatelliteToolbox will not pass the tests withou it.

[fredrikekre]

JuliaLang/METADATA.jl#19108

[c42f]

I just realized... GC preservation should let us do the non-isbits types safely as pointers, like we used to. Right?

No I don't think so. Reading back over #27 reminds me that you need a GC write barrier as well as the pointer update... at the very least (jl_gc_wb in the C code). There's also the issue of inlining the tuple vs not inlining.

[ronisbr]

Just for the records, the tests are now passing without any problems! Thanks everyone :)

I tried really hard to reproduce it. I even copied all the related functions and executed them outside SatelliteToolbox.jl and I was not able to reproduce it. Furthermore, I think it will be an overkill to add the entire NRLMSISE-00 model into a test to StaticArrays to get this particular bug.

[c42f]

Yeah, it might not be as simple as I was imagining above, it could also depend on particular rearrangements which the LLVM optimizer is allowed to do if you don't explicitly preserve the root. I'm not sure.