-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/compile: order mishandles "defer delete(m, k)" when k is a "slow" map key type #24259
Comments
I don’t remember where else this was discussed, but ideally we would do an early rewrite of |
@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 |
And copy: go/src/cmd/compile/internal/gc/walk.go Line 281 in b854339
I think so. And bug-free-ness; these keep cropping up.
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? |
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.
Yeah, that's what "{go,defer} print{,ln}(...)" currently do. |
Change https://golang.org/cl/99017 mentions this issue: |
https://play.golang.org/p/2JNMRctSmRr
The above program prints
"3\n2\n"
, but it should print"3\n0\n"
. (And it does ifK
is changed to a "fast" map key type like uint32.)The problem is that, for "slow" map key types, order.go rewrites
into
but we never mark ktmp as escaping (because escape analysis happens earlier). So in the playground example above, this loop:
is effectively rewritten into:
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.The text was updated successfully, but these errors were encountered: