Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

Revert change to errors.Frame's type #188

Closed
davecheney opened this issue Jan 6, 2019 · 11 comments
Closed

Revert change to errors.Frame's type #188

davecheney opened this issue Jan 6, 2019 · 11 comments
Milestone

Comments

@davecheney
Copy link
Member

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
Copy link

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
Copy link
Member Author

.. 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
Copy link

using runtime.CallersFrames? maybe?

Thank you for tagging.. much appreciated ^_^

@arcana261
Copy link

arcana261 commented Jan 6, 2019

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
Copy link
Member Author

davecheney commented Jan 6, 2019 via email

@arcana261
Copy link

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
Copy link
Member Author

davecheney commented Jan 6, 2019 via email

@uberbo
Copy link

uberbo commented Jan 7, 2019

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

@dcu
Copy link

dcu commented Jan 8, 2019

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

@davecheney
Copy link
Member Author

davecheney commented Jan 8, 2019 via email

@davecheney
Copy link
Member Author

Closed in #193

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants