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: do not report internal compiler error after ordinary compiler error #22252

Closed
rsc opened this issue Oct 13, 2017 · 9 comments
Closed
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Oct 13, 2017

For #21882, instead of checking in the actual fix, I'd like to just silence any "internal compiler error" that happens after a regular compiler error has already been printed. The old C version of the compiler did this and it was quite effective. This change would be very low risk, in contrast to trying to fix actual errors.

@mdempsky
Copy link
Contributor

Currently, we have code in Fatalf to only print stack traces for dev builds, and to instead just print a message asking users to file a bug report for release builds.

Is the issue that behavior isn't working as intended, or that you don't think we should even do that if we've printed an actual error first?

@mdempsky
Copy link
Contributor

Yeah, looks like that code is broken.

@mdempsky
Copy link
Contributor

Changing Fatalf to check for "go" instead of "release" changes #21882 to print:

/tmp/q.go:3:6: invalid recursive type N
/tmp/q.go:3:6: internal compiler error: invalid alignment for N

Please file a bug report including a short program that triggers the error.
https://golang.org/issue/new

Is that desirable, or do you prefer we squelch the error altogether since there was a proper error?

@rsc
Copy link
Contributor Author

rsc commented Oct 13, 2017

Please silence the whole thing. If there was already an error reported and then the compiler shoots itself in the foot, better to just let the user fix the reported error and try again. If no error was reported yet, then of course print the internal error.

This logic was invaluable when Dmitriy was fuzzing the compiler. It just eliminates a whole class of mostly useless bug reports. :-)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/70850 mentions this issue: cmd/compile: omit ICE diagnostics after normal error messages

@mdempsky
Copy link
Contributor

Reopening for Go 1.9.x.

@mdempsky mdempsky reopened this Oct 14, 2017
@rsc rsc added the CherryPickApproved Used during the release process for point releases label Oct 14, 2017
@rsc
Copy link
Contributor Author

rsc commented Oct 14, 2017

CL 22252 OK for Go 1.9.2. (not the right number)
CL 70850 OK for Go 1.9.2.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/70985 mentions this issue: [release-branch.go1.9] cmd/compile: omit ICE diagnostics after normal error messages

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

After we detect errors, the AST is in a precarious state and more
likely to trip useless ICE failures. Instead let the user fix any
existing errors and see if the ICE persists.  This makes Fatalf more
consistent with how panics are handled by hidePanic.

While here, also fix detection for release versions: release version
strings begin with "go" ("go1.8", "go1.9.1", etc), not "release".

Fixes #22252.

Change-Id: I1c400af62fb49dd979b96e1bf0fb295a81c8b336
Reviewed-on: https://go-review.googlesource.com/70850
Run-TryBot: Matthew Dempsky <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
Reviewed-on: https://go-review.googlesource.com/70985
Run-TryBot: Russ Cox <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
@rsc
Copy link
Contributor Author

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:22 UTC

@rsc rsc closed this as completed 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.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants