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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@griesemer
Copy link
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 19, 2018
@griesemer griesemer added this to the Go1.12 milestone Sep 19, 2018
@griesemer griesemer self-assigned this Sep 19, 2018
@griesemer
Copy link
Contributor Author

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

@mvdan
Copy link
Member

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
Copy link
Contributor Author

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

@gopherbot
Copy link

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

@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.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants