all: add internal/asan and internal/msan packages

[mauri870]

There are a couple issues where having asan/msan enabled throw off tests for memory allocations, see #64257 and #64256.

Most tests already skip these memory allocation assertions when they detect that race is enabled, but such functionality is not exposed for asan and msan.

Here is one example of such test:

if n := testing.AllocsPerRun(100, func() { Grow(s2, cap(s2)-len(s2)+1) }); n != 1 {
    errorf := t.Errorf
    if race.Enabled || testenv.OptimizationOff() {
        errorf = t.Logf // this allocates multiple times with race or msan enabled
    }
    errorf("Grow should allocate once when given insufficient capacity; allocated %v times", n)
}

One alternative is using the asan and msan build tags to skip certain tests, but that does not work in the middle of a test, only for entire files.

There is runtime.msanenabled, runtime.asanenabled, syscall.asanenabled we could use, but they are private to their respective packages.

It is worth noting that syscall and runtime duplicate some code related to asan:

• syscall has asanenabled, asanRead, asanWrite that basically call into the runtime.
• runtime has asanenabled as well

I propose we create internal/asan and internal/msan packages akin to internal/race. Then syscall and runtime would call internal/asan.Enabled and syscall could use ASanRead, ASanWrite as well, without having this logic duplicated.

The tests can also use internal/asan.Enabled to check if they should skip tests that allocate memory, which most tests already do currently by checking race.Enabled. We can also use these inside testenv.SkipIfOptimizationOff/testenv.OptimizationOff to make this process more seamless.

Here is the proposed apis for these new internal packages:

internal/asan

//go:build asan

package asan

import (
	"unsafe"
)

const Enabled = true

func Read(addr unsafe.Pointer, len int)
func Write(addr unsafe.Pointer, len int)

The !asan version would have stubs for these functions.

internal/msan

The only place that uses msan currently is the runtime, but having access to a msan.Enabled would be useful in tests as well and would make a good trio with internal/asan and internal/race. Other packages might be able to use the exported functions more easily as well.

The proposed api for internal/msan would then be:

//go:build msan

package msan

import (
	"unsafe"
)

const Enabled = true

func Read(addr unsafe.Pointer, sz uintptr)
func Write(addr unsafe.Pointer, sz uintptr)
func Malloc(addr unsafe.Pointer, sz uintptr)
func Free(addr unsafe.Pointer, sz uintptr)
func Move(dst, src unsafe.Pointer, sz uintptr)

The !msan version would have stubs for these functions.

[mauri870]

cc @golang/runtime

[cherrymui]

We would need to expose new runtime apis for msan, which are currently private. That is why I decided to keep the Proposal status for this issue:
// runtime
func MSanRead(addr unsafe.Pointer, sz uintptr)
func MSanWrite(addr unsafe.Pointer, sz uintptr)
func MSanMalloc(addr unsafe.Pointer, sz uintptr)
func MSanFree(addr unsafe.Pointer, sz uintptr)
func MSanMove(dst, src unsafe.Pointer, sz uintptr)

With internal/msan, do we actually need to export them in the runtime? We can arrange internal/msan to call the assembly functions (e.g. runtime.msanmalloc) directly, and other packages just use internal/msan.

[mauri870]

@cherrymui That works for me, I was trying to structure internal/msan the same way internal/asan is implemented, but it makes sense to call the assembly directly.

[mauri870]

Taking this out of the proposal process as we can proceed without adding new runtime APIs.

[gopherbot]

Change https://go.dev/cl/548695 mentions this issue: internal/asan: add new package

[gopherbot]

Change https://go.dev/cl/548676 mentions this issue: internal/msan: add new package

[mauri870]

With the tree now reopened, I would like to confirm whether the addition of internal/asan and internal/msan is acceptable to the runtime team, as the NeedsDecision label is still associated with this issue. I have prepared https://go.dev/cl/548676 and https://go.dev/cl/548695 in case we decide to proceed with this. Thanks.

[aclements]

This all looks reasonable to me. I'd like to see a follow-up CL that uses these new packages to clean up some of the rest of the tree. Thanks!

[mauri870]

This all looks reasonable to me. I'd like to see a follow-up CL that uses these new packages to clean up some of the rest of the tree. Thanks!

I'm looking forward to working on the cleanup and fixing stdlib tests that currently fail with asan/msan.

Is it okay if I also submit a CL to x/devapp/owners adding these new packages? The runtime team would be the primary maintainer, and I can take on the secondary maintainer role if that is acceptable for the team.

[cherrymui]

I'm looking forward to working on the cleanup and fixing stdlib tests that currently fail with asan/msan.

Thanks for working on this. I think this can be two steps. Refactoring first, then adding the notations to tests. This is probably cleaner and clearer.

[gopherbot]

Change https://go.dev/cl/566735 mentions this issue: devapp/owners: add internal asan and msan packages

[gopherbot]

Change https://go.dev/cl/566755 mentions this issue: syscall: use internal/asan and internal/msan

[gopherbot]

Change https://go.dev/cl/572755 mentions this issue: internal/asan: match runtime.asan{read,write} len parameter type