[PATCH bpf-next v2 00/13] bpf: add btf func info support

[PATCH bpf-next v2 00/13] bpf: add btf func info support

[Yonghong Song]

The BTF support was added to kernel by Commit 69b693f0aefa
("bpf: btf: Introduce BPF Type Format (BTF)"), which introduced
.BTF section into ELF file and is primarily
used for map pretty print.
pahole is used to convert dwarf to BTF for ELF files.

This patch added func info support to the kernel so we can
get better ksym's for bpf function calls. Basically,
pairs of bpf function calls and their corresponding types
are passed to the kernel. Extracting function names from
the types, the kernel is able to construct a ksym for
each function call with embedded function name.

LLVM patch https://reviews.llvm.org/D53261
can generate func info, encoded in .BTF.ext ELF section.
In addition, it also added support for FUNC and FUNC_PROTO
types, which are also supported through this patch set.
The following is an example to show FUNC and
FUNC_PROTO difference, compiled with the above LLVM patch
with Debug mode.

  -bash-4.2$ cat test.c
  int foo(int (*bar)(int)) { return bar(5); }
  -bash-4.2$ clang -target bpf -g -O2 -mllvm -debug-only=btf -c test.c
  Type Table:
  [1] FUNC name_off=1 info=0x0c000001 size/type=2
        param_type=3
  [2] INT name_off=11 info=0x01000000 size/type=4
        desc=0x01000020
  [3] PTR name_off=0 info=0x02000000 size/type=4
  [4] FUNC_PROTO name_off=0 info=0x0d000001 size/type=2
        param_type=2
  
  String Table:
  0 :
  1 : foo
  5 : .text
  11 : int
  15 : test.c
  22 : int foo(int (*bar)(int)) { return bar(5); }
  
  FuncInfo Table:
  sec_name_off=5
        insn_offset=<Omitted> type_id=1
  ...

In the above, type and string tables are in .BTF section and
the func info in .BTF.ext. The "<Omitted>" is the
insn offset which is not available during the dump time but
resolved during later compilation process.
Following the format specification at Patch #9 and examine the
raw data in .BTF.ext section, we have
  FuncInfo Table:
  sec_name_off=5
        insn_offset=0 type_id=1
The (insn_offset, type_id) can be passed to the kernel
so the kernel can find the func name and use it in the ksym.
Below is a demonstration from Patch #13.
  $ 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

For the patchset,
Patch #1  refactors the code to break up btf_type_is_void().
Patch #2  introduces new BTF types BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO.
Patch #3  syncs btf.h header to tools directory.
Patch #4  adds btf func/func_proto self tests in test_btf.
Patch #5  adds kernel interface to load func_info to kernel
          and pass func_info to userspace.
Patch #6  syncs bpf.h header to tools directory.
Patch #7  adds news btf/func_info related fields in libbpf
          program load function.
Patch #8  extends selftest test_btf to test load/retrieve func_type info.
Patch #9  adds .BTF.ext func info support.
Patch #10 changes Makefile to avoid using pahole if llvm is capable of
          generating BTF sections.
Patch #11 refactors to have btf_get_from_id() in libbpf for reuse.
Patch #12 enhance test_btf file testing to test func info.
Patch #13 adds bpftool support for func signature dump.

Yonghong Song (13):
  bpf: btf: Break up btf_type_is_void()
  bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
  tools/bpf: sync kernel btf.h header
  tools/bpf: add btf func/func_proto unit tests in selftest test_btf
  bpf: get better bpf_prog ksyms based on btf func type_id
  tools/bpf: sync kernel uapi bpf.h header to tools directory
  tools/bpf: add new fields for program load in lib/bpf
  tools/bpf: extends test_btf to test load/retrieve func_type info
  tools/bpf: add support to read .BTF.ext sections
  tools/bpf: do not use pahole if clang/llvm can generate BTF sections
  tools/bpf: refactor to implement btf_get_from_id() in lib/bpf
  tools/bpf: enhance test_btf file testing to test func info
  tools/bpf: bpftool: add support for jited func types

Changelogs:
  v1 -> v2:
    . Added missing sign-off
    . Limited the func_name/struct_member_name length for validity test
    . Removed/changed several verifier messages
    . Modified several commit messages to remove line_off reference

 include/linux/bpf.h                          |   2 +
 include/linux/bpf_verifier.h                 |   1 +
 include/linux/btf.h                          |   2 +
 include/uapi/linux/bpf.h                     |  11 +
 include/uapi/linux/btf.h                     |   9 +-
 kernel/bpf/btf.c                             | 325 +++++++++--
 kernel/bpf/core.c                            |  10 +
 kernel/bpf/syscall.c                         |  83 ++-
 kernel/bpf/verifier.c                        |  46 ++
 samples/bpf/Makefile                         |   8 +
 tools/bpf/bpftool/btf_dumper.c               |  96 ++++
 tools/bpf/bpftool/main.h                     |   2 +
 tools/bpf/bpftool/map.c                      |  68 +--
 tools/bpf/bpftool/prog.c                     |  54 ++
 tools/include/uapi/linux/bpf.h               |  11 +
 tools/include/uapi/linux/btf.h               |   9 +-
 tools/lib/bpf/bpf.c                          |   3 +
 tools/lib/bpf/bpf.h                          |   3 +
 tools/lib/bpf/btf.c                          | 305 ++++++++++
 tools/lib/bpf/btf.h                          |  33 ++
 tools/lib/bpf/libbpf.c                       |  53 +-
 tools/testing/selftests/bpf/Makefile         |   8 +
 tools/testing/selftests/bpf/test_btf.c       | 566 ++++++++++++++++++-
 tools/testing/selftests/bpf/test_btf_haskv.c |  16 +-
 tools/testing/selftests/bpf/test_btf_nokv.c  |  16 +-
 25 files changed, 1611 insertions(+), 129 deletions(-)

-- 
2.17.1

[PATCH bpf-next v2 01/13] bpf: btf: Break up btf_type_is_void()

[Yonghong Song]

This patch breaks up btf_type_is_void() into
btf_type_is_void() and btf_type_is_fwd().

It also adds btf_type_nosize() to better describe it is
testing a type has nosize info.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/btf.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 378cef70341c..be406d8906ce 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -306,15 +306,22 @@ static bool btf_type_is_modifier(const struct btf_type *t)
 
 static bool btf_type_is_void(const struct btf_type *t)
 {
-	/* void => no type and size info.
-	 * Hence, FWD is also treated as void.
-	 */
-	return t == &btf_void || BTF_INFO_KIND(t->info) == BTF_KIND_FWD;
+	return t == &btf_void;
+}
+
+static bool btf_type_is_fwd(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info) == BTF_KIND_FWD;
+}
+
+static bool btf_type_nosize(const struct btf_type *t)
+{
+	return btf_type_is_void(t) || btf_type_is_fwd(t);
 }
 
-static bool btf_type_is_void_or_null(const struct btf_type *t)
+static bool btf_type_nosize_or_null(const struct btf_type *t)
 {
-	return !t || btf_type_is_void(t);
+	return !t || btf_type_nosize(t);
 }
 
 /* union is only a special case of struct:
@@ -826,7 +833,7 @@ const struct btf_type *btf_type_id_size(const struct btf *btf,
 	u32 size = 0;
 
 	size_type = btf_type_by_id(btf, size_type_id);
-	if (btf_type_is_void_or_null(size_type))
+	if (btf_type_nosize_or_null(size_type))
 		return NULL;
 
 	if (btf_type_has_size(size_type)) {
@@ -842,7 +849,7 @@ const struct btf_type *btf_type_id_size(const struct btf *btf,
 		size = btf->resolved_sizes[size_type_id];
 		size_type_id = btf->resolved_ids[size_type_id];
 		size_type = btf_type_by_id(btf, size_type_id);
-		if (btf_type_is_void(size_type))
+		if (btf_type_nosize_or_null(size_type))
 			return NULL;
 	}
 
@@ -1164,7 +1171,7 @@ static int btf_modifier_resolve(struct btf_verifier_env *env,
 	}
 
 	/* "typedef void new_void", "const void"...etc */
