-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
x/tools/go/ssa: CreateTestMainPackage broken by fuzzing changes #48547
Comments
Change https://golang.org/cl/351509 mentions this issue: |
For golang/go#48547 Change-Id: I211239497c49b152504466dae963a68b0a4f5f6b Reviewed-on: https://go-review.googlesource.com/c/tools/+/351509 Trust: Bryan C. Mills <bcmills@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Note that CL 351509 doesn't really resolve the problem: we shouldn't leave the test permanently unskipped. We can resolve this issue by one of:
(Or perhaps some other option I haven't thought of.) |
Sorry for removing my original comment, I realized later that the fix just skips some tests. Is CreateTestMainPackage deprecated? Is it possible to use a different API instead, would that be another possible solution? |
Yep! Per https://pkg.go.dev/golang.org/x/tools/go/ssa#Program.CreateTestMainPackage:
That is exactly the “switching |
This is the best option I think. Depending on an unstable API makes it unnecessarily difficult to make changes like this. |
+1 to migrating If there are 0 other users in the wild, is there any objection to deleting |
I suspect that it would be fine to delete |
More info. This API is also used in guru, godoc analysis, and another place in the pointer package. |
Change https://golang.org/cl/357834 mentions this issue: |
Change https://golang.org/cl/360837 mentions this issue: |
@zpavlinovic, anything left to do for this issue, or are we calling it good enough? |
This test was already memory-hungry to begin with, and apparently the switch to go/packages in CL 356513 pushed it over the edge of the 32-bit address space (at least with the default GOGC setting). Rather than trying to precisely tune it to skim under the 32-bit limit, let's just skip the test on platforms with insufficient address space. Updates golang/go#14113 Updates golang/go#48547 Change-Id: Iab99e9ce70a98034194d7c7ad7df7a545ac95ef3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/360837 Trust: Bryan C. Mills <bcmills@google.com> Run-TryBot: Bryan C. Mills <bcmills@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
I am still working on removing usage of this API from guru, which will hopefully allow us to delete this API altogether. However, this particular issue might be interpreted as being concerned with ssa only. If that is the case, then we can close this issue and I can open another issue for tracking the progress of getting rid of this API. |
Either approach is ok by me. |
Then I suggest keeping it simple and just use this issue to address the whole API. (I do not have an eta on when will I finish this, but hopefully soon.) |
Change https://golang.org/cl/363655 mentions this issue: |
Change https://golang.org/cl/363657 mentions this issue: |
For golang/go#48547 Change-Id: I71f061e55d52c45346d34d05ade9c82e9856d6e7 Reviewed-on: https://go-review.googlesource.com/c/tools/+/363655 Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Trust: Zvonimir Pavlinovic <zpavlinovic@google.com> TryBot-Result: Go Bot <gobot@golang.org>
Change https://golang.org/cl/363659 mentions this issue: |
For golang/go#48547 Change-Id: Ief1c4fb6302437a8736d52e87541f8229b02289a Reviewed-on: https://go-review.googlesource.com/c/tools/+/363657 Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
For golang/go#48547 Change-Id: I1337b99c90ce81b58dfaa93e49c6dcb884330b22 Reviewed-on: https://go-review.googlesource.com/c/tools/+/363659 Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
That failure comes from here:
This function is a known technical debt:
CC @jayconrod @ianthehat
The text was updated successfully, but these errors were encountered: