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: unnecessary nil pointer check #40108

Open
randall77 opened this issue Jul 7, 2020 · 2 comments
Open

cmd/compile: unnecessary nil pointer check #40108

randall77 opened this issue Jul 7, 2020 · 2 comments

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Jul 7, 2020

package main

type T [8]byte

func f(p *T, i int) byte {
	return p[i]
}

This code generates a nil pointer check on p.

	0x000e 00014 (/Users/khr/gowork/tmp1.go:6)	MOVQ	"".p+32(SP), DX
	0x0013 00019 (/Users/khr/gowork/tmp1.go:6)	TESTB	AL, (DX)           <- nil pointer check
	0x0015 00021 (/Users/khr/gowork/tmp1.go:6)	MOVQ	"".i+40(SP), AX
	0x001a 00026 (/Users/khr/gowork/tmp1.go:6)	NOP
	0x0020 00032 (/Users/khr/gowork/tmp1.go:6)	CMPQ	AX, $8
	0x0024 00036 (/Users/khr/gowork/tmp1.go:6)	JCC	56                       <- bounds check
	0x0026 00038 (/Users/khr/gowork/tmp1.go:6)	MOVBLZX	(DX)(AX*1), AX
	0x002a 00042 (/Users/khr/gowork/tmp1.go:6)	MOVB	AL, "".~r2+48(SP)
	0x002e 00046 (/Users/khr/gowork/tmp1.go:6)	MOVQ	16(SP), BP
	0x0033 00051 (/Users/khr/gowork/tmp1.go:6)	ADDQ	$24, SP
	0x0037 00055 (/Users/khr/gowork/tmp1.go:6)	RET

We don't really need a nil pointer check, as the index is bounded by the bounds check.
We can subsume the nil pointer check into the load.

This would require the nil pointer pass to know that indexes are bounded for indexed loads, so it can prove that the load still occurs in the zero page even with the largest possible index.
Maybe we need to keep some information from the prove pass around somehow?

@randall77 randall77 added this to the Unplanned milestone Jul 7, 2020
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jul 7, 2020

If p is nil and i is out of bound, this may change the error produced from a nil pointer panic to a bound error. Not sure if this matters. If this does matter, we could do the nil check only on the out-of-bound code path, before issuing an out-of-bound panic.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 12, 2020

The Go spec doesn't currently prescribe whether the dereference or bounds-check happens first. So as long as users aren't confused by the change in error message, this seems fine to me.

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.