-	if (btf_type_is_void(next_type))
+	if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type))
 		goto resolved;
 
 	if (!env_type_is_resolve_sink(env, next_type) &&
@@ -1178,7 +1185,7 @@ static int btf_modifier_resolve(struct btf_verifier_env *env,
 	 * pretty print).
 	 */
 	if (!btf_type_id_size(btf, &next_type_id, &next_type_size) &&
-	    !btf_type_is_void(btf_type_id_resolve(btf, &next_type_id))) {
+	    !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) {
 		btf_verifier_log_type(env, v->t, "Invalid type_id");
 		return -EINVAL;
 	}
@@ -1205,7 +1212,7 @@ static int btf_ptr_resolve(struct btf_verifier_env *env,
 	}
 
 	/* "void *" */
-	if (btf_type_is_void(next_type))
+	if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type))
 		goto resolved;
 
 	if (!env_type_is_resolve_sink(env, next_type) &&
@@ -1235,7 +1242,7 @@ static int btf_ptr_resolve(struct btf_verifier_env *env,
 	}
 
 	if (!btf_type_id_size(btf, &next_type_id, &next_type_size) &&
-	    !btf_type_is_void(btf_type_id_resolve(btf, &next_type_id))) {
+	    !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) {
 		btf_verifier_log_type(env, v->t, "Invalid type_id");
 		return -EINVAL;
 	}
@@ -1396,7 +1403,7 @@ static int btf_array_resolve(struct btf_verifier_env *env,
 	/* Check array->index_type */
 	index_type_id = array->index_type;
 	index_type = btf_type_by_id(btf, index_type_id);
-	if (btf_type_is_void_or_null(index_type)) {
+	if (btf_type_nosize_or_null(index_type)) {
 		btf_verifier_log_type(env, v->t, "Invalid index");
 		return -EINVAL;
 	}
@@ -1415,7 +1422,7 @@ static int btf_array_resolve(struct btf_verifier_env *env,
 	/* Check array->type */
 	elem_type_id = array->type;
 	elem_type = btf_type_by_id(btf, elem_type_id);
-	if (btf_type_is_void_or_null(elem_type)) {
+	if (btf_type_nosize_or_null(elem_type)) {
 		btf_verifier_log_type(env, v->t,
 				      "Invalid elem");
 		return -EINVAL;
@@ -1615,7 +1622,7 @@ static int btf_struct_resolve(struct btf_verifier_env *env,
 		const struct btf_type *member_type = btf_type_by_id(env->btf,
 								member_type_id);
 
-		if (btf_type_is_void_or_null(member_type)) {
+		if (btf_type_nosize_or_null(member_type)) {
 			btf_verifier_log_member(env, v->t, member,
 						"Invalid member");
 			return -EINVAL;
-- 
2.17.1

[PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Yonghong Song]

This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
support to the type section. BTF_KIND_FUNC_PROTO is used
to specify the type of a function pointer. With this,
BTF has a complete set of C types (except float).

BTF_KIND_FUNC is used to specify the signature of a
defined subprogram. BTF_KIND_FUNC_PROTO can be referenced
by another type, e.g., a pointer type, and BTF_KIND_FUNC
type cannot be referenced by another type.

For both BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO types,
the func return type is in t->type (where t is a
"struct btf_type" object). The func args are an array of
u32s immediately following object "t".

As a concrete example, for the C program below,
  $ cat test.c
  int foo(int (*bar)(int)) { return bar(5); }
with LLVM patch https://reviews.llvm.org/D53261
in Debug mode, we have
  $ clang -target bpf -g -O2 -mllvm -debug-only=btf -c test.c
  Type Table:
  [1] FUNC NameOff=1 Info=0x0c000001 Size/Type=2
          ParamType=3
  [2] INT NameOff=11 Info=0x01000000 Size/Type=4
          Desc=0x01000020
  [3] PTR NameOff=0 Info=0x02000000 Size/Type=4
  [4] FUNC_PROTO NameOff=0 Info=0x0d000001 Size/Type=2
          ParamType=2

  String Table:
  0 :
  1 : foo
  5 : .text
  11 : int
  15 : test.c
  22 : int foo(int (*bar)(int)) { return bar(5); }

  FuncInfo Table:
  SecNameOff=5
          InsnOffset=<Omitted> TypeId=1

  ...

(Eventually we shall have bpftool to dump btf information
 like the above.)

Function "foo" has a FUNC type (type_id = 1).
The parameter of "foo" has type_id 3 which is PTR->FUNC_PROTO,
where FUNC_PROTO refers to function pointer "bar".

In FuncInfo Table, for section .text, the function,
with to-be-determined offset (marked as <Omitted>),
has type_id=1 which refers to a FUNC type.
This way, the function signature is
available to both kernel and user space.
Here, the insn offset is not available during the dump time
as relocation is resolved pretty late in the compilation process.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/uapi/linux/btf.h |   9 +-
 kernel/bpf/btf.c         | 280 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 253 insertions(+), 36 deletions(-)

diff --git a/include/uapi/linux/btf.h b/include/uapi/linux/btf.h
index 972265f32871..63f8500e6f34 100644
--- a/include/uapi/linux/btf.h
+++ b/include/uapi/linux/btf.h
@@ -40,7 +40,8 @@ struct btf_type {
 	/* "size" is used by INT, ENUM, STRUCT and UNION.
 	 * "size" tells the size of the type it is describing.
 	 *
-	 * "type" is used by PTR, TYPEDEF, VOLATILE, CONST and RESTRICT.
+	 * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
+	 * FUNC and FUNC_PROTO.
 	 * "type" is a type_id referring to another type.
 	 */
 	union {
@@ -64,8 +65,10 @@ struct btf_type {
 #define BTF_KIND_VOLATILE	9	/* Volatile	*/
 #define BTF_KIND_CONST		10	/* Const	*/
 #define BTF_KIND_RESTRICT	11	/* Restrict	*/
-#define BTF_KIND_MAX		11
-#define NR_BTF_KINDS		12
+#define BTF_KIND_FUNC		12	/* Function	*/
+#define BTF_KIND_FUNC_PROTO	13	/* Function Prototype	*/
+#define BTF_KIND_MAX		13
+#define NR_BTF_KINDS		14
 
 /* For some specific BTF_KIND, "struct btf_type" is immediately
  * followed by extra data.
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index be406d8906ce..763f8e06bc91 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5,6 +5,7 @@
 #include <uapi/linux/types.h>
 #include <linux/seq_file.h>
 #include <linux/compiler.h>
+#include <linux/ctype.h>
 #include <linux/errno.h>
 #include <linux/slab.h>
 #include <linux/anon_inodes.h>
@@ -259,6 +260,8 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
 	[BTF_KIND_VOLATILE]	= "VOLATILE",
 	[BTF_KIND_CONST]	= "CONST",
 	[BTF_KIND_RESTRICT]	= "RESTRICT",
+	[BTF_KIND_FUNC]		= "FUNC",
+	[BTF_KIND_FUNC_PROTO]	= "FUNC_PROTO",
 };
 
 struct btf_kind_operations {
@@ -281,6 +284,9 @@ struct btf_kind_operations {
 static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS];
 static struct btf_type btf_void;
 
+static int btf_resolve(struct btf_verifier_env *env,
+		       const struct btf_type *t, u32 type_id);
+
 static bool btf_type_is_modifier(const struct btf_type *t)
 {
 	/* Some of them is not strictly a C modifier
@@ -314,9 +320,20 @@ static bool btf_type_is_fwd(const struct btf_type *t)
 	return BTF_INFO_KIND(t->info) == BTF_KIND_FWD;
 }
 
+static bool btf_type_is_func(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info) == BTF_KIND_FUNC;
+}
+
+static bool btf_type_is_func_proto(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info) == BTF_KIND_FUNC_PROTO;
+}
+
 static bool btf_type_nosize(const struct btf_type *t)
 {
-	return btf_type_is_void(t) || btf_type_is_fwd(t);
+	return btf_type_is_void(t) || btf_type_is_fwd(t) ||
+	       btf_type_is_func(t) || btf_type_is_func_proto(t);
 }
 
 static bool btf_type_nosize_or_null(const struct btf_type *t)
@@ -433,6 +450,27 @@ static bool btf_name_offset_valid(const struct btf *btf, u32 offset)
 		offset < btf->hdr.str_len;
 }
 
+static bool btf_name_valid_identifier(const struct btf *btf, u32 offset)
+{
+	/* offset must be valid */
+	const char *src = &btf->strings[offset];
+	const char *src_limit;
+
+	if (!isalpha(*src) && *src != '_')
+		return false;
+
+	/* set a limit on identifier length */
+	src_limit = src + KSYM_NAME_LEN;
+	src++;
+	while (*src && src < src_limit) {
+		if (!isalnum(*src) && *src != '_')
+			return false;
+		src++;
+	}
+
+	return !*src;
+}
+
 static const char *btf_name_by_offset(const struct btf *btf, u32 offset)
 {
 	if (!offset)
@@ -747,7 +785,9 @@ static bool env_type_is_resolve_sink(const struct btf_verifier_env *env,
 		/* int, enum or void is a sink */
 		return !btf_type_needs_resolve(next_type);
 	case RESOLVE_PTR:
-		/* int, enum, void, struct or array is a sink for ptr */
+		/* int, enum, void, struct, array or func_ptoto is a sink
+		 * for ptr
+		 */
 		return !btf_type_is_modifier(next_type) &&
 			!btf_type_is_ptr(next_type);
 	case RESOLVE_STRUCT_OR_ARRAY:
@@ -1170,6 +1210,14 @@ static int btf_modifier_resolve(struct btf_verifier_env *env,
 		return -EINVAL;
 	}
 
+	if (btf_type_is_func_proto(next_type)) {
+		if (env->resolve_mode == RESOLVE_PTR)
+			goto resolved;
+
+		btf_verifier_log_type(env, v->t, "Invalid type_id");
+		return -EINVAL;
+	}
+
 	/* "typedef void new_void", "const void"...etc */
 	if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type))
 		goto resolved;
@@ -1185,7 +1233,8 @@ static int btf_modifier_resolve(struct btf_verifier_env *env,
 	 * pretty print).
 	 */
 	if (!btf_type_id_size(btf, &next_type_id, &next_type_size) &&
-	    !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) {
+	    (!btf_type_nosize(btf_type_id_resolve(btf, &next_type_id)) ||
+	     btf_type_is_func(next_type))) {
 		btf_verifier_log_type(env, v->t, "Invalid type_id");
 		return -EINVAL;
 	}
@@ -1212,7 +1261,8 @@ static int btf_ptr_resolve(struct btf_verifier_env *env,
 	}
 
 	/* "void *" */
-	if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type))
+	if (btf_type_is_void(next_type) || btf_type_is_fwd(next_type) ||
+	    btf_type_is_func_proto(next_type))
 		goto resolved;
 
 	if (!env_type_is_resolve_sink(env, next_type) &&
@@ -1242,7 +1292,8 @@ static int btf_ptr_resolve(struct btf_verifier_env *env,
 	}
 
 	if (!btf_type_id_size(btf, &next_type_id, &next_type_size) &&
-	    !btf_type_nosize(btf_type_id_resolve(btf, &next_type_id))) {
+	    (!btf_type_nosize(btf_type_id_resolve(btf, &next_type_id)) ||
+	     btf_type_is_func(next_type))) {
 		btf_verifier_log_type(env, v->t, "Invalid type_id");
 		return -EINVAL;
 	}
@@ -1550,6 +1601,14 @@ static s32 btf_struct_check_meta(struct btf_verifier_env *env,
 			return -EINVAL;
 		}
 
+		if (member->name_off &&
+		    !btf_name_valid_identifier(btf, member->name_off)) {
+			btf_verifier_log_member(env, t, member,
+						"Invalid member name:%s",
+						btf_name_by_offset(btf, member->name_off));
+			return -EINVAL;
+		}
+
 		/* A member cannot be in type void */
 		if (!member->type || !BTF_TYPE_ID_VALID(member->type)) {
 			btf_verifier_log_member(env, t, member,
@@ -1787,6 +1846,142 @@ static struct btf_kind_operations enum_ops = {
 	.seq_show = btf_enum_seq_show,
 };
 
+static s32 btf_func_check_meta(struct btf_verifier_env *env,
+			       const struct btf_type *t,
+			       u32 meta_left)
+{
+	u32 meta_needed = btf_type_vlen(t) * sizeof(u32);
+
+	if (meta_left < meta_needed) {
+		btf_verifier_log_basic(env, t,
+				       "meta_left:%u meta_needed:%u",
+				       meta_left, meta_needed);
+		return -EINVAL;
+	}
+
+	btf_verifier_log_type(env, t, NULL);
+
+	return meta_needed;
+}
+
+static void btf_func_log(struct btf_verifier_env *env,
+			 const struct btf_type *t)
+{
+	u16 nr_args = btf_type_vlen(t), i;
+	u32 *args = (u32 *)(t + 1);
+
+	btf_verifier_log(env, "return=%u args=(", t->type);
+	if (!nr_args) {
+		btf_verifier_log(env, "void");
+		goto done;
+	}
+
+	if (nr_args == 1 && !args[0]) {
+		/* Only one vararg */
+		btf_verifier_log(env, "vararg");
+		goto done;
+	}
+
+	btf_verifier_log(env, "%u", args[0]);
+	for (i = 1; i < nr_args - 1; i++)
+		btf_verifier_log(env, ", %u", args[i]);
+	if (nr_args > 1) {
+		u32 last_arg = args[nr_args - 1];
+
+		if (last_arg)
+			btf_verifier_log(env, ", %u", last_arg);
+		else
+			btf_verifier_log(env, ", vararg");
+	}
+
+done:
+	btf_verifier_log(env, ")");
+}
+
+static int btf_func_check(struct btf_verifier_env *env,
+			  const struct btf_type *t,
+			  u32 type_id)
+{
+	u16 nr_args = btf_type_vlen(t), i;
+	const struct btf *btf = env->btf;
+	const struct btf_type *ret_type;
+	u32 *args = (u32 *)(t + 1);
+	u32 ret_type_id;
+	int err;
+
+	/* Check func return type which could be "void" (t->type == 0) */
+	ret_type_id = t->type;
+	if (ret_type_id) {
+		/* return type is not "void" */
+		ret_type = btf_type_by_id(btf, ret_type_id);
+		if (btf_type_needs_resolve(ret_type) &&
+		    !env_type_is_resolved(env, ret_type_id)) {
+			err = btf_resolve(env, ret_type, ret_type_id);
+			if (err)
+				return err;
+		}
+
+		/* Ensure the return type is a type that has a size */
+		if (!btf_type_id_size(btf, &ret_type_id, NULL)) {
+			btf_verifier_log_type(env, t, "Invalid return type");
+			return -EINVAL;
+		}
+	}
+
+	if (!nr_args)
+		return 0;
+
+	/* Last func arg type_id could be 0 if it is a vararg */
+	if (!args[nr_args - 1])
+		nr_args--;
+
+	err = 0;
+	for (i = 0; i < nr_args; i++) {
+		const struct btf_type *arg_type;
+		u32 arg_type_id;
+
+		arg_type_id = args[i];
+		arg_type = btf_type_by_id(btf, arg_type_id);
+		if (!arg_type) {
+			btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1);
+			err = -EINVAL;
+			break;
+		}
+
+		if (btf_type_needs_resolve(arg_type) &&
+		    !env_type_is_resolved(env, arg_type_id)) {
+			err = btf_resolve(env, arg_type, arg_type_id);
+			if (err)
+				break;
+		}
+
+		if (!btf_type_id_size(btf, &arg_type_id, NULL)) {
+			btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1);
+			err = -EINVAL;
+			break;
+		}
+	}
+
+	return err;
+}
+
+static struct btf_kind_operations func_ops = {
+	.check_meta = btf_func_check_meta,
+	.resolve = btf_df_resolve,
+	/*
+	 * BPF_KIND_FUNC_PROTO cannot be directly referred by
+	 * a struct's member.
+	 *
+	 * It should be a funciton pointer instead.
+	 * (i.e. struct's member -> BTF_KIND_PTR -> BTF_KIND_FUNC_PROTO)
+	 *
+	 * Hence, there is no btf_func_check_member().
+	 */
+	.check_member = btf_df_check_member,
+	.log_details = btf_func_log,
+	.seq_show = btf_df_seq_show,
+};
+
 static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = {
 	[BTF_KIND_INT] = &int_ops,
 	[BTF_KIND_PTR] = &ptr_ops,
@@ -1799,6 +1994,8 @@ static const struct btf_kind_operations * const kind_ops[NR_BTF_KINDS] = {
 	[BTF_KIND_VOLATILE] = &modifier_ops,
 	[BTF_KIND_CONST] = &modifier_ops,
 	[BTF_KIND_RESTRICT] = &modifier_ops,
+	[BTF_KIND_FUNC] = &func_ops,
+	[BTF_KIND_FUNC_PROTO] = &func_ops,
 };
 
 static s32 btf_check_meta(struct btf_verifier_env *env,
@@ -1870,30 +2067,6 @@ static int btf_check_all_metas(struct btf_verifier_env *env)
 	return 0;
 }
 
-static int btf_resolve(struct btf_verifier_env *env,
-		       const struct btf_type *t, u32 type_id)
-{
-	const struct resolve_vertex *v;
-	int err = 0;
-
-	env->resolve_mode = RESOLVE_TBD;
-	env_stack_push(env, t, type_id);
-	while (!err && (v = env_stack_peak(env))) {
-		env->log_type_id = v->type_id;
-		err = btf_type_ops(v->t)->resolve(env, v);
-	}
-
-	env->log_type_id = type_id;
-	if (err == -E2BIG)
-		btf_verifier_log_type(env, t,
-				      "Exceeded max resolving depth:%u",
-				      MAX_RESOLVE_DEPTH);
-	else if (err == -EEXIST)
-		btf_verifier_log_type(env, t, "Loop detected");
-
-	return err;
-}
-
 static bool btf_resolve_valid(struct btf_verifier_env *env,
 			      const struct btf_type *t,
 			      u32 type_id)
@@ -1927,6 +2100,39 @@ static bool btf_resolve_valid(struct btf_verifier_env *env,
 	return false;
 }
 
+static int btf_resolve(struct btf_verifier_env *env,
+		       const struct btf_type *t, u32 type_id)
+{
+	u32 save_log_type_id = env->log_type_id;
+	const struct resolve_vertex *v;
+	int err = 0;
+
+	env->resolve_mode = RESOLVE_TBD;
+	env_stack_push(env, t, type_id);
+	while (!err && (v = env_stack_peak(env))) {
+		env->log_type_id = v->type_id;
+		err = btf_type_ops(v->t)->resolve(env, v);
+	}
+
+	env->log_type_id = type_id;
+	if (err == -E2BIG) {
+		btf_verifier_log_type(env, t,
+				      "Exceeded max resolving depth:%u",
+				      MAX_RESOLVE_DEPTH);
+	} else if (err == -EEXIST) {
+		btf_verifier_log_type(env, t, "Loop detected");
+	}
+
+	/* Final sanity check */
+	if (!err && !btf_resolve_valid(env, t, type_id)) {
+		btf_verifier_log_type(env, t, "Invalid resolve state");
+		err = -EINVAL;
+	}
+
+	env->log_type_id = save_log_type_id;
+	return err;
+}
+
 static int btf_check_all_types(struct btf_verifier_env *env)
 {
 	struct btf *btf = env->btf;
@@ -1949,10 +2155,18 @@ static int btf_check_all_types(struct btf_verifier_env *env)
 				return err;
 		}
 
-		if (btf_type_needs_resolve(t) &&
-		    !btf_resolve_valid(env, t, type_id)) {
-			btf_verifier_log_type(env, t, "Invalid resolve state");
-			return -EINVAL;
+		if (btf_type_is_func(t) || btf_type_is_func_proto(t)) {
+			if (btf_type_is_func(t) &&
+			    (!btf_name_offset_valid(btf, t->name_off) ||
+			     !btf_name_valid_identifier(btf, t->name_off))) {
+				btf_verifier_log_type(env, t,
+						      "Invalid type_id");
+				return -EINVAL;
+			}
+
+			err = btf_func_check(env, t, type_id);
+			if (err)
+				return err;
 		}
 	}
 
-- 
2.17.1

[PATCH bpf-next v2 03/13] tools/bpf: sync kernel btf.h header

[Yonghong Song]

The kernel uapi btf.h is synced to the tools directory.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/include/uapi/linux/btf.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h
index 972265f32871..63f8500e6f34 100644
--- a/tools/include/uapi/linux/btf.h
+++ b/tools/include/uapi/linux/btf.h
@@ -40,7 +40,8 @@ struct btf_type {
 	/* "size" is used by INT, ENUM, STRUCT and UNION.
 	 * "size" tells the size of the type it is describing.
 	 *
-	 * "type" is used by PTR, TYPEDEF, VOLATILE, CONST and RESTRICT.
+	 * "type" is used by PTR, TYPEDEF, VOLATILE, CONST, RESTRICT,
+	 * FUNC and FUNC_PROTO.
 	 * "type" is a type_id referring to another type.
 	 */
 	union {
@@ -64,8 +65,10 @@ struct btf_type {
 #define BTF_KIND_VOLATILE	9	/* Volatile	*/
 #define BTF_KIND_CONST		10	/* Const	*/
 #define BTF_KIND_RESTRICT	11	/* Restrict	*/
-#define BTF_KIND_MAX		11
-#define NR_BTF_KINDS		12
+#define BTF_KIND_FUNC		12	/* Function	*/
+#define BTF_KIND_FUNC_PROTO	13	/* Function Prototype	*/
+#define BTF_KIND_MAX		13
+#define NR_BTF_KINDS		14
 
 /* For some specific BTF_KIND, "struct btf_type" is immediately
  * followed by extra data.
-- 
2.17.1

[PATCH bpf-next v2 04/13] tools/bpf: add btf func/func_proto unit tests in selftest test_btf

[Yonghong Song]

Add several BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
unit tests in bpf selftest test_btf.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/lib/bpf/btf.c                    |   4 +
 tools/testing/selftests/bpf/test_btf.c | 216 +++++++++++++++++++++++++
 2 files changed, 220 insertions(+)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 449591aa9900..33095fc1860b 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -165,6 +165,10 @@ static int btf_parse_type_sec(struct btf *btf, btf_print_fn_t err_log)
 		case BTF_KIND_ENUM:
 			next_type += vlen * sizeof(struct btf_enum);
 			break;
+		case BTF_KIND_FUNC:
+		case BTF_KIND_FUNC_PROTO:
+			next_type += vlen * sizeof(int);
+			break;
 		case BTF_KIND_TYPEDEF:
 		case BTF_KIND_PTR:
 		case BTF_KIND_FWD:
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index f42b3396d622..b6461c3c5e11 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -1374,6 +1374,222 @@ static struct btf_raw_test raw_tests[] = {
 	.map_create_err = true,
 },
 
+{
+	.descr = "func pointer #1",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		/* int (*func)(int, unsigned int) */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 2), 1),	/* [3] */
+		1, 2,
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3),	/* [4] */
+		BTF_END_RAW,
+	},
+	.str_sec = "",
+	.str_sec_size = sizeof(""),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+},
+
+{
+	.descr = "func pointer #2",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		/* void (*func)(int, unsigned int, ....) */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 3), 0),	/* [3] */
+		1, 2, 0,
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3),	/* [4] */
+		BTF_END_RAW,
+	},
+	.str_sec = "",
+	.str_sec_size = sizeof(""),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+},
+
+{
+	.descr = "func pointer #3",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		/* void (*func)(void, int, unsigned int) */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 3), 0),	/* [3] */
+		1, 0, 2,
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3),	/* [4] */
+		BTF_END_RAW,
+	},
+	.str_sec = "",
+	.str_sec_size = sizeof(""),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+	.btf_load_err = true,
+	.err_str = "Invalid arg#2",
+},
+
+{
+	.descr = "func pointer #4",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		/*
+		 * Testing:
+		 * BTF_KIND_CONST => BTF_KIND_TYPEDEF => BTF_KIND_PTR =>
+		 * BTF_KIND_FUNC_PROTO
+		 */
+		/* typedef void (*func_ptr)(int, unsigned int) */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_TYPEDEF, 0, 0), 5),/* [3] */
+		/* const func_ptr */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_CONST, 0, 0), 3),	/* [4] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 6),	/* [5] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 2), 0),	/* [6] */
+		1, 2,
+		BTF_END_RAW,
+	},
+	.str_sec = "",
+	.str_sec_size = sizeof(""),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+},
+
+{
+	.descr = "func pointer #5",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		/*
+		 * Skipped the BTF_KIND_PTR.
+		 * BTF_KIND_CONST => BTF_KIND_TYPEDEF => BTF_KIND_FUNC_PROTO
+		 */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_CONST, 0, 0), 4),	/* [3] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_TYPEDEF, 0, 0), 5),/* [4] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 2), 0),	/* [5] */
+		1, 2,
+		BTF_END_RAW,
+	},
+	.str_sec = "",
+	.str_sec_size = sizeof(""),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+	.btf_load_err = true,
+	.err_str = "Invalid type_id",
+},
+
+{
+	/* Test btf_resolve() in btf_check_func() */
+	.descr = "func pointer #6",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		/* void (*func)(const void *) */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_FUNC_PROTO, 0, 1), 0),	/* [2] */
+		4,
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 2),	/* [3] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_CONST, 0, 0), 5),	/* [4] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 0),	/* [5] */
+		BTF_END_RAW,
+	},
+	.str_sec = "",
+	.str_sec_size = sizeof(""),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+},
+
+{
+	.descr = "func #1",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1),	/* [3] */
+		1, 2,
+		BTF_END_RAW,
+	},
+	.str_sec = "\0A\0",
+	.str_sec_size = sizeof("\0A\0"),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+},
+
+{
+	.descr = "func #2",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 1),	/* [3] */
+		1, 2,
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_PTR, 0, 0), 3),	/* [4] */
+		BTF_END_RAW,
+	},
+	.str_sec = "\0A\0",
+	.str_sec_size = sizeof("\0A\0"),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+	.btf_load_err = true,
+	.err_str = "Invalid type_id",
+},
+
+{
+	.descr = "func #3",
+	.raw_types = {
+		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),	/* [1] */
+		BTF_TYPE_INT_ENC(0, 0, 0, 32, 4),		/* [2] */
+		BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_TYPEDEF, 0, 0), 4),/* [3] */
+		BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 0),	/* [4] */
+		1, 2,
+		BTF_END_RAW,
+	},
+	.str_sec = "\0A\0",
+	.str_sec_size = sizeof("\0A\0"),
+	.map_type = BPF_MAP_TYPE_ARRAY,
+	.map_name = "func_type_check_btf",
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.key_type_id = 1,
+	.value_type_id = 1,
+	.max_entries = 4,
+	.btf_load_err = true,
+	.err_str = "Invalid type_id",
+},
+
 }; /* struct btf_raw_test raw_tests[] */
 
 static const char *get_next_str(const char *start, const char *end)
