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

cmd/compile: eliminate base.Pos and ir.CurFunc #19683

Open
mdempsky opened this issue Mar 23, 2017 · 37 comments
Open

cmd/compile: eliminate base.Pos and ir.CurFunc #19683

mdempsky opened this issue Mar 23, 2017 · 37 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Contributor

This is an umbrella issue for eliminating the global lineno variable from package gc. Filing as a HelpWanted issue because I think it's a relatively low-barrier-to-entry issue that people can usefully contribute to the compiler with small incremental improvements.

Here's the general algorithm:

  1. Identify a function Bar that uses lineno.
  2. Split it into two functions Bar and Barl; Barl takes an additional pos src.XPos parameter, and Bar just calls Barl with lineno.
  3. Change callers of Bar to use Barl. Ideally, the caller can supply the position information directly (e.g., via n.Pos or something); but if necessary, just use lineno and later we'll recursively apply this procedure on the caller function.

An example of this is yyerror and Warn now have yyerrorl and `Warnl' functions that we're trying to use instead.

Note: sometimes uses of lineno should just go away entirely (e.g., CL 38393 / 80c4b53). I suggest pinging here to discuss instances and/or express interest in working on this to avoid duplicating work (e.g., @josharian and I are looking at this in the backend for #15756).

@mdempsky mdempsky added this to the Go1.9Maybe milestone Mar 23, 2017
@josharian
Copy link
Contributor

One caveat. If the function in question calls nod, then it might need to accept both pos srx.XPos and curfn *Node, since both those globals are used in nod. Just as lineno can be the default value of the pos arg, the global Curfn can be the default value of the curfn arg.

I'll send an example CL of this soon, which also introduces nodl.

Unless, @mdempsky, you dislike this, in which case we need another strategy. :)

@griesemer
Copy link
Contributor

Presumably many functions don't need an extra src.XPos because they can get the info from any of the nodes they are working with?

@mdempsky
Copy link
Contributor Author

@josharian I was thinking about that, and I think we can push the Curfn logic into callers. We don't actually need Curfn for OPACK or OLABEL, just ONAME and for setting IsHiddenClosure.

I think ONAME can be handled by refactoring all the n := nod(ONAME, nil, nil); n.Sym = s instances to n := newname(s).

The IsHiddenClosure logic can probably just be set as appropriate in closure.go instead.

@mdempsky
Copy link
Contributor Author

@griesemer Yeah, if the position information can be obtained using existing parameters, that's preferable. But there are a lot of helper functions in dcl.go that construct Nodes, though none of their parameters include position information currently. Some of those will probably need to be extended (e.g., newname?), and others can probably just use src.NoPos (e.g., typenod?).

@josharian
Copy link
Contributor

I think we can push the Curfn logic into callers. ...

SGTM. Do you want to tackle this set of things, or shall I?

@mdempsky
Copy link
Contributor Author

@josharian I can look into it.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38592 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38597 mentions this issue.

@josharian
Copy link
Contributor

@mdempsky nod/Curfn is top of my queue now. Looks like the remaining item there is:

I think ONAME can be handled by refactoring all the n := nod(ONAME, nil, nil); n.Sym = s instances to n := newname(s).

Are you actively working on this? If not, I will start on it.

@mdempsky
Copy link
Contributor Author

@josharian I've got a CL for it, then got side tracked pondering what Node.Addable signifies and whether we could get rid of it.

I'll finish up the CL and mail it in just a sec.

gopherbot pushed a commit that referenced this issue Mar 25, 2017
Concurrent compilation requires providing an
explicit position and curfn to temp.
This implementation of tempAt temporarily
continues to use the globals lineno and Curfn,
so as not to collide with mdempsky's
work for #19683 eliminating the Curfn dependency
from func nod.

Updates #15756
Updates #19683

Change-Id: Ib3149ca4b0740e9f6eea44babc6f34cdd63028a9
Reviewed-on: https://go-review.googlesource.com/38592
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38770 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 28, 2017
Updates #19683

Change-Id: Ic00d5a9807200791cf37553f4f802dbf27beea19
Reviewed-on: https://go-review.googlesource.com/38770
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/38735 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/39191 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 31, 2017
newnamel is newname but with no dependency on lineno or Curfn.
This makes it suitable for use in a concurrent back end.
Use it now to make tempAt global-free.

The decision to push the assignment to n.Name.Curfn
to the caller of newnamel is based on mdempsky's
comments in #19683 that he'd like to do that
for callers of newname as well.

Passes toolstash-check. No compiler performance impact.

Updates #19683
Updates #15756

Change-Id: Idc461a1716916d268c9ff323129830d9a6e4a4d9
Reviewed-on: https://go-review.googlesource.com/39191
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit that referenced this issue Apr 1, 2017
Replace yyerror usages with yyerrorl in function
typecheckswitch.

Updates #19683.

Change-Id: I7188cdecddd2ce4e06b8cee45b57f3765a979405
Reviewed-on: https://go-review.googlesource.com/38597
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ALTree
Copy link
Member

ALTree commented Apr 6, 2017

I'm looking into moving const.go to yyerrorl (there are about a dozen
yyerror calls). A few of those calls are from the toXXX functions, for
example

func toflt(v Val) Val

which takes a Val and cast it to a Mpflt Val. Since those functions
operate on Vals and they never receive a Node, they have no way to
recover position information. I see two options:

  • Change the toXXX functions so that they signal problems via return
    values and move the yyerror calls to the callers of the toXXX functions

  • Add an src.Xpos parameter to the toXXX functions.

Opinions?

@mdempsky
Copy link
Contributor Author

mdempsky commented Apr 6, 2017

@ALTree I think either of those options sounds reasonable. I suspect threading src.XPos is a bit simpler.

If you're feeling adventurous, you can take a look at how go/constant and go/types handle this. E.g., I notice go/constant's corresponding functions (ToInt, ToFloat, etc) just return an invalid value (Unknown), rather than reporting errors internally.

@ALTree
Copy link
Member

ALTree commented Apr 12, 2017

Biggest difficulty I'm having right now (looking at typechecking, but
it's the same in const.go) is that we receive a lot of nodes without
position information.

@mdempsky wrote:

But there are a lot of helper functions in dcl.go that construct
Nodes, though none of their parameters include position information
currently.

But I think there's an even more annoying problem: the fact that for
things like _ or nil we pass always the same node around (you
notice this if you print the *Node address in typecheck while
compiling for example

func main() {
  _()
  _ = 1
}

The two _ nodes have the same address) so using n.Pos in place of
lineno it's pretty much impossible. So either we start generating a
new node for every _ occurrence (ugh) or we'll need to pass src.XPos
to every typechecking function, and find out the position of the _ node
in expr like _() from the context (parent node?).

Currently exploring the latter option.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/40500 mentions this issue.

@mdempsky
Copy link
Contributor Author

So either we start generating a new node for every _ occurrence

I started down this path here: https://go-review.googlesource.com/c/38735/

It had a more substantial performance hit than I was expecting though. I haven't had a chance to investigate why. (Maybe it's really just due to allocating more Nodes.)

Note though that typecheck discards OPARENs, so that solution still loses fine-grained position information for later on in the compiler.

@neelance is currently experimenting in the opposite direction: allocating extra ONAME Nodes, but sharing their underlying Name fields.

lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
Updates golang#19683

Change-Id: Ic00d5a9807200791cf37553f4f802dbf27beea19
Reviewed-on: https://go-review.googlesource.com/38770
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
newnamel is newname but with no dependency on lineno or Curfn.
This makes it suitable for use in a concurrent back end.
Use it now to make tempAt global-free.

The decision to push the assignment to n.Name.Curfn
to the caller of newnamel is based on mdempsky's
comments in golang#19683 that he'd like to do that
for callers of newname as well.

Passes toolstash-check. No compiler performance impact.

Updates golang#19683
Updates golang#15756

Change-Id: Idc461a1716916d268c9ff323129830d9a6e4a4d9
Reviewed-on: https://go-review.googlesource.com/39191
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
Replace yyerror usages with yyerrorl in function
typecheckswitch.

Updates golang#19683.

Change-Id: I7188cdecddd2ce4e06b8cee45b57f3765a979405
Reviewed-on: https://go-review.googlesource.com/38597
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ALTree
Copy link
Member

ALTree commented Apr 23, 2017

While looking at the typechecker I noticed pos info for a few errors is not currently tested. They're not hard to find

  • Replace a yyerror call it with yyerrorl(src.NoXPos, ..), thus destroying pos info in the error
  • Run the tests
  • If nothing fails the call is not tested.

I uploaded a change that adds pos info tests for error messages in the typecheker for every untested yyerror call I could find (and trigger): https://go-review.googlesource.com/c/41477/

Looking for reviewers. This will be useful regardless of the path for lineno removal you choose.

gopherbot pushed a commit that referenced this issue Apr 24, 2017
This change adds line position tests for several yyerror calls in the
typechecker that are currently not tested in any way.

Untested yyerror calls were found by replacing them with

  yerrorl(src.NoXPos, ...)

(thus destroying position information in the error), and then running
the test suite. No failures means no test coverage for the relevant
yyerror call.

For #19683

Change-Id: Iedb3d2f02141b332e9bfa76dbf5ae930ad2fddc3
Reviewed-on: https://go-review.googlesource.com/41477
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/41477 mentions this issue.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/69910 mentions this issue: cmd/compile: use yyerrorl in checkmake

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/70251 mentions this issue: cmd/compile: add two error position tests for the typechecker

gopherbot pushed a commit that referenced this issue Oct 12, 2017
Follow CL 41477 and add two more line position tests for yyerror calls
in the typechecker which are currently not tested.

Update #19683

Change-Id: Iacd865195a3bfba87d8c22655382af267aba47a9
Reviewed-on: https://go-review.googlesource.com/70251
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/80298 mentions this issue: cmd/compile: use NoXPos instead of lineno in typenod

gopherbot pushed a commit that referenced this issue Nov 28, 2017
typenod is only used for anonymous types, which don't logically have
position information.

Passes toolstash-check.

Updates #19683.

Change-Id: I505424ae980b88c7deed5f23502c3cecb3dc0702
Reviewed-on: https://go-review.googlesource.com/80298
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@mdempsky mdempsky modified the milestones: Go1.10, Go1.11 Nov 29, 2017
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/100335 mentions this issue: cmd/compile: use nodl in zeroResults

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 13, 2018
quasilyte added a commit to quasilyte/go-contributing-ru that referenced this issue Apr 1, 2018
New tasks include:
golang/go#19675 cmd/vet: report uses of -0 in float32/64 context
golang/go#19683 cmd/compile: eliminate usages of global lineno
golang/go#19670 x/tools/go/ssa: make opaqueType less annoying to use
golang/go#19636 encoding/base64: decoding is slow
golang/go#23471 x/perf/cmd/benchstat: tips or quickstart for newcomers
golang/go#19577 test: errorcheck support for intraline errors
golang/go#19490 cmd/vet: reduce the amount of false positives for -shadow mode
golang/go#19042 cmd/internal/obj: optimize wrapper method prologue for branch prediction
golang/go#19013 cmd/compile: add tool for understanding/debugging SSA rules
gopherbot pushed a commit that referenced this issue May 8, 2018
Use nodl instead of nod to avoid setting and resetting lineo.

Passes toolstash-check.

Updates #19683

Change-Id: I6a47a7ba43a11352767029eced29f08dff8501a2
Reviewed-on: https://go-review.googlesource.com/100335
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@bradfitz bradfitz modified the milestones: Go1.11, Unplanned May 18, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/114875 mentions this issue: cmd/compile: use embedlineno instead of lineno in copytype

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/114915 mentions this issue: cmd/compile: use yyerrorl(n.Pos, ...) in typecheckdef

gopherbot pushed a commit that referenced this issue Aug 22, 2018
Also remove lineno from typecheckdeftype since copytype was
the only user of it and typecheck uses lineno independently.

toolstach-check passed.

Updates #19683.

Change-Id: I1663fdb8cf519d505cc087c8657dcbff3c8b1a0a
Reviewed-on: https://go-review.googlesource.com/114875
Run-TryBot: Yury Smolsky <yury@smolsky.by>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit that referenced this issue Sep 11, 2018
n.Pos.IsKnown() is not needed because it is performed in setlineno.

toolstash-check passed.

Updates #19683.

Change-Id: I34d6a0e6dc9970679d99e8f3424f289ebf1e86ba
Reviewed-on: https://go-review.googlesource.com/114915
Run-TryBot: Yury Smolsky <yury@smolsky.by>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@cuonglm
Copy link
Member

cuonglm commented Apr 25, 2019

@mdempsky I'm going to change Fatalf to get srx.Pos as argument. Fatalfl will be consistent with other functions, but how about FatalfAt?

For me, the later seems to be more meaningful.

@mdempsky
Copy link
Contributor Author

mdempsky commented May 3, 2019

FatalfAt seems a bit awkward to me (I thought there's a linter that wants fmt-style functions to be suffixed with f?), but it's the least bad option I can think of. And not having a src.XPos-variant of Fatalf has been a definite drag in addressing this issue. So SGTM unless someone can suggest a clearly better alternative.

@josharian
Copy link
Contributor

If we simultaneously change all instances of Fatalf, then we could just leave it called Fatalf. If we want to do it incrementally, FatalfAt seems fine, and we can rename once the migration is complete.

@cuonglm
Copy link
Member

cuonglm commented May 4, 2019

@josharian

If we want to do it incrementally, FatalfAt seems fine, and we can rename once the migration is complete.

Seems a bit easier to do it incrementally. There're other package depend on gc.Fatalf, at least types.Fatalf. There can be more that I don't aware of.

Also I see many func/method in types package, example this one does call Fatalf before a return. Is the call there for debugging purpose? it can be easier to change Fatalf signature if we can eliminate all dependencies of other packages.

@mdempsky
Copy link
Contributor Author

mdempsky commented May 4, 2019

Seems a bit easier to do it incrementally. There're other package depend on gc.Fatalf, at least types.Fatalf. There can be more that I don't aware of.

Incremental is totally fine, but FYI cmd is entirely self-contained. As long as "go test cmd" builds and passes, you've found all of them.

gofmt -r and golang.org/x/tools/cmd/eg are both very effective for working within cmd.

Also I see many func/method in types package, example this one does call Fatalf before a return. Is the call there for debugging purpose?

Fatalf is like panic and never returns.

But unlike panic, the compiler doesn't know that Fatalf doesn't return, and callers still need to satisfy type checking, hence the nil return.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/188539 mentions this issue: cmd/compile: eliminate usage of global Fatalf in ssa.go

gopherbot pushed a commit that referenced this issue Aug 27, 2019
state and ssafn both have their own Fatalf, so use them instead of
global Fatalf.

Updates #19683

Change-Id: Ie02a961d4285ab0a3f3b8d889a5b498d926ed567
Reviewed-on: https://go-review.googlesource.com/c/go/+/188539
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
state and ssafn both have their own Fatalf, so use them instead of
global Fatalf.

Updates golang#19683

Change-Id: Ie02a961d4285ab0a3f3b8d889a5b498d926ed567
Reviewed-on: https://go-review.googlesource.com/c/go/+/188539
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
state and ssafn both have their own Fatalf, so use them instead of
global Fatalf.

Updates golang#19683

Change-Id: Ie02a961d4285ab0a3f3b8d889a5b498d926ed567
Reviewed-on: https://go-review.googlesource.com/c/go/+/188539
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
gopherbot pushed a commit that referenced this issue Sep 26, 2019
Replace usage of yyerror with yyerrorl in checkdefergo and copytype in
typecheck.go.

All covered error messages already appear in the tests and the yyerror
replacement did not lead to any tests failing.

Passes toolstash-check

Updates #19683

Change-Id: I735e83bcda7ddc6a14afb22e50200bcbb9192fc4
Reviewed-on: https://go-review.googlesource.com/c/go/+/69910
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mdempsky mdempsky changed the title cmd/compile: eliminate lineno cmd/compile: eliminate base.Pos Jan 25, 2021
@mdempsky mdempsky changed the title cmd/compile: eliminate base.Pos cmd/compile: eliminate base.Pos and ir.CurFunc Jan 25, 2021
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants