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: check that all n.Val() uses checked for a nil n.Type #22001

Closed
mvdan opened this issue Sep 24, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@mvdan
Copy link
Member

commented Sep 24, 2017

If there was an error in a program related to n, such as the use of an undefined name to declare it, the node may be incomplete. In particular, using n.Val() can cause panics, as is the case of n.Val().Interface().

A recent example: #21988

We should check the compiler to see if there are other instances of n.Val() use without us checking for n.Type == nil (or n.Type != nil). Ideally in an automated manner, as there are more than a hundred .Val() method calls in the gc package.

@mvdan mvdan self-assigned this Sep 24, 2017

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Sep 27, 2017

Change https://golang.org/cl/66450 mentions this issue: cmd/compile: fix another invalid switch case panic

gopherbot pushed a commit that referenced this issue Sep 27, 2017

cmd/compile: fix another invalid switch case panic
Very similar fix to the one made in golang.org/cl/65655. This time it's
for switches on interface values, as we look for duplicates in a
different manner to keep types in mind.

As before, add a small regression test.

Updates #22001.
Fixes #22063.

Change-Id: I9a55d08999aeca262ad276b4649b51848a627b02
Reviewed-on: https://go-review.googlesource.com/66450
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot

This comment has been minimized.

Copy link

commented Sep 27, 2017

Change https://golang.org/cl/66690 mentions this issue: cmd/compile: make Node.Val panic if .Type is nil

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Continuing the discussion from https://golang.org/cl/66690, @mdempsky said:

I think the issue is that consttype(n)==0 indicates a weird state: it's an OLITERAL constant that's missing a Val.

An alternative fix for your swt.go changes would be to just change the "consttype(n) < 0" checks to "consttype(n) <= 0".

I'm trying to think whether it even makes sense to distinguish 0 from -1 in consttype's return value. Maybe collapsing them to a single return value would be the real fix here.

A couple of places, one in swt.go and one in const.go, use consttype(n) >= 0. What should we do with those?

Otherwise, all checks are either == x, != x, or < 0.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

I'm leaning towards:

  1. Change consttype to return 0 for nil and non-OLITERAL nodes.
  2. Change all of the consttype(n) >= 0 calls to consttype(n) > 0, and consttype(n) < 0 to consttype(n) <= 0 (or maybe to consttype(n) == 0).
@mvdan

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Sounds good. I assume that would let us get rid of either or both of the n.Type == nil checks. Do you want to do the CL, or should I?

I wonder what we should do with this issue, though. Perhaps we could rewrite it to "check that none of the Val.Interface() calls can possibly panic", as that is definitely desirable as far as I can see.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

Go ahead and prepare a CL if you're interested.

I think you can just put "Fixes #22001." in it. I think once we cleanup consttype that the underlying concern here has been reasonably addressed.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2017

Fair enough - thanks for your help :)

@gopherbot

This comment has been minimized.

Copy link

commented Oct 10, 2017

Change https://golang.org/cl/69510 mentions this issue: cmd/compile: make bad Ctypes be only 0

@gopherbot gopherbot closed this in f14f7b3 Oct 10, 2017

@golang golang locked and limited conversation to collaborators Oct 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.