Revert change to errors.Frame's type

[davecheney]

In #183 the definition of errors.Frame was altered to type Frame runtime.Frame. This was necessary because of the changes to mid stack inlining in Go 1.12 and the deprecation of runtime.FuncForPC.

https://go-review.googlesource.com/c/go/+/156364 suggests that it may be possible to continue to use runtime.FuncForPC. If this CL lands #183 should be mostly reverted for the 1.0 release.

A future v2 release would probably drop the use of runtime.FuncForPC and redeclare errors.Frame as an opaque struct.

[arcana261]

I agree.. Since we can obtain runtime.Frame from PC's we could have just let good ol' Frame stay there. But I don't know whether it's too late to revert this breaking change or not. Other libraries might have adopted this change :(

[davecheney]

.. Since we can obtain runtime.Frame

How would you do that?

As to breaking change on master, I tagged v0.8.1 before I made this change.

[arcana261]

using runtime.CallersFrames? maybe?

Thank you for tagging.. much appreciated ^_^

[arcana261]

we could alter implementation of format like this:

// format allows stack trace printing calls to be made with a bytes.Buffer.
func (ptrFrame Frame) format(w io.Writer, s fmt.State, verb rune) {
	frames := runtime.CallersFrames([]uintptr{uintptr(ptrFrame)})
	if frames == nil {
		return
	}
	f, _ := frames.Next()

....

[davecheney]

The goal is to return to the original type Frame uintptr declaration. This would only work of the runtime.CallersFrames conversion is accurate when passed a single pc, and that still leaves the problem of converting from a uintptr Frame to a runtime.Func to retrieve the name, file, and line.

…

On 6 Jan 2019, at 21:04, Mohamad mehdi Kharatizadeh ***@***.***> wrote: we could alter implementation of format like this: // format allows stack trace printing calls to be made with a bytes.Buffer. func (ptrFrame MyFrame) format(w io.Writer, s fmt.State, verb rune) { frames := runtime.CallersFrames([]uintptr{uintptr(ptrFrame)}) if frames == nil { return } f, _ := frames.Next() .... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[arcana261]

I understand. I was merely pointing out that we could obtain an instance of runtime.Frame this way instead of #183 . And we could use runtime.Frame.Function, runtime.Frame.Line.. etc. And I've confirmed that we can accurately convert a single PC to runtime.Frame.

Complete listing is as follows:
Basically it's #183 without changing type of errors.Frame. Instead a we convert a single PC to runtime.Frame and the rest is still #183

// format allows stack trace printing calls to be made with a bytes.Buffer.
func (ptrFrame Frame) format(w io.Writer, s fmt.State, verb rune) {
	frames := runtime.CallersFrames([]uintptr{uintptr(ptrFrame)})
	if frames == nil {
		return
	}
	f, _ := frames.Next()

	switch verb {
	case 's':
		switch {
		case s.Flag('+'):
			fn := runtime.Frame(f).Func
			if fn == nil {
				io.WriteString(w, "unknown")
			} else {
				file := runtime.Frame(f).File
				io.WriteString(w, fn.Name())
				io.WriteString(w, "\n\t")
				io.WriteString(w, file)
			}
		default:
			file := runtime.Frame(f).File
			if file == "" {
				file = "unknown"
			}
			io.WriteString(w, path.Base(file))
		}
	case 'd':
		io.WriteString(w, strconv.Itoa(runtime.Frame(f).Line))
	case 'n':
		name := runtime.Frame(f).Function
		io.WriteString(s, funcname(name))
	case 'v':
		ptrFrame.format(w, s, 's')
		io.WriteString(w, ":")
		ptrFrame.format(w, s, 'd')
	}
}

[davecheney]

Thanks for your suggestions. Hopefully with the change landing in go 1.12, we’ll be able to revert to using runtime.FuncForPC

…

On 6 Jan 2019, at 21:56, Mohamad mehdi Kharatizadeh ***@***.***> wrote: I understand. I was merely pointing out that we could obtain an instance of runtime.Frame this way instead of #183 . And we could use runtime.Frame.Function, runtime.Frame.Line.. etc. And I've confirmed that we can accurately convert a single PC to runtime.Frame. Complete listing is as follows: Basically it's #183 without changing type of errors.Frame. Instead a we convert a single PC to runtime.Frame and the rest is still #183 // format allows stack trace printing calls to be made with a bytes.Buffer. func (ptrFrame Frame) format(w io.Writer, s fmt.State, verb rune) { frames := runtime.CallersFrames([]uintptr{uintptr(ptrFrame)}) if frames == nil { return } f, _ := frames.Next() switch verb { case 's': switch { case s.Flag('+'): fn := runtime.Frame(f).Func if fn == nil { io.WriteString(w, "unknown") } else { file := runtime.Frame(f).File io.WriteString(w, fn.Name()) io.WriteString(w, "\n\t") io.WriteString(w, file) } default: file := runtime.Frame(f).File if file == "" { file = "unknown" } io.WriteString(w, path.Base(file)) } case 'd': io.WriteString(w, strconv.Itoa(runtime.Frame(f).Line)) case 'n': name := runtime.Frame(f).Function io.WriteString(s, funcname(name)) case 'v': ptrFrame.format(w, s, 's') io.WriteString(w, ":") ptrFrame.format(w, s, 'd') } } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[uberbo]

Is there any reason for using runtime.Frame(f).Func.Name() instead of runtime.Frame(f).Function?

[dcu]

aws/aws-xray-sdk-go#77 is related to this

[davecheney]

Thanks for letting me know. I’m watching the changes to runtime.FuncForPC and will revert the change in pkg/errors if Go 1.12 reverts their changes to func for pc. At the moment the workaround is to use v0.8.1 which was tagged before the change to errors.Frame.

…

On 7 Jan 2019, at 23:08, Bo Sunesen ***@***.***> wrote: Is there any reason for using runtime.Frame(f).Func.Name() instead of runtime.Frame(f).Function? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[davecheney]

Closed in #193