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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/go/ssa/ssautil: enable reuse of buildssa.Analyzer.Run with different ssa.BuilderModes #44010

Open
sanposhiho opened this issue Jan 30, 2021 · 9 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@sanposhiho
Copy link

sanposhiho commented Jan 30, 2021

Hi馃憢
I propose to enable change ssa.BuilderMode on buildssa Analyzer.
https://github.com/golang/tools/blob/master/go/analysis/passes/buildssa/buildssa.go

There is currently no way to change BuilderMode, and buildssa Analyzer runs with no BuilderMode.
The comment on the buildssa Analyzer says that

// Some Analyzers may need GlobalDebug, in which case we'll have
// to set it globally, but let's wait till we need it.

With the variety of great static analysis tools being developed, I think it should be possible to change the mode so that more creative tools can be developed. (The static analysis tool I am developing, wastedassign, also requires BuilderMode change...)

@gopherbot gopherbot added this to the Proposal milestone Jan 30, 2021
@timothy-king
Copy link
Contributor

For more context for others here is the PR: https://go-review.googlesource.com/c/tools/+/284219

The proposal is to essentially allow for a new function in the buildssa to have allow specifying a BuilderMode by Currying:

func CreateRunner(mode ssa.BuilderMode) func(pass *analysis.Pass) (interface{}, error)

This does address an existing TODO comment in buildssa.

I am not sure exposing this will help many people. This solution seems quite specialized and not not a very flexible interface for the Go x/tools library to maintain going forward. It may help specialized cases like the one above. One possibility is for users that want customization is to fork buildssa to create a new Analyzer with different behavior. While this does lead to code duplication, this is not too bad as you would still be able to achieve your goal.

Have you thought about alternative solutions or libraries that could help you do what you are trying to do? There is an ssautil package. Is there a way we could break up the buildssa run function into 1-2 helper functions we could add to ssautil that are fairly clean/easy to maintain yet would be flexible enough for you support a different BuilderMode?

@sanposhiho
Copy link
Author

sanposhiho commented Feb 3, 2021

Thanks for the comment.

One possibility is for users that want customization is to fork buildssa to create a new Analyzer with different behavior.

Yes, I am currently using that method to customize buildssa in my static analyzer.

Is there a way we could break up the buildssa run function into 1-2 helper functions we could add to ssautil that are fairly clean/easy to maintain yet would be flexible enough for you support a different BuilderMode?

As mentioned in the comments on buildssa.go#37L, the first half of buildssa.run seems to be almost identical to ssautil.BuildPackage function. So how about adding this creating SrcFuncs part as a function of ssautil? This change gives developers the flexibility and eases to write code that works similarly to what is done in buildssaAnalyzer by using ssautil.

@timothy-king
Copy link
Contributor

Looks like ssautil.BuildPackage would need to be split into two parts with second half exported as a new function. I think there is a good case that BuildPackage is not quite what most users want. (With this duplication as evidence.)

So how about adding this creating SrcFuncs part as a function of ssautil?
This is a bit less obviously useful in other circumstances. But I am mildly supportive to adding this or something similar.

@sanposhiho
Copy link
Author

sanposhiho commented Feb 7, 2021

Looks like ssautil.BuildPackage would need to be split into two parts with second half exported as a new function.

What part of "second half" do you mean exactly? 馃憖

This is a bit less obviously useful in other circumstances. But I am mildly supportive to adding this or something similar.

Thanks.

And, what do you think of another idea of creating a function in ssautil that behaves almost exactly like buildssa.run? Would this idea make the function's responsibilities too large and inflexible?

@timothy-king
Copy link
Contributor

Looks like ssautil.BuildPackage would need to be split into two parts with second half exported as a new function.

What part of "second half" do you mean exactly? 馃憖

Lines 74-114 in buildssa.go.

And, what do you think of another idea of creating a function in ssautil that behaves almost exactly like buildssa.run? Would this idea make the function's responsibilities too large and inflexible?

We will would need the API to be clear and easy to maintain. I am certainly open to having 1 function instead of 2. It will depend on the specifics of the API vs plausible alternatives. Without code it is hard to tell.

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Feb 8, 2021
@timothy-king
Copy link
Contributor

On reflection this seems kinda small in scope for a proposal. @sanposhiho Any objections to me moving this from a Proposal to just a regular issue?

@sanposhiho
Copy link
Author

Lines 74-114 in buildssa.go.

Ah, understood. I misread.

So, we are in agreement that we will create a new function in ssautil that behaves like this part, right?

Without code it is hard to tell.

Exactly. Forget it.

On reflection this seems kinda small in scope for a proposal. @sanposhiho Any objections to me moving this from a Proposal to just a regular issue?

OK, agree. 馃憤

@timothy-king timothy-king changed the title proposal: x/tools/go/analysis/passes/buildssa: enable change ssa.BuilderMode on buildssa Analyzer x/tools/go/ssa/ssautil: enable reuse of buildssa.Analyzer.Run with different ssa.BuilderModes Feb 10, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 10, 2021
@timothy-king timothy-king added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal Tools This label describes issues relating to any tools in the x/tools repository. labels Feb 10, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 10, 2021
@timothy-king timothy-king modified the milestones: Proposal, Unplanned Feb 10, 2021
@adonovan
Copy link
Member

Building the SSA twice in two different modes is strictly worse (in code complexity and run-time cost) than choosing the most generous mode and enabling that unconditionally.

Could we simply set the GlobalDebug flag in the buildssa analyzer, and document it?

@timothy-king
Copy link
Contributor

Building the SSA twice in two different modes is strictly worse (in code complexity and run-time cost) than choosing the most generous mode and enabling that unconditionally.

The most generous mode does would come with increased memory cost for everyone. This may be acceptable.

Could we simply set the GlobalDebug flag in the buildssa analyzer, and document it?

Hyrum's law there could be brittle users that this breaks. I remember do remember some internal checkers that used to depend on this. I think buildssa certainly could choose to comply with the contract so this may be acceptable.

An advantage of new func(BuilderMode) *Analyzer function is that it seems future proof in a way that changing the contract of buildssa to promise DebugRefs does not.

Something I have had in the back of my mind is a longer term question is whether ssa should adopt staticcheck ir's change to just have a ast.Node/ast.Expr on each Instruction instead of a token.Pos on each Value/Instruction. This is 2 words instead of 1 word per register, but much less than a new DebugRef instruction (8 words). If folks started to rely on DebugRef, it would be some work to switch to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants