Kindly paging @randall77 @cherrymui as it appeared after
CL https://go-review.googlesource.com/c/134156 aka commit 9a8372f#diff-4e8331dc0915d867dc05240b929434de

Before lower,

v7 (7) = VarDef <mem> {.autotmp_2} v1
v8 (7) = LocalAddr <**float64> {.autotmp_2} v2 v7
v9 (7) = Store <mem> {*float64} v8 v6 v7
v10 (7) = LocalAddr <**float64> {.autotmp_2} v2 v9
v12 (+7) = EqPtr <bool> v5 v10

which looks correct.

After lower,

v7 (7) = VarDef <mem> {.autotmp_2} v1
v8 (7) = LEAQ <**float64> {.autotmp_2} v2 <DEAD>
v9 (7) = MOVQstoreconst <mem> {.autotmp_2} [val=0,off=0] v2 v7
v10 (7) = LEAQ <**float64> {.autotmp_2} v2
v11 (+7) = CMPQload <flags> {"".fp} v3 v10 v1

There is no more memory relationship between the LEAQ and MOVQstoreconst. Note that CMPQload has mem arg v1 (InitMem) instead of v7.

Later, schedule puts the LEAQ and the CMPQload (!) before the VarDef and the store,

v8 (7) = LEAQ <**float64> {.autotmp_2} v2
v11 (+7) = CMPQload <flags> {"".fp} v3 v8 v1
v7 (7) = VarDef <mem> {.autotmp_2} v1
v9 (7) = MOVQstoreconst <mem> {.autotmp_2} [val=0,off=0] v2 v7

which is bad.

Maybe the problem is to choose the correct memory arg at CMPQload generation?

Yuck. Even if the CMPQload has the right memory op, the LEAQ might still float up before the VARDEF.
Another possibility is to stop treating taking the address of something as a read.
(plive.go:312, remove SymAddr from the masking).
It fixes this immediate bug, but I'm not sure it is safe. Presumably all the reads after the address op depend on the memory state generated by the VARDEF, so maybe that's ok...
This would mean that we could remove the memory argument from LocalAddr also.

It might be safe since stack objects went in. plive now really only handles direct accesses, and all accesses through the pointers are indirect.

Another possibility is to stop treating taking the address of something as a read.

Yeah, with stack objects, this might be safe.

I think my idea doesn't quite work either.
If the LEAQ moves up past a safepoint, then it points to junk. We can't let the GC see the results of the LEAQ until the stack object being referenced is at least zeroed.

Maybe we need an LEAQ with memory argument? That would definitely be a verbose CL, I'd like to avoid it if anyone can think of a better way.

@andybons A compiler crash on a valid 8 lines program, introduced during this cycle, is not a release-blocker?

LEAQ is rematerializeable, and I think this is true for similar instructions on all other architectures. It might be fine if we guarantee to always generate LEAQ as late as we can (and we never spill it). (In this particular example, the CMPQload makes the LEAQ being generated too early.)

I think if the LEAQ's result is stored into a stack object, the memory should already be initialized, as the store instruction has the correct memory dependency. I'm not sure how we perform this check in the compiler though.

@ALTree When triaging, we decided that while the code is valid, it’s likely not common (at least in the form presented). I will leave it to @cherrymui and @randall77 to decide its severity.

I agree that the repro isn't likely code. The underlying problem, though, might be more common. Hard to be sure. I'm going to mark it as a release blocker for now.

Thanks, Keith.

I looked at this some more. My patch to just treat LEAQ as not reading doesn't work because of this case:

var x T = ...
p := &x
f(p)
runtime.GC()
q := &x
f(q)

At the runtime.GC, we need x to be live. Otherwise, x is not scanned and its referents not retained. Later, after we do q := &x, x is live again but it still contains its old contents, including its old pointer. That pointer is no longer valid - it may point to a freed span, for example.

This live-dead-live transition is bad. Treating &x as reading x prevents this situation.

Change https://golang.org/cl/153641 mentions this issue: cmd/compile: don't combine load+op if the op has SymAddr arguments