Proposal to add BTF metadata to asm.Instruction

[dylandreimerink]

Hi everyone.

I would like to make BTF line information better available from outside the internal/btf package. I am currently working on a PR to accomplish this and would like some input/feedback before submitting it. The reason I need this functionality is so that I can show the lines of source code when inspecting instructions from the ProgramSpec objects.

My proposal is to add a new InstructionMetadata structure to the asm package and to add a field to asm.Instruction of type *InstructionMetadata. This allows us to add information to the instruction, namely the source file name, line and line number.

type InstructionMetadata struct {
	FileName     string
	Line         string
	LineNumber   int
	ColumnNumber int
}

In the ebpf.loadFunctions func, everything we need to add this information is already available(*btf.Spec, elfSection, symbols). We already parse the btf.LineInfos from the .BTF.ext section, which is not very useful to anyone but the kernel in its current form(offsets into the strings table). Adding 2 functions to the LineInfos object to convert the offsets into actual strings should solve this.

The main benefit for adding the metadata directly to the Instruction is that we can now rewrite asm.Instructions to our harts content and don't have to worry about incorrect offsets in LineInfos. That is because we should be able to marshal these metadata objects back into kernel loadable LineInfos/string table.

Being able to add metadata to instruction created via the DSL might also be a nice feature, since we can include comments which will later be visible via bpftool when needing to inspect/debug the loaded instructions.

Would love to hear what you guys think.

[ti-mo]

@lmb has code that implements this to some degree, but not sure how close it is to landing.

A few observations here. This would all be public API (read & write), so exposing xxxNumber isn't very useful. When unmarshaling insns, this metadata would only contain fully-resolved data (lineinfo, funcinfo, filename).

Then, when marshaling insns, all metadata would be serialized from scratch based on what's in the type graph and extinfos. All encountered strings like type names, file names, func/lineinfos would be interned and emitted to the BTF blob as a new string table.

asm.Metadata could be sufficient.

Metadata would also contain *Map and *Program pointers for instructions that perform map loads and bpf calls respectively, so #515 would no longer be an issue and ProgramSpec.references can be removed.

[dylandreimerink]

This would all be public API (read & write), so exposing xxxNumber isn't very useful

Currently btf.LineInfo already encodes the line and column number in the LineCol field, but you have to mask and bitshift to get at the seperate line and col numbers.

#define BPF_LINE_INFO_LINE_NUM(line_col)        ((line_col) >> 10)
#define BPF_LINE_INFO_LINE_COL(line_col)        ((line_col) & 0x3ff)

Having the plain numbers in Metadata and then doing this unpacking/packing work during unmarshaling/marshaling seems more user-friendly to me.

[lmb]

@dylandreimerink you've hit the nail on the head, per instruction metadata would be great. As Timo says, I've been playing around with this, the current state of affairs is at lmb@6af8ec0. With that infrastructure in place I would fix #515 like so: lmb@ef4a4a6. I'm making the following assumptions:

1. Most instructions won't have metadata, otherwise it would be cheaper (smaller) to just inline it in Instruction proper instead of going via metadata type.
2. Instructions that have metadata won't have ALL metadata that is possible. So either map references or line file info as in your example. Otherwise just using a struct instead of a map like you do would be better.
3. We want to be able to modify asm.Instructions by appending slices, etc. which implies that metadata has to be copy on write. Otherwise multiple copies of asm.Instructions would point at the same metadata.

Do you think these assumptions are reasonable, or would your use case violate 1. maybe?

When unmarshaling insns, this metadata would only contain fully-resolved data (lineinfo, funcinfo, filename).

Can you explain this a bit more? I don't follow, sorry.

Then, when marshaling insns, all metadata would be serialized from scratch based on what's in the type graph and extinfos.

That should be the end goal, but for a start it would be better if we allowed just retrieving metadata in a read-only way. From scratch serialisation of BTF is a lot of work, as discussed.

[dylandreimerink]

Here is what I have so far dylandreimerink@59fbf81. I hope this doesn't make things difficult.

Do you think these assumptions are reasonable, or would your use case violate 1. maybe?

I think this is reasonable, the screenshot in my original post shows what I think is typical, I would say about 1/3 of the instructions have LineInfo associated with it, since most lines of C translate into multiple instructions. The lines themselves are almost never the same, the most wasteful is the filename, since it is both a full path and is also the same for most instructions. Using a form of flystring might be nice, though I am not sure if it it worth the effort.

Can you explain this a bit more

Like @ti-mo mentioned

Then, when marshaling insns, all metadata would be serialized from scratch based on what's in the type graph and extinfos. All encountered strings like type names, file names, func/lineinfos would be interned and emitted to the BTF blob as a new string table.

