Thanks @rkojedzinszky @neolit123 @MikeSpreitzer @brandond @liggitt for help reporting & investigating

Is this still reproducible with a compiler built after CL 274026?

If not, it may be a duplicate of #42876.

Hrm, but that wouldn't explain a repro on 386.

I can reproduce on 386.
It does not reproduce with GOGC=off.

Looks like something is getting clobbered. If I print out the good and bad time structs in hex, I get

85432914 bff5fbf2 0013edd9 00000000 0868a6c0    // good
0a44bde4 0a44bd70 0f446329 00000000 0868a6c0   // bad

The first 2 words there are valid pointers to the heap. Not sure about the 3rd one.

I think this only happens on 32-bit because on 32-bit the time.Time structure is not SSA-able (and thus is always in memory and copied using duffcopy) but on 64-bit it is SSA-able (and thus can live in registers).

I belive the bad code is in (*sampleAndWaterMarkHistograms).innerSet.func1, although I'm not sure yet. My guess is that the return value from Now is somehow getting clobbered before we can copy it out of the outargs region.

Still looking.

FWIW, moving the time.Time struct into func1 seems to prevent it from getting clobbered. The in-tree fix using this approach is at brandond/kubernetes@1a4e511

I think I see the problem. We have (irrelevant code elided):

v52 (176) = InterCall <mem> [24] v49 v51  // Call to Now (sample_and_watermark.go, 3rd line of (*sampleAndWaterMarkHistograms).innerSet.func1)
v53 (?) = OffPtr <*time.Time> [4] v2 // address of return value of Now (in outargs section - very volatile!)
v54 (176) = Move <mem> {time.Time} [20] v15 v53 v52 // save it to destination (v15, the heap storage for the `when` variable)
v60 (177) = Move <mem> {time.Time} [20] v59 v15 v58 // copy from `when` to a temporary (v59, address of a stack slot)
v64 (177) = Move <mem> {time.Time} [20] v53 v63 v62 // copy from temporary (v63, == v59) to the outargs section
v65 (177) = StaticCall <mem> {"".(*sampleAndWaterMarkObserverGenerator).quantize} [32] v64 // call `quantize`

That's all fine. But then we run the opt pass, which has this rule:

(Move {t} [n] dst1 src1 move:(Move {t} [n] dst2 _ mem))
	&& move.Uses == 1
	&& isSamePtr(dst1, dst2) && disjoint(src1, n, dst2, n)
	&& clobber(move)
	=> (Move {t} [n] dst1 src1 mem)

Which basically says, if you're moving from a to b, and then b to c, might as well move directly from a to c.
But, that rule isn't valid if a is a volatile location. We need to get data out of a ASAP.
(Volatile here means can get clobbered by a call.)

After the opt pass, we have the same code, except v60 copies from v53 instead of v15. Which is bad, because that source is the volatile memory area. It's only a problem if there is an intervening call. In this case there is one, but only if the write barrier is enabled. So the corruption doesn't happen until the write barrier is turned on. (The write barrier replaces v54 with a bunch of code that may call runtime.typedmemmove.)

I think the right fix is to add an extra condition to the move optimization rule to disable it if the source is a volatile region.

I have a small repro that also works on 64 bit.
It's been broken since 1.12.
@gopherbot please open backport issues.

Backport issue(s) opened: #43574 (for 1.14), #43575 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Change https://golang.org/cl/282492 mentions this issue: cmd/compile: don't short-circuit copies whose source is volatile

Thanks @randall77 for the fast analysis & fix!