-- 
2.17.1

Re: [PATCH bpf-next v2 00/13] bpf: add btf func info support

[Edward Cree]

I think the BTF work needs to be better documented; at the moment the only way
 to determine how BTF sections are structured is to read through the headers,
 and cross-reference with the DWARF spec to guess at the semantics of various
 fields.  I've been working on adding BTF support to ebpf_asm, and finding
 very frustrating the amount of guesswork required.
Therefore please make sure that each patch extending the BTF format includes
 documentation patches describing both the layout and the semantics of the new
 extensions.  For example in patch #9 there is no explanation of
 btf_ext_header.line_info_off and btf_ext_header.line_info_len (they're not
 even used by the code, so one cannot reverse-engineer it); while it's fairly
 clear that they indicate the bounds of the line_info subsection, there is no
 specification of what this subsection contains.

-Ed

Re: [PATCH bpf-next v2 00/13] bpf: add btf func info support

[Yonghong Song]

On 10/17/18 4:02 AM, Edward Cree wrote:
> I think the BTF work needs to be better documented; at the moment the only way
>   to determine how BTF sections are structured is to read through the headers,
>   and cross-reference with the DWARF spec to guess at the semantics of various
>   fields.  I've been working on adding BTF support to ebpf_asm, and finding
>   very frustrating the amount of guesswork required.
> Therefore please make sure that each patch extending the BTF format includes
>   documentation patches describing both the layout and the semantics of the new

Make sense. I will add some comments to describe the layout in patch #9.

>   extensions.  For example in patch #9 there is no explanation of
>   btf_ext_header.line_info_off and btf_ext_header.line_info_len (they're not
>   even used by the code, so one cannot reverse-engineer it); while it's fairly
>   clear that they indicate the bounds of the line_info subsection, there is no

The line_info field is added because it is implemented in llvm. I 
imported it to kernel tools directory to be compatible with what llvm 
generates although we did not process it yet. I will add a comment on this.

In the long term, I guess we should add description of format etc.
in Documentation/bpf directory like BTF.rst.

>   specification of what this subsection contains.
> 
> -Ed
> 

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 17/10/18 08:23, Yonghong Song wrote:
> This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
> support to the type section. BTF_KIND_FUNC_PROTO is used
> to specify the type of a function pointer. With this,
> BTF has a complete set of C types (except float).
>
> BTF_KIND_FUNC is used to specify the signature of a
> defined subprogram. BTF_KIND_FUNC_PROTO can be referenced
> by another type, e.g., a pointer type, and BTF_KIND_FUNC
> type cannot be referenced by another type.
Why are distinct kinds created for these?  A function body is
 a value of function type, and since there's no way (in C) to
 declare a variable of function type (only pointer-to-
 function), any declaration of function type must necessarily
 be a BTF_KIND_FUNC, whereas any other reference to a function
 type (e.g. a declaration of type pointer to function type)
 must, as you state above, be a BTF_KIND_FUNC_PROTO.
In fact, you can tell the difference just from name_off, since
 a (C-legal) BTF_KIND_FUNC_PROTO will always be anonymous (as
 the pointee of a pointer type), while a BTF_KIND_FUNC will
 have the name of the subprogram.

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Yonghong Song]

On 10/17/18 9:13 AM, Edward Cree wrote:
> On 17/10/18 08:23, Yonghong Song wrote:
>> This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
>> support to the type section. BTF_KIND_FUNC_PROTO is used
>> to specify the type of a function pointer. With this,
>> BTF has a complete set of C types (except float).
>>
>> BTF_KIND_FUNC is used to specify the signature of a
>> defined subprogram. BTF_KIND_FUNC_PROTO can be referenced
>> by another type, e.g., a pointer type, and BTF_KIND_FUNC
>> type cannot be referenced by another type.
> Why are distinct kinds created for these?  A function body is
>   a value of function type, and since there's no way (in C) to
>   declare a variable of function type (only pointer-to-
>   function), any declaration of function type must necessarily
>   be a BTF_KIND_FUNC, whereas any other reference to a function
>   type (e.g. a declaration of type pointer to function type)
>   must, as you state above, be a BTF_KIND_FUNC_PROTO.
> In fact, you can tell the difference just from name_off, since
>   a (C-legal) BTF_KIND_FUNC_PROTO will always be anonymous (as
>   the pointee of a pointer type), while a BTF_KIND_FUNC will
>   have the name of the subprogram.

What you stated is true, BTF_KIND_FUNC_PROTO corresponds to
dwarf subroutine tag which has no name while BTF_KIND_FUNC
must have a valid name. The original design is to have both
since they are corresponding to different dwarf constructs.

Martin, what do you think?

> 
> -Ed
> 

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Martin Lau]

On Wed, Oct 17, 2018 at 10:25:21AM -0700, Yonghong Song wrote:
> 
> 
> On 10/17/18 9:13 AM, Edward Cree wrote:
> > On 17/10/18 08:23, Yonghong Song wrote:
> >> This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
> >> support to the type section. BTF_KIND_FUNC_PROTO is used
> >> to specify the type of a function pointer. With this,
> >> BTF has a complete set of C types (except float).
> >>
> >> BTF_KIND_FUNC is used to specify the signature of a
> >> defined subprogram. BTF_KIND_FUNC_PROTO can be referenced
> >> by another type, e.g., a pointer type, and BTF_KIND_FUNC
> >> type cannot be referenced by another type.
> > Why are distinct kinds created for these?  A function body is
> >   a value of function type, and since there's no way (in C) to
> >   declare a variable of function type (only pointer-to-
> >   function), any declaration of function type must necessarily
> >   be a BTF_KIND_FUNC, whereas any other reference to a function
> >   type (e.g. a declaration of type pointer to function type)
> >   must, as you state above, be a BTF_KIND_FUNC_PROTO.
> > In fact, you can tell the difference just from name_off, since
> >   a (C-legal) BTF_KIND_FUNC_PROTO will always be anonymous (as
> >   the pointee of a pointer type), while a BTF_KIND_FUNC will
> >   have the name of the subprogram.
> 
> What you stated is true, BTF_KIND_FUNC_PROTO corresponds to
> dwarf subroutine tag which has no name while BTF_KIND_FUNC
> must have a valid name. The original design is to have both
> since they are corresponding to different dwarf constructs.
> 
> Martin, what do you think?
I prefer to have separate kinds.  We need a way to distinguish them.
For example, the BTF verifier is checking it.  Having two kinds is
cleaner instead of resorting to other hints from 'struct btf_type'.
We don't lack of bits for kind.


> 
> > 
> > -Ed
> > 

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 17/10/18 18:50, Martin Lau wrote:
> On Wed, Oct 17, 2018 at 10:25:21AM -0700, Yonghong Song wrote:
>> What you stated is true, BTF_KIND_FUNC_PROTO corresponds to
>> dwarf subroutine tag which has no name while BTF_KIND_FUNC
>> must have a valid name. The original design is to have both
>> since they are corresponding to different dwarf constructs.
>>
>> Martin, what do you think?
> I prefer to have separate kinds.  We need a way to distinguish them.
> For example, the BTF verifier is checking it.  Having two kinds is
> cleaner instead of resorting to other hints from 'struct btf_type'.
> We don't lack of bits for kind.
But my point is that (a) they can be distinguished by how they are
 used, and (b) the _only_ difference between them is how they are
 used.  In this C code:
    int a = 5;
    int foo(int x) { return a; }
    int *b = &a;
    int (*bar)(int) = &foo;
 foo and *bar are _the same type_, just as a and *b are.  It's just
 that C has a slightly odd way of writing
    int foo(int) = lambda x: a;
 and foo itself is implicitly sorta-const.
