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.

Sign up for GitHub

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

Jump to bottom

x/tools/go/ssa: CreateTestMainPackage broken by fuzzing changes #48547

Closed
bcmills opened this issue Sep 22, 2021 · 17 comments
Closed

x/tools/go/ssa: CreateTestMainPackage broken by fuzzing changes #48547

bcmills opened this issue Sep 22, 2021 · 17 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository. Unfortunate
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 22, 2021 

~/x/tools$ gotip version
go version devel go1.18-051df0d72 Wed Sep 22 03:45:00 2021 +0000 linux/amd64

~/x/tools$ gotip test ./go/pointer
Entering directory `/usr/local/google/home/bcmills/x/tools/go/pointer'
Entering directory `/usr/local/google/home/bcmills/x/tools/go/pointer'
2021/09/22 11:06:56 internal error type-checking main$testmain: main$testmain.go:43:59: not enough arguments in call to testing.MainStart
FAIL    golang.org/x/tools/go/pointer   2.067s
FAIL

That failure comes from here:

		log.Fatalf("internal error type-checking %s: %v", path, err)

This function is a known technical debt:

// TODO(adonovan): throws this all away now that x/tools/go/packages
// provides access to the actual synthetic test main files.

CC @jayconrod @ianthehat
@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. release-blocker fuzz Issues related to native fuzzing support labels Sep 22, 2021
@bcmills bcmills added this to the Go1.18 milestone Sep 22, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 22, 2021
@gopherbot
Copy link

Change https://golang.org/cl/351509 mentions this issue: go/pointer: skip tests that depend on an unstable API changed in Go 1.18

gopherbot pushed a commit to golang/tools that referenced this issue Sep 22, 2021
go/pointer: skip tests that depend on an unstable API changed in Go 1.18
efaec4e 
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>
@zpavlinovic zpavlinovic added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Sep 28, 2021
@bcmills
Copy link
Contributor Author

bcmills commented Sep 28, 2021 •
edited

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:

  • switching x/tools/go/ssa to use x/tools/go/packages instead of rolling its own test binary, or
  • updating x/tools/go/ssa to use the new 1.18 signature when working with a 1.18 toolchain, or
  • reverting the signature change (and perhaps finding some other way to pass the list of fuzz tests to the package).

(Or perhaps some other option I haven't thought of.)

@zpavlinovic
Copy link
Contributor

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?

@bcmills
Copy link
Contributor Author

bcmills commented Sep 28, 2021

Is CreateTestMainPackage deprecated?

Yep! Per https://pkg.go.dev/golang.org/x/tools/go/ssa#Program.CreateTestMainPackage:
“Deprecated: Use golang.org/x/tools/go/packages to access synthetic testmain packages.”

Is it possible to use a different API instead, would that be another possible solution?

That is exactly the “switching x/tools/go/ssa to use x/tools/go/packages” option, I think. 🙂

@zpavlinovic zpavlinovic self-assigned this Sep 28, 2021
@jayconrod
Copy link
Contributor

switching x/tools/go/ssa to use x/tools/go/packages instead of rolling its own test binary, or

This is the best option I think. Depending on an unstable API makes it unnecessarily difficult to make changes like this.

@timothy-king
Copy link
Contributor

+1 to migrating x/tools away from using this function.

If there are 0 other users in the wild, is there any objection to deleting CreateTestMainPackage or replacing its body with panic("deprecated")? This is clearly a semantic change for the module. Is there a good precedent for this in x/tools?

@bcmills
Copy link
Contributor Author

bcmills commented Sep 29, 2021

I suspect that it would be fine to delete CreateTestMainPackage entirely if it no longer works. That is a breaking change, but OTOH we don't yet promise strict API stability in the x/ repos (that's part of why they exist!), and in this case the package doc already carries a prominent warning that the API “IS EXPERIMENTAL AND IS LIKELY TO CHANGE”.

@katiehockman katiehockman removed release-blocker fuzz Issues related to native fuzzing support labels Oct 6, 2021
@katiehockman katiehockman modified the milestones: Go1.18, Unreleased Oct 6, 2021
@zpavlinovic
Copy link
Contributor

zpavlinovic commented Oct 6, 2021 •
edited

More info. This API is also used in guru, godoc analysis, and another place in the pointer package.

@gopherbot
Copy link

Change https://golang.org/cl/357834 mentions this issue: godoc/analysis: replace use of deprecated ssa.CreateTestMainPackage

@gopherbot
Copy link

Change https://golang.org/cl/360837 mentions this issue: go/pointer: skip TestInput on 32-bit platforms

@bcmills
Copy link
Contributor Author

bcmills commented Nov 2, 2021

@zpavlinovic, anything left to do for this issue, or are we calling it good enough?

gopherbot pushed a commit to golang/tools that referenced this issue Nov 2, 2021
go/pointer: skip TestInput on 32-bit platforms
182bbdc 
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>
@zpavlinovic
Copy link
Contributor

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.

@bcmills
Copy link
Contributor Author

bcmills commented Nov 3, 2021

Either approach is ok by me.

@zpavlinovic
Copy link
Contributor

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.)

@gopherbot
Copy link

Change https://golang.org/cl/363655 mentions this issue: cmd/guru: remove use of deprecated ssa.CreateTestMainPackage

@gopherbot
Copy link

Change https://golang.org/cl/363657 mentions this issue: go/pointer: remove use of deprecated ssa.CreateTestMainPackage

gopherbot pushed a commit to golang/tools that referenced this issue Nov 12, 2021
@zpavlinovic
cmd/guru: remove use of deprecated ssa.CreateTestMainPackage
49ce184 
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>
@gopherbot
Copy link

Change https://golang.org/cl/363659 mentions this issue: go/ssa: remove deprecated FindTets and CreateTestMainPackage

gopherbot pushed a commit to golang/tools that referenced this issue Nov 16, 2021
@zpavlinovic
go/pointer: remove use of deprecated ssa.CreateTestMainPackage
8122e49 
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>
gopherbot pushed a commit to golang/tools that referenced this issue Nov 23, 2021
@zpavlinovic
go/ssa: remove deprecated FindTests and CreateTestMainPackage
7cf1f38 
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>
@MatthiasPetermann MatthiasPetermann mentioned this issue Dec 27, 2021
Closed
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository. Unfortunate
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@jayconrod @timothy-king @zpavlinovic @bcmills @gopherbot @katiehockman