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
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@josharian
Copy link
Contributor

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
Member

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?

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 24, 2020
@josharian
Copy link
Contributor Author

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

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

@cherrymui
Copy link
Member

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

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

@golang golang locked and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants