runtime: (theoretically) redundant package check in isAsyncSafePoint

[prattmic]

runtime.isAsyncSafePoint determines if it safe to preempt a PC.

We always consider the runtime unsafe since it contains a bunch of different unsafe areas (long term we'd like to be more precise).

isAsyncSafePoint has a check if the PC is from a runtime package. This check is a bit odd for a few reasons:

1. It doesn't actually contains all runtime packages (as defined by objabi), notably it is missing internal/runtime/.... This is what first brought this code to my attention.
2. The compiler marks "all points" in runtime functions as unsafe, so why do we need this check at all?

Given (2), I tried removing the check entirely, which caused spectacular crashes, so it is definitely necessary.

There are a few problems in the way of removing this check:

The check looks at the innermost function of a PC because a runtime function may be inlined into a non-runtime function (Originally added in https://go.dev/cl/211978 for runtime.UnlockOSThread). But the compiler only marks functions as unsafe when compiling a runtime package, which is not the case at the inlining site. The compiler should check if it inlined a runtime function (or simply not inline runtime functions).

Additionally, it's not strictly an issue, but looking at the inline tree to determine safety is rather fragile. After inlining and optimizations it wouldn't be difficult to land on a PC that is reported as part of the outer function but it still technically within an unsafe region. More robust would be for the entire outer function to get marked as unsafe (or, again, don't inline runtime functions).

Second, despite the compiling runtime check, runtime functions still end up with safe points. e.g., I got crashes after preemption of runtime.typePointers.next. Looking at "preempt safety" from @randall77's github.com/randall77/dumpgc:

function: runtime.typePointers.next
        entry: 41a460
        argbytes: 40
        frame size: 40
                [41a460:41a460]: 0
                [41a461:41a467]: 8
                [41a468:41a581]: 40
                [41a582:41a582]: 8
                [41a583:41a583]: 0
                [41a584:41a594]: 40
                [41a595:41a595]: 8
                [41a596:41a596]: 0
                [41a597:41a5a6]: 40
                [41a5a7:41a5a7]: 8
                [41a5a8:41a5a8]: 0
        preempt safety:
                [41a460:41a46c]: safe
                [41a46d:41a5a8]: unsafe
        entry args@sp+0x8: 0001
        live pointer maps, args@sp+0x30 locals@sp+0x18:
                [41a460:41a4da]: args:0001 locals:0
                [41a4db:41a5a8]: args:0000 locals:1

Despite being in the runtime (and //go:nosplit!), the function prologue is still marked as unsafe. I think this because the compiler's safety decision is in SSA, but the prologue is in cmd/internal/obj, which is fairly far removed. obj should probably apply the same policy as SSA.

cc @cherrymui @mknyszek @golang/runtime

[gabyhelp]

Related Code Changes

• cmd, runtime: eliminate runtime.no_pointers_stackmap
• cmd/compile, runtime: make bad safe-points throw
• cmd/compile: fix missing unsafe-points
• cmd/compile: fix unsafe-points with stack maps
• runtime: run safe-point function before entering _Psyscall
• cmd/internal/obj, runtime: preempt & restart some instruction sequences
• runtime: use innermost frame's func name for async preemption check
• cmd/internal/obj/arm64: mark unsafe points
• runtime/trace: avoid frame pointer unwinding for events during cgocallbackg

Related Discussions

• How does isAsyncSafePoint check preemptable？

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}