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
Comments
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:
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? |
Thanks for the comment.
Yes, I am currently using that method to customize buildssa in my static analyzer.
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. |
Looks like
|
What part of "second half" do you mean exactly? 馃憖
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? |
Lines 74-114 in buildssa.go.
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. |
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? |
Ah, understood. I misread. So, we are in agreement that we will create a new function in ssautil that behaves like this part, right?
Exactly. Forget it.
OK, agree. 馃憤 |
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? |
The most generous mode does would come with increased memory cost for everyone. This may be acceptable.
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 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. |
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
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...)
The text was updated successfully, but these errors were encountered: