Skip to content
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: bad types originating from ssa rule to inline memmove #37381

Closed
josharian opened this issue Feb 22, 2020 · 5 comments
Closed

cmd/compile: bad types originating from ssa rule to inline memmove #37381

josharian opened this issue Feb 22, 2020 · 5 comments
Labels

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Feb 22, 2020

generic.rules contains this rule:

(StaticCall {sym} s1:(Store _ (Const(64|32) [sz]) s2:(Store  _ src s3:(Store {t} _ dst mem))))
	&& isSameSym(sym,"runtime.memmove")
	&& t.(*types.Type).IsPtr() // avoids TUINTPTR, see issue 30061
	&& s1.Uses == 1 && s2.Uses == 1 && s3.Uses == 1
	&& isInlinableMemmove(dst,src,sz,config)
	&& clobber(s1) && clobber(s2) && clobber(s3)
	-> (Move {t.(*types.Type).Elem()} [sz] dst src mem)

The type of the moved object created here often doesn't match the size of the move.

For example, the most common case is copying a few bytes, so, for example, t.(*types.Type).Elem() might be byte while sz is 4. But the size of a byte is 1, not 4.

This doesn't matter right now, because the backends don't pay any attention to the type of Move, only the size.

The right type here is an array type, with elem t.(*types.Type).Elem() and bound sz / sizeof(t.(*types.Type).Elem()). However, for concurrency safety, we cannot create new array types in the backend. (It happens to work with the compiler as it is at the moment, but it is brittle; if you add new optimizations it is easy to end up requiring the size of that new array type, which panics.)

I'm not quite sure what we should do here. Maybe just remove the type Aux from Move?

cc @randall77 @cherrymui

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Feb 23, 2020

The type Aux for Move is used for the write barrier pass to insert write barriers. But for types that don't contain pointers, it doesn't really matter -- the write barrier pass just needs to know it doesn't contain pointers. (For pointerful types, it does need to be correct.)

Also, on some architectures, the type's alignment is used in lowering.

So for the memmove case, it is fine to use byte type (for now), which is pointerless and has minimum alignment. Maybe leave a comment?

@josharian
Copy link
Contributor Author

@josharian josharian commented Feb 25, 2020

I couldn't quite convince myself that the current inline-memmove rule doesn't screw this up. I think it may be possible on amd64 to copy 2 pointers with this (say [2]*int) but have the type only indicate one pointer (*int). I'll send a CL that documents this and makes it obvious that the problematic case cannot occur.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 25, 2020

Change https://golang.org/cl/220683 mentions this issue: cmd/compile: ensure that inlined memmove calls have pointer-free types

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Feb 25, 2020

I think memmove is only used for pointerless types. For pointerful types, it needs a write barrier so it will use typedmemmove.

@josharian
Copy link
Contributor Author

@josharian josharian commented Feb 25, 2020

Oh right, thanks. 😊 I've updated the CL to be documentation only.

@gopherbot gopherbot closed this in 4ae1879 Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.