On 24/08/2022 13:26, 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@xxxxxxxxx>
>
> ---
> 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(-)
>
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index f4f3e7ec6d44..114b2a60afc8 100755
> --- a/scripts/bpf_doc.py
> +++ b/scripts/bpf_doc.py
> @@ -50,6 +50,10 @@ class Helper(APIElement):
> @desc: textual description of the helper function
> @ret: description of the return value of the helper function
> """
> + def __init__(self, *args, **kwargs):
> + super().__init__(*args, **kwargs)
> + self.enum_value = None
Should this be self.enum_val?
Looks good otherwise.
> +
> def proto_break_down(self):
> """
> Break down helper function protocol into smaller chunks: return type,
> @@ -92,6 +96,7 @@ class HeaderParser(object):
> self.commands = []
> self.desc_unique_helpers = set()
> self.define_unique_helpers = []
> + self.helper_enum_vals = {}
> self.desc_syscalls = []
> self.enum_syscalls = []
>
> @@ -248,30 +253,54 @@ 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+\))+|\\\\')
> + # Searches for one FN(\w+) define or a backslash for newline
> + p = re.compile('\s*FN\((\w+)\)|\\\\')
> fn_defines_str = ''
> + i = 1 # 'unspec' is skipped as mentioned above
> while True:
> capture = p.match(self.line)
> if capture:
> fn_defines_str += self.line
> + self.helper_enum_vals[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 assign_helper_values(self):
> + seen_helpers = set()
> + for helper in self.helpers:
> + proto = helper.proto_break_down()
> + name = proto['name']
> + try:
> + enum_val = self.helper_enum_vals[name]
> + except KeyError:
> + raise Exception("Helper %s is missing from enum bpf_func_id" % name)
> +
> + # Enforce current practice of having the descriptions ordered
> + # by enum value.
> + seen_helpers.add(name)
> + desc_val = len(seen_helpers)
> + if desc_val != enum_val:
> + raise Exception("Helper %s comment order (#%d) must be aligned with its position (#%d) in enum bpf_func_id" % (name, desc_val, enum_val))
> +
> + helper.enum_val = enum_val
> +
> def run(self):
> self.parse_desc_syscall()
> self.parse_enum_syscall()
> self.parse_desc_helpers()
> self.parse_define_helpers()
> + self.assign_helper_values()
> self.reader.close()
>
> ###############################################################################
> @@ -796,7 +825,7 @@ class PrinterHelpers(Printer):
> comma = ', '
> print(one_arg, end='')
>
> - print(') = (void *) %d;' % len(self.seen_helpers))
> + print(') = (void *) %d;' % helper.enum_val)
> print('')
>
> ###############################################################################
