On Wed, Aug 24, 2022 at 11:11 AM Eyal Birger <eyal.birger@xxxxxxxxx> wrote: > > The helper value is ABI as defined by enum bpf_func_id. > As bpf_helper_defs.h is used for the userpace part, it must be consistent > with this enum. I think the way we implicitly define the value of those BPF_FUNC_ enums is also suboptimal. It makes it much harder to cherry-pick and backport only few latest helpers onto old kernels (there was a case backporting one of the pretty trivial timestamp fetching helpers without backporting other stuff). It's also quite hard to correlate llvm-objdump output with just `call 123;` instruction into which helper it is. If each FN(xxx) definition in __BPF_FUNC_MAPPER was taking explicit integer number, I think it would be a big win and make things better all around. Is there any opposition to doing that? But regardless, applied this patch to bpf-next as well as an improvement. > > Before this change the comments order was used by the bpf_doc script in > order to set the helper values defined in the helpers file. > > When adding new helpers it is very puzzling when the userspace application > breaks in weird places if the comment is inserted instead of appended - > because the generated helper ABI is incorrect and shifted. > > This commit sets the helper value to the enum value. > > In addition it is currently the practice to have the comments appended > and kept in the same order as the enum. As such, add an assertion > validating the comment order is consistent with enum value. > > In case a different comments ordering is desired, this assertion can > be lifted. > > Signed-off-by: Eyal Birger <eyal.birger@xxxxxxxxx> > > --- > v4: fix variable name typo > v3: based on feedback from Quentin Monnet: > - move assertion to parser > - avoid using define_unique_helpers as elem_number_check() relies on > it being an array > - set enum_val in helper object instead of passing as a dict to the > printer > > v2: based on feedback from Quentin Monnet: > - assert the current comment ordering > - match only one FN in each line > --- > scripts/bpf_doc.py | 39 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) > [...]
