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 lineno #19683

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

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 23, 2017

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

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Mar 23, 2017

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

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Mar 23, 2017

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

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Mar 23, 2017

@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

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Mar 23, 2017

@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

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Mar 23, 2017

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

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

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Mar 23, 2017

@josharian I can look into it.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 24, 2017

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 24, 2017

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

@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Mar 24, 2017

@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

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Mar 24, 2017

@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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 28, 2017

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 28, 2017

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 31, 2017

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

This comment has been minimized.

Copy link
Member

@ALTree 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

This comment has been minimized.

Copy link
Member Author

@mdempsky 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

This comment has been minimized.

Copy link
Member

@ALTree 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 12, 2017

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

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Apr 12, 2017

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 added 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 added 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 added 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

This comment has been minimized.

Copy link
Member

@ALTree 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 25, 2017

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 19, 2017

Change https://golang.org/cl/64791 mentions this issue: cmd/compile: eliminate some Node AST uses in methodfunc

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 10, 2017

Change https://golang.org/cl/69690 mentions this issue: cmd/compile: eliminate some lineno uses

gopherbot pushed a commit that referenced this issue Oct 11, 2017
Focused on ranges, selects and switches for this one.

While at it, simplify some vars in typecheckselect.

Updates #19683.

Change-Id: Ib6aabe0f6826cb1930483aeb4bb2de1ff8052d9e
Reviewed-on: https://go-review.googlesource.com/69690
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 11, 2017

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 12, 2017

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 28, 2017

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 13, 2018

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

@andybons andybons added the NeedsFix 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 28, 2018

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 28, 2018

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

This comment has been minimized.

Copy link
Contributor

@cuonglm 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

This comment has been minimized.

Copy link
Member Author

@mdempsky 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

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented May 4, 2019

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

This comment has been minimized.

Copy link
Contributor

@cuonglm 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

This comment has been minimized.

Copy link
Member Author

@mdempsky 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 2, 2019

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 added 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 added 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.