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: FuncForPC not returning correct function #58300

Closed
randall77 opened this issue Feb 3, 2023 · 8 comments
Closed

cmd/compile: FuncForPC not returning correct function #58300

randall77 opened this issue Feb 3, 2023 · 8 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@randall77
Copy link
Contributor

package main

import (
	"reflect"
	"runtime"
)

func f(n int) int {
	return n % 2
}

func g(n int) int {
	return f(n)
}

func name(fn any) (res string) {
	return runtime.FuncForPC(uintptr(reflect.ValueOf(fn).Pointer())).Name()
}

func main() {
	println(name(f))
	println(name(g))
}

Prints

main.f
main.f

where it should print

main.f
main.g

This happens because the first instruction of g is an instruction inlined from f.

This came up recently because the scheduler change https://go-review.googlesource.com/c/go/+/270940 subtly changes the ordering, and line numbering, of assembly instructions at the start of functions.

@randall77 randall77 added this to the Go1.21 milestone Feb 3, 2023
@randall77 randall77 self-assigned this Feb 3, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 3, 2023
@gopherbot
Copy link

Change https://go.dev/cl/465076 mentions this issue: cmd/compile: mark the initial memory with the function start position

@cherrymui
Copy link
Member

FuncForPC is documented as "If pc represents multiple functions because of inlining, it returns the *Func describing the innermost function, but with an entry of the outermost function."

So if the PC of the first instruction is from an inlined function, returning the inner function seems reasonable?

@randall77
Copy link
Contributor Author

randall77 commented Feb 3, 2023

Yes, it is reasonable. But people are using FuncForPC(reflect.ValueOf(fn).Pointer()) to get from a function closure to the name of the function it implements. It's unfortunate, but that's what people are doing, and marking the first instruction as from an inlined callee makes that pattern not work.
I think we can make sure the first instruction is not from anything inlined. For most cases, it's easy - it is only leaf functions calling inlined leaf functions where it matters. All others have a prologue with the right function name. So we just need to do something for this particular case. The obvious choices are to either lie about the function name for the first instruction, or add a nop before the body labeled with the right function name.

@thediveo
Copy link

thediveo commented Feb 4, 2023

Out of curiosity: what is the correct way to handle the result returned from FuncForPC in such inlining situations?

@randall77
Copy link
Contributor Author

@thediveo You could use it for, e.g., a sampling profiler. Just take the result of FuncForPC and increment a counter somewhere based on the results.

I think the more relevant question here is, what is the correct way to handle the result returned from refelect.Pointer when passed a func type. Currently the answer to that is, "compare to 0" and not much else.

@thediveo
Copy link

thediveo commented Feb 4, 2023

@randall77 I probably didn't pose my question well: I wasn't asking for what I would need this information, as I've used it several times in the past in my own projects, such as a simplistic plugin-manager that derives package names from the caller's PC. What I was trying to ask is: is there a way to correctly detect the situation brought up in this issue in existing code and to properly handle it as to not take the inlined code as source, but the function where the inline code was put at the beginning?

@randall77
Copy link
Contributor Author

@thediveo Not with FuncForPC. At least until/if this issue is fixed.

You can get the right answer, even in the presence of inlining, by using runtime.Callers + runtime.CallersFrames. It sounds like if you're using the caller's PC, that's the right strategy for you. And by that I mean, don't get the caller's PC some other way (runtime.Caller? Not sure how you're getting caller PCs) - it is provided implicitly in the result of runtime.Callers.

This issue is about getting the name of a function from a func that has not yet been called, which if I understand correctly is not your issue.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 4, 2023
johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
People are using this to get the name of the function from a function type:

runtime.FuncForPC(reflect.ValueOf(fn).Pointer()).Name()

Unfortunately, this technique falls down when the first instruction
of the function is from an inlined callee. Then the expression above
gets you the name of the inlined function instead of the function itself.

To fix this, ensure that the first instruction is never from an inlinee.
Normally functions have prologs so those are already fine. In just the
cases where a function is a leaf with no local variables, and an instruction
from an inlinee appears first in the prog list, add a nop at the start
of the function to hold a non-inlined position.

Consider the nop a "mini-prolog" for leaf functions.

Fixes golang#58300

Change-Id: Ie37092f4ac3167fe8e5ef4a2207b14abc1786897
Reviewed-on: https://go-review.googlesource.com/c/go/+/465076
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/467977 mentions this issue: cmd/compile: ensure FuncForPC works on closures that start with NOPs

gopherbot pushed a commit that referenced this issue Mar 3, 2023
A 0-sized no-op shouldn't prevent us from detecting that the first
instruction is from an inlined callee.

Update #58300

Change-Id: Ic5f6ed108c54a32c05e9b2264b516f2cc17e4619
Reviewed-on: https://go-review.googlesource.com/c/go/+/467977
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants