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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/compile: transform environment variable GOSSAFUNC into the option that is passed to compiler #25942

Closed
ysmolski opened this issue Jun 18, 2018 · 16 comments
Closed

cmd/compile: transform environment variable GOSSAFUNC into the option that is passed to compiler #25942

ysmolski opened this issue Jun 18, 2018 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12

Comments

@ysmolski
Copy link
Member

ysmolski commented Jun 18, 2018 •
edited

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 to go 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:

 	// TODO(rsc): Convince compiler team not to add more magic environment variables,
	// or perhaps restrict the environment variables passed to subprocesses.
	magic := []string{
		"GOCLOBBERDEADHASH",
		"GOSSAFUNC",
		"GO_SSA_PHI_LOC_CUTOFF",
		"GOSSAHASH",
	}

CC: @josharian @randall77
@josharian
Copy link
Contributor

SGTM.

I'd suggest -ssadump instead, though. GOSSAFUNC was introduced while developing the SSA backend. It was used to enable SSA for a particular func. Now that it is mostly about dumping information, may as well update its name.

I'd suggest also converting GOSSAHASH to -hash while we're at it. (A hash could be useful in any part of the compiler, not just SSA.)

And maybe GO_SSA_PHI_LOC_CUTOFF to something like -phicutoff. Or maybe better just delete it. I don't see it being used much in the future, and if it is needed, it will be on a transient basis, and can be locally hacked in again.

That leaves only GOCLOBBERDEADHASH. I don't have an opinion about that one. Maybe Keith does.

cc also @dr2chase @cherrymui

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 18, 2018
@andybons andybons added this to the Unplanned milestone Jun 18, 2018
@cherrymui
Copy link
Member

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.

magical hidden

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.

@dr2chase
Copy link
Contributor

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.
If we could lose the spew to standard out, that might be nice, but it would also be nice to be able to get at the tree IR too (the tree input to SSA appears at the beginning of the spew). Note that we also have the ability to dump text-formatted SSA for a given function/phase already -- it was done this way for debugging giant machine-generated files where the full dump is so large that editing is painful.

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:

  • lose GO_SSA_PHI_LOC_CUTOFF
  • improve/replace GOSSAFUNC
  • leave GOSSAHASH alone (for now)

@randall77
Copy link
Contributor

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

@josharian
Copy link
Contributor

Is this "-gcflags -ssadump" or -gcflags -d=-ssadump?.

IIUC, the proposal is "-gcflags -ssadump".

If we could lose the spew to standard out, that might be nice, but it would also be nice to be able to get at the tree IR too

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.

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?

Yes, that'd be great.

@ysmolski ysmolski modified the milestones: Unplanned, Go1.12 Jul 17, 2018
@ysmolski
Copy link
Member Author

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?

@ysmolski
Copy link
Member Author

Trying to convert GOSSAFUNC I came to this required mumbo-jumbo:
go build -gcflags=all=-ssadump=genssa cmd/compile

Compare it to GOSSAFUNC=genssa go build cmd/compile

I think that having GOSSAFUNC is not so bad after all. I am not sure if that would be an improvement. Thoughts?

@cherrymui
Copy link
Member

I'm cleaning the std output of GOSSAFUNC. I have removed IR after eash phase.

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

@ysmolski
Copy link
Member Author

Personally I would't want the IR dump being removed.

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.

@gopherbot
Copy link

Change https://golang.org/cl/126604 mentions this issue: cmd/compile: cache the value of environment variable GOSSAFUNC

@gopherbot
Copy link

Change https://golang.org/cl/126603 mentions this issue: cmd/compile: clean the output of GOSSAFUNC

@dr2chase
Copy link
Contributor

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,
but it gets the debugging out even when the input is huge (and buffering ssa.html then becomes a problem).

@ysmolski
Copy link
Member Author

Those changes above do not convert GOSSAFUNC into -ssadump yet, I did them to allow the inlined functions sources feature. I am hesitant to kill the GOSSAFUNC. I found that it is much easier to use it rather than the gcflags parameter.

About writing into stdout when GOSSAFUNC is provided. Should we perhaps introduce some modifier to printout into stdout? I really don't want to have stdout when I want to have ssa.html. I want to enable to see stuff in stdout occasionally. Maybe GOSSAFUNC=Foo+ could tell the compiler to dump to ssa.html and stdout, while GOSSAFUNC=Foo would dump only to ssa.html?

@cherrymui
Copy link
Member

I want to enable to see stuff in stdout occasionally. Maybe GOSSAFUNC=Foo+ could tell the compiler to dump to ssa.html and stdout, while GOSSAFUNC=Foo would dump only to ssa.html?

SGTM.

@ysmolski
Copy link
Member Author

Cool. I have incorporated this behaviour into the https://go-review.googlesource.com/c/126603/

gopherbot pushed a commit that referenced this issue Aug 22, 2018
@ysmolski @bradfitz
cmd/compile: cache the value of environment variable GOSSAFUNC
e34f660 
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>
gopherbot pushed a commit that referenced this issue Aug 23, 2018
@ysmolski @josharian
cmd/compile: clean the output of GOSSAFUNC
c35069d 
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>
@ysmolski
Copy link
Member Author

Closing it since the solution turned out to be acceptable for the most. Let me repeat the way it was resolved:

GOSSAFUNC=Foo+ tells the compiler to dump to ssa.html and stdout, while GOSSAFUNC=Foo will dump only to ssa.html

Feel free to reopen if you want any additions.

@golang golang locked and limited conversation to collaborators Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
7 participants
@josharian @ysmolski @andybons @dr2chase @randall77 @gopherbot @cherrymui