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: typecheck should insert OCONVIFACE for some OAS2FUNC; possible memory corruption in escape analysis #46725

Closed
mdempsky opened this issue Jun 13, 2021 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

In CL 153841, I changed typecheck to rewrite f(g()) calls and return g() statements for multi-valued g() to use temporaries so the backend wouldn't need to worry about tuple-valued expressions anywhere except for in OAS2FUNC statements. In particular, this was motivated to fix memory corruption issues in the old escape analysis code like mentioned in #29197 (comment). (Here is a clearer reformulation of that test case; it used to fail with "GC'd early".)

However, @randall77 asked about suggestions on how to handle a similar issue needed for inserting implicit conversions for GC-shape stenciling in generics, and it made me realize the same issue affects normal OAS2FUNC statements. E.g. this minor variation of the above test case still fails: https://play.golang.org/p/qnq7h6kpSOL

We should probably extend typecheck to check if any of the LHS expressions of an OAS2FUNC are not identical to the respective function call result; and if so, insert autotmps like we already do for f(g()) calls and return g() statements.

Note: even if just one variable/result pair has mismatched types, we should insert autotmps for all of them, to ensure we preserve order-of-evaluation semantics. (Theoretically we could try optimizing this, but I expect it's infrequent enough that we can let SSA handle that instead.)

/cc @cuonglm

@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 13, 2021
@mdempsky mdempsky added this to the Go1.18 milestone Jun 13, 2021
@gopherbot
Copy link

Change https://golang.org/cl/327651 mentions this issue: cmd/compile: rewrite a, b = f() to use temporaries when type not identical

@gopherbot
Copy link

Change https://golang.org/cl/327650 mentions this issue: cmd/compile: factor out rewrite multi-valued f()

gopherbot pushed a commit that referenced this issue Jun 14, 2021
So next CL can reuse code to rewrite OAS2FUNC.

Passes toolstash -cmp.

For #46725

Change-Id: I1113ed615b6d6b9494dd87000ce342d7a46d9e7b
Reviewed-on: https://go-review.googlesource.com/c/go/+/327650
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@golang golang locked and limited conversation to collaborators Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

2 participants