-
Notifications
You must be signed in to change notification settings - Fork 730
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
Use instruction metadata to simplify linking and CO-RE relocation #606
Conversation
Split off the |
I'll move |
149ab47
to
ef79de7
Compare
This is now in a state where it can be reviewed! I've updated the PR description with the latest commit messages, please take a look. I'm a little bit unhappy with
The snag is that
By abusing the map we can avoid introducing a new type, but none of our code really wants to use a map. An ordered slice is much better. For that we'd need:
This means a lot of additional types, which I don't particularly like. Maybe you have another idea? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, have a few first observations that might or might not apply, let me know what you think.
if relo.kind == reloTypeIDLocal { | ||
// Filtering out reloTypeIDLocal here makes our lives a lot easier | ||
// down the line, since it doesn't have a target at all. | ||
if len(relo.accessor) > 1 || relo.accessor[0] != 0 { | ||
return nil, fmt.Errorf("%s: unexpected accessor %v", relo.kind, relo.accessor) | ||
} | ||
|
||
result[uint64(relo.insnOff)] = COREFixup{ | ||
result[i] = COREFixup{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we can't use Type instead of TypeID here. What is the remaining significance of a type ID? If we know the IDs of the local and target types, and we have both specs available within this function, what's stopping us from putting inflated types here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is libbpf behaviour, since relo.kind == reloTypeIDLocal
we need an ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also creates problems for #643 of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is probably something that needs solving in order to get rid of IDs. (as a follow-up, to implement #643)
internal/btf/btf.go
Outdated
func LoadSpecFromReader(rd io.ReaderAt) (*Spec, error) { | ||
// Returns ErrNotFound if reading from an ELF which contains no BTF. ExtInfos | ||
// may be nil. | ||
func LoadSpecFromReader(rd io.ReaderAt) (*Spec, *ExtInfos, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen you refer to eliminating *ExtInfos
here in the future in your commit message, but can we get away with doing that sooner rather than later? Could we put *ExtInfos
(back?) into the btf.Spec
instead of returning it as a separate value here? IMO LoadSpec*
should return a single value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to remove ExtInfos completely, but #606 (comment) is my best idea how to do it and I don't like it very much.
Embedding ExtInfos in Spec has it the wrong way round: when reading BTF we always have a Spec, but we don't always have ExtInfos. So modeling the relationship as ExtInfo{Spec} is more natural. For example, naked vmlinux BTF has no ext infos. Check the call sites of loadRawSpec
, almost always we pass nil, nil
and aren't interested in ExtInfos in the first place. With ExtInfos split out / removed a good amount of code can be refactored to be less deeply nested.
I was considering adding a separate constructor LoadExtInfos(io.Reader)
and making LoadSpec
skip ExtInfos. However I can't come up with a way to achieve that without (a) adding *elf.File
to the btf
API surface like LoadExtInfos(*elf.File)
(and all the SafeELFFile baggage) or (b) parsing the ELF one more time. (We already parse the ELF header twice in the ELF loader due to this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, check this out: 640c4b0 Kinda terrible, but we could use it to have LoadExtInfosFromReader(io.ReaderAt)
and LoadSpecFromReader(io.ReaderAt)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the following:
I'd like to remove ExtInfos completely, but #606 (comment) is my best idea how to do it and I don't like it very much.
FuncInfo
is wire format, the metadata would be a btf.Func
. Func currently still holds a TypeID, so we can easily generate a FuncInfo based on the ID and the instruction's offset when marshaling.
Later on, when BTF marshaling lands, we can remove TypeID from btf.Func and generate BTF blobs that only contain the program's func and lineinfos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding ExtInfos: could we somehow break out that review and discussion into a separate PR? Taking *elf.File
is not the way to go for the reasons you mentioned, but for me primarily due to #550.
If we're reworking the btf/extinfo reader API, we should definitely take the opportunity to address this current limitation and come to a solution that works for module BTF as well. Actually parsing (and 'linking') module BTF might be a bigger chunk of work, but at least the API should allow for it to be added later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding ExtInfos: could we somehow break out that review and discussion into a separate PR?
I was trying to find a way to not return ExtInfos
from LoadSpec...()
since you seemed opposed. If you can live with that solution I'm happy to shelve that whole discussion for now.
As a follow up to #606 (comment), what if ExtInfos becomes this:
|
Please take a look at da1770b:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting! Left a few more comments. Should the latest commit be squashed?
if relo.kind == reloTypeIDLocal { | ||
// Filtering out reloTypeIDLocal here makes our lives a lot easier | ||
// down the line, since it doesn't have a target at all. | ||
if len(relo.accessor) > 1 || relo.accessor[0] != 0 { | ||
return nil, fmt.Errorf("%s: unexpected accessor %v", relo.kind, relo.accessor) | ||
} | ||
|
||
result[uint64(relo.insnOff)] = COREFixup{ | ||
result[i] = COREFixup{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is probably something that needs solving in order to get rid of IDs. (as a follow-up, to implement #643)
linker.go
Outdated
return nil, nil | ||
// marshalExtInfos encodes function and line info embedded in insns into kernel | ||
// wire format. | ||
func marshalExtInfos(insns asm.Instructions) (funcInfos, lineInfos []byte, _ error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this live in package btf, or is there an issue with a dependency on asm
?
My hope was to get rid of FuncInfo and LineInfo, since all the information they carry can now be represented by asm.Instructions
. e.g. *Func and the instruction's position in the slice as you've shown below.
Ultimately, we should end up with only Line and Func, and their respective wire formats, without the Func/LineInfo
intermediates.
Pull constructors for FuncInfo, LineInfo and CORERelocation out of splitExtInfos. This makes the function smaller and makes removing it later easier. Instead of taking a *Spec in the constructors, take a []Type and stringTable directly. This makes splitting ExtInfos out of *Spec easier later on.
This makes bpfCORERelo consistent with bpfFuncInfo and bpfLineInfo. It seems like there is a latent bug here as well, since we used to subtract bpfFuncInfo.InsnOff from bpfCORERelo.InsnOff, but for some reason the tests pass.
04f12e3
to
4d44715
Compare
Use per-instruction metadata to store func info, line info and CO-RE relocations. As a result, simply concatenating two Instruction slices preserves the ext_info without extra code to keep track of offsets. The simplest way to achieve is to assign ext_infos per section, before we split into individual functions. This also avoids having to split ext_infos in the first place. Storing ext_infos in metadata in turn allows / requires removing code that relies on stable offsets, the most notable being applying COREFixups. Instead of tracking which offset a fixup should be applied to we change coreRelocate to instead return results in the same order as CORERelocations are passed. In the caller we remember which instruction originated the relocation and apply the fixup directly instead of iterating all instructions once more. Instead of storing ExtInfos in Spec we split it off into a separate exported type. This makes more sense since every BTF ELF has a Spec, but ExtInfos are optional. It turns out that only the ELF loader doesn't care about ExtInfos in the first place, so we allow skipping ExtInfos altogether by introducing LoadSpecAndExtInfosFromReader. Finally we can remove btf.Program since we don't need an intermediate type to hold metadata anymore. Fixes cilium#522
btf.Program is gone, so let's get rid of Map as well.
btf: move CORERelocate to core.go
btf: add constructors for various *Info
btf: track bpfCORERelo.InsnOff in instructions instead of bytes
btf: store ExtInfos in instruction metadata
btf: remove Map