What am I missing?

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Martin Lau]

On Thu, Oct 18, 2018 at 05:47:11PM +0100, Edward Cree wrote:
> On 17/10/18 18:50, Martin Lau wrote:
> > On Wed, Oct 17, 2018 at 10:25:21AM -0700, Yonghong Song wrote:
> >> What you stated is true, BTF_KIND_FUNC_PROTO corresponds to
> >> dwarf subroutine tag which has no name while BTF_KIND_FUNC
> >> must have a valid name. The original design is to have both
> >> since they are corresponding to different dwarf constructs.
> >>
> >> Martin, what do you think?
> > I prefer to have separate kinds.  We need a way to distinguish them.
> > For example, the BTF verifier is checking it.  Having two kinds is
> > cleaner instead of resorting to other hints from 'struct btf_type'.
> > We don't lack of bits for kind.
> But my point is that (a) they can be distinguished by how they are
>  used, and (b) the _only_ difference between them is how they are
>  used.  In this C code:
>     int a = 5;
>     int foo(int x) { return a; }
>     int *b = &a;
>     int (*bar)(int) = &foo;
>  foo and *bar are _the same type_, just as a and *b are.  It's just
>  that C has a slightly odd way of writing
>     int foo(int) = lambda x: a;
>  and foo itself is implicitly sorta-const.
> What am I missing?
The BTF verification and bpf_prog_load() has to treat
them differently.

Are you suggesting they can be treated the same for
the kernel verification purpose?

or

The concern is, having two kind, BTF_KIND_FUNC_PROTO and
BTF_KIND_FUNC, is confusing because they have almost the
same BTF metadata?

>From the BTF perspective, they are different because
they are allowed to contain different information.
For example, only "foo" can have func_info in patch 5
(the to-be-added line_info can only belong to "foo" also).

This patch 2 is also doing checks on func_proto.
e.g. in btf_ptr_resolve().

The early idea is to have "foo_a", "foo_b", "foo_c"...
in a separate BTF section called "func" section (on top of
the current "type" and "string" section).  By doing this,
only one BTF_KIND_FUNC would be needed as you suggested
because "foo" could be clearly distinguished from "bar" by
observing they belong to different BTF sections.  The down
side is the "func" section will have its own id space separated
from the current "type" section id space and it is less ideal
for debugging purpose.

Hence, "foo" is contained in the "type" section also
and added FUNC_PROTO to distinguish them.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 18/10/18 19:12, Martin Lau wrote:
> The BTF verification and bpf_prog_load() has to treat
> them differently.
>
> Are you suggesting they can be treated the same for
> the kernel verification purpose?
>
> or
>
> The concern is, having two kind, BTF_KIND_FUNC_PROTO and
> BTF_KIND_FUNC, is confusing because they have almost the
> same BTF metadata?
>
> From the BTF perspective, they are different because
> they are allowed to contain different information.
> For example, only "foo" can have func_info in patch 5
> (the to-be-added line_info can only belong to "foo" also).
Perhaps I'm not properly understanding how BTF is to be used.
But from my perspective, 'foo' is not a type; it's an instance
 of a type; i.e. it's a value of type int ()(int).  Then by a
 kind of metonymy, we are including a BTF record that defines
 the type 'foo' as int()(int), in order to state that this is
 the type _of_ foo.  Similarly, we are including a BTF record
 defining 'bar' as the type int (*)(int), thus stating that
 the type _of_ bar is int (*)(int).
So everything that's special about 'foo' relates to the fact
 that it is a named, rather than anonymous, type, which is
 being used to indicate that there is an actual object 'foo'
 of this type.
And this is *identical* to how the BTF record "name 'a', type
 int" declares an object a, the BTF record "name 'b', type
 int *" declares an object b, and the anonymous int type
 referenced by b's btf_type.type does not declare an object.

So either types and objects are represented the same way in
 BTF, or not; it's inconsistent to make a distinction between
 them for functions but not for other types.

I know C thinks functions are special, but that's a wart in C
 that maybe makes sense in its context but not elsewhere.

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Martin Lau]

On Thu, Oct 18, 2018 at 08:41:52PM +0100, Edward Cree wrote:
> On 18/10/18 19:12, Martin Lau wrote:
> > The BTF verification and bpf_prog_load() has to treat
> > them differently.
> >
> > Are you suggesting they can be treated the same for
> > the kernel verification purpose?
> >
> > or
> >
> > The concern is, having two kind, BTF_KIND_FUNC_PROTO and
> > BTF_KIND_FUNC, is confusing because they have almost the
> > same BTF metadata?
> >
> > From the BTF perspective, they are different because
> > they are allowed to contain different information.
> > For example, only "foo" can have func_info in patch 5
> > (the to-be-added line_info can only belong to "foo" also).
> Perhaps I'm not properly understanding how BTF is to be used.
> But from my perspective, 'foo' is not a type; it's an instance
>  of a type; i.e. it's a value of type int ()(int).  Then by a
>  kind of metonymy, we are including a BTF record that defines
>  the type 'foo' as int()(int), in order to state that this is
>  the type _of_ foo.  Similarly, we are including a BTF record
>  defining 'bar' as the type int (*)(int), thus stating that
>  the type _of_ bar is int (*)(int).
> So everything that's special about 'foo' relates to the fact
>  that it is a named, rather than anonymous, type, which is
>  being used to indicate that there is an actual object 'foo'
>  of this type.
> And this is *identical* to how the BTF record "name 'a', type
>  int" declares an object a, the BTF record "name 'b', type
>  int *" declares an object b, and the anonymous int type
>  referenced by b's btf_type.type does not declare an object.
> 
> So either types and objects are represented the same way in
>  BTF, or not; it's inconsistent to make a distinction between
>  them for functions but not for other types.
> 
> I know C thinks functions are special, but that's a wart in C
>  that maybe makes sense in its context but not elsewhere.
As I have mentioned earlier, it is also special to
the kernel because the BTF verifier and bpf_prog_load()
need to do different checks for FUNC and FUNC_PROTO to
ensure they are sane.

First, we need to agree that the kernel needs to verify
them differently.

and then we can proceed to discuss how to distinguish them.
We picked the current way to avoid adding a
new BTF function section and keep it
strict forward to distinguish them w/o relying
on other hints from 'struct btf_type'.

Are you suggesting another way of doing it?

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 18/10/18 22:19, Martin Lau wrote:
> As I have mentioned earlier, it is also special to
> the kernel because the BTF verifier and bpf_prog_load()
> need to do different checks for FUNC and FUNC_PROTO to
> ensure they are sane.
>
> First, we need to agree that the kernel needs to verify
> them differently.
>
> and then we can proceed to discuss how to distinguish them.
> We picked the current way to avoid adding a
> new BTF function section and keep it
> strict forward to distinguish them w/o relying
> on other hints from 'struct btf_type'.
>
> Are you suggesting another way of doing it?
But you *do* have such a new section.
The patch comment talks about a 'FuncInfo Table' which appears to
 map (section, insn_idx) to type_id.  (I think this gets added in
 .BTF.ext per patch 9?)  So when you're looking at a FUNC type
 because you looked up a type_id from that table, you know it's
 the signature of a subprogram, and you're checking it as such.
Whereas, if you were doing something with some other type and it
 referenced a FUNC type (e.g., in the patch comment's example,
 you're checking foo's first argument against the type bar) in
 its type_id, you know you're using it as a formal type (a FUNC_
 PROTO in your parlance) and not as a subprogram.
The context in which you are using a type entry tells you which
 kind it is.  And the verifier can and should be smart enough to
 know what it's doing in this way.

And it's entirely reasonable for the same type entry to get used
 for both those cases; in my example, you'd have a FUNC type for
 int foo(int), referenced both by the func_info entry for foo
 and by the PTR type for bar.  And if you had another subprogram
 int baz(int), its func_info entry could reference the same
 type_id, because the (reference to the) name of the function
 should live in the func_info, not in the type.

What you are proposing seems to be saying "if we have this
 particular special btf_kind, then this BTF entry doesn't just
 define a type, it declares a subprogram of that type".  Oh,
 and with the name of the type as the subprogram name.  Which
 just creates blurry confusion as to whether BTF entries define
 types or declare objects; IMNSHO the correct approach is for
 objects to be declared elsewhere and to reference BTF types by
 their type_id.
Which is what the func_info table in patch 9 appears to do.

(It also rather bothers me the way we are using special type
 names to associate maps with their k/v types, rather than
 extending the records in the maps section to include type_ids
 referencing them.  It's the same kind of weird implicitness,
 and if I'd spotted it when it was going in I'd've nacked it,
 but I suppose it's ABI now and too late to change.)

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Martin Lau]

On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote:
> On 18/10/18 22:19, Martin Lau wrote:
> > As I have mentioned earlier, it is also special to
> > the kernel because the BTF verifier and bpf_prog_load()
> > need to do different checks for FUNC and FUNC_PROTO to
> > ensure they are sane.
> >
> > First, we need to agree that the kernel needs to verify
> > them differently.
> >
> > and then we can proceed to discuss how to distinguish them.
> > We picked the current way to avoid adding a
> > new BTF function section and keep it
> > strict forward to distinguish them w/o relying
> > on other hints from 'struct btf_type'.
> >
> > Are you suggesting another way of doing it?
> But you *do* have such a new section.
> The patch comment talks about a 'FuncInfo Table' which appears to
Note that the new section, which contains the FuncInfo Table,
is in a new ELF section ".BTF.ext" instead of the ".BTF".
It is not in the ".BTF" section because it is only useful during
bpf_prog_load().

I was meaning a new function section within ".BTF".

>  map (section, insn_idx) to type_id.  (I think this gets added in
>  .BTF.ext per patch 9?)  So when you're looking at a FUNC type
>  because you looked up a type_id from that table, you know it's
>  the signature of a subprogram, and you're checking it as such.
> Whereas, if you were doing something with some other type and it
>  referenced a FUNC type (e.g., in the patch comment's example,
>  you're checking foo's first argument against the type bar) in
>  its type_id, you know you're using it as a formal type (a FUNC_
>  PROTO in your parlance) and not as a subprogram.
> The context in which you are using a type entry tells you which
>  kind it is.  And the verifier can and should be smart enough to
>  know what it's doing in this way.
> 
> And it's entirely reasonable for the same type entry to get used
>  for both those cases; in my example, you'd have a FUNC type for
>  int foo(int), referenced both by the func_info entry for foo
>  and by the PTR type for bar.  And if you had another subprogram
>  int baz(int), its func_info entry could reference the same
>  type_id, because the (reference to the) name of the function
>  should live in the func_info, not in the type.
IIUC, I think what you are suggesting here is to use (type_id, name)
to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}",
"int foo3(int) {}" where type_id here is referring to the same
DW_TAG_subroutine_type, and only define that _one_
DW_TAG_subroutine_type in the BTF "type" section.

That will require more manipulation/type-merging in the dwarf2btf
process and it could get quite complicated.

Note that CTF is also fully spelling out the return type
and arg types for each DW_TAG_subprogram in a separate
function section (still within the same ELF section).
The only difference here is they are merged into the type
section and FUNC_PROTO is added.

If the concern is having both FUNC and FUNC_PROTO is confusing,
we could go back to the CTF way which adds a new function section
in ".BTF" and it is only for DW_TAG_subprogram.
BTF_KIND_FUNC_PROTO is then no longer necessary.
Some of new BTF verifier checkings may actually go away also.
The down side is there will be two id spaces.

> 
> What you are proposing seems to be saying "if we have this
>  particular special btf_kind, then this BTF entry doesn't just
>  define a type, it declares a subprogram of that type".  Oh,
>  and with the name of the type as the subprogram name.  Which
>  just creates blurry confusion as to whether BTF entries define
>  types or declare objects; IMNSHO the correct approach is for
>  objects to be declared elsewhere and to reference BTF types by
>  their type_id.
> Which is what the func_info table in patch 9 appears to do.
> 
> (It also rather bothers me the way we are using special type
>  names to associate maps with their k/v types, rather than
>  extending the records in the maps section to include type_ids
>  referencing them.  It's the same kind of weird implicitness,
>  and if I'd spotted it when it was going in I'd've nacked it,
>  but I suppose it's ABI now and too late to change.)
> 
> -Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Martin Lau]

On Fri, Oct 19, 2018 at 12:36:49PM -0700, Martin Lau wrote:
> On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote:
> > On 18/10/18 22:19, Martin Lau wrote:
> > > As I have mentioned earlier, it is also special to
> > > the kernel because the BTF verifier and bpf_prog_load()
> > > need to do different checks for FUNC and FUNC_PROTO to
> > > ensure they are sane.
> > >
> > > First, we need to agree that the kernel needs to verify
> > > them differently.
> > >
> > > and then we can proceed to discuss how to distinguish them.
> > > We picked the current way to avoid adding a
> > > new BTF function section and keep it
> > > strict forward to distinguish them w/o relying
> > > on other hints from 'struct btf_type'.
> > >
> > > Are you suggesting another way of doing it?
> > But you *do* have such a new section.
> > The patch comment talks about a 'FuncInfo Table' which appears to
> Note that the new section, which contains the FuncInfo Table,
> is in a new ELF section ".BTF.ext" instead of the ".BTF".
> It is not in the ".BTF" section because it is only useful during
> bpf_prog_load().
> 
> I was meaning a new function section within ".BTF".
> 
> >  map (section, insn_idx) to type_id.  (I think this gets added in
> >  .BTF.ext per patch 9?)  So when you're looking at a FUNC type
> >  because you looked up a type_id from that table, you know it's
> >  the signature of a subprogram, and you're checking it as such.
> > Whereas, if you were doing something with some other type and it
> >  referenced a FUNC type (e.g., in the patch comment's example,
> >  you're checking foo's first argument against the type bar) in
> >  its type_id, you know you're using it as a formal type (a FUNC_
> >  PROTO in your parlance) and not as a subprogram.
> > The context in which you are using a type entry tells you which
> >  kind it is.  And the verifier can and should be smart enough to
> >  know what it's doing in this way.
> > 
> > And it's entirely reasonable for the same type entry to get used
> >  for both those cases; in my example, you'd have a FUNC type for
> >  int foo(int), referenced both by the func_info entry for foo
> >  and by the PTR type for bar.  And if you had another subprogram
> >  int baz(int), its func_info entry could reference the same
> >  type_id, because the (reference to the) name of the function
> >  should live in the func_info, not in the type.
> IIUC, I think what you are suggesting here is to use (type_id, name)
> to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}",
> "int foo3(int) {}" where type_id here is referring to the same
> DW_TAG_subroutine_type, and only define that _one_
> DW_TAG_subroutine_type in the BTF "type" section.
> 
> That will require more manipulation/type-merging in the dwarf2btf
> process and it could get quite complicated.
> 
> Note that CTF is also fully spelling out the return type
> and arg types for each DW_TAG_subprogram in a separate
> function section (still within the same ELF section).
> The only difference here is they are merged into the type
> section and FUNC_PROTO is added.
> 
> If the concern is having both FUNC and FUNC_PROTO is confusing,
> we could go back to the CTF way which adds a new function section
> in ".BTF" and it is only for DW_TAG_subprogram.
> BTF_KIND_FUNC_PROTO is then no longer necessary.
> Some of new BTF verifier checkings may actually go away also.
> The down side is there will be two id spaces.
Discussed a bit offline with folks about the two id spaces
situation and it is not good for debugging purpose.

