elf_reader: permit multiple .data* sections#1546
Conversation
mejedi
force-pushed
the
multiple-data-sections
branch
3 times, most recently
from
bb9beb1 to
ed1d302
Compare
ti-mo
left a comment
ti-mo
left a comment
There was a problem hiding this comment.
Thanks for adding tests! Could we make the patch shorter, though? There's no real need to gate this on 5.2 as far as I know, and we have a similar test for e.g. .rodata.test in loader.c.
Not sure if using LoadAndAssign and using prog.Test() add much value, either. If a .data.test map is present in the CollectionSpec, we're good.
I'd like to limit the amount of objects we have in testdata/, as each additional object slows down make a bit. I think many of them could be written in a single .c file, or at least fewer than we have today.
Thanks again!
mejedi
force-pushed
the
multiple-data-sections
branch
5 times, most recently
from
53b4fd7 to
76181e1
Compare
| } | ||
|
|
||
| expectedLineInfoCount := 26 | ||
| expectedLineInfoCount := 28 |
There was a problem hiding this comment.
That's the downside of overloading loader.c, I guess. To be honest, I do not understand where 26 is coming from, but it (kind of) makes sense that the value grew by 2 after two lines were added. @ti-mo
There was a problem hiding this comment.
Good question, that's quite straightforward to investigate!
if src := ins.Source(); src != nil {
lineInfoCount++
+ fmt.Println(src)
+ fmt.Println(ins)
}Results in:
...
LoadMapValue dst: r1, fd: 40 off: 0
return static_fn(arg) + global_fn(arg) + arg2;
AddReg dst: r0 src: r1
return static_fn(arg) + global_fn(arg) + arg2;
Exit
int __attribute__((noinline)) global_fn(uint32_t arg) {
...
return static_fn(arg) + global_fn2(arg) + global_fn3(arg);
AddReg dst: r8 src: r0
return static_fn(arg) + global_fn2(arg) + global_fn3(arg);
MovReg dst: r0 src: r8
One instruction for the variable access and one for accumulating into r0. (this is on main) Adding another variable to the return statement would result in 2 extra insns/lineinfo.
|
@ti-mo Thank you for having a look. I reduced the amount of changes by piggy-backing on the I suspect that all data sections share the same machinery for resolving relocations. Therefore a test ensuring that a program referencing variables in |
Mirror libbpf behavior. Quoting the respective commit message[1]: > Having the ability to use multiple .data sections and putting > dedicated global variables into separate .data.custom section also > opens up some new and interesting possibilities (like dynamically > sizing global arrays, without using BPF_MAP_TYPE_ARRAY map). [1] libbpf/libbpf#274 Another motivation for dedicated data sections is a more granular access control, such as making a subset of variables BPF_F_RDONLY_PROG (userspace can alter but BPF can't). Also note that verifier doesn't prevent overflowing .data buffers into adjacent variables as long as the write is within the backing array map bounds. Signed-off-by: Nick Zavaritsky <[email protected]>
ti-mo
force-pushed
the
multiple-data-sections
branch
from
76181e1 to
2435814
Compare
|
Pushed a small fixup to add the |
Up to you but |
It's not only useful for The readme you linked applies to kernel code proper and mainly cites concurrency concerns, which doesn't apply to bpf as much. There are some mailing list threads about the use of |
2 participants
Mirror libbpf behavior. Quoting the respective commit message[1]:
[1] libbpf/libbpf#274
Another motivation for dedicated data sections is a more granular access control, such as making a subset of variables BPF_F_RDONLY_PROG (userspace can alter but BPF can't). Also note that verifier doesn't prevent overflowing .data buffers into adjacent variables as long as the write is within the backing array map bounds.