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: investigate proper use of Node.copy (reminder issue) #27765

Closed
griesemer opened this issue Sep 19, 2018 · 4 comments
Closed

cmd/compile: investigate proper use of Node.copy (reminder issue) #27765

griesemer opened this issue Sep 19, 2018 · 4 comments
Assignees
Milestone

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 19, 2018

Node.copy doesn't do anything special with Node.Orig. This can cause problems when the original node (n) is modified and n.Orig == n (that is, when n.Orig points to itself).

The code takes care of this at the call site at (cmd/compile/internal/gc/) const.go:1203 and also typecheck.go:2926 ( https://go-review.googlesource.com/c/go/+/136395 ). See also #26855.

Making the appropriate change in Node.copy (and remove the special handling outside) appears to introduce failures. Investigate.

cc: @mdempsky

@griesemer griesemer added this to the Go1.12 milestone Sep 19, 2018
@griesemer griesemer self-assigned this Sep 19, 2018
@griesemer

This comment has been minimized.

Copy link
Contributor Author

@griesemer griesemer commented Sep 20, 2018

FWIW, cmd/compile/internal/types/type.go:725 (Type.copy) does exactly the expected Orig update for Type structs.

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Sep 20, 2018

This method originated from a cleanup: 19ee2ef

At the time, I simply did the bare minimum to simplify the code. Most of the old pieces of code left Node.Orig alone, so that wasn't changed.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

@griesemer griesemer commented Sep 20, 2018

@mvdan Yes, I have seen that. I think your change was fine. This is a pre-existing problem.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 20, 2018

Change https://golang.org/cl/136540 mentions this issue: cmd/compile/internal/gc: fix Node.copy and introduce (raw|sep)copy

@gopherbot gopherbot closed this in ce58a39 Sep 20, 2018
@golang golang locked and limited conversation to collaborators Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.