cmd/compile: order mishandles "defer delete(m, k)" when k is a "slow" map key type

[mdempsky]

https://play.golang.org/p/2JNMRctSmRr

The above program prints "3\n2\n", but it should print "3\n0\n". (And it does if K is changed to a "fast" map key type like uint32.)

The problem is that, for "slow" map key types, order.go rewrites

defer delete(m, k)

into

ktmp := k
defer mapdelete(m, &ktmp)

but we never mark ktmp as escaping (because escape analysis happens earlier). So in the playground example above, this loop:

for _, k := range ks {
	defer delete(m, k)
}

is effectively rewritten into:

    var ktmp K
    for _, k := range ks {
            ktmp = k
            defer delete(m, &ktmp)
    }

So when the defers finally execute at function return, they all try to delete the last k value.

A similar issue affects go delete(m, k), but there's no way to use this construct in legitimate code.

[josharian]

I don’t remember where else this was discussed, but ideally we would do an early rewrite of defer builtin(x, y) and (go builtin(x, y)) to defer func() { builtin(x, y) } to eliminate this entire class of bugs. And also allow simplifying poorly-tested chunks of the compiler.

[mdempsky]

@josharian Yeah, we do that already for print and println. I expect the same can be done easily for delete.

It seems a little unfortunate to generate wrapper functions if we can avoid it (e.g., close and panic don't need them, and probably never will), though maybe simplicity/correctness outweighs the small space saving potential.

We also can't do that for recover, because defer recover() and defer func() { recover() } have different semantics: https://play.golang.org/p/vn9mFDCAp3o

[josharian]

I expect the same can be done easily for delete.

And copy:

go/src/cmd/compile/internal/gc/walk.go

Line 281 in b854339

case OCOPY:

maybe simplicity/correctness outweighs the small space saving potential.

I think so. And bug-free-ness; these keep cropping up.

We also can't do that for recover

True. And almost not panic, because it adds a frame to the stack.

Hmmm. This probably means that we need to pass the fully evaluated arguments to print/printlnt/delete/copy, in order to make sure any panics occur in the appropriate frame.

...and delete can actually panic (attempting to delete an interface key whose dynamic type doesn't support hash/eq), which means that the delete might actually not be able to be shoved into a hidden closure, on pain of having a bad stack trace. Or maybe having one mystery entry on the top of that extremely rare stack trace doesn't matter that much in practice?

[mdempsky]

True. And almost not panic, because it adds a frame to the stack.

I think if we're really worried about the wrappers showing up in stack frames, we should arrange to have the runtime elide them. As you point out, delete can panic too, and that needs to be wrapped in some cases.

This probably means that we need to pass the fully evaluated arguments to print/printlnt/delete/copy, in order to make sure any panics occur in the appropriate frame.

Yeah, that's what "{go,defer} print{,ln}(...)" currently do.

[gopherbot]

Change https://golang.org/cl/99017 mentions this issue: cmd/compile: fix miscompilation of "defer delete(m, k)"