If we must get rid of FUNC_PROTO, it is better to use the
name_off==0 check instead of adding a new function section.
We will go for this path in the next respin.

> 
> > 
> > What you are proposing seems to be saying "if we have this
> >  particular special btf_kind, then this BTF entry doesn't just
> >  define a type, it declares a subprogram of that type".  Oh,
> >  and with the name of the type as the subprogram name.  Which
> >  just creates blurry confusion as to whether BTF entries define
> >  types or declare objects; IMNSHO the correct approach is for
> >  objects to be declared elsewhere and to reference BTF types by
> >  their type_id.
> > Which is what the func_info table in patch 9 appears to do.
> > 
> > (It also rather bothers me the way we are using special type
> >  names to associate maps with their k/v types, rather than
> >  extending the records in the maps section to include type_ids
> >  referencing them.  It's the same kind of weird implicitness,
> >  and if I'd spotted it when it was going in I'd've nacked it,
> >  but I suppose it's ABI now and too late to change.)
> > 
> > -Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 19/10/18 20:36, Martin Lau wrote:
> On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote:
>> But you *do* have such a new section.
>> The patch comment talks about a 'FuncInfo Table' which appears to
> Note that the new section, which contains the FuncInfo Table,
> is in a new ELF section ".BTF.ext" instead of the ".BTF".
> It is not in the ".BTF" section because it is only useful during
> bpf_prog_load().
I thought it was because it needed to be munged by the loader/linker?

> IIUC, I think what you are suggesting here is to use (type_id, name)
> to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}",
> "int foo3(int) {}" where type_id here is referring to the same
> DW_TAG_subroutine_type, and only define that _one_
> DW_TAG_subroutine_type in the BTF "type" section.
Yes, something like that.

> If the concern is having both FUNC and FUNC_PROTO is confusing,
The concern is that you're conflating different entities (types
 and instances); FUNC_PROTO is just a symptom/canary of that.

> we could go back to the CTF way which adds a new function section
> in ".BTF" and it is only for DW_TAG_subprogram.
> BTF_KIND_FUNC_PROTO is then no longer necessary.
> Some of new BTF verifier checkings may actually go away also.
> The down side is there will be two id spaces.
Two id spaces... one for types and the other for subprograms.
These are different things, so why would you _want_ them to share
 an id space?  I don't, for instance, see any situation in which
 you'd want some other record to have a field that could reference
 either.
And the 'subprogram id' doesn't have to be just for subprograms;
 it could be for instances generally — like I've been saying, a
 variable declaration is to an object type what a subprogram is to
 a function type, just with a few complications like "subprograms
 can only appear at file scope, not nested in other functions" and
 "variables of function type are immutable".
(I'm assuming that at some point we're going to want to be able to
 have BTF information for e.g. variables stored on a subprogram's
 stack, if only for stuff like single-stepping in a debugger in
 userspace with some sort of mock.  At that point, the variable
 has to have its own record — you can't just have some sort of
 magic type record because e.g. "struct foo bar;" has two names,
 one for the type and one for the variable.)

> Discussed a bit offline with folks about the two id spaces
> situation and it is not good for debugging purpose.
Could you unpack this a bit more?

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Martin Lau]

On Fri, Oct 19, 2018 at 10:26:53PM +0100, Edward Cree wrote:
> On 19/10/18 20:36, Martin Lau wrote:
> > On Fri, Oct 19, 2018 at 06:04:11PM +0100, Edward Cree wrote:
> >> But you *do* have such a new section.
> >> The patch comment talks about a 'FuncInfo Table' which appears to
> > Note that the new section, which contains the FuncInfo Table,
> > is in a new ELF section ".BTF.ext" instead of the ".BTF".
> > It is not in the ".BTF" section because it is only useful during
> > bpf_prog_load().
> I thought it was because it needed to be munged by the loader/linker?
> 
> > IIUC, I think what you are suggesting here is to use (type_id, name)
> > to describe DW_TAG_subprogram "int foo1(int) {}", "int foo2(int) {}",
> > "int foo3(int) {}" where type_id here is referring to the same
> > DW_TAG_subroutine_type, and only define that _one_
> > DW_TAG_subroutine_type in the BTF "type" section.
> Yes, something like that.
> 
> > If the concern is having both FUNC and FUNC_PROTO is confusing,
> The concern is that you're conflating different entities (types
>  and instances); FUNC_PROTO is just a symptom/canary of that.
> 
> > we could go back to the CTF way which adds a new function section
> > in ".BTF" and it is only for DW_TAG_subprogram.
> > BTF_KIND_FUNC_PROTO is then no longer necessary.
> > Some of new BTF verifier checkings may actually go away also.
> > The down side is there will be two id spaces.
> Two id spaces... one for types and the other for subprograms.
> These are different things, so why would you _want_ them to share
>  an id space?  I don't, for instance, see any situation in which
>  you'd want some other record to have a field that could reference
>  either.
> And the 'subprogram id' doesn't have to be just for subprograms;
>  it could be for instances generally — like I've been saying, a
>  variable declaration is to an object type what a subprogram is to
>  a function type, just with a few complications like "subprograms
>  can only appear at file scope, not nested in other functions" and
>  "variables of function type are immutable".
> (I'm assuming that at some point we're going to want to be able to
>  have BTF information for e.g. variables stored on a subprogram's
>  stack, if only for stuff like single-stepping in a debugger in
>  userspace with some sort of mock.  At that point, the variable
>  has to have its own record — you can't just have some sort of
>  magic type record because e.g. "struct foo bar;" has two names,
>  one for the type and one for the variable.)
> 
btf_type is not exactly a C type.

btf_type is a debug-info.  Each btf_type carries specific
debug information.  Name is part of the debug-info/btf_type.
If something carries different debug-info, it is another btf_type.
Like struct, the member's names of struct is part of the btf_type.
A struct with the same member's types but different member's names
is a different btf_type.

The same go for function.  The function with different function
names and arg names is a different btf_type.

> > Discussed a bit offline with folks about the two id spaces
> > situation and it is not good for debugging purpose.
> Could you unpack this a bit more?
Having two id spaces for debug-info is confusing.  They are
all debug-info at the end.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

I've spent a bit more time thinking about / sleeping on this, and I
 still think there's a major disagreement here.  Basically it seems
 like I'm saying "the design of BTF is wrong" and you're saying "but
 it's the design" (with the possible implication — I'm not entirely
 sure — of "but that's what DWARF does").
So let's back away from the details about FUNC/PROTO, and talk in
 more general terms about what a BTF record means.
There are two classes of things we might want to put in debug-info:
 * There exists a type T
 * I have an instance X (variable, subprogram, etc.) of type T
Both of these may need to reference other types, and have the same
 space of possible things T could be, but there the similarity ends:
 they are semantically different things.
Indeed, the only reason for any record of the first class is to
 define types referenced by records of the second class.  Some
 concrete examples of records of the second class are:
1) I have a map named "foo" with key-type T1 and value-type T2
2) I have a subprogram named "bar" with prototype T3
3) I am using stack slot fp-8 to store a value of type T4
4) I am casting ctx+8 to a pointer type T5 before dereferencing it
Currently we have (1) and this patch series adds (2), both done
 through records that look like they are just defining a type (i.e.
 the first class of record) but have 'magic' semantics (in the case
 of (1), special names of the form ____btf_map_foo.  How anyone
 thought that was a clean and tasteful design is beyond me.)
What IMHO the design *should* be, is that we have a 'types'
 subsection that *only* contains records of the first class, and
 then other subsections to hold records of the second class that
 reference records of the first class by ID.  So for (1) you'd have
 either additional fields in struct bpf_map_def (we've extended that
 several times before, after all), or you'd have a maps table in
 .BTF that links map names ("foo", not "____btf_map_foo"!) with type
 IDs for its key and value:
    struct btf_map_record {
        __u32 name_off; /* name of map */
        __u32 key_type_id; /* index in "types" table */
        __u32 value_type_id; /* ditto */
    }
(Note the absence of any meaningless struct type as created by
 BPF_ANNOTATE_KV_PAIR.  That kind of source-level hack should be
 converted by the compiler's BTF output module into something less
 magic, rather than baked into the format definition.)
Then for (2) you'd have a functions table in .BTF that links subprog
 names, start offsets, and signatures/prototypes:
    struct btf_func_record {
        __u32 name_off; /* name of function */
        __u16 subprog_secn; /* section index in which func appears */
        __u16 subprog_start; /* offset in section of func entry point */
        __u32 type_id; /* index in "types" table of func signature */
    }

I believe this is a much cleaner design, which will be easier to extend
 in the future to add things like (3) and (4) for source-line-level
 debug information.  I also believe that if someone had written
 documentation describing the original design, semantics of the various
 BTF records, etc., it would have been immediately obvious that the
 design was needlessly confusing and ad-hoc.

On 20/10/18 00:27, Martin Lau wrote:
> Like struct, the member's names of struct is part of the btf_type.
> A struct with the same member's types but different member's names
> is a different btf_type.
Yes, but that's not what I'm talking about.  I'm talking about structs
 with the same member names, but with different names of the structs.
As in the following C snippet:

struct foo {
    int i;
};

int main(void)
{
    struct foo x;
    struct foo y;

    x.i = 0
    y.i = x.i;

    return y.i;
}

We have one type 'struct foo' (name "foo"), but two _instances_ of
 that type (names "x", "y").  We *cannot* use a single BTF record to
 express both "x" and its type, because its type has a name of its
 own ("foo") and there is only room in struct btf_type for one name.
Thus we must have one record for the instance "x" and another record
 for the type "foo", with the former referencing the latter.

> Having two id spaces for debug-info is confusing.  They are
> all debug-info at the end.
But they have different semantics!  Just because you have a term,
 "debug-info", that's defined to cover both, doesn't mean that they
 are the same thing.  You might as well say that passport numbers and
 telephone numbers should be drawn from the same numbering space,
 because they're both "personal information", and never mind that one
 identifies a person and the other identifies a telephone.
It's having the _same_ id space for entities that are almost, but not
 quite, entirely unlike each other that's confusing.

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On Thu, Nov 01, 2018 at 09:08:37PM +0000, Edward Cree wrote:
> I've spent a bit more time thinking about / sleeping on this, and I
>  still think there's a major disagreement here.  Basically it seems
>  like I'm saying "the design of BTF is wrong" and you're saying "but
>  it's the design" (with the possible implication — I'm not entirely
>  sure — of "but that's what DWARF does").
> So let's back away from the details about FUNC/PROTO, and talk in
>  more general terms about what a BTF record means.
> There are two classes of things we might want to put in debug-info:
>  * There exists a type T
>  * I have an instance X (variable, subprogram, etc.) of type T
> Both of these may need to reference other types, and have the same
>  space of possible things T could be, but there the similarity ends:
>  they are semantically different things.
> Indeed, the only reason for any record of the first class is to
>  define types referenced by records of the second class.  Some
>  concrete examples of records of the second class are:
> 1) I have a map named "foo" with key-type T1 and value-type T2
> 2) I have a subprogram named "bar" with prototype T3
> 3) I am using stack slot fp-8 to store a value of type T4
> 4) I am casting ctx+8 to a pointer type T5 before dereferencing it
> Currently we have (1) and this patch series adds (2), both done
>  through records that look like they are just defining a type (i.e.
>  the first class of record) but have 'magic' semantics (in the case
>  of (1), special names of the form ____btf_map_foo.  How anyone
>  thought that was a clean and tasteful design is beyond me.)
> What IMHO the design *should* be, is that we have a 'types'
>  subsection that *only* contains records of the first class, and
>  then other subsections to hold records of the second class that
>  reference records of the first class by ID.

