| Message ID | 20220824091940.1578705-1-eyal.birger@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] bpf/scripts: assert helper enum value is aligned with comment order | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 65 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-3 | fail | Logs for Kernel LATEST on z15 with gcc |
On 24/08/2022 10:19, Eyal Birger 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. > > 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@gmail.com> > > --- > > v2: based on feedback from Quentin Monnet: > - assert the current comment ordering > - match only one FN in each line > --- > scripts/bpf_doc.py | 31 +++++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) > > diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py > index f4f3e7ec6d44..80dfb230459d 100755 > --- a/scripts/bpf_doc.py > +++ b/scripts/bpf_doc.py > @@ -91,7 +91,7 @@ class HeaderParser(object): > self.helpers = [] > self.commands = [] > self.desc_unique_helpers = set() > - self.define_unique_helpers = [] > + self.define_unique_helpers = {} > self.desc_syscalls = [] > self.enum_syscalls = [] > > @@ -248,24 +248,24 @@ class HeaderParser(object): > break > > def parse_define_helpers(self): > - # Parse the number of FN(...) in #define __BPF_FUNC_MAPPER to compare > - # later with the number of unique function names present in description. > + # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the > + # number of unique function names present in description and use the > + # correct enumeration value. > # Note: seek_to(..) discards the first line below the target search text, > # resulting in FN(unspec) being skipped and not added to self.define_unique_helpers. > self.seek_to('#define __BPF_FUNC_MAPPER(FN)', > 'Could not find start of eBPF helper definition list') > - # Searches for either one or more FN(\w+) defines or a backslash for newline > - p = re.compile('\s*(FN\(\w+\))+|\\\\') > - fn_defines_str = '' > + # Searches for one FN(\w+) define or a backslash for newline > + p = re.compile('\s*FN\((\w+)\)|\\\\') > + i = 1 # 'unspec' is skipped as mentioned above > while True: > capture = p.match(self.line) > if capture: > - fn_defines_str += self.line > + self.define_unique_helpers[capture.expand(r'bpf_\1')] = i > + i += 1 > else: > break > self.line = self.reader.readline() > - # Find the number of occurences of FN(\w+) > - self.define_unique_helpers = re.findall('FN\(\w+\)', fn_defines_str) > > def run(self): > self.parse_desc_syscall() > @@ -608,6 +608,7 @@ class PrinterHelpers(Printer): > def __init__(self, parser): > self.elements = parser.helpers > self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER') > + self.define_unique_helpers = parser.define_unique_helpers > > type_fwds = [ > 'struct bpf_fib_lookup', > @@ -796,7 +797,17 @@ class PrinterHelpers(Printer): > comma = ', ' > print(one_arg, end='') > > - print(') = (void *) %d;' % len(self.seen_helpers)) > + helper_val = self.define_unique_helpers[proto['name']] > + > + # Assert helper description order is aligned with the enum value Thanks! But this check should go in the parser, not in the printer. One reason is that it won't show when generating the man page, with this version; another reason is that it should return early and avoid generating an incomplete, non-functional header if the assertion fails. > + desc_val = len(self.seen_helpers) > + if helper_val != desc_val: > + print("Helper %s comment order (#%d) must be aligned with its enum " > + "order (#%d)" % (proto['name'], desc_val, helper_val), Please don't split the string on two lines, never mind if it goes over 80 characters. Makes it harder to grep otherwise. Suggestion on the message: "with its enum order (<nb>)" -> "with its position (<nb>) in enum bpf_func_id"? > + file=sys.stderr) > + sys.exit(1) Can we raise an Exception instead of printing to stderr + exiting? > + > + print(') = (void *) %d;' % helper_val) > print('') > > ##############################################################################
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index f4f3e7ec6d44..80dfb230459d 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -91,7 +91,7 @@ class HeaderParser(object): self.helpers = [] self.commands = [] self.desc_unique_helpers = set() - self.define_unique_helpers = [] + self.define_unique_helpers = {} self.desc_syscalls = [] self.enum_syscalls = [] @@ -248,24 +248,24 @@ class HeaderParser(object): break def parse_define_helpers(self): - # Parse the number of FN(...) in #define __BPF_FUNC_MAPPER to compare - # later with the number of unique function names present in description. + # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the + # number of unique function names present in description and use the + # correct enumeration value. # Note: seek_to(..) discards the first line below the target search text, # resulting in FN(unspec) being skipped and not added to self.define_unique_helpers. self.seek_to('#define __BPF_FUNC_MAPPER(FN)', 'Could not find start of eBPF helper definition list') - # Searches for either one or more FN(\w+) defines or a backslash for newline - p = re.compile('\s*(FN\(\w+\))+|\\\\') - fn_defines_str = '' + # Searches for one FN(\w+) define or a backslash for newline + p = re.compile('\s*FN\((\w+)\)|\\\\') + i = 1 # 'unspec' is skipped as mentioned above while True: capture = p.match(self.line) if capture: - fn_defines_str += self.line + self.define_unique_helpers[capture.expand(r'bpf_\1')] = i + i += 1 else: break self.line = self.reader.readline() - # Find the number of occurences of FN(\w+) - self.define_unique_helpers = re.findall('FN\(\w+\)', fn_defines_str) def run(self): self.parse_desc_syscall() @@ -608,6 +608,7 @@ class PrinterHelpers(Printer): def __init__(self, parser): self.elements = parser.helpers self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER') + self.define_unique_helpers = parser.define_unique_helpers type_fwds = [ 'struct bpf_fib_lookup', @@ -796,7 +797,17 @@ class PrinterHelpers(Printer): comma = ', ' print(one_arg, end='') - print(') = (void *) %d;' % len(self.seen_helpers)) + helper_val = self.define_unique_helpers[proto['name']] + + # Assert helper description order is aligned with the enum value + desc_val = len(self.seen_helpers) + if helper_val != desc_val: + print("Helper %s comment order (#%d) must be aligned with its enum " + "order (#%d)" % (proto['name'], desc_val, helper_val), + file=sys.stderr) + sys.exit(1) + + print(') = (void *) %d;' % helper_val) print('') ###############################################################################
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. 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@gmail.com> --- v2: based on feedback from Quentin Monnet: - assert the current comment ordering - match only one FN in each line --- scripts/bpf_doc.py | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)