CollectionSpec: add new Variables field to interact with constants and global variables

[ti-mo]

See individual commits for details and design considerations.

See https://deploy-preview-1564--ebpf-go.netlify.app/concepts/global-variables for documentation and a quick overview of the API.

Minor breaking change: RewriteConstants and the new CollectionSpec.Variables no longer expose global variables with the static keyword as they cannot be distinguished from function-scoped, local variables. This conforms to libbpf behaviour that doesn't expose static vars in bpf-skeleton.

[mejedi]

LGTM

[dylandreimerink]

This is great! Can't find anything to complain about

[alban]

FYI in Inspektor Gadget, we have unit tests with ebpf.CollectionSpec defined manually (not read from an ELF file):
https://github.com/inspektor-gadget/inspektor-gadget/blob/0cce8189cbaebdfc9c9a49ccc1bb471369e4154e/pkg/kallsyms/kallsyms_test.go#L87-L122

Then we call spec.RewriteConstants to check if our kallsyms package works correctly. (our kallsyms package offers a different feature set than cilium/ebpf one, so we can't use remove it)

When I update to the new cilium/ebpf with this PR, the tests fail because spec.RewriteConstants does not find the Variables section, even if the spec has the correct ".rodata" map:

        	Error Trace:	/home/runner/work/inspektor-gadget/inspektor-gadget/pkg/kallsyms/kallsyms_test.go:162
        	Error:      	Expected nil, but got: &fmt.wrapError{msg:"rewriting constants: rewrite constants: some constants are missing from .rodata: socket_file_ops_addr, bpf_prog_fops_addr", err:(*fmt.wrapError)(0xc0000320e0)}
        	Test:       	TestSpecRewriteConstants
        	Messages:   	specUpdateAddresses failed: rewriting constants: rewrite constants: some constants are missing from .rodata: socket_file_ops_addr, bpf_prog_fops_addr

In this PR, it is not possible to instantiate a ebpf.CollectionSpec with variables because the fields of spec.Variables[*] are not exported. Could those fields be exported? Or any idea how to write the unit tests differently?

Found while working on inspektor-gadget/inspektor-gadget#3595

cc @ti-mo @burak-ok

[lmb]

I haven't looked at the PR in detail, but so far all the other *Specs have had only public fields for this reason (with some temporary exceptions which we managed to fix over time). I think it would be desirable to be able to create VariableSpec just like ProgramSpec, etc.

[ti-mo]

@alban Thanks for the feedback! Note that the next release will deprecate RewriteConstants, it's now just a wrapper around spec.Variables as you've noticed. The .rodata mention in the error string is a red herring, it doesn't actually check the .rodata map anymore

Note that we don't really encourage manually creating CollectionSpecs. We do it in a few places in tests for testing specific behaviours, but nothing major.

As for your problem, I assume you're testing for ErrMissingConstant (iirc). Going forward, you could populate CollectionSpec.Variables with empty values and let your kallsyms parser just do a membership check on the map.

Alternatively, I wouldn't mind adding a NewVariableSpec() constructor, it could be useful beyond tests. Exporting fields will make for ugly API as VariableSpec are primarily meant as accessors.

[lmb]

Alternatively, I wouldn't mind adding a NewVariableSpec() constructor, it could be useful beyond tests. Exporting fields will make for ugly API as VariableSpec are primarily meant as accessors.

What if we turn things around, and instead of variable spec being accessors for special maps we synthesise the necessary maps by assembling the VariableSpec into a map?

P.S. The implication being that instructions would start referring to VariableSpec instead of using a stringly reference + offset encoded in the instruction.

[mejedi]

What if we turn things around, and instead of variable spec being accessors for special maps we synthesise the necessary maps by assembling the VariableSpec into a map?

There’re no variableSpecs created for hidden variables (intentionally), hence the process is lossy. It is not just BTF, potentially even the size can change.

Today, it is possible to create custom data sections and manipulate the mapSpec for tricks like resizable buffers with fast access or fine-grained protection (BPF_F_MAP_RDONLY_PROG, etc.) It depends on having a mapSpec accessible.