Such pure type approach wouldn't be practical.
BTF is not pure type information. BTF is everything that verifier needs
to know to make safety decisions that bpf instruction set doesn't have.
For example two anonymous structs:
struct {
 int a;
 int b;
} var1;
struct {
  int c;
  int d;
} var2;
from C point of view have the same type, but from BTF point of view
they are different. Names of the fields are essential part of the BTF
because the purpose of BTF is to provide information about bpf objects
for debugging and safety reasons.
Similarly
int (*) (void *src, void *dst, int len);
and
int (*) (void *dst, void *src, int len);
are the same from C and compiler point of view,
but they are different in BTF, because names carry information
that needs to be preserved.
Same goes for function declarations. The function name and argument
names are part of the 'type description'.
We shouldn't be using word 'type' in pure form otherwise
it will cause confusion like this thread demonstrated.
Beyond prog names expressed in BTF we're adding global variables support.
They will be expressed in BTF as a new KIND.
Think of all global variables in a single .c file as fields of
anonymous structure. They have offsets from beginning of .bss,
sizes, further references into btf_type_ids and most importantly names.
Another thing we're working on is spin_lock support.
There we also have to rely on BTF to make sure that the certain
bytes of map's value or cgroup local storage that belong to spin_lock
will be masked for lookup/update calls.
typedef u32 bpf_spin_lock;
will be recognized by the verifier by its name.
May be we will introduce new KIND_ for spin_lock too and convert name
into KIND in btf writer. That is TBD. The main point that
names of types, variables, functions has to be expressed in BTF
as one coherent graph of information.
Splitting pure types into one section, variables into another,
functions into yet another is not practical, since the same
modifiers (like const or volatile) need to be applied to
variables and functions. At the end all sections will have
the same style of encoding, hence no need to duplicate
the encoding three times and instead it's cleaner to encode
all of them BTF-style via different KINDs.
vmlinux's BTF will include all global variables and functions,
so that tracing scripts can reference to particular variable
or function argument by name making kernel debuging easier
and less error prone.
Consider that right now typical bcc's trace.py script looks like:
trace.py -I linux/skbuff.h -I net/sock.h \
  'skb_recv_datagram(struct sock *sk, unsigned int flags, int noblock, int *err) \
    "sk_recv_q:%llx next:%llx prev:%llx" \
    &sk->sk_receive_queue,sk->sk_receive_queue.next,sk->sk_receive_queue.prev'
where func declaration is copy pasted from the C source
and there are no guarantees that argument names and their order match
what vmlinux actually has.
With fully named BTF of vmlinux the tracing scripts will become more robust.
Note that BTF_KIND_FUNC is pretty much the same as BTF_KIND_STRUCT with
an addition of return type.
Kernel specific things like per-cpu attribute of the variable will
be BTF KIND too. This is information that tooling and kernel needs
and current BTF graph design is perfectly suited to carry such info.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 06/11/18 06:29, Alexei Starovoitov wrote:
> BTF is not pure type information. BTF is everything that verifier needs
> to know to make safety decisions that bpf instruction set doesn't have.
Yes, I'm not disputing that and never have.
I'm just saying that it will be much cleaner and better if it's
 internally organised differently.

> Splitting pure types into one section, variables into another,
> functions into yet another is not practical, since the same
> modifiers (like const or volatile) need to be applied to
> variables and functions. At the end all sections will have
> the same style of encoding, hence no need to duplicate
> the encoding three times and instead it's cleaner to encode
> all of them BTF-style via different KINDs.
This shows that you've misunderstood what I'm proposing, probably
 I explained it poorly so I'll try again.
I'm not suggesting that the 'functions' and 'variables' sections
 would have _type_ records in them, only that they would reference
 records in the 'types' section.  So if for instance we have
    int foo(int x) { int quux; /* ... */ }
    int bar(int y) { /* ... */ }
 in the source, then in the 'types' section we would have
1 INT 32bits encoding=signed offset=0
2 FUNC args=(name="x" type=1,), ret=1
3 FUNC args=(name="y" type=1,), ret=1
 while in the 'functions' section we would have
1 "foo" type=2 start_insn_idx=23 insn_count=19 (... maybe other info too...)
2 "bar" type=3 start_insn_idx=42 insn_count=5
 and in the 'variables' section we might have
1 "quux" type=1 where=stack func=1 offset=-8

Thus the graph of types lives entirely in the 'types' section, but
 things-that-are-not-types don't.  I'm not making a distinction
 between "pure types" and (somehow) impure types; I'm making a
 distinction between types (with all their impurities) and
 *instances* of those types.
Note that these 'sections' may all really be regions of the '.BTF'
 ELF section, if that makes the implementation easier.  Also, the
 'functions' and 'variables' sections _won't_ have the same style
 of encoding as the 'types', because they're storing entirely
 different data and in fact don't need variable record sizes.

And note that in this case any const or volatile qualifiers happen
 _in the 'types' section_, because they're just another way of
 deriving a type, and the records in other sections that might want
 them will just point at a BTF_KIND_CONST or BTF_KIND_VOLATILE
 record in the 'types' section.

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On Tue, Nov 06, 2018 at 06:52:11PM +0000, Edward Cree wrote:
> On 06/11/18 06:29, Alexei Starovoitov wrote:
> > BTF is not pure type information. BTF is everything that verifier needs
> > to know to make safety decisions that bpf instruction set doesn't have.
> Yes, I'm not disputing that and never have.
> I'm just saying that it will be much cleaner and better if it's
>  internally organised differently.
> 
> > Splitting pure types into one section, variables into another,
> > functions into yet another is not practical, since the same
> > modifiers (like const or volatile) need to be applied to
> > variables and functions. At the end all sections will have
> > the same style of encoding, hence no need to duplicate
> > the encoding three times and instead it's cleaner to encode
> > all of them BTF-style via different KINDs.
> This shows that you've misunderstood what I'm proposing, probably
>  I explained it poorly so I'll try again.
> I'm not suggesting that the 'functions' and 'variables' sections
>  would have _type_ records in them, only that they would reference
>  records in the 'types' section.  So if for instance we have
>     int foo(int x) { int quux; /* ... */ }
>     int bar(int y) { /* ... */ }
>  in the source, then in the 'types' section we would have
> 1 INT 32bits encoding=signed offset=0
> 2 FUNC args=(name="x" type=1,), ret=1
> 3 FUNC args=(name="y" type=1,), ret=1
>  while in the 'functions' section we would have
> 1 "foo" type=2 start_insn_idx=23 insn_count=19 (... maybe other info too...)
> 2 "bar" type=3 start_insn_idx=42 insn_count=5

that looks very weird to me. Why split func name from argument names?
The 'function name' as seen by the BTF may not be the symbol name
as seen in elf file.
Like C++ mangles names for elf. If/when we have C++ front-end for BPF
the BTF func name will be 'class::method' string.
While elf symbol name will still be mangled string in the elf file.

btw libbpf processes that elf symbol name for bpf prog already
and passes it to the kernel as bpf_attr.prog_name.
BTF's func name should be the one seen in the source.
Whatever that source code might be.
There are C, bpftrace, p4 and python frontends. These languages
should be free to put into BTF KIND_FUNC name that makes sense
from the language point of view.
Ideally it's C-like name, so when bpftool prints that BTF
it would be meaningful.

btw we've been thinking to teach libbpf to generate BTF KIND_FUNC
on the fly based on elf symbol name (when real BTF is missing in the elf),
but decided not to go that route for now.

>  and in the 'variables' section we might have
> 1 "quux" type=1 where=stack func=1 offset=-8

that doesn't work. stack slots can be reused by compiler.
variable can be in the register too.
right now we're not planning to have an equivalent of such dwarf
debug info in BTF, since -O2 is mandatory and with optimizations
variable tracking (gcc term) is not effective.
Instead we will annotate every load/store with btf type id.
Meaning no plans to tackle local variables.

The global variables for given .c file will look like single KIND_STRUCT
(which is variable length)
and when libbpf will learn to 'link' multiple .o the BTF deduplication
work (which we're doing for vmlinux) will apply as-is to
combine multiple .o together.

> 
> Thus the graph of types lives entirely in the 'types' section, but
>  things-that-are-not-types don't.  I'm not making a distinction
>  between "pure types" and (somehow) impure types; I'm making a
>  distinction between types (with all their impurities) and
>  *instances* of those types.
> Note that these 'sections' may all really be regions of the '.BTF'
>  ELF section, if that makes the implementation easier.  Also, the
>  'functions' and 'variables' sections _won't_ have the same style
>  of encoding as the 'types', because they're storing entirely
>  different data and in fact don't need variable record sizes.

yes we do see these things differently.
To us function name is the debug info that fits well into BTF description.
Whereas you see the function name part of function declaration
as something 'entirely different'. I don't quite get that point.
In C elf symbol name and in-source func name are the same,
which is probably causing this terminology confusion.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 06/11/18 21:56, Alexei Starovoitov wrote:
> that looks very weird to me. Why split func name from argument names?
> The 'function name' as seen by the BTF may not be the symbol name
> as seen in elf file.
The symbol name will be in the symbol table, which is not the same
 thing as the functions table in BTF that I'm proposing.  (They do
 look a little similar as I included an insn_idx for functions that
 partially duplicates the offset given in the symbol table.  But
 that's necessary precisely for the reason you mention, that the
 function name != the symbol name in general.)
"Splitting" func name from argument names is partly to potentially
 save space — if we'd had "int bar(int x)" instead then 'bar' could
 share its type record with 'foo'.  And partly just because the
 name of the function itself is no more part of its type than the
 name of an integer variable is part of the integer's type.
(Whereas names of parameters are like names of struct members:
 while they are not part of the 'pure type' from a language
 perspective, they are part of the type from the perspective of
 debugging, which is why they belong in the BTF type record.)

> There are C, bpftrace, p4 and python frontends. These languages
> should be free to put into BTF KIND_FUNC name that makes sense
> from the language point of view.
I'm paying attention to BTF because I'm adding support for it into
 my ebpf_asm.  Don't you think I *know* that frontends for BPF are
 more than just C?

>>  and in the 'variables' section we might have
>> 1 "quux" type=1 where=stack func=1 offset=-8
> that doesn't work. stack slots can be reused by compiler.
And who says that there can't be multiple records pointing to the
 same stack slot with different types & names?

> Instead we will annotate every load/store with btf type id.
That's certainly more useful; but I think most useful of all is to
 have *both* (though the stack slot types should be optional).

> The global variables for given .c file will look like single KIND_STRUCT
That's exactly the kind of superficially-clever but nasty hack
 that results from the continued insistence on conflating types
 and instances (objects).  In the long run it will make
 maintenance harder, and frustrate new features owing to the need
 to find new hacks to shoehorn them into the same model.
Instead there should be entries for the globals in something like
 the variables table I mentioned,
2 "fred" type=1 where=global func=0 offset=8
 in which 'func' is unused and 'offset' gives offset in .bss.
 'where' might also include indication of whether it's static.

Then for linkage you can extend this with index of which file it
 came from.

But maybe discussing global variables is a bit premature as eBPF
 doesn't have any such thing yet.

