Julia GC: fix stack marking #3199
Conversation
This commit fixes two separate issues. 1. The processor stack was not being marked correctly, as Julia made wrong assumptions about the end of the processor stack; we now use the value of `stack_bottom` that is being supplied to InitBags(). 2. CHANGED_BAG() was not being called on CurrLVars when a collection was initiated, unlike for the GASMAN implementation, where this happens in VarsBeforeCollectBags().
| // is only called when the current lvars bag is being changed. Thus, | ||
| // we have to add a write barrier at the start of the GC, too. | ||
| if (STATE(CurrLVars)) | ||
| CHANGED_BAG(STATE(CurrLVars)); |
There was a problem hiding this comment.
It's OK to have this in here for now, to get a quick fix; but on the long run, I prefer to fix this properly, by providing a generic "pre-gc hook" in both GASMAN (which already has one) and Julia GC, and then setting that to VarsBeforeCollectBags. Also, this "optimization" for lvars needs to be documented in a better place. But neither of these are change request for this PR; me or somebody else can take care of it in a future PR.
There was a problem hiding this comment.
I agree. My goal here was to get the fix out ASAP. As the code is identical to VarsBeforeCollectBags(), this should be unified.
| @@ -527,6 +528,9 @@ void GapRootScanner(int full) | |||
| // current_task != root_task. | |||
| char * stackend = (char *)jl_task_stack_buffer(task, &size, &tid); | |||
| stackend += size; | |||
| if (JuliaTLS->tid == 0 && JuliaTLS->root_task == task) { | |||
| stackend = (char *)GapStackBottom; | |||
There was a problem hiding this comment.
Hmm, but note that this only will work right if GAP is the main process, loading Julia later. If, however, one is starting Julia and from there loading GAP, then GapStackBottom may not be at the base of the stack frame.
This is closely related to (part of) the discussion on issue #3089 ; see also PR #3096
There was a problem hiding this comment.
Yes, and GapTaskScanner() may also need an update. That said, it should properly be handled so that Julia gets to know the proper end of the stack for task switching.
There was a problem hiding this comment.
ISTM there needs to be a SetStackBottomBags (as in #3096) for Julia GC as well.
There was a problem hiding this comment.
Possible, but this code is very much in flux, so it doesn't really matter at this stage.
rbehrends
force-pushed
the
julia-gc-stack-scanning
branch
from
e9ba8c7 to
45905ca
Compare
Codecov Report
@@ Coverage Diff @@
## master #3199 +/- ##
==========================================
+ Coverage 84.76% 84.76% +<.01%
==========================================
Files 688 688
Lines 335862 335866 +4
==========================================
+ Hits 284685 284689 +4
Misses 51177 51177
|
fingolfin
added
kind: bug: crash
|
I can confirm that this fixes #3114 for me; I hope @ThomasBreuer can confirm that as well (and then check which of the other crashes he was getting are also fixed; hopefully all.... ;-) |
There was a problem hiding this comment.
Yes, the problem from #3114 disappears in the changed version.
Thanks for the fix.
|
Backported to stable-4.10 in af3bb41 |
olexandr-konovalov
added
the
release notes: added
fingolfin
added
the
kind: bug
This pull request fixes two separate bugs, related to #3114.
made wrong assumptions about the end of the processor stack;
we now use the value of
stack_bottomthat is being suppliedto
InitBags().CHANGED_BAG()was not being called onCurrLVarswhen a collectionwas initiated, unlike for the GASMAN implementation, where this
happens in
VarsBeforeCollectBags().Fixes #3114