golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.59k stars 17.61k forks source link

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

Open prattmic opened 2 years ago

prattmic commented 2 years ago

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

gopherbot commented 2 years ago

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

cherrymui commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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).