> yes we do see these things differently.
> To us function name is the debug info that fits well into BTF description.
> Whereas you see the function name part of function declaration
> as something 'entirely different'.
I'm not saying that the function name is 'entirely different'
 to the rest of the type.  (Though I do think it doesn't
 belong in the type, that's a weaker and contingent point.)
I'm saying that the *function* is entirely different to its
 *type*.  It's a category error to conflate them:
    f: x ↦ x + 1
 is a function.
    int → int
 is a type, and specifically the type of the object named "f".
(And the nature of mathematical notation for functions happens
 to put the name 'x' in the former, whereas we are putting the
 parameter name in the latter, but that's irrelevant.)
Similarly, "1" is an integer, but "integer" is a type, and is
 not itself an integer, while "1" is not a type.  They are at
 different meta-levels.

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On Tue, Nov 06, 2018 at 10:58:42PM +0000, Edward Cree wrote:
>  share its type record with 'foo'.  And partly just because the
>  name of the function itself is no more part of its type than the
>  name of an integer variable is part of the integer's type.

correct. function name is not part of its type.
function name is part of BTF that provide debug info about the function.

Function name and function argument names are part of the same debug info.
Splitting them makes no sense.

> (Whereas names of parameters are like names of struct members:
>  while they are not part of the 'pure type' from a language
>  perspective, they are part of the type from the perspective of
>  debugging, which is why they belong in the BTF type record.)

struct name and struct field names live in the same BTF record.
Similarly function name and function argument names should be
in the same BTF record, so we can reuse most of the BTF validation
and BTF parsing logic by doing so.
The minor difference between KIND_STRUCT and KIND_FUNC is
an addition of return type_id.
Everything else is common.
imo that speaks for itself that it's a correct path forward.

> > There are C, bpftrace, p4 and python frontends. These languages
> > should be free to put into BTF KIND_FUNC name that makes sense
> > from the language point of view.
> I'm paying attention to BTF because I'm adding support for it into
>  my ebpf_asm.  Don't you think I *know* that frontends for BPF are
>  more than just C?

assembler is not a high level language.
I believe it's a proper trade-off to make C easier to use
in expense of some ugliness in your ebpf_asm.

> > The global variables for given .c file will look like single KIND_STRUCT
> That's exactly the kind of superficially-clever but nasty hack
>  that results from the continued insistence on conflating types
>  and instances (objects).  In the long run it will make
>  maintenance harder, and frustrate new features owing to the need
>  to find new hacks to shoehorn them into the same model.

Let's keep 'nasty hack' claims out of this discussion.
I find the current BTF design and KIND_FUNC addition to be elegant
and appropriate.

> Instead there should be entries for the globals in something like
>  the variables table I mentioned,
> 2 "fred" type=1 where=global func=0 offset=8
>  in which 'func' is unused and 'offset' gives offset in .bss.
>  'where' might also include indication of whether it's static.

'static' like boolean flag? That won't help introspection.
To properly describe 'static' functions more information is necessary.
I don't like to invent new formats. BTF is extensible description
of any debug info. I prefer to keep all debug info in one place
and in one common format.

> I'm saying that the *function* is entirely different to its
>  *type*.  It's a category error to conflate them:
>     f: x -> x + 1
>  is a function.

BTF does not describe function. BTF describes debug info about function.
BPF program is the function.
BTF is not *type* only format. It's debug info format.
Trying to make BTF into type only is not going to work.
It's already more than type only as I showed earlier.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 07/11/18 00:59, Alexei Starovoitov wrote:
> Function name and function argument names are part of the same debug info.
> Splitting them makes no sense.
... except where combining them involves creating pain elsewhere.
Sure the function name *could* go in the type record, but there
 still needs to be a separate function record in a functions
 table (because types are not instances), and that being the
 case the latter _may as well_ be where the name lives so that
 multiple functions (and pointers to them) can all share the
 same type record when the param names etc. all match.

> struct name and struct field names live in the same BTF record.
No, the struct _type_ name lives in the same record as the field
 names, but an _instance_ name doesn't.  I.e. in
    struct foo {int x;} bar;
 the BTF type record holds the names 'foo' and 'x', but not 'bar'
 because that's not the name of the _type_.  Indeed there isn't
 room in the record for both 'foo' and 'bar' because there's only
 one name_off field for the type.
And I argue that the name of a function is more like 'bar' than
 'foo' here, not least from the point of view of which C namespace
 they occupy.

> Similarly function name and function argument names should be
> in the same BTF record, so we can reuse most of the BTF validation
> and BTF parsing logic by doing so.
I think it's incredibly short-sighted to focus on 'what can most
 easily be done with the existing implementation' when designing a
 file format which is intended to have 'long legs'.

> assembler is not a high level language.
I never said it was.
> I believe it's a proper trade-off to make C easier to use
> in expense of some ugliness in your ebpf_asm.
Please respond to the arguments I make, rather than unrelated
 arguments that you might imagine me making.  Asm is merely a
cause of my present interest in BTF, it is not the lens
 through which I see the whole thing.

> Let's keep 'nasty hack' claims out of this discussion.
> I find the current BTF design and KIND_FUNC addition to be elegant
> and appropriate.
Whereas I don't, and I don't feel like my core criticisms have
 been addressed _at all_.  The only answer I get to "BTF should
 store type and instance information in separate records" is
 "it's a debuginfo", no indication of why that's a meaningful
 noun let alone why it implies they should be conflated in the
 format.
And please explain what's "elegant" about how map types are
 currently handled.

> BTF is not *type* only format. It's debug info format.
> Trying to make BTF into type only is not going to work.
> It's already more than type only as I showed earlier.
Again, as I have *repeatedly* said, I am not trying to remove
 non-type information from BTF.  I am just trying to organise
 BTF to consist of separate _parts_ for types and instances,
 rather than forcing both into the same Procrustean bed.

(I don't feel like we're making progress in understanding one
 another here; maybe we should have a telephone discussion?
 Sadly I'm not going to Plumbers, else that would be the
 perfect place to thrash this out.)

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On Wed, Nov 07, 2018 at 07:29:31PM +0000, Edward Cree wrote:
> Whereas I don't, and I don't feel like my core criticisms have
>  been addressed _at all_.  The only answer I get to "BTF should
>  store type and instance information in separate records" is
>  "it's a debuginfo",
...
>  I am just trying to organise
>  BTF to consist of separate _parts_ for types and instances,
>  rather than forcing both into the same Procrustean bed.

BTF does not have and should not have instances.
BTF is debug info only.
This is not negotiable.

Loading BTF into the kernel does not create an instance of anything.
The kernel verifies BTF and BTF blob sits there waiting to be pointed at
by the actual instance.
Today these instances are maps and programs.
A single program can have multiple instances of the functions
which debug info is described in BTF.
Multiple prograrms (with multiple functions) can point
to the same BTF blob description.
Consider that in single C file there will be multliple functions,
many maps, global variables (in the future) they will be created
by individual prog_load/map_create syscalls.
Before that the _single_ blob of BTF data for the whole C file
will be loaded, since functions, maps share most of
the type and name information.

In elf files the instances of functions have names.
These names go into symbol table of the elf. Dynamic linker
is using these symbols to connect different instances.
In BPF world we use IDs to connect instances.
There is btd if, map id, prog id.
You can argue that bpf_prog_load has prog_name attribute.
So the instance of the program sort of have name already,
but this name is for debug only. It doesn't have to be unique
and the kernel is not relying on the name to connect instances.

Example:
1: int foo(struct __sk_buff *skb, char arg2)
2: {
3:  return skb->len;
4: }

Line 1 goes into BTF to describe the function (with names and types).
Line 1 also goes into line_info .BTF.ext elf section as a string.
Line 3 and 4 go into line_info as well as strings.

llvm compiles it into something:
  .text
  .globl  foo
  .type   foo, @function
foo:
  r0 = *(u32 *)(r1 + 0)
  exit
  .section .BTF
  .byte X
  .byte Y
  .section .BTF.ext
  .byte X
  .byte Y

That 'foo' name goes into symbol table and together with the body
form the future instance of the function.
libbpf takes 'foo' from symbol table of elf and passes it
as bpf_attr.prog_name to the kernel along with bpf instructions
to create an instance.
The name 'foo' in symbol table doesn't have to match with
BTF 'foo' name and '.*foo.*' substring in line_info.

If you decide to use the sequence:
"""
  .globl  foo
  .type   foo, @function
foo:
""
in your ebpf_asm to parse and store into BTF as a function name
I would strongly argue against, since it would be incorrectly
conflating the instance of the function with debug purpose
of BTF.

Right now we do not have a concrete proposal for assembler text of BTF.
When LLVM emits BTF into .s it emits them as raw bytes.
So I'm looking forward to your ideas how to describe BTF in .s
Note such .s must have freedom to describe 'int bar(struct __sk_buff *a1, char a2)'
as debug info while having '.globl foo; foo:' as symbol name.

Your other 'criticism' was about libbpf's bpf_map_find_btf_info()
and ____btf_map_* hack. Yes. It is a hack and I'm open to change it
if there are better suggestions. It's a convention between
libbpf and program writers that produce elf. It's not a kernel abi.
Nothing to do with BTF and this instance vs debug info discussion.

> (I don't feel like we're making progress in understanding one
>  another here; maybe we should have a telephone discussion?
>  Sadly I'm not going to Plumbers, else that would be the
>  perfect place to thrash this out.)

Happy to jump on the call to explain it again.
10:30am pacific time works for me tomorrow.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 07/11/18 21:49, Alexei Starovoitov wrote:
> On Wed, Nov 07, 2018 at 07:29:31PM +0000, Edward Cree wrote:
>> Whereas I don't, and I don't feel like my core criticisms have
>>  been addressed _at all_.  The only answer I get to "BTF should
>>  store type and instance information in separate records" is
>>  "it's a debuginfo",
> ...
>>  I am just trying to organise
>>  BTF to consist of separate _parts_ for types and instances,
>>  rather than forcing both into the same Procrustean bed.
> BTF does not have and should not have instances.
> BTF is debug info only.
> This is not negotiable.
I'm not saying the instances go in BTF, I'm saying that debug info
 *about* instances goes in BTF (it already does, as you keep saying
 BTF is "not just pure types"), and that that ought to be
 distinguished within the format from debug info about types.

> So I'm looking forward to your ideas how to describe BTF in .s
> Note such .s must have freedom to describe 'int bar(struct __sk_buff *a1, char a2)'
> as debug info while having '.globl foo; foo:' as symbol name.
I've pushed out a branch with what I have; see
 https://github.com/solarflarecom/ebpf_asm/tree/btfdoc
 (with some examples in dropper.s and documentation in the README).
In particular note that right now the BTF section is entirely
 decoupled from the .text, so indeed there is nothing right now
 tying function names to symbol names.  I do not yet have anything
 generating FuncInfo (or LineInfo) tables, but when I do that will
 remain decoupled.

> Your other 'criticism' was about libbpf's bpf_map_find_btf_info()
> and ____btf_map_* hack. Yes. It is a hack and I'm open to change it
> if there are better suggestions. It's a convention between
> libbpf and program writers that produce elf. It's not a kernel abi.
> Nothing to do with BTF and this instance vs debug info discussion.
It's everything to do with it: it's defining a type with a magic name
 (____btf_map_foo) when what we really want to do is declare an
 instance (the map 'foo').  And it may not be a kernel ABI, but it's
 a part of the file format you're defining (whether that's just a
 'convention' or something more), and if you want the BTF ecosystem
 to be more than just an llvm monoculture then the format needs to be
 properly specified so that others can work with it.

> Happy to jump on the call to explain it again.
> 10:30am pacific time works for me tomorrow.
That works for me (that's in ~30 minutes from now if I've converted
 correctly.)  Please email me offlist with the phone number to call.

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On Thu, Nov 08, 2018 at 05:58:56PM +0000, Edward Cree wrote:
> 
> > Happy to jump on the call to explain it again.
> > 10:30am pacific time works for me tomorrow.
> That works for me (that's in ~30 minutes from now if I've converted
>  correctly.)  Please email me offlist with the phone number to call.

no offlist. public link for anyone to join:
https://bluejeans.com/867080076/

I have hard cutoff at 11am though.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On Thu, Nov 8, 2018 at 10:21 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Nov 08, 2018 at 05:58:56PM +0000, Edward Cree wrote:
> >
> > > Happy to jump on the call to explain it again.
> > > 10:30am pacific time works for me tomorrow.
> > That works for me (that's in ~30 minutes from now if I've converted
> >  correctly.)  Please email me offlist with the phone number to call.
>
> no offlist. public link for anyone to join:
> https://bluejeans.com/867080076/
>
> I have hard cutoff at 11am though.

same link let's continue at 1pm PST.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 08/11/18 19:42, Alexei Starovoitov wrote:
> same link let's continue at 1pm PST. 
So, one thing we didn't really get onto was maps, and you mentioned that it
 wasn't really clear what I was proposing there.
What I have in mind comes in two parts:
1) map type.  A new BTF_KIND_MAP with metadata 'key_type', 'value_type'
 (both are type_ids referencing other BTF type records), describing the
 type "map from key_type to value_type".
2) record in the 'instances' table.  This would have a name_off (the
 name of the map), a type_id (pointing at a BTF_KIND_MAP in the 'types'
 table), and potentially also some indication of what symbol (from
 section 'maps') refers to this map.  This is pretty much the exact
 same metadata that a function in the 'instances' table has, the only
 differences being
 (a) function's type_id points at a BTF_KIND_FUNC record
 (b) function's symbol indication refers from .text section
 (c) in future functions may be nested inside other functions, whereas
 AIUI a map can't live inside a function.  (But a variable, which is
 the other thing that would want to go in an 'instances' table, can.)
So the 'instances' table record structure looks like

struct btf_instance {
    __u32 type_id; /* Type of object declared.  An index into type section */
    __u32 name_off; /* Name of object.  An offset into string section */
    __u32 parent; /* Containing object if any (else 0).  An index into instance section */
};

and we extend the BTF header:

struct btf_header {
    __u16   magic;
    __u8    version;
    __u8    flags;
    __u32   hdr_len;

    /* All offsets are in bytes relative to the end of this header */
    __u32   type_off;      /* offset of type section       */
    __u32   type_len;      /* length of type section       */
    __u32   str_off;       /* offset of string section     */
    __u32   str_len;       /* length of string section     */
    __u32   inst_off;      /* offset of instance section   */
    __u32   inst_len;      /* length of instance section   */
};

Then in the .BTF.ext section, we have both

struct bpf_func_info {
    __u32 prog_symbol; /* Index of symbol giving address of subprog */
    __u32 inst_id; /* Index into instance section */
}

struct bpf_map_info {
{
    __u32 map_symbol; /* Index of symbol creating this map */
    __u32 inst_id; /* Index into instance section */
}

(either living in different subsections, or in a single table with
 the addition of a kind field, or in a single table relying on the
 ultimately referenced type to distinguish funcs from maps).

Note that the name (in btf_instance) of a map or function need not
 match the name of the corresponding symbol; we use the .BTF.ext
 section to tie together btf_instance IDs and symbol IDs.  Then in
 the case of functions (subprogs), the prog_symbol can be looked
 up in the ELF symbol table to find the address (== insn_offset)
 of the subprog, as well as the section containing it (since that
 might not be .text).  Similarly in the case of maps the BTF info
 about the map is connected with the info in the maps section.

Now when the loader has munged this, what it passes to the kernel
 might not have map_symbol, but instead map_fd.  Instead of
 prog_symbol it will have whatever identifies the subprog in the
 blob of stuff it feeds to the kernel (so probably insn_offset).

All this would of course require a bit more compiler support than
 the current BPF_ANNOTATE_KV_PAIR, since that just causes the
 existing BTF machinery to declare a specially constructed struct
 type.  At the C level you could still have BPF_ANNOTATE_KV_PAIR
 and the '____bpf_map_foo' name, but then the compiler would
 recognise that and convert it into an instance record by looking
 up the name 'foo' in its "maps" section.  That way the special
 ____bpf_map_* handling (which ties map names to symbol names,
 also) would be entirely compiler-internal and not 'leak out' into
 the definition of the format.  Frontends for other languages
 which do possess a native map type (e.g. Python dict) might have
 other ways of indicating the key/value type of a map at source
 level (e.g. PEP 484) and could directly generate the appropriate
 BTF_KIND_MAP and bpf_map_info records rather than (as they would
 with the current design) having to encode the information as a
 struct ____bpf_map_foo type-definition.


While I realise the desire to concentrate on one topic at once, I
 think this question of maps should be discussed in tomorrow's
 call, since it is when we start having other kinds of instances
 besides functions that the advantages of my design become
 apparent, unifying the process of 'declaration' of functions,
 maps, and (eventually) variables while separating them all from
 the process of 'definition' of the types of all three.

Thank you for your continued patience with me.
-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Yonghong Song]

On 11/8/18 2:56 PM, Edward Cree wrote:
> On 08/11/18 19:42, Alexei Starovoitov wrote:
>> same link let's continue at 1pm PST.
> So, one thing we didn't really get onto was maps, and you mentioned that it
>   wasn't really clear what I was proposing there.
> What I have in mind comes in two parts:
> 1) map type.  A new BTF_KIND_MAP with metadata 'key_type', 'value_type'
>   (both are type_ids referencing other BTF type records), describing the
>   type "map from key_type to value_type".
> 2) record in the 'instances' table.  This would have a name_off (the
>   name of the map), a type_id (pointing at a BTF_KIND_MAP in the 'types'
>   table), and potentially also some indication of what symbol (from
>   section 'maps') refers to this map.  This is pretty much the exact
>   same metadata that a function in the 'instances' table has, the only
>   differences being
>   (a) function's type_id points at a BTF_KIND_FUNC record
>   (b) function's symbol indication refers from .text section
>   (c) in future functions may be nested inside other functions, whereas
>   AIUI a map can't live inside a function.  (But a variable, which is
>   the other thing that would want to go in an 'instances' table, can.)
> So the 'instances' table record structure looks like
> 
> struct btf_instance {
>      __u32 type_id; /* Type of object declared.  An index into type section */
>      __u32 name_off; /* Name of object.  An offset into string section */
>      __u32 parent; /* Containing object if any (else 0).  An index into instance section */
> };
> 
> and we extend the BTF header:
> 
> struct btf_header {
>      __u16   magic;
>      __u8    version;
>      __u8    flags;
>      __u32   hdr_len;
> 
>      /* All offsets are in bytes relative to the end of this header */
>      __u32   type_off;      /* offset of type section       */
>      __u32   type_len;      /* length of type section       */
>      __u32   str_off;       /* offset of string section     */
>      __u32   str_len;       /* length of string section     */
>      __u32   inst_off;      /* offset of instance section   */
>      __u32   inst_len;      /* length of instance section   */
> };
> 
> Then in the .BTF.ext section, we have both
> 
> struct bpf_func_info {
>      __u32 prog_symbol; /* Index of symbol giving address of subprog */
>      __u32 inst_id; /* Index into instance section */
> }
> 
> struct bpf_map_info {
> {
>      __u32 map_symbol; /* Index of symbol creating this map */
>      __u32 inst_id; /* Index into instance section */
> }
> 
> (either living in different subsections, or in a single table with
>   the addition of a kind field, or in a single table relying on the
>   ultimately referenced type to distinguish funcs from maps).
> 
> Note that the name (in btf_instance) of a map or function need not
>   match the name of the corresponding symbol; we use the .BTF.ext
>   section to tie together btf_instance IDs and symbol IDs.  Then in
>   the case of functions (subprogs), the prog_symbol can be looked
>   up in the ELF symbol table to find the address (== insn_offset)
>   of the subprog, as well as the section containing it (since that
>   might not be .text).  Similarly in the case of maps the BTF info
>   about the map is connected with the info in the maps section.
> 
> Now when the loader has munged this, what it passes to the kernel
>   might not have map_symbol, but instead map_fd.  Instead of
>   prog_symbol it will have whatever identifies the subprog in the
>   blob of stuff it feeds to the kernel (so probably insn_offset).
> 
> All this would of course require a bit more compiler support than
>   the current BPF_ANNOTATE_KV_PAIR, since that just causes the
>   existing BTF machinery to declare a specially constructed struct
>   type.  At the C level you could still have BPF_ANNOTATE_KV_PAIR
>   and the '____bpf_map_foo' name, but then the compiler would
>   recognise that and convert it into an instance record by looking
>   up the name 'foo' in its "maps" section.  That way the special
>   ____bpf_map_* handling (which ties map names to symbol names,

Compiler in general does not do transformation based on variable
or struct type names by default, so this probably should stay
in the loader.

>   also) would be entirely compiler-internal and not 'leak out' into
>   the definition of the format.  Frontends for other languages
>   which do possess a native map type (e.g. Python dict) might have
>   other ways of indicating the key/value type of a map at source
>   level (e.g. PEP 484) and could directly generate the appropriate
>   BTF_KIND_MAP and bpf_map_info records rather than (as they would
>   with the current design) having to encode the information as a
>   struct ____bpf_map_foo type-definition.

You mean a python application can generate bpf byte codes and
BTFs (include map types)? That will be different from the C/LLVM
use case. The python app. probably will be the loader as well.

One option is to pass BPF specific flag like 
"-map-type-prefix="___bpf_map_" and LLVM will generate BTF_KIND_MAP type
for any structure with name "___bpf_map_<...>". But if this is
the case, user can just search the type table for struct name
"___bpf_map_<...>" and llvm does not need to do anything. Note that
once user passes "-map-type-prefix="___bpf_map_" to llvm, the
definition of the format is already leaked.

So I feel that this probably belongs to the loader.

> 
> 
> While I realise the desire to concentrate on one topic at once, I
>   think this question of maps should be discussed in tomorrow's
>   call, since it is when we start having other kinds of instances
>   besides functions that the advantages of my design become
>   apparent, unifying the process of 'declaration' of functions,
>   maps, and (eventually) variables while separating them all from
>   the process of 'definition' of the types of all three.
> 
> Thank you for your continued patience with me.
> -Ed
> 

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On Thu, Nov 08, 2018 at 10:56:55PM +0000, Edward Cree wrote:
> On 08/11/18 19:42, Alexei Starovoitov wrote:
> > same link let's continue at 1pm PST. 
> So, one thing we didn't really get onto was maps, and you mentioned that it
>  wasn't really clear what I was proposing there.

let's discuss ground rules first.
1. the patches should be upstreamable in both kernel _and_ llvm

For example you propose 'prog_symbol' down below.
I'm sure you can do it in your assembler. We can also do it in pahole
(because it operates on dwarf inside elf), but there is no way for us
to do it in llvm. Because symbol table gen comes last. dwarf, btf sections have
been completed before that. llvm 'fixup' logic also done before symbol table.
So to add something like 'prog_symbol' into .BTF.ext would mean that would need
an extra pass after the last one that messes with the stuff already finalized.
That will get nacked.
Another example: in our first llvm->btf patchset we were using llvm's dwarf to
generate btf. That got nacked and we had to do 2k+ lines complete rewrite
to generate btf from llvm ir. That forced us to generate btf slightly
differently than from dwarf. The difference is not fundamental (like bitfields).
But it drives the point that elf format is _secondary_ to #1 rule above.

Another example: you're proposing to teach bpf backend to recognize
____bpf_map* name. That is not something we can upstream in llvm either.

Similarly on the kernel api side we decided to craft an api in a way
that what is passed to the kernel is returned back in the same way.
Sort of like if kernel understands 'struct bpf_func_info' from patch 5
it should speak back in the same language.
I think it's important to agree on that principle to continue discussion.

Another key point is obvious, but worth repeating.
kernel abi is cast in stone. elf format is not.
which means that what was defined in include/uapi/linux/btf.h is fixed.
That is the format of .BTF section that llvm/pahole emits.
Whereas .BTF.ext section is _not_ defined in kernel uapi.
It defines the elf format and we can change it the future.
The format of .BTF.ext is the protocol between libbpf and llvm.
We define .BTF.ext in libbpf with libbpf license and coding style
and independently define it in llvm with its license and its coding
style.
We need to discuss .BTF.ext and make sure it's extensible, so we can
upgrade libbpf and llvm independently of each other,
but it doesn't have the same requirements as kernel abi.
Like in the kernel abi the extensibility of any structure means
that uknown fields should be zero.
Ex: sys_bpf() accepts 'bpf_attr' and size.
If user's size is larger than kernel size. All uknown fields
must be zero.
In case of libbpf <-> llvm that is not the case.
libbpf _has_ to deal with non-zero unknown fields in some way.
Like it can print warning, but it has to accept the extended format.
Otherwise upgrading llvm will not be possible without upgrading libbpf.
Similarly when both llvm is new and libbpf is new, but kernel is old,
libbpf has to pass to the kernel only the fields it knows about.
(though it may understand all of them).
So libbpf has to deal will all combinations: old/new kernel,
old/new llvm (and corresponding .BTF and .BTF.ext sections).

Now my understanding that you're mainly concerned with elf file format, correct?
I'm making this guess, because you're arguing strongly about KIND_MAP
and blasting our __btf_map* hack.
I'm all for improving this part, but it cannot go into .BTF,
because we already have an api to pass btf_key_type_id, btf_value_type_id
in MAP_CREATE syscall.
Adding new BTF_KIND_MAP into .BTF would be pointless, since we cannot
change MAP_CREATE.

As discussed on the call, currently we're designing KIND_VARIABLE
that will describe global/static variables in .BTF and corresponding
prog_load api changes for the kernel. We'll be talking about it
at bpf uconf. Imo current 'struct bpf_map_def map_foo;' hack
that libbpf/iproute2 and other loaders are dealing with
will be cleaned up by this KIND_VARIABLE BTF support.
Because of rule #1 we cannot pattern match 'bpf_map_def' name
in llvm. All global variables have to be dealt in the common way
by the llvm. We can add new __builtin_ specifically for map creation
and ask folks to start using new interface in .c programs for
map creation. All that is exciting, but I'd like to table that
discussion and focus on this patch set which is about adding
bpf_func_infos, BTF vs BTF.ext split, instances vs types.

> What I have in mind comes in two parts:
> 1) map type.  A new BTF_KIND_MAP with metadata 'key_type', 'value_type'
>  (both are type_ids referencing other BTF type records), describing the
>  type "map from key_type to value_type".
> 2) record in the 'instances' table.  This would have a name_off (the
>  name of the map), a type_id (pointing at a BTF_KIND_MAP in the 'types'
>  table), and potentially also some indication of what symbol (from
>  section 'maps') refers to this map.  This is pretty much the exact
>  same metadata that a function in the 'instances' table has, the only
>  differences being
>  (a) function's type_id points at a BTF_KIND_FUNC record
>  (b) function's symbol indication refers from .text section
>  (c) in future functions may be nested inside other functions, whereas
>  AIUI a map can't live inside a function.  (But a variable, which is
>  the other thing that would want to go in an 'instances' table, can.)
> So the 'instances' table record structure looks like
> 
> struct btf_instance {
>     __u32 type_id; /* Type of object declared.  An index into type section */
>     __u32 name_off; /* Name of object.  An offset into string section */
>     __u32 parent; /* Containing object if any (else 0).  An index into instance section */
> };

I have two issues with above structure:
1. it's not naturally extensible.
Meaning it's possible to define new struct by bumping version in btf
header, but it's not clean.
2. parent field has to be checked for loops

Both of these issues are resolved by existing BTF and kernel code.
BTF layout is extensible. BTF kernel verifier already checks for loops.

Hence I propose to use existing 'struct btf_type' for these new
instances section instead of inventing new format.

> and we extend the BTF header:
> 
> struct btf_header {
>     __u16   magic;
>     __u8    version;
>     __u8    flags;
>     __u32   hdr_len;
> 
>     /* All offsets are in bytes relative to the end of this header */
>     __u32   type_off;      /* offset of type section       */
>     __u32   type_len;      /* length of type section       */
>     __u32   str_off;       /* offset of string section     */
>     __u32   str_len;       /* length of string section     */
>     __u32   inst_off;      /* offset of instance section   */
>     __u32   inst_len;      /* length of instance section   */

The addition of above two fields is certainly doable
and fits design of BTF well. Kernel side already has code to
deal with multiple BTF sections.
We don't even need to bump version for that.
That's great, but the key point here is that btf.h is both
kernel abi and file format.
If llvm emits this instance section into elf file
libbpf loader should better pass it to the kernel as well.
Otherwise having populated instance section in elf and
empty instances for the kernel will make kernel patch to uapi/btf.h
non-upstreamble. Why ? Because we do not add fields to uapi structs
that kernel will not be using.
I suspect you're actually proposing to pass populated instance section
to the kernel as part of BTF_LOAD sys_bpf command. So all good.
Just making sure we're on the same page. Correct?

So the tweak to your 'struct btf_instance' proposal
is to use 'struct btf_type' of variable length instead
in this instances section.
And define new BTF_KIND_FUNC that can only be used
in the instances.
In the future we will add BTF_KIND_VARIABLE in there too.
As far as type section there we will use BTF_KIND_FUNC_PROTO only.
It will always have empty name and can have full names
for function arguments.

BTF_KIND_FUNC in instances section must have non-zero name_off
(which will describe function name) and type_id pointing
to BTF_KIND_FUNC_PROTO in type section.
Multiple KIND_FUNC potentially can point to the same KIND_FUNC_PROTO
if all argument names are the same or all of them are empty.
(and types match, of course).
I'm not counting on overall BTF size compression because of that,
but it's kinda nice to have this option.

> Then in the .BTF.ext section, we have both
> 
> struct bpf_func_info {
>     __u32 prog_symbol; /* Index of symbol giving address of subprog */
>     __u32 inst_id; /* Index into instance section */
> }

Right, for .BPF.ext section we can keep existing
'struct bpf_func_info' with the minor tweak:

struct bpf_func_info {
       __u32   insn_offset;
       __u32   inst_id;  // this is now index in the instance section
};

Since instance section will have KIND_FUNC and KIND_VARIABLE in the future
the kernel has to check that 'bpf_func_info->inst_id' in .BTF.ext
points to KIND_FUNC in .BTF.

How does this sound?

If we agree to that it will unblock us for this patch set
and for follow on patch set that adds 'struct bpf_line_info'
based on the same principles.

> 
> struct bpf_map_info {
> {
>     __u32 map_symbol; /* Index of symbol creating this map */
>     __u32 inst_id; /* Index into instance section */
> }

The map discussion I'd like to table for now due to reasons
outlined above.
 
>  think this question of maps should be discussed in tomorrow's
>  call, since it is when we start having other kinds of instances

turned out most of us have a conflict, so the earliest is 1:30pm on Friday.
still works for you?

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 09/11/18 04:35, Alexei Starovoitov wrote:
> On Thu, Nov 08, 2018 at 10:56:55PM +0000, Edward Cree wrote:
>>  think this question of maps should be discussed in tomorrow's
>>  call, since it is when we start having other kinds of instances
> turned out most of us have a conflict, so the earliest is 1:30pm on Friday.
> still works for you?

Yep (that's 9.30pm GMT right?)

I'm assuming same bluejeans link again.

-Ed

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On 11/9/18 12:00 PM, Edward Cree wrote:
> On 09/11/18 04:35, Alexei Starovoitov wrote:
>> On Thu, Nov 08, 2018 at 10:56:55PM +0000, Edward Cree wrote:
>>>   think this question of maps should be discussed in tomorrow's
>>>   call, since it is when we start having other kinds of instances
>> turned out most of us have a conflict, so the earliest is 1:30pm on Friday.
>> still works for you?
> 
> Yep (that's 9.30pm GMT right?)
> 
> I'm assuming same bluejeans link again.

same link, but i cannot make it right now.
have to extinguish few fires.
may be at 2pm (unlikely) or 3pm (more likely) PST?

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Edward Cree]

On 09/11/18 21:14, Alexei Starovoitov wrote:
> same link, but i cannot make it right now.
> have to extinguish few fires.
> may be at 2pm (unlikely) or 3pm (more likely) PST?

Yep I can do either of those, just let me know which when you can.

Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO

[Alexei Starovoitov]

On 11/9/18 1:28 PM, Edward Cree wrote:
> On 09/11/18 21:14, Alexei Starovoitov wrote:
>> same link, but i cannot make it right now.
>> have to extinguish few fires.
>> may be at 2pm (unlikely) or 3pm (more likely) PST?
> 
> Yep I can do either of those, just let me know which when you can.

still swamped. but see the light.
let's do 3pm