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

[Yonghong Song <yhs@fb.com>]

On 10/17/18 4:11 AM, Edward Cree wrote:
> 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.

Thanks for the review! Will change to use the "do while" format
as you suggested.

>> +#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?

No particular reason. I will try to use it in the next revision.

>> +#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);
>      }

Good suggestion. Will make change in the next revision.

> 
> -Ed
>