Also, CC – @mknyszek

I'll take a look.

This looks like #43824 . The most recent ones are all failing in markroot with a nil gp, which is the same as #43824.

I poked a little bit more and there's one thing I don't understand: why is the failing address exactly zero? Shouldn't the address be slightly offset for the atomicstatus field? Is there a nil pointer check here, before the load?

Also, the code around this is kind of fishy:

    default:
        // the rest is scanning goroutine stacks
        workCounter = &gcController.stackScanWork
        var gp *g
        if work.baseStacks <= i && i < work.baseEnd {
            // N.B. Atomic read of allglen in gcMarkRootPrepare
            // acts as a barrier to ensure that allgs must be large
            // enough to contain all relevant Gs.
            gp = allgs[i-work.baseStacks]
        } else {
            throw("markroot: bad index")
        }

        // remember when we've first observed the G blocked
        // needed only to output in traceback
        status := readgstatus(gp) // We are not in a scan state
        if (status == _Gwaiting || status == _Gsyscall) && gp.waitsince == 0 {
            gp.waitsince = work.tstart
        }

Note the unsynchronized access to allg and the N.B. above it. A lot of the recent failures appear to be on weak memory model architectures, like MIPS and arm64. The amd64 ones are also weird environments, like an Android emulator (Android amd64 is weird enough on its own). Maybe that's how a nil pointer is slipping through?

It's unclear to me whether this is a beta blocker, though. As @cherrymui pointed out, it seems old.

CC @aclements @prattmic

why is the failing address exactly zero? Shouldn't the address be slightly offset for the atomicstatus field? Is there a nil pointer check here, before the load?

I looked into this a while ago. Yes, there is a nil check. At least on some architectures, atomic load instruction is not marked "fault on nil" in the compiler, so the compiler does not eliminate the nil check. We probably want to fix that (but that won't fix the issue).

I can definitely see this failing on a weak memory machine. I don't think it even requires subtlety around STW.

1. We start a GC. gcMarkRootPrepare snapshots allglen atomically. We restart the world.
2. Thread 1 does allgadd, which appends to allgs. Suppose this requires a grow, so thread 1 allocates a new all-nil slice, copies the old slice into the new slice (write A), and assigns the new slice to allgs (write B).
3. Thread 2 concurrently enters markroot and reads from allgs. It reads the slice base (read A) and then the n'th value in the slice (read B).

On a weak memory machine, thread 2 could observe write B before write A (or, equivalently, the reads could be reordered), leading to thread 2 reading a nil from allgs.

(Edited to add: Actually, I don't think the reads can be reordered because they're dependent, but the writes could be.)

I've got a couple ideas for how to fix this. Probably the most straightforward is to save a copy of allgs during gcMarkRootPrepare and use that in markroot.

Does it work if we use atomicAllGIndex ? allgptr is written atomically so if it points to the new memory it should observe the writes to the array?

Probably, though then we also have to call atomicAllG either on every markroot or save that somewhere. (In general I really don't like the atomicAllG API... I might be happier with that if atomicAllG just returned a slice.)

Change https://golang.org/cl/368134 mentions this issue: runtime: fix racy allgs access on weak memory architectures

2022-09-05T08:28:34-83b083e-bd5595d/linux-mips-rtrk

This looks like it may be related? It's a nil pointer dereference in readgstatus from markroot.

It looks like cmd/dist crashed, which is built from the bootstrap toolchain, which is go1.17.13, which doesn't seem to include the fix above.

Given that it affects the bootstrap toolchain, should the fix be backported to release-branch.go1.17? (Or would we rather live with the crashes until the bootstrap version is raised again?)

I think we should backport the fix.

Normally we just add a workaround to tip's toolchain to avoid bootstrap bugs, but I suspect there isn't a feasible workaround in this case.

Go 1.17 is no longer supported. Not sure how we're going to backport. I guess we could cherry-pick to release-branch.go1.17. We still need to manually update the bootstrap toolchains on the builders, right?

We can release another 1.17.x even though it is not supported. Didn't we do that for 1.4.x? Or maybe I am misremembering.

We never did an updated 1.4.x release. We only backported things to the release branch and posted new src tar files.

That said, we're in a brave new world and we can chart whatever course we think best. Since this seems to be entirely the relaxed memory model architectures, one option is to bump all those builders to Go 1.18 or Go 1.19. We'll still have plenty on Go 1.17 to keep us honest (a few builders are already bumped for other reasons, like the port being new since Go 1.17).

If people generally agree with bumping the bootstrap toolchain forward on those systems, I'm happy to do the work or advise someone who wants to learn the process.

Sounds like we have 3 paths forward here:

1. Do nothing, it's not that common and we may want to update the bootstrap toolchain in the next release anyway.
2. Backport the fix to the bootstrap compiler.
3. Move just the relaxed memory architecture bootstrap toolchains up a version or two.

I think this just needs a decision and someone to do the work (or no one to do no work).

I talked with Austin briefly about this. I think (2) is probably wrong; it's strictly more work than (3).
I don't think we should do (1) and leave lingering flakes (assuming flakes are still happening), so I'm inclined to (3).

Re "assuming flakes are still happening", taking a closer look at the history, it looks like there were many failures during run.bash originally, and that got fixed and released.

Now we have a single flake where the build faulted in readgstatus in a similar way, during bootstrap, which suggests maybe older toolchains are problematic. But it's just one flake. It might be best to wait for a couple more to really be sure that this is the problem. I did a greplogs for runtime.readgstatus, and I found this fault too, which is from a commit back in May (not too old) and was during run.bash, so not bootstrap-dependent. It's interesting because it is pprof faulting on readgstatus, while the world is ostensibly stopped.

2022-05-09T20:57:58-3f43096/openbsd-arm-jsing

Maybe there is something lurking here, or maybe not. Let's keep accumulating failures mentioning runtime.readgstatus and see what we find.

Since we're waiting, I put this in WaitingForInfo. At the very least when it closes we'll have a signal that we might want to look back here. I'll keep it assigned to myself to make sure I get notified.

Moved to #55304 to not confuse the old and new bug. Closing this.