libgo patch committed: Stricter GC checking
diff mbox series

Message ID CAOyqgcVk7TUH6EruEDwUpPpyyZOMdGnrRe2ftX7a2MV1hDpGuQ@mail.gmail.com
State New
Headers show
Series
  • libgo patch committed: Stricter GC checking
Related show

Commit Message

Ian Lance Taylor Aug. 19, 2019, 7:09 p.m. UTC
This libgo patch by Cherry Zhang enables stricter GC checking in libgo.

With https://golang.org/cl/190599
(https://gcc.gnu.org/ml/gcc-patches/2019-08/msg01220.html), along with
what we do in greyobject, we ensure that we only mark allocated heap
objects.  As a result we can be more strict in GC:

- Enable "sweep increased allocation count" check, which checks that
the number of mark bits set are no more than the number of allocation
bits.

- Enable invalid pointer check on heap scan. We only trace allocated
heap objects, which should not contain invalid pointer.

This also makes the libgo runtime more convergent with the gc runtime.

Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu.  Committed
to mainline.

Ian

Patch
diff mbox series

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 274614)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@ 
-85857977230437f2b3dcbeea009efbb8b2789039
+b0ba5daa8216a0424b24f74466cedab0b986f3b4
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/go/runtime/mcentral.go
===================================================================
--- libgo/go/runtime/mcentral.go	(revision 274169)
+++ libgo/go/runtime/mcentral.go	(working copy)
@@ -56,15 +56,6 @@  retry:
 			c.empty.insertBack(s)
 			unlock(&c.lock)
 			s.sweep(true)
-
-			// With gccgo's conservative GC, the returned span may
-			// now be full. See the comments in mspan.sweep.
-			if uintptr(s.allocCount) == s.nelems {
-				s.freeindex = s.nelems
-				lock(&c.lock)
-				goto retry
-			}
-
 			goto havespan
 		}
 		if s.sweepgen == sg-1 {
Index: libgo/go/runtime/mgcsweep.go
===================================================================
--- libgo/go/runtime/mgcsweep.go	(revision 274169)
+++ libgo/go/runtime/mgcsweep.go	(working copy)
@@ -326,39 +326,16 @@  func (s *mspan) sweep(preserve bool) boo
 		freeToHeap = true
 	}
 	nfreed := s.allocCount - nalloc
-
-	// This check is not reliable with gccgo, because of
-	// conservative stack scanning. The test boils down to
-	// checking that no new bits have been set in gcmarkBits since
-	// the span was added to the sweep count. New bits are set by
-	// greyobject. Seeing a new bit means that a live pointer has
-	// appeared that was not found during the mark phase. That can
-	// not happen when pointers are followed strictly. However,
-	// with conservative checking, it is possible for a pointer
-	// that will never be used to appear live and to cause a mark
-	// to be added. That is unfortunate in that it causes this
-	// check to be inaccurate, and it will keep an object live
-	// unnecessarily, but provided the pointer is not really live
-	// it is not otherwise a problem. So we disable the test for gccgo.
-	nfreedSigned := int(nfreed)
 	if nalloc > s.allocCount {
-		if usestackmaps {
-			print("runtime: nelems=", s.nelems, " nalloc=", nalloc, " previous allocCount=", s.allocCount, " nfreed=", nfreed, "\n")
-			throw("sweep increased allocation count")
-		}
-
-		// For gccgo, adjust the freed count as a signed number.
-		nfreedSigned = int(s.allocCount) - int(nalloc)
-		if uintptr(nalloc) == s.nelems {
-			s.freeindex = s.nelems
-		}
+		print("runtime: nelems=", s.nelems, " nalloc=", nalloc, " previous allocCount=", s.allocCount, " nfreed=", nfreed, "\n")
+		throw("sweep increased allocation count")
 	}
 
 	s.allocCount = nalloc
 	wasempty := s.nextFreeIndex() == s.nelems
 	s.freeindex = 0 // reset allocation index to start of span.
 	if trace.enabled {
-		getg().m.p.ptr().traceReclaimed += uintptr(nfreedSigned) * s.elemsize
+		getg().m.p.ptr().traceReclaimed += uintptr(nfreed) * s.elemsize
 	}
 
 	// gcmarkBits becomes the allocBits.
@@ -374,7 +351,7 @@  func (s *mspan) sweep(preserve bool) boo
 	// But we need to set it before we make the span available for allocation
 	// (return it to heap or mcentral), because allocation code assumes that a
 	// span is already swept if available for allocation.
-	if freeToHeap || nfreedSigned <= 0 {
+	if freeToHeap || nfreed == 0 {
 		// The span must be in our exclusive ownership until we update sweepgen,
 		// check for potential races.
 		if s.state != mSpanInUse || s.sweepgen != sweepgen-1 {
@@ -387,11 +364,8 @@  func (s *mspan) sweep(preserve bool) boo
 		atomic.Store(&s.sweepgen, sweepgen)
 	}
 
-	if spc.sizeclass() != 0 {
-		c.local_nsmallfree[spc.sizeclass()] += uintptr(nfreedSigned)
-	}
-
-	if nfreedSigned > 0 && spc.sizeclass() != 0 {
+	if nfreed > 0 && spc.sizeclass() != 0 {
+		c.local_nsmallfree[spc.sizeclass()] += uintptr(nfreed)
 		res = mheap_.central[spc].mcentral.freeSpan(s, preserve, wasempty)
 		// mcentral.freeSpan updates sweepgen
 	} else if freeToHeap {
Index: libgo/go/runtime/runtime1.go
===================================================================
--- libgo/go/runtime/runtime1.go	(revision 274169)
+++ libgo/go/runtime/runtime1.go	(working copy)
@@ -352,19 +352,12 @@  func parsedebugvars() {
 	// defaults
 	debug.cgocheck = 1
 
-	// Unfortunately, because gccgo uses conservative stack scanning,
-	// we can not enable invalid pointer checking. It is possible for
-	// memory block M1 to point to M2, and for both to be dead.
-	// We release M2, causing the entire span to be released.
-	// Before we release M1, a stack pointer appears that point into it.
-	// This stack pointer is presumably dead, but causes M1 to be marked.
-	// We scan M1 and see the pointer to M2 on a released span.
-	// At that point, if debug.invalidptr is set, we crash.
-	// This is not a problem, assuming that M1 really is dead and
-	// the pointer we discovered to it will not be used.
-	if usestackmaps {
-		debug.invalidptr = 1
-	}
+	// Gccgo uses conservative stack scanning, so we cannot check
+	// invalid pointers on stack. But we can still enable invalid
+	// pointer check on heap scanning. When scanning the heap, we
+	// ensure that we only trace allocated heap objects, which should
+	// not contain invalid pointers.
+	debug.invalidptr = 1
 
 	for p := gogetenv("GODEBUG"); p != ""; {
 		field := ""