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

Jump to bottom

cmd/compile: late nilcheck elim doesn't elim later nilcheck #20962

Open
josharian opened this issue Jul 10, 2017 · 3 comments
Open

cmd/compile: late nilcheck elim doesn't elim later nilcheck #20962

josharian opened this issue Jul 10, 2017 · 3 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Unplanned

Comments

@josharian
Copy link
Contributor

Reproduce:

  1. Disable the regular (early) nilcheckelim pass.
  2. go tool compile -d=nil x.go this code:
package p

//go:noinline
func fx10k() *[10000]int { return nil }

func f3(x *[10000]int) {
	x = fx10k()
	_ = x[9999]
	_ = x[9999]
}

Result:

x.go:8:7: removed nil check
x.go:9:7: generated nil check

This is backwards. We should generate the first nil check and remove the second one, on pain of panicking on the wrong line.

I'm not sure whether this impacts any code in practice right now. Obvious reproducers like the one above already have their duplicate nil checks removed by the early nilcheck pass. I noticed it when implementing the TODO at the end of nilcheck2 caused the wrong nil checks to be eliminated from test/nilptr3.go.

Opinions about the correct fix are welcome.

cc @randall77
@josharian josharian added this to the Go1.10 milestone Jul 10, 2017
@mdempsky
Copy link
Member

At tip I get:

$ go tool compile -d=nil /tmp/u.go
/tmp/u.go:8:7: generated nil check
/tmp/u.go:9:7: removed nil check

So it seems like this has been fixed at some point?

@mdempsky mdempsky reopened this Nov 29, 2017
@mdempsky
Copy link
Member

Oh, step 1 seems to require modifying the compiler, though I wasn't able to immediately figure out how to do that.

Either way, code at tip seems to be working correctly, so I'm bumping this to 1.11. I'll leave to @josharian and @randall77 to decide whether this is still worth pursuing.

@mdempsky mdempsky modified the milestones: Go1.10, Go1.11 Nov 29, 2017
@josharian
Copy link
Contributor Author

Oh, step 1 seems to require modifying the compiler, though I wasn't able to immediately figure out how to do that.

Comment out this line:

go/src/cmd/compile/internal/ssa/compile.go

Line 343 in 24cdb0f

{name: "nilcheckelim", fn: nilcheckelim},

I'll leave to @josharian and @randall77 to decide whether this is still worth pursuing.

Not important for 1.11; this is a latent bug waiting to surface when someone next improves late nilcheck. Moving to unplanned.

We should, however, double-check that there's a test in the tree that would catch this bug.

@josharian josharian modified the milestones: Go1.11, Unplanned Feb 13, 2018
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@mdempsky @josharian @gopherbot