My idea is to re-generate the extinfos blobs we pass to the kernel from the marshaled instruction metadata + BTF spec. What we get from that is that any changes made between unmarshalling and loading will not break the instruction offsets specified in extinfos and the actual instructions loaded. It also would allow programs crafted with the DSL to include comments or the actual go filename and line number which created it(manually or automatically with runtime.Caller).

That should be the end goal, but for a start it would be better if we allowed just retrieving metadata in a read-only way. From scratch serialisation of BTF is a lot of work, as discussed.

Agreed, I am not looking to do all of this is one massive PR. My initial goal is to be able to access the filenames and source code lines as strings via the public API so it can be utilized in edb. But I think we can take the feature much further than that in later PR's.

[dylandreimerink]

2. Instructions that have metadata won't have ALL metadata that is possible. So either map references or line file info as in your example. Otherwise just using a struct instead of a map like you do would be better.

Right, hadn't thought of including the references in the metadata as well, since one comes from the ELF symbol table/relocation table and the other info from .BTF.ext. If we do want to combine them, it should definitely be possible to have both map refs and line info at the same time.

3. We want to be able to modify asm.Instructions by appending slices, etc. which implies that metadata has to be copy on write. Otherwise multiple copies of asm.Instructions would point at the same metadata.

That is a good point, I opted for a pointer save space if instructions don't have metadata, but if we want to be able to copy instructions easily, having metadata as value might be a better idea, assuming empty strings and zero ints are not the end of the works memory-footprint wise.

[dylandreimerink]

So, in summary.

There are 2 goals:

1. we want to add *Map and *Program references to instructions to avoid them unloading after rewriting Map file descriptor is closed after RewriteMaps #515
2. we want to add .BTF.ext info(line info + func info) to instructions, initially to added source annotation to asm.Instructions and perhaps later (future PR) so we can use the metadata to marshal custom BTF/BTF.ext blobs from rewriten/generated code.

We already have some metadata in the form of Instruction.Reference and Instruction.Symbol. Both the existing and proposed new metadata is sparse, most instructions don't have metadata. Thus it makes sense to combine the existing metadata and new metadata into a structure that takes little space when not used.

I see three approaches:

1. Use a map like in @lmb 's branch, and then uses methods on Instruction to get and set values. Costs us 1 machine word if empty, but comes with some memory overhead if created.
2. Make a pointer to a Metadata struct which itself contains pointers to metadata, also costs us 1 machine word if not used at all and 1 additional machine word for every nil pointer in the Metadata struct.
3. Just add all metadata pointers directly to asm.Instruction, by far the simplest code, but instructions will always be larger even if they don't have metadata, about 5 additional pointers(filename, line, linecol, map, prog). (the same case if we were to include a Metadata struct by value)

No matter what we do, asm.Instruction structs can't be copied by value anymore after adding *Map and *Program references, since the pointers are copied. So we will have to check if we currently rely on that behavior.

That leaves me with 3 questions:

• Which of the 3 approaches do you think is best?
• Do we already know if we rely on copyable asm.Instruction's?
• @lmb what would be the best way to merge our work, do you plan on merging your part soon? in which case I will wait for that and make a PR based that. Or are you currently busy with other work? in which case would it be cool with you if I merge your work so far into my branch and fix Map file descriptor is closed after RewriteMaps #515 in the process?

[lmb]

Great summary, thanks! I agree with the goals.

Regarding the possible approaches, option 3 is basically what we have now and it's pretty wasteful. I'd rather not do this. With this in mind, option 1 & 2 are roughly equivalent to me, provided that in both cases we hide the internal representation of metadata behind getters and setters (so no exported InstructionMetadata). Doing this will allow us to swap out the implementation later and also let's us deal with default values of metadata in the getters.

No matter what we do, asm.Instruction structs can't be copied by value anymore after adding *Map and *Program references, since the pointers are copied. So we will have to check if we currently rely on that behavior.

When designing asm I strived to make Instructions as much like a normal slice as possible. I'd like to preserve this, which is why my approach uses copy on write for the metadata. That way it's safe to copy instructions and modify them later.

Using the map is a cute way to make copy on write really simple, but I'm sure there is a way to make it work with a plain struct as well. A reason I haven't put up a PR for the map based metadata is that it feels a bit too clever. So my proposal would be that we combine the two approaches:

1. Re-use the public API of option 1
2. Store metadata in an unexported plain struct as in option 2
3. Make that struct copy on write somehow

If copy on write ends up being a pain we go back to option 1. I think I'll have time to work on this on Monday / Tuesday at the earliest. If you feel up to it you can also give it a go, just let me know.

[dylandreimerink]

Fully agree, I have some time on my hands, so I will give it a go. I will link the PR if/when I have something to show.

[dylandreimerink]

I think I managed a fairly decent PR, looking forward to your review whenever you have time #567