-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: transform environment variable GOSSAFUNC into the option that is passed to compiler #25942
Comments
SGTM. I'd suggest I'd suggest also converting GOSSAHASH to And maybe GO_SSA_PHI_LOC_CUTOFF to something like That leaves only GOCLOBBERDEADHASH. I don't have an opinion about that one. Maybe Keith does. cc also @dr2chase @cherrymui |
I personally feel that environment variable is easier to write in the command line, because it can be always at the beginning of the line, instead of somewhere in the middle. That said, I'm open to this change.
I'm also not sure this is a bad thing. It is supposed to be used for people who work on the compiler, not general use. |
I haven't used GO_SSA_PHI_LOC_CUTOFF in ages, if it vanished I would not notice. -ssadump would work fine for me, though it is much easier to put it at the beginning of the command line. Is this "-gcflags -ssadump" or -gcflags -d=-ssadump?. Would it we be acceptable to also print an informational message about where the file was put, since it will be some package's source directory? That can sometimes be a little tricky. GOSSAHASH I haven't used lately, but when I do use it, I tend to use a harness that sets the environment variable(s), and the harness can cope with multi-point errors though those are rare. So if it were up to me:
|
I would leave CLOBBERDEADHASH. When using this feature, it's often not clear which compiler invocation caused it (especially the calls of the compiler that originate from test files). |
IIUC, the proposal is "-gcflags -ssadump".
I believe that Yury plans to add the Node AST dump as second column in ssa.html. So: source code, then AST, then initial SSA, then passes. At that point, I agree: We could eliminate all stdout spew, which would be really nice.
Yes, that'd be great. |
I'm cleaning the std output of GOSSAFUNC. I have removed IR after eash phase. I leave the Node AST as it is because we don't dump it into ssa.html. What should I do with progs being printed to std? We have it in ssa.html already. Am I okay to remove progs output from stdout? |
Trying to convert GOSSAFUNC I came to this required mumbo-jumbo: Compare it to I think that having GOSSAFUNC is not so bad after all. I am not sure if that would be an improvement. Thoughts? |
Personally I would't want the IR dump being removed. When I debug large functions, with thousands of Values, searching/grepping in a text file is much easier and faster than searching in browser. (Of course you could argue that I could grep the html instead...) |
I don't want to break anyones habits. I guess this is a no-go? Is there any sense to split dump into stdout and ssa.html as separate options? I guess not much. I can finish the "sources of all inlined functions" (#25904) with the old environment variable. |
Change https://golang.org/cl/126604 mentions this issue: |
Change https://golang.org/cl/126603 mentions this issue: |
I should add, the existing dump option (which I don't recommend for general use) is -gcflags="all=ssa/phase/dump=funcname" with special values "build" and "all" for phase. This writes one file per phase+function, with sequence numbers added to simplify ordering and also in case of collisions. It's not very nice, |
Those changes above do not convert About writing into stdout when |
SGTM. |
Cool. I have incorporated this behaviour into the https://go-review.googlesource.com/c/126603/ |
Store the value of GOSSAFUNC in a global variable to avoid multiple calls to os.Getenv from gc.buildssa and gc.mkinlcall1. The latter is implemented in the CL 126606. Updates #25942 Change-Id: I58caaef2fee23694d80dc5a561a2e809bf077fa4 Reviewed-on: https://go-review.googlesource.com/126604 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Since we print almost everything to ssa.html in the GOSSAFUNC mode, there is a need to stop spamming stdout when user just wants to see ssa.html. This changes cleans output of the GOSSAFUNC debug mode. To enable the dump of the debug data to stdout, one must put suffix + after the function name like that: GOSSAFUNC=Foo+ Otherwise gc will not print the IR and ASM to stdout after each phase. AST IR is still sent to stdout because it is not included into ssa.html. It will be fixed in a separate change. The change adds printing out the full path to the ssa.html file. Updates #25942 Change-Id: I711e145e05f0443c7df5459ca528dced273a62ee Reviewed-on: https://go-review.googlesource.com/126603 Run-TryBot: Yury Smolsky <yury@smolsky.by> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Closing it since the solution turned out to be acceptable for the most. Let me repeat the way it was resolved:
Feel free to reopen if you want any additions. |
Right now
GOSSAFUNC
is a magical environment variable that allows to peek under the hood of SSA generation. It is an indispensable tool, and we need to think about converting it from magical hidden environment variable into command line option supplied togo tool compile
.I propose this option:
-ssafunc=<function>
My plan is read this option to the global variable and use that in place of GOSSAFUNC.
And in inlining phase I want to record to which functions calls were inlined from *n if
ssafunc==n.funcname()
. It is to display the sources of all inlined functions in ssa.html.Also the comment by @rsc in
src/cmd/go/internal/work/exec.go
motivated me:CC: @josharian @randall77
The text was updated successfully, but these errors were encountered: