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

On 10/17/18 4:11 AM, Edward Cree wrote:

On 17/10/18 08:24, Yonghong Song wrote:

quoted

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 <redacted>
---
  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.

quoted

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

quoted

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