Well, this patch makes the test pass but causes a slew of problems on real programs:

--- a/src/cmd/compile/internal/gc/ssa.go
+++ b/src/cmd/compile/internal/gc/ssa.go
@@ -600,7 +600,13 @@ func (s *state) instrument(t *types.Type, addr *ssa.Value, wr bool) {
        var fn *obj.LSym
        needWidth := false
 
-       if flag_msan {
+       if flag_msan && t.Etype == TSTRUCT {
+               for _, f := range t.FieldSlice() {
+                       addr = s.newValue1I(ssa.OpOffPtr, types.NewPtr(t), f.Offset, addr)
+                       s.instrument(f.Type, addr, wr)
+               }
+               return
+       } else if flag_msan {
                fn = msanread
                if wr {
                        fn = msanwrite

Hmm.

I assume this also happens with go1.10 (so it's not a recent regression)?

Yep. Just verified that it happens with go1.10.3. Given that this case isn't tested by any of the sample programs in misc/cgo/testsanitizers I wouldn't be surprised if it's been broken since msan support was introduced.

Oh boy. This doesn't seem particularly easy to fix. LLVM's msan transformation (https://github.com/llvm-mirror/llvm/blob/6fed3d8070003bd65e0af3b70aaa04f7ca12260e/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L1431) is more nuanced than gc's. Specifically, LLVM doesn't consider a load or a store of uninitialized memory to be problematic. It's what you do afterwards that counts. Your program won't blow up until e.g. you perform arithmetic using uninitialized memory or index into an array using uninitialized memory.

Perhaps we could modify gc to only call msanread/msanwrite for non-padding bytes when copying struct types.

Sorry, I didn't even know the compiler added padding fields at all. In widstruct in cmd/compile/internal/gc/align.go it just sets offsets based on alignment, implicitly adding padding.

Oh! Perhaps it's cmd/cgo that's to blame. Thanks. I'll keep digging.

Ah, yes, cmd/cgo does add padding fields to the Go version of C structs in (*typeconv).pad in cmd/cgo/gcc.go.

Ok, great. Excluding the blank identifiers that cmd/cgo writes from msan instrumentation lets Cockroach get even further in boot.

In other news, I've stumbled across an edge case that I'm not sure what to make of. Consider a program that uses an ostensibly-initialized C struct as a Go map key:

// msan.go

package main

/*
#include <stdlib.h>

struct s {
 	int i;
	char c;
};

struct s* mks(void) {
	struct s* s = malloc(sizeof(struct s));
	s->i = 0xdeadbeef;
	s->c = 'n';
	return s;
}
*/
import "C"

func main() {
	m := map[C.struct_s]struct{}{}
	m[*C.mks()] = struct{}{}
}

Go will read the uninitialized padding bytes to hash the struct for the map key. Yikes! Though perhaps a valid answer here is "don't do that."

I’m AFK but I believe Syms have an IsBlank method.

Go will read the uninitialized padding bytes to hash the struct for the map key.

The autogenerated hash and eq functions should ignore blank fields in structs. If they don’t, it’s a bug.

The autogenerated hash and eq functions should ignore blank fields in structs. If they don’t, it’s a bug.

Ah. Then perhaps mapassign and friends are simply reporting too coarsely to msan. Maybe here:

I’m AFK but I believe Syms have an IsBlank method.

Yep, thanks. I've included the patch I've been experimenting with below. (I'm sure there's somewhere better to put this code.)

diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go
index ff2b93d..234954d 100644
--- a/src/cmd/compile/internal/gc/ssa.go
+++ b/src/cmd/compile/internal/gc/ssa.go
@@ -600,6 +600,16 @@ func (s *state) instrument(t *types.Type, addr *ssa.Value, wr bool) {
        var fn *obj.LSym
        needWidth := false
 
+       if flag_msan && t.IsStruct() {
+               for _, f := range t.FieldSlice() {
+                       if !f.Sym.IsBlank() {
+                               faddr := s.newValue1I(ssa.OpOffPtr, f.Type.PtrTo(), f.Offset, addr)
+                               s.instrument(f.Type, faddr, wr)
+                       }
+               }
+               return
+       }
+
        if flag_msan {
                fn = msanread
                if wr {

Then perhaps mapassign and friends are simply reporting too coarsely to msan

Looks like it. Perhaps there should be an msanreadtype that takes in type information and uses it to avoid reading blank fields.

Also, not that it matters for this conversation, but it appears to me (on my phone) that the msanread call there could be moved inside the “if map is nil” return path. Might improve perf.

The ideal way to share a patch is gerrit. Or a PR (which gets imported to gerrit).

Looks like it. Perhaps there should be an msanreadtype that takes in type information and uses it to avoid reading blank fields.

Yep, definitely an option. Or the calls to msanread could be moved closer to the actual memory reads in the hash functions.

I believe the msanread call is there only to handle the case in which we don’t call the hash function, which should have its own instrumentation.