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

proposal: runtime: rationalize runtime.Errors #14443

Open
aclements opened this Issue Feb 21, 2016 · 8 comments

Comments

Projects
None yet
9 participants
@aclements
Member

aclements commented Feb 21, 2016

Package runtime defines an Error interface for runtime errors and exactly one exported type that implements it: TypeAssertionError. But there are about a half dozen other panics in the runtime caused by programmer errors that are detected at runtime; things like "index out of range" and "divide by zero". Currently these are just string panics, which means they're inconsistent with TypeAssertionError and contain no detail about, for example, what the index was or what range it was out of. We should make these panics raise types that implement runtime.Error and carry actual details about the error.

This is a potentially breaking change, albeit a weak one, since programs can check the string returned by the Error() method to detect these panics (and since this is the only way to detect them right now, I suspect this happens in practice). We could introduce exported types for these panics without breaking compatibility, but can't add more information to their Error() message.

@aclements aclements added the Go2 label Feb 21, 2016

@aclements aclements added this to the Unplanned milestone Feb 21, 2016

@randall77

This comment has been minimized.

Contributor

randall77 commented Feb 21, 2016

Adding the index and the range will increase the size of the binary, as we will then have to add code to pass the index and size to the runtime for every bounds check.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Feb 22, 2016

@randall77, instead of passing the index and size and bloating the code, could the panicindex function walk up the stack and know which registers/offsets to pull those two numbers from?

@randall77

This comment has been minimized.

Contributor

randall77 commented Feb 22, 2016

@bradfitz Maybe. It would require a custom calling convention.

We typically do:

CMP index, bound
JAE fail

fail:
CALL panicindex

So inside panicindex the stack doesn't tell you what registers the index and bound are in. Or even if they are in registers; one of them may be a constant or be in memory.
If we forced both index and bounds to be in registers, maybe we could add functions like panicindex_rax_rcx - they could save the named registers somewhere before using standard Go to construct the panic message.

It's certainly a whole lot easier to just do:

fail:
MOV index, 0(SP)
MOV bound, 8(SP)
CALL panicindex

Just a question of whether that extra code is worth it.

@jeffallen

This comment has been minimized.

Contributor

jeffallen commented Mar 13, 2017

Could panicindex use arch-specific code to decode the CMP (find it by looking up the stack one frame) in order to find the index? This could be incrementally implemented: for architectures not yet decoding the CMP instruction, continue to not report the index.

Linux on MIPS is able to fixup unaligned accesses by decoding the offending instruction and emulating it in software (https://www.linux-mips.org/wiki/Alignment#Transparent_fixing_by_the_kernel). This would be the same idea: in the normal case (index in bounds) the code is unchanged from the optimal code, and in the failure case extra effort is expended to go figure out what went wrong.

@dantoye

This comment has been minimized.

dantoye commented Mar 13, 2017

Personally I say bloat the code as much as you want if it leads to more helpful error's.

@randall77

This comment has been minimized.

Contributor

randall77 commented Mar 13, 2017

@jeffallen It isn't easy to find the CMP. The panicindex is out of line and there is no record of where the branch to it came from.
In my example above there could be multiple JAE fail instructions - how do you know which one went to the panicindex?
We could solve this by not combining panicindex calls. That wouldn't be too bad, they only get combined within a source line anyway. Also, we could put the panicindex inline so we wouldn't have to scan the whole function to find it.
Still, this is tricky to get right. Walking backwards in an x86 instruction stream is hard. There may be no CMP (the compiler may have proved it will always fail. It may use a SUB to get the same result (#17638), ...).

@mdempsky

This comment has been minimized.

Member

mdempsky commented Mar 13, 2017

As a representative example, there are 4747 calls to panicslice/panicindex in cmd/go. Two extra "MOV reg1, (SP); MOV reg2, 8(SP)" instructions on amd64 would cost about 9 bytes. So that's about 43k extra bytes of program overhead so that errors (or at least back traces) can include index/length information.

cmd/go is about 10.7MB large, so that would amount to an executable file size increase of 0.4%.

@navytux

This comment has been minimized.

Contributor

navytux commented Mar 26, 2017

One more use case for this would be: having details on SIGBUS received when accessing mmaped file if actual IO caused by access ended with error. Specifically the details would allow to turn SIGBUS into EIO error with information about problematic offset in the file.

More context: https://groups.google.com/d/msg/golang-nuts/11rdExWP6ac/226CPanVBAAJ

@rsc rsc changed the title from runtime: rationalize runtime.Errors to proposal: runtime: rationalize runtime.Errors Jun 17, 2017

@gopherbot gopherbot added the Proposal label Jun 17, 2017

@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Jan 9, 2018

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