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: reports wrong line number when calling a method in multiple lines #22083

Closed
yhuang0 opened this Issue Sep 28, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@yhuang0

yhuang0 commented Sep 28, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.9

Does this issue reproduce with the latest release?

Yes

What did you do?

Use method to access array in struct.
https://play.golang.org/p/oJ_Uh9hGN0

What did you expect to see?

I expected the error in the stack trace to point to line 16.

What did you see instead?

Stack trace pointed to line 13 instead, the first invocation of Get()

@mvdan

This comment has been minimized.

Member

mvdan commented Sep 28, 2017

Reproduces on master.

Here's an interesting twist: https://play.golang.org/p/8G8BxckMZq

It seems to use the first method call as the position, so in the case above it uses the first GetB call, not the first GetA call. Probably the compiler recording the line number incorrectly, perhaps because it reuses something between these calls?

/cc @mdempsky

@mvdan mvdan added this to the Go1.10 milestone Sep 28, 2017

@mvdan mvdan added the NeedsFix label Sep 28, 2017

@mvdan mvdan changed the title from Stack Trace inaccurate? to cmd/compile: reports wrong line number when calling a method in multiple lines Sep 28, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Oct 2, 2017

Change https://golang.org/cl/67470 mentions this issue: cmd/compile: Multiple invocations of an inlined function generate distinct panics

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 2, 2017

An incorrect traceback can be very confusing, so marking this as a 1.9.1 candidate.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.9.1 Oct 2, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Oct 2, 2017

Change https://golang.org/cl/67632 mentions this issue: cmd/compile: fix merge rules for panic calls

@gopherbot gopherbot closed this in 41eabc0 Oct 3, 2017

@randall77

This comment has been minimized.

Contributor

randall77 commented Oct 3, 2017

Reopening for 1.9.1 (or 1.9.2?)

@randall77 randall77 reopened this Oct 3, 2017

@rsc rsc modified the milestones: Go1.9.1, Go1.9.2 Oct 4, 2017

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 13, 2017

This seems like a pretty subtle change. The win here is that some line numbers get fixed. Is that worth the risk? Should we just leave this for Go 1.10?

@randall77

This comment has been minimized.

Contributor

randall77 commented Oct 13, 2017

It's not the end of the world if doesn't get in 1.9. It's just backtrace line numbers, and needs a lot of things to happen to trigger it (inlining, bounds check fail or divide by zero, multiple identical call sites).

Two arguments in its favor:

  • It is a regression from 1.8
  • The fix is 2 lines
@rsc

This comment has been minimized.

Contributor

rsc commented Oct 13, 2017

The fix is 2 lines but it affects the generated code, right? Because panics that were merged now are not. That could in turn affect other optimizations and might stir other issues around, right?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 13, 2017

I think this is fairly important for 1.9, actually. The incorrect tracebacks have already led people to spend time poring over the wrong code. When the stack trace points you directly at a specific line, it's hard to understand that the problem is actually at a completely different, possibly quite distant, line in the same function.

@randall77

This comment has been minimized.

Contributor

randall77 commented Oct 13, 2017

@rsc Yes, it does affect the generated code. I'm not particularly worried about not merging two calls that we used to merge. That's the normal state, having unmergeable calls. That path is very well exercised.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 14, 2017

CL 67632 OK for Go 1.9.2.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 15, 2017

Change https://golang.org/cl/70980 mentions this issue: [release-branch.go1.9] cmd/compile: fix merge rules for panic calls

gopherbot pushed a commit that referenced this issue Oct 25, 2017

[release-branch.go1.9] cmd/compile: fix merge rules for panic calls
Use entire inlining call stack to decide whether two panic calls
can be merged. We used to merge panic calls when only the leaf
line numbers matched, but that leads to places higher up the call
stack being merged incorrectly.

Fixes #22083

Change-Id: Ia41400a80de4b6ecf3e5089abce0c42b65e9b38a
Reviewed-on: https://go-review.googlesource.com/67632
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-on: https://go-review.googlesource.com/70980
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@rsc

This comment has been minimized.

Contributor

rsc commented Oct 26, 2017

go1.9.2 has been packaged and includes:

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:09:19 UTC

@rsc rsc closed this Oct 26, 2017

@golang golang locked and limited conversation to collaborators Oct 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.