[WIP] feat: concrete type arguments #4353
Conversation
github-actions
bot
added
area/ebpf
area/testing
area/events
area/build
area/grpc
labels
|
|
||
| switch argType { | ||
| case u8T: | ||
| case trace.U8_T: |
There was a problem hiding this comment.
How about reducing the size of readArgFromBuff() by replacing the switch/if branches with an already initialized array containing pointer functions for each type (ebpfMsgDecoder.Decode*)?
There was a problem hiding this comment.
This is basically the planned next step. I plan to move the decoding mapping to a hash map similar to the processor and derive functions. In this way we, and plugin authors, can easily introduce new decoding strategies.
A useful example already exists in the code: we have two different submission formats for string arrays. Therefore we have a single "presentation type" with two "decoding strategies".
There was a problem hiding this comment.
@geyslan I've opted not to make this step in this PR but rather in a followup so that we merge smaller chunks at a time as this is a large scale change.
NDStrahilevitz
force-pushed
the
hard_type_args
branch
from
15f5ce3
to
d48f44c
Compare
NDStrahilevitz
force-pushed
the
hard_type_args
branch
2 times, most recently
from
74d839d
to
9df64bd
Compare
NDStrahilevitz
changed the title
NDStrahilevitz
changed the title
NDStrahilevitz
changed the title
NDStrahilevitz
force-pushed
the
hard_type_args
branch
from
9df64bd
to
03e90d1
Compare
| // If the size is greater than 255, assign 0 (making it evident) and handle it as a special case. | ||
| static u8 type_size_table[ARG_TYPE_MAX_ARRAY + 1] = { | ||
| static u32 type_size_table[ARG_TYPE_MAX_ARRAY + 1] = { |
There was a problem hiding this comment.
Why do we need to change to u32?
There was a problem hiding this comment.
leaving at u8 introduced BTF errors. Moving to u32 moved the error to the verifier which I could eventually sort out. In addition I believe @geyslan suggested a move to u32 for some reason which I can't recall...
There was a problem hiding this comment.
When I implemented such an array, I was lucky enough to pass through the verifier with flying colours. But when you changed its elements, the verifer surely started complaining about misaligned accesses or even reverted to its familiar state of "not knowing how to count". Changing the size of the array elements to the word size would be a fix, as you did.
pkg/ebpf/c/common/buffer.h
Outdated
| return 1; | ||
| } | ||
| // Read into buffer after the argument index | ||
| if (bpf_probe_read_kernel(&(buf->args[buffer_index_offset]), size, ptr) < 0) |
There was a problem hiding this comment.
How do you know that we are reading from a kernel pointer here?
There was a problem hiding this comment.
This is used only in syscall argument extraction which I think is copied from the bpf context first. Anyway we can't be certain and there would be no harm done moving back to the generic probe_read.
| eventDerivations derive.Table | ||
| eventsSorter *sorting.EventsChronologicalSorter | ||
| eventsPool *sync.Pool | ||
| eventArgumentTypes map[events.ID][]trace.DecodeAs |
There was a problem hiding this comment.
Arguments/Parameters should be replaced by data fields, which is the terminology used by the new event structure
There was a problem hiding this comment.
Renaming to eventDataDecodeTypes is ok for you?
The PR has two goals:
Currently only the first goal is achieved. |
NDStrahilevitz
force-pushed
the
hard_type_args
branch
from
03e90d1
to
f56e9bc
Compare
1. Reduce the type variance via hardcoding possible types 2. Move argument types to the types package 3. Make corresponding changes in eBPF code
Move some functions around, rename and improve documentation.
NDStrahilevitz
force-pushed
the
hard_type_args
branch
from
f56e9bc
to
8fc7341
Compare
1. Explain what the PR does
"Replace me with
make check-proutput"2. Explain how to test it
3. Other comments