runtime: Reenable "pointer-to-heap-doesn't-point-to-object" check. #9880
Labels
Milestone
Comments
mikioh
changed the title
|
CL https://golang.org/cl/10818 mentions this issue. |
|
CL https://golang.org/cl/11502 mentions this issue. |
aclements
added a commit
that referenced
this issue
Jun 29, 2015
Memory for stacks is manually managed by the runtime and, currently (with one exception) we free stack spans immediately when the last stack on a span is freed. However, the garbage collector assumes that spans can never transition from non-free to free during scan or mark. This disagreement makes it possible for the garbage collector to mark uninitialized objects and is blocking us from re-enabling the bad pointer test in the garbage collector (issue #9880). For example, the following sequence will result in marking an uninitialized object: 1. scanobject loads a pointer slot out of the object it's scanning. This happens to be one of the special pointers from the heap into a stack. Call the pointer p and suppose it points into X's stack. 2. X, running on another thread, grows its stack and frees its old stack. 3. The old stack happens to be large or was the last stack in its span, so X frees this span, setting it to state _MSpanFree. 4. The span gets reused as a heap span. 5. scanobject calls heapBitsForObject, which loads the span containing p, which is now in state _MSpanInUse, but doesn't necessarily have an object at p. The not-object at p gets marked, and at this point all sorts of things can go wrong. We already have a partial solution to this. When shrinking a stack, we put the old stack on a queue to be freed at the end of garbage collection. This was done to address exactly this problem, but wasn't a complete solution. This commit generalizes this solution to both shrinking and growing stacks. For stacks that fit in the stack pool, we simply don't free the span, even if its reference count reaches zero. It's fine to reuse the span for other stacks, and this enables that. At the end of GC, we sweep for cached stack spans with a zero reference count and free them. For larger stacks, we simply queue the stack span to be freed at the end of GC. Ideally, we would reuse these large stack spans the way we can small stack spans, but that's a more invasive change that will have to wait until after the freeze. Fixes #11267. Change-Id: Ib7f2c5da4845cc0268e8dc098b08465116972a71 Reviewed-on: https://go-review.googlesource.com/11502 Reviewed-by: Russ Cox <[email protected]>
rsc
added a commit
that referenced
this issue
Jul 15, 2015
For #9880. Let's see what breaks. Change-Id: Ic8b99a604e60177a448af5f7173595feed607875 Reviewed-on: https://go-review.googlesource.com/10818 Reviewed-by: Austin Clements <[email protected]> Run-TryBot: Austin Clements <[email protected]>
rsc
added a commit
that referenced
this issue
Jul 16, 2015
Broke arm64. Update #9880. This reverts commit 38d9b2a. Change-Id: I35fa21005af2183828a9d8b195ebcfbe45ec5138 Reviewed-on: https://go-review.googlesource.com/12247 Reviewed-by: Russ Cox <[email protected]>
|
CL https://golang.org/cl/12847 mentions this issue. |
|
CL https://golang.org/cl/12848 mentions this issue. |
|
CL https://golang.org/cl/12846 mentions this issue. |
|
CL https://golang.org/cl/12845 mentions this issue. |
|
CL https://golang.org/cl/12849 mentions this issue. |
rsc
added a commit
that referenced
this issue
Jul 29, 2015
If the compiler doesn't do it, cmd/internal/obj/arm64 will, and that will break the zeroing of ambiguously live values done in zerorange, which in turn produces uninitialized pointer cells that the GC trips over. For #9880. Change-Id: Ice97c30bc8b36d06b7b88d778d87fab8e1827fdc Reviewed-on: https://go-review.googlesource.com/12847 Reviewed-by: Austin Clements <[email protected]>
rsc
added a commit
that referenced
this issue
Jul 29, 2015
arm64 requires either no stack frame or a frame with a size that is 8 mod 16 (adding the saved LR will make it 16-aligned). The cmd/internal/obj/arm64 has been silently aligning frames, but it led to a terrible bug when the compiler and obj disagreed on the frame size, and it's just generally confusing, so we're going to make misaligned frames an error instead of something that is silently changed. This CL prepares by updating assembly files. Note that the changes in this CL are already being done silently by cmd/internal/obj/arm64, so there is no semantic effect here, just a clarity effect. For #9880. Change-Id: Ibd6928dc5fdcd896c2bacd0291bf26b364591e28 Reviewed-on: https://go-review.googlesource.com/12845 Reviewed-by: Austin Clements <[email protected]>
rsc
added a commit
that referenced
this issue
Jul 29, 2015
The nosplit stack overflow checks were confused about morestack. The comment about not having correct SP information at the call to morestack was true, but that was a real bug, not something to work around. I fixed that problem in CL 12144. With that fixed, no need to special-case morestack in the way done here. This cleanup and simplification of the code was the first step to fixing a bug that happened when I started working on the arm64 frame size adjustments, but the cleanup was sufficient to make the bug go away. For #9880. Change-Id: I16b69a5c16b6b8cb4090295d3029c42d606e3b9b Reviewed-on: https://go-review.googlesource.com/12846 Reviewed-by: Austin Clements <[email protected]>
rsc
added a commit
that referenced
this issue
Jul 29, 2015
…frames The layout code has to date insisted on stack frames that are 16-aligned including the saved LR, and it ensured this by growing the frame itself. This breaks code that refers to values near the top of the frame by positive offset from SP, and in general it's too magical: if you see TEXT xxx, $N, you expect that the frame size is actually N, not sometimes N and sometimes N+8. This led to a serious bug in the compiler where ambiguously live values were not being zeroed correctly, which in turn triggered an assertion in the GC about finding only valid pointers. The compiler has been fixed to always emit aligned frames, and the hand-written assembly has also been fixed. Now that everything is aligned, make unaligned an error instead of something to "fix" silently. For #9880. Change-Id: I05f01a9df174d64b37fa19b36a6b6c5f18d5ba2d Reviewed-on: https://go-review.googlesource.com/12848 Reviewed-by: Austin Clements <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
We used to have a check for pointers that point into the heap but don't point to an object. This check is useful for debugging live variable info, global ptr maps, etc. It is currently turned off. We should re-enable it for 1.5. It is currently broken at least because of holes in 32-bit heaps #9872 . There are probably other reasons as well.
The check is currently disabled in runtime/mbitmap.go:heapBitsForObject, line 181.
The text was updated successfully, but these errors were encountered: