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.

Sign up for GitHub

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

Jump to bottom

cmd/link: inlinedCall entries missing FuncInfo-based fields (FuncID) in -linkshared mode #55954

Open
prattmic opened this issue Sep 29, 2022 · 4 comments
Open

cmd/link: inlinedCall entries missing FuncInfo-based fields (FuncID) in -linkshared mode #55954

prattmic opened this issue Sep 29, 2022 · 4 comments
Assignees
@prattmic
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@prattmic
Copy link
Member

Follow-up to #54959 with another failing case.

When constructing runtime.inlinedCall entries, we fetch the inlined function FuncID from the FuncInfo.

When building in -linkshared mode, if the inlined function is from the standard library (e.g., sync/atomic.(*Int64).Add), then it is an "external" symbol and Loader.FuncInfo will find no FuncInfo because we don't load an auxs for external symbols (that would likely happen here, though I don't think the ELF contains the auxs at all.

The result is that the inlinedCall is left with the default FuncID (FuncID_normal), even if that is incorrect.

Note that this is not a problem for the other FuncInfo users, notably the pcsp, pcfile, pcline, etc tables. Those ultimately end up in the moduledata. There are multiple moduledatas: one for libstd.so and one for the main binary. Linking of libstd.so has access to all the FuncInfos it needs to construct the moduledata for libstd.so. It is only a problem for inlinedCall because we need cross-module data.

There are two ways I can see of addressing this:

  1. Ensure FuncInfo availability for external symbols. Since the symbol aux data is not directly available in libstd.so, this could be done by reverse engineering it from the shared object moduledata symbol (blegh), or (I believe) in addition to to the packageshlib pointing to libstd.so, the importcfg also includes the packagefile with the original package Go object file. We could reach into that file just for the symbol aux data.

  2. Eliminate the FuncInfo-based data from inlinedCall. If we ensure that every inlined function has a _func in the function table, then at runtime we could lookup the _func from the inlined symbol name and get the metadata from there. For cross-module inlining, this means that the linker never needs to know the metadata at all.

cc @mdempsky @cherrymui
@prattmic prattmic added this to the Backlog milestone Sep 29, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 29, 2022
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 29, 2022
@gopherbot
Copy link

Change https://go.dev/cl/429637 mentions this issue: cmd/link/internal/ld: panic if inlined functions missing FuncInfo

@cherrymui cherrymui modified the milestones: Backlog, Unplanned Sep 30, 2022
@cherrymui
Copy link
Member

cherrymui commented Sep 30, 2022 •
edited

I'm not sure it is actually a problem. Most of the special funcIDs are runtime internal. They are probably never inlined outside of the runtime. Then there is FuncID_wrapper. Wrapper functions are dupOK, therefore always have a local definition in the current package (therefore DSO).

Also, I'm not sure inlining across DSO boundary is well defined anyway (currently we do it, but maybe it isn't actually desired). One idea of a shared object is that it can be replaced without replacing other DSOs in the same program. If we inline across DSO boundary, the inlined copy may potentially become out of sync with the actually definition.

@prattmic
Copy link
Member Author

I agree it isn't much of a problem right now. I was actually wondering if we can remove funcID from inlinedCall entirely because I'm not sure if wrapper functions are ever inlined (if a wrapper function is used somewhere that it can be inlined, then the compiler might not need to generate a wrapper at all? But I'm not sure about this). I hadn't considered that wrappers should always be local.

Anyways, I'm currently planning to add function start line numbers to inlinedCall via FuncInfo for the PGO proposal. In that case it will matter, as we won't find the line number for cross-DSO inlined functions.

If we stop cross-DSO inlining entirely then indeed this problem would go away entirely.

@cherrymui
Copy link
Member

if a wrapper function is used somewhere that it can be inlined, then the compiler might not need to generate a wrapper at all?

This is at least true for ABI wrappers. I'm not really sure about other type of wrappers. I agree that we probably should do it this way (if not already).

gopherbot pushed a commit that referenced this issue Sep 30, 2022
@prattmic
cmd/link/internal/ld: panic if inlined functions missing FuncInfo
8a0cf71 
All inlined functions are Go functions, and thus should be capable of
having a FuncInfo. Missing FuncInfo is likely indication of a compiler
bug that dropped the symbol too early, failing to add it to the symbol
list used for writing output. I believe all existing cases have been
fixed; this check will prevent regressions.

The exception is -linkshared mode. There symbols are loaded from the
shared library, and the FuncInfo is not available. This is a bug, as it
can result in incorrect the FuncID in inlinedCall, but it is very
involved to fix.

For #54959.
For #55954.

Change-Id: Ib0dc4f1ea62525b55f68604d6013ff33223fdcdd
Reviewed-on: https://go-review.googlesource.com/c/go/+/429637
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@prattmic prattmic self-assigned this Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prattmic prattmic

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@prattmic @dmitshur @gopherbot @cherrymui