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: unsafe.Pointer arithmetic causes insertion of redundant nil checks #27180
Comments
I have just stumbled upon this with a real life case: // Go's syscall.Msghdr lacks SetIovlen
func setIovlen(msg *syscall.Msghdr, iovlen int) {
const iovlenIs32bit = unsafe.Sizeof(msg.Iovlen) == 4
if iovlenIs32bit {
*(*uint32)(unsafe.Pointer(&msg.Iovlen)) = uint32(iovlen)
} else {
*(*uint64)(unsafe.Pointer(&msg.Iovlen)) = uint64(iovlen)
}
}
func main() {
var msg syscall.Msghdr
setIovlen(&msg, 10) // line 20
...
} Both 1.10.3 and 1.11rc2 generate 0x001f 00031 (t.go:20) LEAQ "".msg+24(SP), AX
0x0024 00036 (t.go:20) PCDATA $2, $0
0x0024 00036 (t.go:20) TESTB AL, (AX)
0x0026 00038 (t.go:20) MOVL $10, "".msg+24(SP) |
The question is, can we assume that all pointers generated by arithmetic are not nil?
That being said, I think we should still get rid of the nil checks after any unsafe arithmetic. I don't think anyone is actually manufacturing nil pointers this way. We're solidly in unsafe territory here, which is exempt from the Go 1 compatibility guarantee. For @growler's specific case, we could recognize a nil check of a LEAQ of a local variable can never be nil. That wouldn't require any change in behavior. |
Maybe this is where I'm misunderstanding the semantics the compiler tries to achieve, because I don't understand what you mean.
My understanding is that this is irrelevant, because calculating or otherwise obtaining a nil pointer is not something that warrants any action, regardless of whether it is an unsafe pointer or not. Dereferencing a pointer is different, that implies a nil check. Usually this happens implicitly, but if the dereference is optimized out because the result of the load isn't relevant to observed outcome, it needs to be replaced by a nil check to preserve the panic prescribed by the spec. But dereferencing is independent from pointer arithmetic. (I did test my own version with a nil pointer and it did panic iirc) So I actually don't understand why the nil check is inserted there in the first place, not why it's not optimized out. |
But there is a dereference. |
Here's a concrete program that demonstrates the difference:
Should this panic, or return the contents of address 0x77777777?
If we assume that pointer arithmetic always generates a non-nil result, then we'd instead do the other. |
Is it possible to left the nil dereference exception to be catched by the HW and getting rid of the SW detection? |
We do use hardware detection. Usually, we can merge a nil check and a subsequent load if they are both using the same base pointer and if the constant offset in the load is small enough. It looks like that optimization gets defeated by unsafe arithmetic.
The load and nil check have different base pointers, because the constant offset was merged with the load, but not merged with the nil check. We could probably allow merging constant offsets into nil checks. That would fix @FlorianUekermann's case, but probably isn't a general solution to this problem. |
Exactly, if there is a dereference already, why insert an extra one. I guess I don't understand why it makes a difference if the dereference happens in TESTB or MOVQ (in Go asm). |
Our comments overlapped. But I have an idea what I'm getting wrong now: Isn't there a guard range above 0, which is never used to catch these cases without explicit checks for nil? |
AFAICT, at least on nixes there's one. But consider it's just a VM page-sized thing, so it'll be 4K on some if not most systems. Meaning accessing of field with offset >= 4096 using a nil struct pointer will not be HW detected. Same goes for an array with item offset large enough and this later case would be much more common in many programs. I didn't realized this when asking for the HW detection above. |
I'm not sure what you're asking here, but we use this (from cmd/compile/internal/ssa/nilcheck.go):
|
@randall77, does it make sense to open a specific issue for nil check on pointers to local variables then? |
@growler: Sure, although we already do that normally:
There's something a bit more subtle going on here. Looks like we're nil-checking offsets from local addresses, instead of those addresses themselves. That might be what needs fixing. |
You might try adding |
Works like a charm: 0x001f 00031 (t.go:20) MOVL $10, "".msg+24(SP) I was too quick about tests though, it has failed |
Ship it! |
|
The nil check inside the function body has been removed as well, which I think is not desired result, since function might get called by a plenty of ways. Here is the function body compiled by 1.11: 0x0000 00000 (t.go:12) PCDATA $2, $1
0x0000 00000 (t.go:12) PCDATA $0, $1
0x0000 00000 (t.go:12) MOVQ "".msg+8(SP), AX
0x0005 00005 (t.go:12) PCDATA $2, $2
0x0005 00005 (t.go:12) LEAQ 24(AX), CX
0x0009 00009 (t.go:12) PCDATA $2, $1
0x0009 00009 (t.go:12) TESTB AL, (CX)
0x000b 00011 (t.go:12) MOVQ "".iovlen+16(SP), CX
0x0010 00016 (t.go:12) PCDATA $2, $0
0x0010 00016 (t.go:12) MOVL CX, 24(AX) and here is compiled with the suggested change: 0x0000 00000 (t.go:12) PCDATA $2, $0
0x0000 00000 (t.go:12) PCDATA $0, $0
0x0000 00000 (t.go:12) MOVQ "".iovlen+16(SP), AX
0x0005 00005 (t.go:12) PCDATA $2, $1
0x0005 00005 (t.go:12) PCDATA $0, $1
0x0005 00005 (t.go:12) MOVQ "".msg+8(SP), CX
0x000a 00010 (t.go:12) PCDATA $2, $0
0x000a 00010 (t.go:12) MOVL AX, 24(CX) |
That should be ok. The nil check of |
I see. That makes sense. Also, the function failing test is compiled to the same code by both versions, the only thing different is compiler's debug output:
The failing function is func f(t *TT) *byte {
// See issue 17242.
s := &t.SS // ERROR "removed nil check"
return &s.x // ERROR "generated nil check"
} (Well, I don't understand what happened, but at least now it seems more logical, first compiler generates a nil check while dereferencing |
Change https://golang.org/cl/131735 mentions this issue: |
Removes unnecessary nil-check when referencing offset from an address. Suggested by Keith Randall in #27180. Updates #27180 Change-Id: I326ed7fda7cfa98b7e4354c811900707fee26021 Reviewed-on: https://go-review.googlesource.com/131735 Reviewed-by: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
Merging constant offsets into |
Change https://golang.org/cl/146058 mentions this issue: |
Most testing done using
go version go1.10.1 linux/amd64
I have tried a bunch of releases, from 1.7, to 1.10.3
Caveat: I may just be confused about something, but I have spent quite some time trying to come up with reasons why the generated code makes sense and the mailing list didn't provide an answer either.
The pointer arithmetic pattern described in the
unsafe
documentation seems to cause the compiler to emit assembly to calculate and dereference the same pointer twice. The simplest way to reproduce this is:Looking at the (shortened) Go assembly we see this:
I would have expected something like this function (which I have tested and it seems to have the same behavior):
I have tried to come up with explanations for what the LEAQ, TESTB sequence is good for, but failed. It seems like the compiler is inserting the equivalent of
_ = *(*uint64)(unsafe.Pointer(uintptr(a) + 8))
before the return statement.I have tested countless variants of similar and not so similar functions containing pointer arithmetic and the result is always that both address calculation and dereference are duplicated, if an address is changed by a non-zero amount (
*(*uint64)(unsafe.Pointer(uintptr(a) + 0))
doesn't have this issue).I realize that this technically doesn't violate the spec or documented behavior, but the situation seems unfortunate (yes, I benchmarked the impact on less contrived examples).
ssa.html.log (renamed to make github happy)
The text was updated successfully, but these errors were encountered: