Re: [PATCH bpf-next v2 13/13] tools/bpf: bpftool: add support for jited func types

[Edward Cree <ecree@solarflare.com>]

On 17/10/18 08:24, Yonghong Song wrote:
> This patch added support to print function signature
> if btf func_info is available. Note that ksym
> now uses function name instead of prog_name as
> prog_name has a limit of 16 bytes including
> ending '\0'.
>
> The following is a sample output for selftests
> test_btf with file test_btf_haskv.o:
>
>   $ bpftool prog dump jited id 1
>   int _dummy_tracepoint(struct dummy_tracepoint_args * ):
>   bpf_prog_b07ccb89267cf242__dummy_tracepoint:
>      0:   push   %rbp
>      1:   mov    %rsp,%rbp
>     ......
>     3c:   add    $0x28,%rbp
>     40:   leaveq
>     41:   retq
>
>   int test_long_fname_1(struct dummy_tracepoint_args * ):
>   bpf_prog_2dcecc18072623fc_test_long_fname_1:
>      0:   push   %rbp
>      1:   mov    %rsp,%rbp
>     ......
>     3a:   add    $0x28,%rbp
>     3e:   leaveq
>     3f:   retq
>
>   int test_long_fname_2(struct dummy_tracepoint_args * ):
>   bpf_prog_89d64e4abf0f0126_test_long_fname_2:
>      0:   push   %rbp
>      1:   mov    %rsp,%rbp
>     ......
>     80:   add    $0x28,%rbp
>     84:   leaveq
>     85:   retq
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  tools/bpf/bpftool/btf_dumper.c | 96 ++++++++++++++++++++++++++++++++++
>  tools/bpf/bpftool/main.h       |  2 +
>  tools/bpf/bpftool/prog.c       | 54 +++++++++++++++++++
>  3 files changed, 152 insertions(+)
>
> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> index 55bc512a1831..a31df4202335 100644
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -249,3 +249,99 @@ int btf_dumper_type(const struct btf_dumper *d, __u32 type_id,
>  {
>  	return btf_dumper_do_type(d, type_id, 0, data);
>  }
> +
> +#define BTF_PRINT_STRING(str)						\
> +	{								\
> +		pos += snprintf(func_sig + pos, size - pos, str);	\
> +		if (pos >= size)					\
> +			return -1;					\
> +	}
Usual kernel practice for this sort of macro is to use
    do { \
    } while(0)
 to ensure correct behaviour if the macro is used within another control
 flow statement, e.g.
    if (x)
        BTF_PRINT_STRING(x);
    else
        do_something_else();
 will not compile with the bare braces as the else will be detached.
> +#define BTF_PRINT_ONE_ARG(fmt, arg)					\
> +	{								\
> +		pos += snprintf(func_sig + pos, size - pos, fmt, arg);	\
> +		if (pos >= size)					\
> +			return -1;					\
> +	}
Any reason for not just using a variadic macro?
> +#define BTF_PRINT_TYPE_ONLY(type)					\
> +	{								\
> +		pos = __btf_dumper_type_only(btf, type, func_sig,	\
> +					     pos, size);		\
> +		if (pos == -1)						\
> +			return -1;					\
> +	}
> +
> +static int __btf_dumper_type_only(struct btf *btf, __u32 type_id,
> +				  char *func_sig, int pos, int size)
> +{
> +	const struct btf_type *t = btf__type_by_id(btf, type_id);
> +	const struct btf_array *array;
> +	int i, vlen;
> +
> +	switch (BTF_INFO_KIND(t->info)) {
> +	case BTF_KIND_INT:
> +		BTF_PRINT_ONE_ARG("%s ",
> +				  btf__name_by_offset(btf, t->name_off));
> +		break;
> +	case BTF_KIND_STRUCT:
> +		BTF_PRINT_ONE_ARG("struct %s ",
> +				  btf__name_by_offset(btf, t->name_off));
> +		break;
> +	case BTF_KIND_UNION:
> +		BTF_PRINT_ONE_ARG("union %s ",
> +				  btf__name_by_offset(btf, t->name_off));
> +		break;
> +	case BTF_KIND_ENUM:
> +		BTF_PRINT_ONE_ARG("enum %s ",
> +				  btf__name_by_offset(btf, t->name_off));
> +		break;
> +	case BTF_KIND_ARRAY:
> +		array = (struct btf_array *)(t + 1);
> +		BTF_PRINT_TYPE_ONLY(array->type);
> +		BTF_PRINT_ONE_ARG("[%d]", array->nelems);
> +		break;
> +	case BTF_KIND_PTR:
> +		BTF_PRINT_TYPE_ONLY(t->type);
> +		BTF_PRINT_STRING("* ");
> +		break;
> +	case BTF_KIND_UNKN:
> +	case BTF_KIND_FWD:
> +	case BTF_KIND_TYPEDEF:
> +		return -1;
> +	case BTF_KIND_VOLATILE:
> +		BTF_PRINT_STRING("volatile ");
> +		BTF_PRINT_TYPE_ONLY(t->type);
> +		break;
> +	case BTF_KIND_CONST:
> +		BTF_PRINT_STRING("const ");
> +		BTF_PRINT_TYPE_ONLY(t->type);
> +		break;
> +	case BTF_KIND_RESTRICT:
> +		BTF_PRINT_STRING("restrict ");
> +		BTF_PRINT_TYPE_ONLY(t->type);
> +		break;
> +	case BTF_KIND_FUNC:
> +	case BTF_KIND_FUNC_PROTO:
> +		BTF_PRINT_TYPE_ONLY(t->type);
> +		BTF_PRINT_ONE_ARG("%s(", btf__name_by_offset(btf, t->name_off));
> +		vlen = BTF_INFO_VLEN(t->info);
> +		for (i = 0; i < vlen; i++) {
> +			__u32 arg_type = ((__u32 *)(t + 1))[i];
> +
> +			BTF_PRINT_TYPE_ONLY(arg_type);
> +			if (i != (vlen - 1))
> +				BTF_PRINT_STRING(", ");
> +		}
In this kind of loop I find it cleaner to print the comma before the item;
 that way the test becomes i != 0.  Thus:
    for (i = 0; i < vlen; i++) {
        __u32 arg_type = ((__u32 *)(t + 1))[i];

        if (i)
            BTF_PRINT_STRING(", ");
        BTF_PRINT_TYPE_ONLY(arg_type);
    }

-Ed