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

[mdempsky]

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

[josharian]

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]

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]

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

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

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

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

[mdempsky]

@josharian I can look into it.

[gopherbot]

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

[gopherbot]

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

[josharian]

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

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

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

[gopherbot]

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

[gopherbot]

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

[ALTree]

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]

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

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]

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

[mdempsky]

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.

[ALTree]

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]

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

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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

[gopherbot]

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

[cuonglm]

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

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]

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]

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

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]

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