go: cmd/compile: map increment can give incorrect result

What version of Go are you using (`go version`)?

go version devel +187c3a6 Sun Jun 17 21:35:39 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

No. It’s on tip only.

What did you do?

Run this program:

package main

import (
	"fmt"
)

const (
	key1 = ""
	key2 = "x"
)

func main() {
	m := make(map[string]int)
	m[key1] = 99
	delete(m, key1)
	n1 := m[key2]
	m[key2]++
	n2 := m[key2]
	if n2 != n1+1 {
		fmt.Printf("BUG!!!!! %v; incremented %v to %v\n", key2, n1, n2)
	}
}

On tip, it prints BUG!!!!! x; incremented 0 to 100. It seems that the increment operator might be looking at the old value that has been deleted.

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 20 (19 by maintainers)

Most upvoted comments

side note for anyone else wondering about that: The newly introduced map bulk deletion (mapclear) still works as expected in this case.

package main

import (
        "fmt"
)

const (
        key1 = ""
        key2 = "x"
)

func main() {
        m := make(map[string]int)
        m[key1] = 99
        for k := range m {
                delete(m, k)
        }
        n1 := m[key2]
        m[key2]++
        n2 := m[key2]
        if n2 != n1+1 {
                fmt.Printf("BUG!!!!! %v; incremented %v to %v\n", key2, n1, n2)
        }
}

nightlyone on Jun 21, 2018

@vkuzmin-uber yes there is such an optimization in place and @josharian pointed out the issue where it was discussed and implemented. I have been curious whether that code path was affected too and quickly shared my results.

The test case which you added and named TestIncrementAfterBulkClearKeyStringValueInt looks good to me and I verified that there is only one needed, since we have only one mapclear.

Great job!

nightlyone on Jun 23, 2018

We need only change mapdelete to ensure that empty slots are zeroed. I think that might be acceptable. Or the wrapper @mdempsky suggested would work (mapassignop, which calls mapassign2, then zeroes the result slot if the 2nd return value was false).

randall77 on Jun 19, 2018

I think what should be ideally avoided in a solution for speeding up m[k] += 1 for go1.11 and then making it correct is that we regress in performance for normal map assign m[k] = 1 from go1.10 to go1.11. Approximating over google profiling data seems to suggest normal map assign is consuming more time overall so the trade off of speeding up one and making the other slower should be carefully investigated. But as noted writing to a small memory location that will be overwritten again anyway might not be much of a problem.

martisch on Jun 19, 2018