* [PATCH v2 bpf-next 0/4] libbpf: BTF dumper support for typed data
@ 2021-01-17 22:16 Alan Maguire
2021-01-17 22:16 ` [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines Alan Maguire
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Alan Maguire @ 2021-01-17 22:16 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, morbo,
shuah, bpf, netdev, linux-kselftest, linux-kernel, Alan Maguire
Add a libbpf dumper function that supports dumping a representation
of data passed in using the BTF id associated with the data in a
manner similar to the bpf_snprintf_btf helper.
Default output format is identical to that dumped by bpf_snprintf_btf(),
for example a "struct sk_buff" representation would look like this:
struct sk_buff){
(union){
(struct){
.next = (struct sk_buff *)0xffffffffffffffff,
.prev = (struct sk_buff *)0xffffffffffffffff,
(union){
.dev = (struct net_device *)0xffffffffffffffff,
.dev_scratch = (long unsigned int)18446744073709551615,
},
},
...
Patches 1 and 2 make functions available that are needed during
dump operations.
Patch 3 implements the dump functionality in a manner similar
to that in kernel/bpf/btf.c, but with a view to fitting into
libbpf more naturally. For example, rather than using flags,
boolean dump options are used to control output.
Patch 4 is a selftest that utilizes a dump printf function
to snprintf the dump output to a string for comparison with
expected output. Tests deliberately mirror those in
snprintf_btf helper test to keep output consistent.
Changes since RFC [1]
- The initial approach explored was to share the kernel code
with libbpf using #defines to paper over the different needs;
however it makes more sense to try and fit in with libbpf
code style for maintenance. A comment in the code points at
the implementation in kernel/bpf/btf.c and notes that any
issues found in it should be fixed there or vice versa;
mirroring the tests should help with this also
(Andrii)
[1] https://lore.kernel.org/bpf/1610386373-24162-1-git-send-email-alan.maguire@oracle.com/T/#t
Alan Maguire (4):
libbpf: add btf_has_size() and btf_int() inlines
libbpf: make skip_mods_and_typedefs available internally in libbpf
libbpf: BTF dumper support for typed data
selftests/bpf: add dump type data tests to btf dump tests
tools/lib/bpf/btf.h | 36 +
tools/lib/bpf/btf_dump.c | 974 ++++++++++++++++++++++
tools/lib/bpf/libbpf.c | 4 +-
tools/lib/bpf/libbpf.map | 5 +
tools/lib/bpf/libbpf_internal.h | 2 +
tools/testing/selftests/bpf/prog_tests/btf_dump.c | 233 ++++++
6 files changed, 1251 insertions(+), 3 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines
2021-01-17 22:16 [PATCH v2 bpf-next 0/4] libbpf: BTF dumper support for typed data Alan Maguire
@ 2021-01-17 22:16 ` Alan Maguire
2021-01-21 4:11 ` Andrii Nakryiko
2021-01-17 22:16 ` [PATCH v2 bpf-next 2/4] libbpf: make skip_mods_and_typedefs available internally in libbpf Alan Maguire
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Alan Maguire @ 2021-01-17 22:16 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, morbo,
shuah, bpf, netdev, linux-kselftest, linux-kernel, Alan Maguire
BTF type data dumping will use them in later patches, and they
are useful generally when handling BTF data.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/lib/bpf/btf.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 1237bcd..0c48f2e 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -294,6 +294,20 @@ static inline bool btf_is_datasec(const struct btf_type *t)
return btf_kind(t) == BTF_KIND_DATASEC;
}
+static inline bool btf_has_size(const struct btf_type *t)
+{
+ switch (BTF_INFO_KIND(t->info)) {
+ case BTF_KIND_INT:
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION:
+ case BTF_KIND_ENUM:
+ case BTF_KIND_DATASEC:
+ return true;
+ default:
+ return false;
+ }
+}
+
static inline __u8 btf_int_encoding(const struct btf_type *t)
{
return BTF_INT_ENCODING(*(__u32 *)(t + 1));
@@ -309,6 +323,11 @@ static inline __u8 btf_int_bits(const struct btf_type *t)
return BTF_INT_BITS(*(__u32 *)(t + 1));
}
+static inline __u32 btf_int(const struct btf_type *t)
+{
+ return *(__u32 *)(t + 1);
+}
+
static inline struct btf_array *btf_array(const struct btf_type *t)
{
return (struct btf_array *)(t + 1);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 bpf-next 2/4] libbpf: make skip_mods_and_typedefs available internally in libbpf
2021-01-17 22:16 [PATCH v2 bpf-next 0/4] libbpf: BTF dumper support for typed data Alan Maguire
2021-01-17 22:16 ` [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines Alan Maguire
@ 2021-01-17 22:16 ` Alan Maguire
2021-01-21 4:13 ` Andrii Nakryiko
2021-01-17 22:16 ` [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data Alan Maguire
2021-01-17 22:16 ` [PATCH v2 bpf-next 4/4] selftests/bpf: add dump type data tests to btf dump tests Alan Maguire
3 siblings, 1 reply; 12+ messages in thread
From: Alan Maguire @ 2021-01-17 22:16 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, morbo,
shuah, bpf, netdev, linux-kselftest, linux-kernel, Alan Maguire
btf_dump.c will need it for type-based data display.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/lib/bpf/libbpf.c | 4 +---
tools/lib/bpf/libbpf_internal.h | 2 ++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 2abbc38..4ef84e1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -73,8 +73,6 @@
#define __printf(a, b) __attribute__((format(printf, a, b)))
static struct bpf_map *bpf_object__add_map(struct bpf_object *obj);
-static const struct btf_type *
-skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id);
static int __base_pr(enum libbpf_print_level level, const char *format,
va_list args)
@@ -1885,7 +1883,7 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
return 0;
}
-static const struct btf_type *
+const struct btf_type *
skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id)
{
const struct btf_type *t = btf__type_by_id(btf, id);
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 969d0ac..c25d2df 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -108,6 +108,8 @@ static inline void *libbpf_reallocarray(void *ptr, size_t nmemb, size_t size)
void *btf_add_mem(void **data, size_t *cap_cnt, size_t elem_sz,
size_t cur_cnt, size_t max_cnt, size_t add_cnt);
int btf_ensure_mem(void **data, size_t *cap_cnt, size_t elem_sz, size_t need_cnt);
+const struct btf_type *skip_mods_and_typedefs(const struct btf *btf, __u32 id,
+ __u32 *res_id);
static inline bool libbpf_validate_opts(const char *opts,
size_t opts_sz, size_t user_sz,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data
2021-01-17 22:16 [PATCH v2 bpf-next 0/4] libbpf: BTF dumper support for typed data Alan Maguire
2021-01-17 22:16 ` [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines Alan Maguire
2021-01-17 22:16 ` [PATCH v2 bpf-next 2/4] libbpf: make skip_mods_and_typedefs available internally in libbpf Alan Maguire
@ 2021-01-17 22:16 ` Alan Maguire
2021-01-21 6:56 ` Andrii Nakryiko
2021-01-17 22:16 ` [PATCH v2 bpf-next 4/4] selftests/bpf: add dump type data tests to btf dump tests Alan Maguire
3 siblings, 1 reply; 12+ messages in thread
From: Alan Maguire @ 2021-01-17 22:16 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, morbo,
shuah, bpf, netdev, linux-kselftest, linux-kernel, Alan Maguire
Add a BTF dumper for typed data, so that the user can dump a typed
version of the data provided.
The API is
int btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
const struct btf_dump_emit_type_data_opts *opts,
void *data);
...where the id is the BTF id of the data pointed to by the "void *"
argument; for example the BTF id of "struct sk_buff" for a
"struct skb *" data pointer. Options supported are
- a starting indent level (indent_lvl)
- a set of boolean options to control dump display, similar to those
used for BPF helper bpf_snprintf_btf(). Options are
- compact : omit newlines and other indentation
- noname: omit member names
- zero: show zero-value members
Default output format is identical to that dumped by bpf_snprintf_btf(),
for example a "struct sk_buff" representation would look like this:
struct sk_buff){
(union){
(struct){
.next = (struct sk_buff *)0xffffffffffffffff,
.prev = (struct sk_buff *)0xffffffffffffffff,
(union){
.dev = (struct net_device *)0xffffffffffffffff,
.dev_scratch = (long unsigned int)18446744073709551615,
},
},
...
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/lib/bpf/btf.h | 17 +
tools/lib/bpf/btf_dump.c | 974 +++++++++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/libbpf.map | 5 +
3 files changed, 996 insertions(+)
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 0c48f2e..7937124 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -180,6 +180,23 @@ struct btf_dump_emit_type_decl_opts {
btf_dump__emit_type_decl(struct btf_dump *d, __u32 id,
const struct btf_dump_emit_type_decl_opts *opts);
+
+struct btf_dump_emit_type_data_opts {
+ /* size of this struct, for forward/backward compatibility */
+ size_t sz;
+ int indent_level;
+ /* below match "show" flags for bpf_show_snprintf() */
+ bool compact;
+ bool noname;
+ bool zero;
+};
+#define btf_dump_emit_type_data_opts__last_field zero
+
+LIBBPF_API int
+btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
+ const struct btf_dump_emit_type_data_opts *opts,
+ void *data);
+
/*
* A set of helpers for easier BTF types handling
*/
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 2f9d685..04d604f 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -10,6 +10,8 @@
#include <stddef.h>
#include <stdlib.h>
#include <string.h>
+#include <ctype.h>
+#include <endian.h>
#include <errno.h>
#include <linux/err.h>
#include <linux/btf.h>
@@ -19,14 +21,31 @@
#include "libbpf.h"
#include "libbpf_internal.h"
+#define BITS_PER_BYTE 8
+#define BITS_PER_U128 (sizeof(__u64) * BITS_PER_BYTE * 2)
+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
+#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
+#define BITS_ROUNDUP_BYTES(bits) \
+ (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+
static const char PREFIXES[] = "\t\t\t\t\t\t\t\t\t\t\t\t\t";
static const size_t PREFIX_CNT = sizeof(PREFIXES) - 1;
+
static const char *pfx(int lvl)
{
return lvl >= PREFIX_CNT ? PREFIXES : &PREFIXES[PREFIX_CNT - lvl];
}
+static const char SPREFIXES[] = " ";
+static const size_t SPREFIX_CNT = sizeof(SPREFIXES) - 1;
+
+static const char *spfx(int lvl)
+{
+ return lvl >= SPREFIX_CNT ? SPREFIXES : &SPREFIXES[SPREFIX_CNT - lvl];
+}
+
enum btf_dump_type_order_state {
NOT_ORDERED,
ORDERING,
@@ -53,6 +72,49 @@ struct btf_dump_type_aux_state {
__u8 referenced: 1;
};
+#define BTF_DUMP_DATA_MAX_NAME_LEN 256
+
+/*
+ * Common internal data for BTF type data dump operations.
+ *
+ * The implementation here is similar to that in kernel/bpf/btf.c
+ * that supports the bpf_snprintf_btf() helper, so any bugs in
+ * type data dumping here are likely in that code also.
+ *
+ * One challenge with showing nested data is we want to skip 0-valued
+ * data, but in order to figure out whether a nested object is all zeros
+ * we need to walk through it. As a result, we need to make two passes
+ * when handling structs, unions and arrays; the first path simply looks
+ * for nonzero data, while the second actually does the display. The first
+ * pass is signalled by state.depth_check being set, and if we
+ * encounter a non-zero value we set state.depth_to_show to the depth
+ * at which we encountered it. When we have completed the first pass,
+ * we will know if anything needs to be displayed if
+ * state.depth_to_show > state.depth. See btf_dump_emit_[struct,array]_data()
+ * for the implementation of this.
+ *
+ */
+struct btf_dump_data {
+ bool compact;
+ bool noname;
+ bool zero;
+ __u8 indent_lvl; /* base indent level */
+ /* below are used during iteration */
+ struct {
+ __u8 depth;
+ __u8 depth_to_show;
+ __u8 depth_check;
+ __u8 array_member:1,
+ array_terminated:1;
+ __u16 array_encoding;
+ __u32 type_id;
+ const struct btf_type *type;
+ const struct btf_member *member;
+ char name[BTF_DUMP_DATA_MAX_NAME_LEN];
+ int err;
+ } state;
+};
+
struct btf_dump {
const struct btf *btf;
const struct btf_ext *btf_ext;
@@ -89,6 +151,10 @@ struct btf_dump {
* name occurrences
*/
struct hashmap *ident_names;
+ /*
+ * data for typed display.
+ */
+ struct btf_dump_data data;
};
static size_t str_hash_fn(const void *key, void *ctx)
@@ -1438,3 +1504,911 @@ static const char *btf_dump_ident_name(struct btf_dump *d, __u32 id)
{
return btf_dump_resolve_name(d, id, d->ident_names);
}
+
+static void __btf_dump_emit_type_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset);
+
+static const struct btf_type *skip_mods(const struct btf *btf,
+ __u32 id, __u32 *res_id)
+{
+ const struct btf_type *t = btf__type_by_id(btf, id);
+
+ while (btf_is_mod(t)) {
+ id = t->type;
+ t = btf__type_by_id(btf, t->type);
+ }
+
+ if (res_id)
+ *res_id = id;
+
+ return t;
+}
+
+#define BTF_MAX_ITER 10
+#define BTF_KIND_BIT(kind) (1ULL << kind)
+
+/*
+ * Populate dump->data.state.name with type name information.
+ * Format of type name is
+ *
+ * [.member_name = ] (type_name)
+ */
+static const char *btf_dump_data_name(struct btf_dump *d)
+{
+ /* BTF_MAX_ITER array suffixes "[]" */
+ const char *array_suffixes = "[][][][][][][][][][]";
+ const char *array_suffix = &array_suffixes[strlen(array_suffixes)];
+ /* BTF_MAX_ITER pointer suffixes "*" */
+ const char *ptr_suffixes = "**********";
+ const char *ptr_suffix = &ptr_suffixes[strlen(ptr_suffixes)];
+ const char *name = NULL, *prefix = "", *parens = "";
+ const struct btf_member *m = d->data.state.member;
+ const struct btf_type *t = d->data.state.type;
+ const struct btf_array *array;
+ __u32 id = d->data.state.type_id;
+ const char *member = NULL;
+ bool show_member = false;
+ __u64 kinds = 0;
+ int i;
+
+ d->data.state.name[0] = '\0';
+
+ /*
+ * Don't show type name if we're showing an array member;
+ * in that case we show the array type so don't need to repeat
+ * ourselves for each member.
+ */
+ if (d->data.state.array_member)
+ return "";
+
+ /* Retrieve member name, if any. */
+ if (m) {
+ member = btf_name_of(d, m->name_off);
+ show_member = strlen(member) > 0;
+ id = m->type;
+ }
+
+ /*
+ * Start with type_id, as we have resolved the struct btf_type *
+ * via btf_dump_emit_modifier_data() past the parent typedef to the
+ * child struct, int etc it is defined as. In such cases, the type_id
+ * still represents the starting type while the struct btf_type *
+ * in our d->data.state points at the resolved type of the typedef.
+ */
+ t = btf__type_by_id(d->btf, id);
+ if (!t)
+ return "";
+
+ /*
+ * The goal here is to build up the right number of pointer and
+ * array suffixes while ensuring the type name for a typedef
+ * is represented. Along the way we accumulate a list of
+ * BTF kinds we have encountered, since these will inform later
+ * display; for example, pointer types will not require an
+ * opening "{" for struct, we will just display the pointer value.
+ *
+ * We also want to accumulate the right number of pointer or array
+ * indices in the format string while iterating until we get to
+ * the typedef/pointee/array member target type.
+ *
+ * We start by pointing at the end of pointer and array suffix
+ * strings; as we accumulate pointers and arrays we move the pointer
+ * or array string backwards so it will show the expected number of
+ * '*' or '[]' for the type. BTF_SHOW_MAX_ITER of nesting of pointers
+ * and/or arrays and typedefs are supported as a precaution.
+ *
+ * We also want to get typedef name while proceeding to resolve
+ * type it points to so that we can add parentheses if it is a
+ * "typedef struct" etc.
+ *
+ * Qualifiers ("const", "volatile", "restrict") are simply skipped
+ * as they complicate simple type name display without adding much
+ * in the case of displaying a cast in front of the data to be
+ * displayed.
+ */
+ for (i = 0; i < BTF_MAX_ITER; i++) {
+
+ switch (BTF_INFO_KIND(t->info)) {
+ case BTF_KIND_TYPEDEF:
+ if (!name)
+ name = btf_name_of(d, t->name_off);
+ kinds |= BTF_KIND_BIT(BTF_KIND_TYPEDEF);
+ id = t->type;
+ break;
+ case BTF_KIND_ARRAY:
+ kinds |= BTF_KIND_BIT(BTF_KIND_ARRAY);
+ parens = "[";
+ if (!t)
+ return "";
+ array = btf_array(t);
+ if (array_suffix > array_suffixes)
+ array_suffix -= 2;
+ id = array->type;
+ break;
+ case BTF_KIND_PTR:
+ kinds |= BTF_KIND_BIT(BTF_KIND_PTR);
+ if (ptr_suffix > ptr_suffixes)
+ ptr_suffix -= 1;
+ id = t->type;
+ break;
+ default:
+ id = 0;
+ break;
+ }
+ if (!id)
+ break;
+ t = skip_mods(d->btf, id, NULL);
+ }
+ /* We may not be able to represent this type; bail to be safe */
+ if (i == BTF_MAX_ITER) {
+ pr_warn("iters %d exceeded %d when displaying type name:[%u]\n",
+ i, BTF_MAX_ITER, id);
+ return "";
+ }
+
+ if (!name)
+ name = btf_name_of(d, t->name_off);
+
+ switch (BTF_INFO_KIND(t->info)) {
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION:
+ prefix = BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT ?
+ "struct" : "union";
+ /* if it's an array of struct/union, parens is already set */
+ if (!(kinds & (BTF_KIND_BIT(BTF_KIND_ARRAY))))
+ parens = "{";
+ break;
+ case BTF_KIND_ENUM:
+ prefix = "enum";
+ break;
+ default:
+ break;
+ }
+
+ /* pointer does not require parens */
+ if (kinds & BTF_KIND_BIT(BTF_KIND_PTR))
+ parens = "";
+ /* typedef does not require struct/union/enum prefix */
+ if (kinds & BTF_KIND_BIT(BTF_KIND_TYPEDEF))
+ prefix = "";
+
+ if (!name)
+ name = "";
+
+ /* Even if we don't want type name info, we want parentheses etc */
+ if (d->data.noname)
+ snprintf(d->data.state.name, sizeof(d->data.state.name), "%s",
+ parens);
+ else
+ snprintf(d->data.state.name, sizeof(d->data.state.name),
+ "%s%s%s(%s%s%s%s%s%s)%s",
+ /* first 3 strings comprise ".member = " */
+ show_member ? "." : "",
+ show_member ? member : "",
+ show_member ? " = " : "",
+ /* ...next is our prefix (struct, enum, etc) */
+ prefix,
+ strlen(prefix) > 0 && strlen(name) > 0 ? " " : "",
+ /* ...this is the type name itself */
+ name,
+ /* ...suffixed by the appropriate '*', '[]' suffixes */
+ strlen(name) > 0 && strlen(ptr_suffix) > 0 ? " " : "",
+ ptr_suffix,
+ array_suffix, parens);
+
+ return d->data.state.name;
+}
+
+static const char *btf_dump_data_indent(struct btf_dump *d)
+{
+ if (d->data.compact)
+ return "";
+ return spfx(d->data.indent_lvl + d->data.state.depth);
+}
+
+static const char *btf_dump_data_newline(struct btf_dump *d)
+{
+ return d->data.compact ? "" : "\n";
+}
+
+static const char *btf_dump_data_delim(struct btf_dump *d)
+{
+ if (d->data.state.depth == 0)
+ return "";
+
+ if (d->data.compact &&
+ d->data.state.type &&
+ BTF_INFO_KIND(d->data.state.type->info) == BTF_KIND_UNION)
+ return "|";
+
+ return ",";
+}
+
+static void btf_dump_data_printf(struct btf_dump *d,
+ const char *fmt, ...)
+{
+ va_list args;
+
+ /*
+ * Just checking if there is non-zero data to display at this depth,
+ * so nothing is displayed.
+ */
+ if (d->data.state.depth_check)
+ return;
+ va_start(args, fmt);
+ d->printf_fn(d->opts.ctx, fmt, args);
+ va_end(args);
+}
+
+/* Macros are used here as btf_type_value[s]() prepends and appends
+ * format specifiers to the format specifier passed in; these do the work of
+ * adding indentation, delimiters etc while the caller simply has to specify
+ * the type value(s) in the format specifier + value(s).
+ */
+#define btf_dump_emit_type_value(d, fmt, value) \
+ do { \
+ if ((value) != 0 || d->data.zero || \
+ d->data.state.depth == 0) { \
+ btf_dump_data_printf(d, "%s%s" fmt "%s%s", \
+ btf_dump_data_indent(d), \
+ btf_dump_data_name(d), \
+ value, \
+ btf_dump_data_delim(d), \
+ btf_dump_data_newline(d)); \
+ if (d->data.state.depth > \
+ d->data.state.depth_to_show) \
+ d->data.state.depth_to_show = \
+ d->data.state.depth; \
+ } \
+ } while (0)
+
+#define btf_dump_emit_type_values(d, fmt, ...) \
+ do { \
+ btf_dump_data_printf(d, "%s%s" fmt "%s%s", \
+ btf_dump_data_indent(d), \
+ btf_dump_data_name(d), \
+ __VA_ARGS__, \
+ btf_dump_data_delim(d), \
+ btf_dump_data_newline(d)); \
+ if (d->data.state.depth > \
+ d->data.state.depth_to_show) \
+ d->data.state.depth_to_show = \
+ d->data.state.depth; \
+ } while (0)
+
+/* Set the type we are starting to show. */
+static void btf_dump_start_type(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 type_id)
+{
+ d->data.state.type = t;
+ d->data.state.type_id = type_id;
+ d->data.state.name[0] = '\0';
+}
+
+static void btf_dump_end_type(struct btf_dump *d)
+{
+ d->data.state.type = NULL;
+ d->data.state.type_id = 0;
+ d->data.state.name[0] = '\0';
+}
+
+static void btf_dump_start_aggr_type(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 type_id)
+{
+ btf_dump_start_type(d, t, type_id);
+
+ btf_dump_data_printf(d, "%s%s%s",
+ btf_dump_data_indent(d),
+ btf_dump_data_name(d),
+ btf_dump_data_newline(d));
+ d->data.state.depth++;
+}
+
+static void btf_dump_end_aggr_type(struct btf_dump *d,
+ const char *suffix)
+{
+ d->data.state.depth--;
+ btf_dump_data_printf(d, "%s%s%s%s",
+ btf_dump_data_indent(d),
+ suffix,
+ btf_dump_data_delim(d),
+ btf_dump_data_newline(d));
+ btf_dump_end_type(d);
+}
+
+static void btf_dump_start_member(struct btf_dump *d,
+ const struct btf_member *m)
+{
+ d->data.state.member = m;
+}
+
+static void btf_dump_start_array_member(struct btf_dump *d)
+{
+ d->data.state.array_member = 1;
+ btf_dump_start_member(d, NULL);
+}
+
+static void btf_dump_end_member(struct btf_dump *d)
+{
+ d->data.state.member = NULL;
+}
+
+static void btf_dump_end_array_member(struct btf_dump *d)
+{
+ d->data.state.array_member = 0;
+ btf_dump_end_member(d);
+}
+
+static void btf_dump_start_array_type(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 type_id,
+ __u16 array_encoding)
+{
+ d->data.state.array_encoding = array_encoding;
+ d->data.state.array_terminated = 0;
+ btf_dump_start_aggr_type(d, t, type_id);
+}
+
+static void btf_dump_end_array_type(struct btf_dump *d)
+{
+ d->data.state.array_encoding = 0;
+ d->data.state.array_terminated = 0;
+ btf_dump_end_aggr_type(d, "]");
+}
+
+static void btf_dump_start_struct_type(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 type_id)
+{
+ btf_dump_start_aggr_type(d, t, type_id);
+}
+
+static void btf_dump_end_struct_type(struct btf_dump *d)
+{
+ btf_dump_end_aggr_type(d, "}");
+}
+
+static void btf_dump_emit_df_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ btf_dump_data_printf(d, "<unsupported kind:%u>",
+ BTF_INFO_KIND(t->info));
+}
+
+static void btf_dump_emit_int128(struct btf_dump *d, void *data)
+{
+ /* data points to a __int128 number.
+ * Suppose
+ * int128_num = *(__int128 *)data;
+ * The below formulas shows what upper_num and lower_num represents:
+ * upper_num = int128_num >> 64;
+ * lower_num = int128_num & 0xffffffffFFFFFFFFULL;
+ */
+ __u64 upper_num, lower_num;
+
+#ifdef __BIG_ENDIAN_BITFIELD
+ upper_num = *(__u64 *)data;
+ lower_num = *(__u64 *)(data + 8);
+#else
+ upper_num = *(__u64 *)(data + 8);
+ lower_num = *(__u64 *)data;
+#endif
+ if (upper_num == 0)
+ btf_dump_emit_type_value(d, "0x%llx", lower_num);
+ else
+ btf_dump_emit_type_values(d, "0x%llx%016llx", upper_num,
+ lower_num);
+}
+
+static void btf_int128_shift(__u64 *print_num, __u16 left_shift_bits,
+ __u16 right_shift_bits)
+{
+ __u64 upper_num, lower_num;
+
+#ifdef __BIG_ENDIAN_BITFIELD
+ upper_num = print_num[0];
+ lower_num = print_num[1];
+#else
+ upper_num = print_num[1];
+ lower_num = print_num[0];
+#endif
+
+ /* shake out un-needed bits by shift/or operations */
+ if (left_shift_bits >= 64) {
+ upper_num = lower_num << (left_shift_bits - 64);
+ lower_num = 0;
+ } else {
+ upper_num = (upper_num << left_shift_bits) |
+ (lower_num >> (64 - left_shift_bits));
+ lower_num = lower_num << left_shift_bits;
+ }
+
+ if (right_shift_bits >= 64) {
+ lower_num = upper_num >> (right_shift_bits - 64);
+ upper_num = 0;
+ } else {
+ lower_num = (lower_num >> right_shift_bits) |
+ (upper_num << (64 - right_shift_bits));
+ upper_num = upper_num >> right_shift_bits;
+ }
+
+#ifdef __BIG_ENDIAN_BITFIELD
+ print_num[0] = upper_num;
+ print_num[1] = lower_num;
+#else
+ print_num[0] = lower_num;
+ print_num[1] = upper_num;
+#endif
+}
+
+static void btf_dump_emit_bitfield_data(struct btf_dump *d,
+ void *data,
+ __u8 bits_offset,
+ __u8 nr_bits)
+{
+ __u16 left_shift_bits, right_shift_bits;
+ __u8 nr_copy_bytes;
+ __u8 nr_copy_bits;
+ __u64 print_num[2] = {};
+
+ nr_copy_bits = nr_bits + bits_offset;
+ nr_copy_bytes = BITS_ROUNDUP_BYTES(nr_copy_bits);
+
+ memcpy(print_num, data, nr_copy_bytes);
+
+#ifdef __BIG_ENDIAN_BITFIELD
+ left_shift_bits = bits_offset;
+#else
+ left_shift_bits = BITS_PER_U128 - nr_copy_bits;
+#endif
+ right_shift_bits = BITS_PER_U128 - nr_bits;
+
+ btf_int128_shift(print_num, left_shift_bits, right_shift_bits);
+ btf_dump_emit_int128(d, print_num);
+}
+
+static void btf_dump_emit_int_bits(struct btf_dump *d,
+ const struct btf_type *t,
+ void *data,
+ __u8 bits_offset)
+{
+ __u32 int_data = btf_int(t);
+ __u8 nr_bits = BTF_INT_BITS(int_data);
+ __u8 total_bits_offset;
+
+ /*
+ * bits_offset is at most 7.
+ * BTF_INT_OFFSET() cannot exceed 128 bits.
+ */
+ total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
+ data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
+ bits_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+ btf_dump_emit_bitfield_data(d, data, bits_offset, nr_bits);
+}
+
+static void btf_dump_emit_int_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 type_id,
+ void *data,
+ __u8 bits_offset)
+{
+ __u32 int_data = btf_int(t);
+ __u8 encoding = BTF_INT_ENCODING(int_data);
+ bool sign = encoding & BTF_INT_SIGNED;
+ __u8 nr_bits = BTF_INT_BITS(int_data);
+
+ btf_dump_start_type(d, t, type_id);
+
+ if (bits_offset || BTF_INT_OFFSET(int_data) ||
+ BITS_PER_BYTE_MASKED(nr_bits)) {
+ btf_dump_emit_int_bits(d, t, data, bits_offset);
+ goto out;
+ }
+
+ switch (nr_bits) {
+ case 128:
+ btf_dump_emit_int128(d, data);
+ break;
+ case 64:
+ if (sign)
+ btf_dump_emit_type_value(d, "%lld", *(__s64 *)data);
+ else
+ btf_dump_emit_type_value(d, "%llu", *(__u64 *)data);
+ break;
+ case 32:
+ if (sign)
+ btf_dump_emit_type_value(d, "%d", *(__s32 *)data);
+ else
+ btf_dump_emit_type_value(d, "%u", *(__u32 *)data);
+ break;
+ case 16:
+ if (sign)
+ btf_dump_emit_type_value(d, "%d", *(__s16 *)data);
+ else
+ btf_dump_emit_type_value(d, "%u", *(__u16 *)data);
+ break;
+ case 8:
+ if (d->data.state.array_encoding == BTF_INT_CHAR) {
+ /* check for null terminator */
+ if (d->data.state.array_terminated)
+ break;
+ if (*(char *)data == '\0') {
+ d->data.state.array_terminated = 1;
+ break;
+ }
+ if (isprint(*(char *)data)) {
+ btf_dump_emit_type_value(d, "'%c'",
+ *(char *)data);
+ break;
+ }
+ }
+ if (sign)
+ btf_dump_emit_type_value(d, "%d", *(__s8 *)data);
+ else
+ btf_dump_emit_type_value(d, "%u", *(__u8 *)data);
+ break;
+ default:
+ btf_dump_emit_int_bits(d, t, data, bits_offset);
+ break;
+ }
+out:
+ btf_dump_end_type(d);
+}
+
+static void btf_dump_emit_modifier_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ t = skip_mods_and_typedefs(d->btf, id, NULL);
+ __btf_dump_emit_type_data(d, t, id, data, bits_offset);
+}
+
+static void btf_dump_emit_var_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ __u32 linkage = btf_var(t)->linkage;
+
+ btf_dump_data_printf(d, "%s%s =",
+ linkage ? "" : "static ",
+ btf_name_of(d, t->name_off));
+ t = btf__type_by_id(d->btf, t->type);
+ __btf_dump_emit_type_data(d, t, t->type, data, bits_offset);
+}
+
+static void __btf_dump_emit_array_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ const struct btf_array *array = btf_array(t);
+ const struct btf_type *elem_type;
+ __u32 i, elem_size = 0, elem_type_id;
+ __u16 encoding = 0;
+
+ elem_type_id = array->type;
+ elem_type = skip_mods_and_typedefs(d->btf, elem_type_id, NULL);
+ if (elem_type && btf_has_size(elem_type))
+ elem_size = elem_type->size;
+
+ if (elem_type && btf_is_int(elem_type)) {
+ __u32 int_type = btf_int(elem_type);
+
+ encoding = BTF_INT_ENCODING(int_type);
+
+ /*
+ * BTF_INT_CHAR encoding never seems to be set for
+ * char arrays, so if size is 1 and element is
+ * printable as a char, we'll do that.
+ */
+ if (elem_size == 1)
+ encoding = BTF_INT_CHAR;
+ }
+
+ btf_dump_start_array_type(d, t, id, encoding);
+
+ if (!elem_type)
+ goto out;
+
+ for (i = 0; i < array->nelems; i++) {
+
+ btf_dump_start_array_member(d);
+
+ __btf_dump_emit_type_data(d, elem_type, elem_type_id,
+ data, bits_offset);
+ data += elem_size;
+
+ btf_dump_end_array_member(d);
+
+ if (d->data.state.array_terminated)
+ break;
+ }
+out:
+ btf_dump_end_array_type(d);
+}
+
+static void btf_dump_emit_array_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ const struct btf_member *m = d->data.state.member;
+
+ /*
+ * First check if any members would be shown (are non-zero).
+ * See comments above "struct btf_dump_data" definition for more
+ * details on how this works at a high-level.
+ */
+ if (d->data.state.depth > 0 && !d->data.zero) {
+ if (!d->data.state.depth_check) {
+ d->data.state.depth_check = d->data.state.depth + 1;
+ d->data.state.depth_to_show = 0;
+ }
+ __btf_dump_emit_array_data(d, t, id, data, bits_offset);
+ d->data.state.member = m;
+
+ if (d->data.state.depth_check != d->data.state.depth + 1)
+ return;
+ d->data.state.depth_check = 0;
+
+ if (d->data.state.depth_to_show <= d->data.state.depth)
+ return;
+ /*
+ * Reaching here indicates we have recursed and found
+ * non-zero array member(s).
+ */
+ }
+ __btf_dump_emit_array_data(d, t, id, data, bits_offset);
+}
+
+#define for_each_member(i, struct_type, member) \
+ for (i = 0, member = btf_members(struct_type); \
+ i < btf_vlen(struct_type); \
+ i++, member++)
+
+static void __btf_dump_emit_struct_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ const struct btf_member *member;
+ __u32 i;
+
+ btf_dump_start_struct_type(d, t, id);
+
+ for_each_member(i, t, member) {
+ const struct btf_type *member_type;
+ __u32 member_offset, bitfield_size;
+ __u32 bytes_offset;
+ __u8 bits8_offset;
+
+ member_type = btf__type_by_id(d->btf, member->type);
+ btf_dump_start_member(d, member);
+
+ member_offset = btf_member_bit_offset(t, i);
+ bitfield_size = btf_member_bitfield_size(t, i);
+ bytes_offset = BITS_ROUNDDOWN_BYTES(member_offset);
+ bits8_offset = BITS_PER_BYTE_MASKED(member_offset);
+ if (bitfield_size) {
+ btf_dump_start_type(d, member_type, member->type);
+ btf_dump_emit_bitfield_data(d,
+ data + bytes_offset,
+ bits8_offset,
+ bitfield_size);
+ btf_dump_end_type(d);
+ } else {
+ __btf_dump_emit_type_data(d, member_type, member->type,
+ data + bytes_offset, bits8_offset);
+ }
+ btf_dump_end_member(d);
+ }
+ btf_dump_end_struct_type(d);
+}
+
+static void btf_dump_emit_struct_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ const struct btf_member *m = d->data.state.member;
+
+ /*
+ * First check if any members would be shown (are non-zero).
+ * See comments above "struct btf_dump_data" definition for more
+ * details on how this works at a high-level.
+ */
+ if (d->data.state.depth > 0 && !d->data.zero) {
+ if (!d->data.state.depth_check) {
+ d->data.state.depth_check = d->data.state.depth + 1;
+ d->data.state.depth_to_show = 0;
+ }
+ __btf_dump_emit_struct_data(d, t, id, data, bits_offset);
+ /* Restore saved member data here */
+ d->data.state.member = m;
+ if (d->data.state.depth_check != d->data.state.depth + 1)
+ return;
+ d->data.state.depth_check = 0;
+
+ if (d->data.state.depth_to_show <= d->data.state.depth)
+ return;
+ /*
+ * Reaching here indicates we have recursed and found
+ * non-zero child values.
+ */
+ }
+
+ __btf_dump_emit_struct_data(d, t, id, data, bits_offset);
+}
+
+static void btf_dump_emit_ptr_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ btf_dump_start_type(d, t, id);
+
+ btf_dump_emit_type_value(d, "%p", *(void **)data);
+ btf_dump_end_type(d);
+}
+
+static void btf_dump_emit_enum_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ const struct btf_enum *enums = btf_enum(t);
+ __s64 value;
+ __u16 i;
+
+ btf_dump_start_type(d, t, id);
+
+ switch (t->size) {
+ case 8:
+ value = *(__s64 *)data;
+ break;
+ case 4:
+ value = *(__s32 *)data;
+ break;
+ case 2:
+ value = *(__s16 *)data;
+ break;
+ case 1:
+ value = *(__s8 *)data;
+ break;
+ default:
+ pr_warn("unexpected size %d for enum, id:[%u]\n", t->size,
+ id);
+ d->data.state.err = -EINVAL;
+ return;
+ }
+
+ for (i = 0; i < btf_vlen(t); i++) {
+ if (value == enums[i].val) {
+ btf_dump_emit_type_value(d, "%s",
+ btf_name_of(d,
+ enums[i].name_off));
+ btf_dump_end_type(d);
+ return;
+ }
+ }
+
+ btf_dump_emit_type_value(d, "%d", value);
+ btf_dump_end_type(d);
+}
+
+#define for_each_vsi(i, struct_type, member) \
+ for (i = 0, member = btf_var_secinfos(struct_type); \
+ i < btf_vlen(struct_type); \
+ i++, member++)
+
+static void btf_dump_emit_datasec_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ const struct btf_var_secinfo *vsi;
+ const struct btf_type *var;
+ __u32 i;
+
+ btf_dump_start_type(d, t, id);
+
+ btf_dump_emit_type_value(d, "section (\"%s\") = {",
+ btf_name_of(d, t->name_off));
+ for_each_vsi(i, t, vsi) {
+ var = btf__type_by_id(d->btf, vsi->type);
+ if (i)
+ btf_dump_data_printf(d, ",");
+ __btf_dump_emit_type_data(d, var, vsi->type,
+ data + vsi->offset,
+ bits_offset);
+ }
+ btf_dump_end_type(d);
+}
+
+typedef void (*btf_dump_emit_kind_data)(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset);
+
+static btf_dump_emit_kind_data dump_emit_kind_data[NR_BTF_KINDS] = {
+ &btf_dump_emit_df_data,
+ &btf_dump_emit_int_data,
+ &btf_dump_emit_ptr_data,
+ &btf_dump_emit_array_data,
+ &btf_dump_emit_struct_data,
+ &btf_dump_emit_struct_data,
+ &btf_dump_emit_enum_data,
+ &btf_dump_emit_df_data,
+ &btf_dump_emit_modifier_data,
+ &btf_dump_emit_modifier_data,
+ &btf_dump_emit_modifier_data,
+ &btf_dump_emit_modifier_data,
+ &btf_dump_emit_df_data,
+ &btf_dump_emit_df_data,
+ &btf_dump_emit_var_data,
+ &btf_dump_emit_datasec_data,
+};
+
+static void __btf_dump_emit_type_data(struct btf_dump *d,
+ const struct btf_type *t,
+ __u32 id,
+ void *data,
+ __u8 bits_offset)
+{
+ dump_emit_kind_data[BTF_INFO_KIND(t->info)](d, t, id, data,
+ bits_offset);
+}
+
+static void btf_dump_emit_type_data(struct btf_dump *d, __u32 id, void *data)
+{
+ const struct btf_type *t = btf__type_by_id(d->btf, id);
+
+ memset(&d->data.state, 0, sizeof(d->data.state));
+
+ if (!t) {
+ pr_warn("no type info, id [%u]\n", id);
+ d->data.state.err = -EINVAL;
+ return;
+ }
+
+ __btf_dump_emit_type_data(d, t, id, data, 0);
+}
+
+int btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
+ const struct btf_dump_emit_type_data_opts *opts,
+ void *data)
+{
+ int err;
+
+ if (!OPTS_VALID(opts, btf_dump_emit_type_data_opts))
+ return -EINVAL;
+
+ d->data.indent_lvl = OPTS_GET(opts, indent_level, 0);
+ d->data.compact = OPTS_GET(opts, compact, false);
+ d->data.noname = OPTS_GET(opts, noname, false);
+ d->data.zero = OPTS_GET(opts, zero, false);
+ btf_dump_emit_type_data(d, id, data);
+ err = d->data.state.err;
+ memset(&d->data, 0, sizeof(d->data));
+ return err;
+}
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 1c0fd2d..b81c069 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -350,3 +350,8 @@ LIBBPF_0.3.0 {
xsk_setup_xdp_prog;
xsk_socket__update_xskmap;
} LIBBPF_0.2.0;
+
+LIBBPF_0.4.0 {
+ global:
+ btf_dump__emit_type_data;
+} LIBBPF_0.3.0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 bpf-next 4/4] selftests/bpf: add dump type data tests to btf dump tests
2021-01-17 22:16 [PATCH v2 bpf-next 0/4] libbpf: BTF dumper support for typed data Alan Maguire
` (2 preceding siblings ...)
2021-01-17 22:16 ` [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data Alan Maguire
@ 2021-01-17 22:16 ` Alan Maguire
2021-01-21 7:01 ` Andrii Nakryiko
3 siblings, 1 reply; 12+ messages in thread
From: Alan Maguire @ 2021-01-17 22:16 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, morbo,
shuah, bpf, netdev, linux-kselftest, linux-kernel, Alan Maguire
Test various type data dumping operations by comparing expected
format with the dumped string; an snprintf-style printf function
is used to record the string dumped.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/testing/selftests/bpf/prog_tests/btf_dump.c | 233 ++++++++++++++++++++++
1 file changed, 233 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index c60091e..262561f4 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -232,6 +232,237 @@ void test_btf_dump_incremental(void)
btf__free(btf);
}
+#define STRSIZE 2048
+#define EXPECTED_STRSIZE 256
+
+void btf_dump_snprintf(void *ctx, const char *fmt, va_list args)
+{
+ char *s = ctx, new[STRSIZE];
+
+ vsnprintf(new, STRSIZE, fmt, args);
+ strncat(s, new, STRSIZE);
+ vfprintf(ctx, fmt, args);
+}
+
+/* skip "enum "/"struct " prefixes */
+#define SKIP_PREFIX(_typestr, _prefix) \
+ do { \
+ if (strstr(_typestr, _prefix) == _typestr) \
+ _typestr += strlen(_prefix) + 1; \
+ } while (0)
+
+int btf_dump_data(struct btf *btf, struct btf_dump *d,
+ char *ptrtype, __u64 flags, void *ptr,
+ char *str, char *expectedval)
+{
+ struct btf_dump_emit_type_data_opts opts = { 0 };
+ int ret = 0, cmp;
+ __s32 type_id;
+
+ opts.sz = sizeof(opts);
+ opts.compact = true;
+ if (flags & BTF_F_NONAME)
+ opts.noname = true;
+ if (flags & BTF_F_ZERO)
+ opts.zero = true;
+ SKIP_PREFIX(ptrtype, "enum");
+ SKIP_PREFIX(ptrtype, "struct");
+ SKIP_PREFIX(ptrtype, "union");
+ type_id = btf__find_by_name(btf, ptrtype);
+ if (CHECK(type_id <= 0, "find type id",
+ "no '%s' in BTF: %d\n", ptrtype, type_id)) {
+ ret = -ENOENT;
+ goto err;
+ }
+ str[0] = '\0';
+ ret = btf_dump__emit_type_data(d, type_id, &opts, ptr);
+ if (CHECK(ret < 0, "btf_dump__emit_type_data",
+ "failed: %d\n", ret))
+ goto err;
+
+ cmp = strncmp(str, expectedval, EXPECTED_STRSIZE);
+ if (CHECK(cmp, "ensure expected/actual match",
+ "'%s' does not match expected '%s': %d\n",
+ str, expectedval, cmp))
+ ret = -EFAULT;
+
+err:
+ if (ret)
+ btf_dump__free(d);
+ return ret;
+}
+
+#define TEST_BTF_DUMP_DATA(_b, _d, _str, _type, _flags, _expected, ...) \
+ do { \
+ char _expectedval[EXPECTED_STRSIZE] = _expected; \
+ char __ptrtype[64] = #_type; \
+ char *_ptrtype = (char *)__ptrtype; \
+ static _type _ptrdata = __VA_ARGS__; \
+ void *_ptr = &_ptrdata; \
+ \
+ if (btf_dump_data(_b, _d, _ptrtype, _flags, _ptr, \
+ _str, _expectedval)) \
+ return; \
+ } while (0)
+
+/* Use where expected data string matches its stringified declaration */
+#define TEST_BTF_DUMP_DATA_C(_b, _d, _str, _type, _opts, ...) \
+ TEST_BTF_DUMP_DATA(_b, _d, _str, _type, _opts, \
+ "(" #_type ")" #__VA_ARGS__, __VA_ARGS__)
+
+void test_btf_dump_data(void)
+{
+ struct btf *btf = libbpf_find_kernel_btf();
+ char str[STRSIZE];
+ struct btf_dump_opts opts = { .ctx = str };
+ struct btf_dump *d;
+
+ if (CHECK(!btf, "get kernel BTF", "no kernel BTF found"))
+ return;
+
+ d = btf_dump__new(btf, NULL, &opts, btf_dump_snprintf);
+
+ if (CHECK(!d, "new dump", "could not create BTF dump"))
+ return;
+
+ /* Verify type display for various types. */
+
+ /* simple int */
+ TEST_BTF_DUMP_DATA_C(btf, d, str, int, 0, 1234);
+ TEST_BTF_DUMP_DATA(btf, d, str, int, BTF_F_NONAME, "1234", 1234);
+
+ /* zero value should be printed at toplevel */
+ TEST_BTF_DUMP_DATA(btf, d, str, int, 0, "(int)0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, int, BTF_F_NONAME, "0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, int, BTF_F_ZERO, "(int)0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, int, BTF_F_NONAME | BTF_F_ZERO,
+ "0", 0);
+ TEST_BTF_DUMP_DATA_C(btf, d, str, int, 0, -4567);
+ TEST_BTF_DUMP_DATA(btf, d, str, int, BTF_F_NONAME, "-4567", -4567);
+
+ /* simple char */
+ TEST_BTF_DUMP_DATA_C(btf, d, str, char, 0, 100);
+ TEST_BTF_DUMP_DATA(btf, d, str, char, BTF_F_NONAME, "100", 100);
+ /* zero value should be printed at toplevel */
+ TEST_BTF_DUMP_DATA(btf, d, str, char, 0, "(char)0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, char, BTF_F_NONAME, "0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, char, BTF_F_ZERO, "(char)0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, char, BTF_F_NONAME | BTF_F_ZERO,
+ "0", 0);
+
+ /* simple typedef */
+ TEST_BTF_DUMP_DATA_C(btf, d, str, uint64_t, 0, 100);
+ TEST_BTF_DUMP_DATA(btf, d, str, u64, BTF_F_NONAME, "1", 1);
+ /* zero value should be printed at toplevel */
+ TEST_BTF_DUMP_DATA(btf, d, str, u64, 0, "(u64)0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, u64, BTF_F_NONAME, "0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, u64, BTF_F_ZERO, "(u64)0", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, u64, BTF_F_NONAME | BTF_F_ZERO,
+ "0", 0);
+
+ /* typedef struct */
+ TEST_BTF_DUMP_DATA_C(btf, d, str, atomic_t, 0, {.counter = (int)1,});
+ TEST_BTF_DUMP_DATA(btf, d, str, atomic_t, BTF_F_NONAME, "{1,}",
+ {.counter = 1,});
+ /* typedef with 0 value should be printed at toplevel */
+ TEST_BTF_DUMP_DATA(btf, d, str, atomic_t, 0, "(atomic_t){}",
+ {.counter = 0,});
+ TEST_BTF_DUMP_DATA(btf, d, str, atomic_t, BTF_F_NONAME, "{}",
+ {.counter = 0,});
+ TEST_BTF_DUMP_DATA(btf, d, str, atomic_t, BTF_F_ZERO,
+ "(atomic_t){.counter = (int)0,}",
+ {.counter = 0,});
+ TEST_BTF_DUMP_DATA(btf, d, str, atomic_t, BTF_F_NONAME | BTF_F_ZERO,
+ "{0,}", {.counter = 0,});
+ /* enum where enum value does (and does not) exist */
+ TEST_BTF_DUMP_DATA_C(btf, d, str, enum bpf_cmd, 0, BPF_MAP_CREATE);
+ TEST_BTF_DUMP_DATA(btf, d, str, enum bpf_cmd, 0,
+ "(enum bpf_cmd)BPF_MAP_CREATE", 0);
+ TEST_BTF_DUMP_DATA(btf, d, str, enum bpf_cmd, BTF_F_NONAME,
+ "BPF_MAP_CREATE",
+ BPF_MAP_CREATE);
+ TEST_BTF_DUMP_DATA(btf, d, str, enum bpf_cmd,
+ BTF_F_NONAME | BTF_F_ZERO,
+ "BPF_MAP_CREATE", 0);
+
+ TEST_BTF_DUMP_DATA(btf, d, str, enum bpf_cmd, BTF_F_ZERO,
+ "(enum bpf_cmd)BPF_MAP_CREATE",
+ BPF_MAP_CREATE);
+ TEST_BTF_DUMP_DATA(btf, d, str, enum bpf_cmd,
+ BTF_F_NONAME | BTF_F_ZERO,
+ "BPF_MAP_CREATE", BPF_MAP_CREATE);
+ TEST_BTF_DUMP_DATA_C(btf, d, str, enum bpf_cmd, 0, 2000);
+ TEST_BTF_DUMP_DATA(btf, d, str, enum bpf_cmd, BTF_F_NONAME,
+ "2000", 2000);
+
+ /* simple struct */
+ TEST_BTF_DUMP_DATA_C(btf, d, str, struct btf_enum, 0,
+ {.name_off = (__u32)3,.val = (__s32)-1,});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct btf_enum, BTF_F_NONAME,
+ "{3,-1,}",
+ { .name_off = 3, .val = -1,});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct btf_enum, BTF_F_NONAME, "{-1,}",
+ { .name_off = 0, .val = -1,});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct btf_enum,
+ BTF_F_NONAME | BTF_F_ZERO,
+ "{0,-1,}",
+ { .name_off = 0, .val = -1,});
+ /* empty struct should be printed */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct btf_enum, 0,
+ "(struct btf_enum){}",
+ { .name_off = 0, .val = 0,});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct btf_enum, BTF_F_NONAME, "{}",
+ { .name_off = 0, .val = 0,});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct btf_enum, BTF_F_ZERO,
+ "(struct btf_enum){.name_off = (__u32)0,.val = (__s32)0,}",
+ { .name_off = 0, .val = 0,});
+
+ /* struct with pointers */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct list_head, 0,
+ "(struct list_head){.next = (struct list_head *)0x1,}",
+ { .next = (struct list_head *)1 });
+ /* NULL pointer should not be displayed */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct list_head, 0,
+ "(struct list_head){}",
+ { .next = (struct list_head *)0 });
+ /* struct with char array */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct bpf_prog_info, 0,
+ "(struct bpf_prog_info){.name = (char[])['f','o','o',],}",
+ { .name = "foo",});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct bpf_prog_info, BTF_F_NONAME,
+ "{['f','o','o',],}",
+ {.name = "foo",});
+ /* leading null char means do not display string */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct bpf_prog_info, 0,
+ "(struct bpf_prog_info){}",
+ {.name = {'\0', 'f', 'o', 'o'}});
+ /* handle non-printable characters */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct bpf_prog_info, 0,
+ "(struct bpf_prog_info){.name = (char[])[1,2,3,],}",
+ { .name = {1, 2, 3, 0}});
+
+ /* struct with non-char array */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct __sk_buff, 0,
+ "(struct __sk_buff){.cb = (__u32[])[1,2,3,4,5,],}",
+ { .cb = {1, 2, 3, 4, 5,},});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct __sk_buff, BTF_F_NONAME,
+ "{[1,2,3,4,5,],}",
+ { .cb = { 1, 2, 3, 4, 5},});
+ /* For non-char, arrays, show non-zero values only */
+ TEST_BTF_DUMP_DATA(btf, d, str, struct __sk_buff, 0,
+ "(struct __sk_buff){.cb = (__u32[])[1,],}",
+ { .cb = { 0, 0, 1, 0, 0},});
+
+ /* struct with bitfields */
+ TEST_BTF_DUMP_DATA_C(btf, d, str, struct bpf_insn, 0,
+ {.code = (__u8)1,.dst_reg = (__u8)0x2,.src_reg = (__u8)0x3,.off = (__s16)4,.imm = (__s32)5,});
+ TEST_BTF_DUMP_DATA(btf, d, str, struct bpf_insn, BTF_F_NONAME,
+ "{1,0x2,0x3,4,5,}",
+ { .code = 1, .dst_reg = 0x2, .src_reg = 0x3, .off = 4,
+ .imm = 5,});
+}
+
+
void test_btf_dump() {
int i;
@@ -245,4 +476,6 @@ void test_btf_dump() {
}
if (test__start_subtest("btf_dump: incremental"))
test_btf_dump_incremental();
+ if (test__start_subtest("btf_dump: data"))
+ test_btf_dump_data();
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines
2021-01-17 22:16 ` [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines Alan Maguire
@ 2021-01-21 4:11 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 4:11 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, morbo,
Shuah Khan, bpf, Networking, open list:KERNEL SELFTEST FRAMEWORK,
open list
On Sun, Jan 17, 2021 at 2:22 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> BTF type data dumping will use them in later patches, and they
> are useful generally when handling BTF data.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/lib/bpf/btf.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 1237bcd..0c48f2e 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -294,6 +294,20 @@ static inline bool btf_is_datasec(const struct btf_type *t)
> return btf_kind(t) == BTF_KIND_DATASEC;
> }
>
> +static inline bool btf_has_size(const struct btf_type *t)
> +{
> + switch (BTF_INFO_KIND(t->info)) {
> + case BTF_KIND_INT:
> + case BTF_KIND_STRUCT:
> + case BTF_KIND_UNION:
> + case BTF_KIND_ENUM:
> + case BTF_KIND_DATASEC:
> + return true;
> + default:
> + return false;
> + }
> +}
it's not clear what "has_size" means, actually. E.g., array type
definitely has size, it's not just as readily available. And you are
actually misusing this in your algorithm, I'll point it out in the
respective patch. Please remove this, or if absolutely necessary move
into btf_dump.c as an inner static function.
> +
> static inline __u8 btf_int_encoding(const struct btf_type *t)
> {
> return BTF_INT_ENCODING(*(__u32 *)(t + 1));
> @@ -309,6 +323,11 @@ static inline __u8 btf_int_bits(const struct btf_type *t)
> return BTF_INT_BITS(*(__u32 *)(t + 1));
> }
>
> +static inline __u32 btf_int(const struct btf_type *t)
> +{
> + return *(__u32 *)(t + 1);
> +}
> +
there is btf_int_encoding(), btf_ind_offset() and btf_int_bits() that
properly decompose what btf_int() above returns. I'm not convinced
btf_int() has to be exposed as a public API.
> static inline struct btf_array *btf_array(const struct btf_type *t)
> {
> return (struct btf_array *)(t + 1);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 bpf-next 2/4] libbpf: make skip_mods_and_typedefs available internally in libbpf
2021-01-17 22:16 ` [PATCH v2 bpf-next 2/4] libbpf: make skip_mods_and_typedefs available internally in libbpf Alan Maguire
@ 2021-01-21 4:13 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 4:13 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, morbo,
Shuah Khan, bpf, Networking, open list:KERNEL SELFTEST FRAMEWORK,
open list
On Sun, Jan 17, 2021 at 2:20 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> btf_dump.c will need it for type-based data display.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
Given we make it into an internal API, let's call it
btf_skip_mods_and_typedefs()? Otherwise all ok.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> tools/lib/bpf/libbpf.c | 4 +---
> tools/lib/bpf/libbpf_internal.h | 2 ++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2abbc38..4ef84e1 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -73,8 +73,6 @@
> #define __printf(a, b) __attribute__((format(printf, a, b)))
>
> static struct bpf_map *bpf_object__add_map(struct bpf_object *obj);
> -static const struct btf_type *
> -skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id);
>
> static int __base_pr(enum libbpf_print_level level, const char *format,
> va_list args)
> @@ -1885,7 +1883,7 @@ static int bpf_object__init_user_maps(struct bpf_object *obj, bool strict)
> return 0;
> }
>
> -static const struct btf_type *
> +const struct btf_type *
> skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id)
> {
> const struct btf_type *t = btf__type_by_id(btf, id);
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 969d0ac..c25d2df 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -108,6 +108,8 @@ static inline void *libbpf_reallocarray(void *ptr, size_t nmemb, size_t size)
> void *btf_add_mem(void **data, size_t *cap_cnt, size_t elem_sz,
> size_t cur_cnt, size_t max_cnt, size_t add_cnt);
> int btf_ensure_mem(void **data, size_t *cap_cnt, size_t elem_sz, size_t need_cnt);
> +const struct btf_type *skip_mods_and_typedefs(const struct btf *btf, __u32 id,
> + __u32 *res_id);
>
> static inline bool libbpf_validate_opts(const char *opts,
> size_t opts_sz, size_t user_sz,
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data
2021-01-17 22:16 ` [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data Alan Maguire
@ 2021-01-21 6:56 ` Andrii Nakryiko
2021-01-21 19:51 ` Andrii Nakryiko
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 6:56 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, morbo,
Shuah Khan, bpf, Networking, open list:KERNEL SELFTEST FRAMEWORK,
open list
On Sun, Jan 17, 2021 at 2:22 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Add a BTF dumper for typed data, so that the user can dump a typed
> version of the data provided.
>
> The API is
>
> int btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
> const struct btf_dump_emit_type_data_opts *opts,
> void *data);
>
> ...where the id is the BTF id of the data pointed to by the "void *"
> argument; for example the BTF id of "struct sk_buff" for a
> "struct skb *" data pointer. Options supported are
>
> - a starting indent level (indent_lvl)
> - a set of boolean options to control dump display, similar to those
> used for BPF helper bpf_snprintf_btf(). Options are
> - compact : omit newlines and other indentation
> - noname: omit member names
> - zero: show zero-value members
>
> Default output format is identical to that dumped by bpf_snprintf_btf(),
> for example a "struct sk_buff" representation would look like this:
>
> struct sk_buff){
> (union){
> (struct){
Curious, these explicit anonymous (union) and (struct), is that
preferred way for explicitness, or is it just because it makes
implementation simpler and thus was chosen? I.e., if the goal was to
mimic C-style data initialization, you'd just have plain .next = ...,
.prev = ..., .dev = ..., .dev_scratch = ..., all on the same level. So
just checking for myself.
> .next = (struct sk_buff *)0xffffffffffffffff,
> .prev = (struct sk_buff *)0xffffffffffffffff,
> (union){
> .dev = (struct net_device *)0xffffffffffffffff,
> .dev_scratch = (long unsigned int)18446744073709551615,
> },
> },
> ...
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
So just keep in mind that I went through this from bottom to the top,
some comments would read more naturally that way, probably :)
> tools/lib/bpf/btf.h | 17 +
> tools/lib/bpf/btf_dump.c | 974 +++++++++++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.map | 5 +
> 3 files changed, 996 insertions(+)
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 0c48f2e..7937124 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -180,6 +180,23 @@ struct btf_dump_emit_type_decl_opts {
> btf_dump__emit_type_decl(struct btf_dump *d, __u32 id,
> const struct btf_dump_emit_type_decl_opts *opts);
>
> +
> +struct btf_dump_emit_type_data_opts {
> + /* size of this struct, for forward/backward compatibility */
> + size_t sz;
> + int indent_level;
probably would be good to mention that it's in number of spaces and
that amount of spaces will be added to each output line (or each
except the first, I haven't payed attention in the code, sorry).
> + /* below match "show" flags for bpf_show_snprintf() */
> + bool compact;
> + bool noname;
> + bool zero;
zero -> emit_zeroes?
noname -> skip_names? is it field names only? then maybe skip_field_names?
> +};
> +#define btf_dump_emit_type_data_opts__last_field zero
> +
> +LIBBPF_API int
> +btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
> + const struct btf_dump_emit_type_data_opts *opts,
> + void *data);
> +
> /*
> * A set of helpers for easier BTF types handling
> */
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 2f9d685..04d604f 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -10,6 +10,8 @@
> #include <stddef.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <ctype.h>
> +#include <endian.h>
> #include <errno.h>
> #include <linux/err.h>
> #include <linux/btf.h>
> @@ -19,14 +21,31 @@
> #include "libbpf.h"
> #include "libbpf_internal.h"
>
> +#define BITS_PER_BYTE 8
> +#define BITS_PER_U128 (sizeof(__u64) * BITS_PER_BYTE * 2)
I'm sorry, but why?.. is it ever going to change? isn't 64 or 8 or 128
completely obvious within the context where it's used?
> +#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
> +#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
> +#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
aka, bits / 8
> +#define BITS_ROUNDUP_BYTES(bits) \
> + (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
aka, roundup(bits, 8)?
> +
> static const char PREFIXES[] = "\t\t\t\t\t\t\t\t\t\t\t\t\t";
> static const size_t PREFIX_CNT = sizeof(PREFIXES) - 1;
>
> +
> static const char *pfx(int lvl)
> {
> return lvl >= PREFIX_CNT ? PREFIXES : &PREFIXES[PREFIX_CNT - lvl];
> }
>
> +static const char SPREFIXES[] = " ";
> +static const size_t SPREFIX_CNT = sizeof(SPREFIXES) - 1;
> +
> +static const char *spfx(int lvl)
> +{
> + return lvl >= SPREFIX_CNT ? SPREFIXES : &SPREFIXES[SPREFIX_CNT - lvl];
> +}
would using tabs for indentation be too horrible?
> +
> enum btf_dump_type_order_state {
> NOT_ORDERED,
> ORDERING,
> @@ -53,6 +72,49 @@ struct btf_dump_type_aux_state {
> __u8 referenced: 1;
> };
>
> +#define BTF_DUMP_DATA_MAX_NAME_LEN 256
> +
> +/*
> + * Common internal data for BTF type data dump operations.
> + *
> + * The implementation here is similar to that in kernel/bpf/btf.c
> + * that supports the bpf_snprintf_btf() helper, so any bugs in
> + * type data dumping here are likely in that code also.
> + *
> + * One challenge with showing nested data is we want to skip 0-valued
> + * data, but in order to figure out whether a nested object is all zeros
> + * we need to walk through it. As a result, we need to make two passes
> + * when handling structs, unions and arrays; the first path simply looks
> + * for nonzero data, while the second actually does the display. The first
> + * pass is signalled by state.depth_check being set, and if we
> + * encounter a non-zero value we set state.depth_to_show to the depth
> + * at which we encountered it. When we have completed the first pass,
> + * we will know if anything needs to be displayed if
> + * state.depth_to_show > state.depth. See btf_dump_emit_[struct,array]_data()
> + * for the implementation of this.
So this approach is quite convoluted, IMO. You are trying to reuse
printing "infrastructure" to determine if anything is going to be
printed. It works, I suppose, but make code more convoluted, has this
additional depth_to_show-related state, etc... Let's think about
alternative ways to check whether data has some non-zero members.
Naively, you could just say that you need to check if entire memory
region taken up by the type has a single non-zero byte. You didn't do
it probably due to non-zero padding bytes, which are sort of not
important and we want to ignore them, is that right?
So if we do want to ignore non-zero padding bytes, then I think it's
still much better to write a much simpler recursive traversal function
that will go over all members of array/struct/datasec and check if
they are zero (using byte comparison, nothing fancy). Even bitfields
are not hard to handle in such case.
Surely, you'll implement traversal once for zero checking and once for
actual printing, but traversal is trivial for the former and will be
simpler than what you have currently for the latter. Instead all the
checks with depth and so on, you will effectively have:
if (btf_dump_is_all_zero(...))
return;
btf_dump_data_for_real(...)
> + *
> + */
> +struct btf_dump_data {
> + bool compact;
> + bool noname;
> + bool zero;
> + __u8 indent_lvl; /* base indent level */
> + /* below are used during iteration */
> + struct {
> + __u8 depth;
> + __u8 depth_to_show;
> + __u8 depth_check;
> + __u8 array_member:1,
> + array_terminated:1;
> + __u16 array_encoding;
> + __u32 type_id;
> + const struct btf_type *type;
> + const struct btf_member *member;
> + char name[BTF_DUMP_DATA_MAX_NAME_LEN];
unlike kernel, here in user-space we have unresticted recursive
functions, it's quite hard to justify this state machine approach with
set_member/unset_member and so on. You have a function that emits
member, so pass that member as an input argument. Please see if you
can simplify all that further. I believe after some iteration on
implementation we'll end up with less and simpler code overall.
> + int err;
> + } state;
> +};
> +
> struct btf_dump {
> const struct btf *btf;
> const struct btf_ext *btf_ext;
> @@ -89,6 +151,10 @@ struct btf_dump {
> * name occurrences
> */
> struct hashmap *ident_names;
> + /*
> + * data for typed display.
> + */
> + struct btf_dump_data data;
> };
>
> static size_t str_hash_fn(const void *key, void *ctx)
> @@ -1438,3 +1504,911 @@ static const char *btf_dump_ident_name(struct btf_dump *d, __u32 id)
> {
> return btf_dump_resolve_name(d, id, d->ident_names);
> }
> +
> +static void __btf_dump_emit_type_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset);
> +
> +static const struct btf_type *skip_mods(const struct btf *btf,
> + __u32 id, __u32 *res_id)
> +{
> + const struct btf_type *t = btf__type_by_id(btf, id);
> +
> + while (btf_is_mod(t)) {
> + id = t->type;
> + t = btf__type_by_id(btf, t->type);
> + }
> +
> + if (res_id)
> + *res_id = id;
> +
> + return t;
> +}
> +
> +#define BTF_MAX_ITER 10
> +#define BTF_KIND_BIT(kind) (1ULL << kind)
> +
> +/*
> + * Populate dump->data.state.name with type name information.
> + * Format of type name is
> + *
> + * [.member_name = ] (type_name)
> + */
> +static const char *btf_dump_data_name(struct btf_dump *d)
> +{
this whole function almost entirely can be handled by
btf_dump__emit_type_decl() to emit (struct whatever) part. It has no
artificial limits to nestedness (so void
****************************** doesn't scare it), and it doesn't need
artificially limited intermediate buffer. Please try to utilize it
here and cut on unnecessary code we'll need to maintain here. Note the
skip_mods option, which you probably want to use here.
> + /* BTF_MAX_ITER array suffixes "[]" */
> + const char *array_suffixes = "[][][][][][][][][][]";
> + const char *array_suffix = &array_suffixes[strlen(array_suffixes)];
> + /* BTF_MAX_ITER pointer suffixes "*" */
> + const char *ptr_suffixes = "**********";
> + const char *ptr_suffix = &ptr_suffixes[strlen(ptr_suffixes)];
> + const char *name = NULL, *prefix = "", *parens = "";
> + const struct btf_member *m = d->data.state.member;
> + const struct btf_type *t = d->data.state.type;
> + const struct btf_array *array;
> + __u32 id = d->data.state.type_id;
> + const char *member = NULL;
> + bool show_member = false;
> + __u64 kinds = 0;
> + int i;
> +
> + d->data.state.name[0] = '\0';
> +
> + /*
> + * Don't show type name if we're showing an array member;
> + * in that case we show the array type so don't need to repeat
> + * ourselves for each member.
> + */
> + if (d->data.state.array_member)
> + return "";
> +
> + /* Retrieve member name, if any. */
> + if (m) {
> + member = btf_name_of(d, m->name_off);
> + show_member = strlen(member) > 0;
> + id = m->type;
> + }
> +
> + /*
> + * Start with type_id, as we have resolved the struct btf_type *
> + * via btf_dump_emit_modifier_data() past the parent typedef to the
> + * child struct, int etc it is defined as. In such cases, the type_id
> + * still represents the starting type while the struct btf_type *
> + * in our d->data.state points at the resolved type of the typedef.
> + */
> + t = btf__type_by_id(d->btf, id);
> + if (!t)
> + return "";
> +
> + /*
> + * The goal here is to build up the right number of pointer and
> + * array suffixes while ensuring the type name for a typedef
> + * is represented. Along the way we accumulate a list of
> + * BTF kinds we have encountered, since these will inform later
> + * display; for example, pointer types will not require an
> + * opening "{" for struct, we will just display the pointer value.
> + *
> + * We also want to accumulate the right number of pointer or array
> + * indices in the format string while iterating until we get to
> + * the typedef/pointee/array member target type.
> + *
> + * We start by pointing at the end of pointer and array suffix
> + * strings; as we accumulate pointers and arrays we move the pointer
> + * or array string backwards so it will show the expected number of
> + * '*' or '[]' for the type. BTF_SHOW_MAX_ITER of nesting of pointers
> + * and/or arrays and typedefs are supported as a precaution.
> + *
> + * We also want to get typedef name while proceeding to resolve
> + * type it points to so that we can add parentheses if it is a
> + * "typedef struct" etc.
> + *
> + * Qualifiers ("const", "volatile", "restrict") are simply skipped
> + * as they complicate simple type name display without adding much
> + * in the case of displaying a cast in front of the data to be
> + * displayed.
> + */
> + for (i = 0; i < BTF_MAX_ITER; i++) {
> +
> + switch (BTF_INFO_KIND(t->info)) {
> + case BTF_KIND_TYPEDEF:
> + if (!name)
> + name = btf_name_of(d, t->name_off);
> + kinds |= BTF_KIND_BIT(BTF_KIND_TYPEDEF);
> + id = t->type;
> + break;
> + case BTF_KIND_ARRAY:
> + kinds |= BTF_KIND_BIT(BTF_KIND_ARRAY);
> + parens = "[";
> + if (!t)
> + return "";
> + array = btf_array(t);
> + if (array_suffix > array_suffixes)
> + array_suffix -= 2;
> + id = array->type;
> + break;
> + case BTF_KIND_PTR:
> + kinds |= BTF_KIND_BIT(BTF_KIND_PTR);
> + if (ptr_suffix > ptr_suffixes)
> + ptr_suffix -= 1;
> + id = t->type;
> + break;
> + default:
> + id = 0;
> + break;
> + }
> + if (!id)
> + break;
> + t = skip_mods(d->btf, id, NULL);
> + }
> + /* We may not be able to represent this type; bail to be safe */
> + if (i == BTF_MAX_ITER) {
> + pr_warn("iters %d exceeded %d when displaying type name:[%u]\n",
> + i, BTF_MAX_ITER, id);
> + return "";
> + }
> +
> + if (!name)
> + name = btf_name_of(d, t->name_off);
> +
> + switch (BTF_INFO_KIND(t->info)) {
> + case BTF_KIND_STRUCT:
> + case BTF_KIND_UNION:
> + prefix = BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT ?
> + "struct" : "union";
> + /* if it's an array of struct/union, parens is already set */
> + if (!(kinds & (BTF_KIND_BIT(BTF_KIND_ARRAY))))
> + parens = "{";
> + break;
> + case BTF_KIND_ENUM:
> + prefix = "enum";
> + break;
> + default:
> + break;
> + }
> +
> + /* pointer does not require parens */
> + if (kinds & BTF_KIND_BIT(BTF_KIND_PTR))
> + parens = "";
> + /* typedef does not require struct/union/enum prefix */
> + if (kinds & BTF_KIND_BIT(BTF_KIND_TYPEDEF))
> + prefix = "";
> +
> + if (!name)
> + name = "";
> +
> + /* Even if we don't want type name info, we want parentheses etc */
> + if (d->data.noname)
> + snprintf(d->data.state.name, sizeof(d->data.state.name), "%s",
> + parens);
> + else
> + snprintf(d->data.state.name, sizeof(d->data.state.name),
> + "%s%s%s(%s%s%s%s%s%s)%s",
> + /* first 3 strings comprise ".member = " */
> + show_member ? "." : "",
> + show_member ? member : "",
> + show_member ? " = " : "",
> + /* ...next is our prefix (struct, enum, etc) */
> + prefix,
> + strlen(prefix) > 0 && strlen(name) > 0 ? " " : "",
> + /* ...this is the type name itself */
> + name,
> + /* ...suffixed by the appropriate '*', '[]' suffixes */
> + strlen(name) > 0 && strlen(ptr_suffix) > 0 ? " " : "",
> + ptr_suffix,
> + array_suffix, parens);
> +
> + return d->data.state.name;
> +}
> +
> +static const char *btf_dump_data_indent(struct btf_dump *d)
> +{
> + if (d->data.compact)
> + return "";
> + return spfx(d->data.indent_lvl + d->data.state.depth);
> +}
> +
> +static const char *btf_dump_data_newline(struct btf_dump *d)
> +{
> + return d->data.compact ? "" : "\n";
> +}
> +
> +static const char *btf_dump_data_delim(struct btf_dump *d)
> +{
> + if (d->data.state.depth == 0)
> + return "";
> +
> + if (d->data.compact &&
> + d->data.state.type &&
> + BTF_INFO_KIND(d->data.state.type->info) == BTF_KIND_UNION)
> + return "|";
this is unexpected distinction... why such decision was made?
> +
> + return ",";
> +}
> +
> +static void btf_dump_data_printf(struct btf_dump *d,
> + const char *fmt, ...)
> +{
> + va_list args;
> +
> + /*
> + * Just checking if there is non-zero data to display at this depth,
> + * so nothing is displayed.
> + */
> + if (d->data.state.depth_check)
> + return;
really-really don't like this approach, see above for more thoughts
> + va_start(args, fmt);
> + d->printf_fn(d->opts.ctx, fmt, args);
> + va_end(args);
> +}
> +
> +/* Macros are used here as btf_type_value[s]() prepends and appends
> + * format specifiers to the format specifier passed in; these do the work of
> + * adding indentation, delimiters etc while the caller simply has to specify
> + * the type value(s) in the format specifier + value(s).
> + */
> +#define btf_dump_emit_type_value(d, fmt, value) \
> + do { \
> + if ((value) != 0 || d->data.zero || \
> + d->data.state.depth == 0) { \
> + btf_dump_data_printf(d, "%s%s" fmt "%s%s", \
> + btf_dump_data_indent(d), \
> + btf_dump_data_name(d), \
> + value, \
> + btf_dump_data_delim(d), \
> + btf_dump_data_newline(d)); \
> + if (d->data.state.depth > \
> + d->data.state.depth_to_show) \
> + d->data.state.depth_to_show = \
> + d->data.state.depth; \
> + } \
> + } while (0)
> +
> +#define btf_dump_emit_type_values(d, fmt, ...) \
> + do { \
> + btf_dump_data_printf(d, "%s%s" fmt "%s%s", \
> + btf_dump_data_indent(d), \
> + btf_dump_data_name(d), \
> + __VA_ARGS__, \
> + btf_dump_data_delim(d), \
> + btf_dump_data_newline(d)); \
> + if (d->data.state.depth > \
> + d->data.state.depth_to_show) \
> + d->data.state.depth_to_show = \
> + d->data.state.depth; \
> + } while (0)
> +
these macro can be easily written as functions by calling
btf_dump_data_printf three times, why macro?..
> +/* Set the type we are starting to show. */
> +static void btf_dump_start_type(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 type_id)
> +{
> + d->data.state.type = t;
> + d->data.state.type_id = type_id;
> + d->data.state.name[0] = '\0';
> +}
> +
> +static void btf_dump_end_type(struct btf_dump *d)
> +{
> + d->data.state.type = NULL;
> + d->data.state.type_id = 0;
> + d->data.state.name[0] = '\0';
> +}
> +
> +static void btf_dump_start_aggr_type(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 type_id)
> +{
> + btf_dump_start_type(d, t, type_id);
> +
> + btf_dump_data_printf(d, "%s%s%s",
> + btf_dump_data_indent(d),
> + btf_dump_data_name(d),
> + btf_dump_data_newline(d));
> + d->data.state.depth++;
> +}
> +
> +static void btf_dump_end_aggr_type(struct btf_dump *d,
> + const char *suffix)
> +{
> + d->data.state.depth--;
> + btf_dump_data_printf(d, "%s%s%s%s",
> + btf_dump_data_indent(d),
> + suffix,
> + btf_dump_data_delim(d),
> + btf_dump_data_newline(d));
> + btf_dump_end_type(d);
> +}
> +
> +static void btf_dump_start_member(struct btf_dump *d,
> + const struct btf_member *m)
> +{
> + d->data.state.member = m;
> +}
> +
> +static void btf_dump_start_array_member(struct btf_dump *d)
> +{
> + d->data.state.array_member = 1;
> + btf_dump_start_member(d, NULL);
> +}
> +
> +static void btf_dump_end_member(struct btf_dump *d)
> +{
> + d->data.state.member = NULL;
> +}
> +
> +static void btf_dump_end_array_member(struct btf_dump *d)
> +{
> + d->data.state.array_member = 0;
> + btf_dump_end_member(d);
> +}
> +
> +static void btf_dump_start_array_type(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 type_id,
> + __u16 array_encoding)
> +{
> + d->data.state.array_encoding = array_encoding;
> + d->data.state.array_terminated = 0;
> + btf_dump_start_aggr_type(d, t, type_id);
> +}
> +
> +static void btf_dump_end_array_type(struct btf_dump *d)
> +{
> + d->data.state.array_encoding = 0;
> + d->data.state.array_terminated = 0;
> + btf_dump_end_aggr_type(d, "]");
> +}
> +
> +static void btf_dump_start_struct_type(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 type_id)
> +{
> + btf_dump_start_aggr_type(d, t, type_id);
> +}
> +
> +static void btf_dump_end_struct_type(struct btf_dump *d)
> +{
> + btf_dump_end_aggr_type(d, "}");
> +}
> +
> +static void btf_dump_emit_df_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + btf_dump_data_printf(d, "<unsupported kind:%u>",
> + BTF_INFO_KIND(t->info));
> +}
> +
> +static void btf_dump_emit_int128(struct btf_dump *d, void *data)
> +{
> + /* data points to a __int128 number.
> + * Suppose
> + * int128_num = *(__int128 *)data;
> + * The below formulas shows what upper_num and lower_num represents:
> + * upper_num = int128_num >> 64;
> + * lower_num = int128_num & 0xffffffffFFFFFFFFULL;
> + */
> + __u64 upper_num, lower_num;
> +
> +#ifdef __BIG_ENDIAN_BITFIELD
> + upper_num = *(__u64 *)data;
> + lower_num = *(__u64 *)(data + 8);
> +#else
> + upper_num = *(__u64 *)(data + 8);
> + lower_num = *(__u64 *)data;
> +#endif
> + if (upper_num == 0)
> + btf_dump_emit_type_value(d, "0x%llx", lower_num);
> + else
> + btf_dump_emit_type_values(d, "0x%llx%016llx", upper_num,
> + lower_num);
see below, %llx or %lx, depending on the platform. So what libbpf is
doing in situations like this is explicitly casting to undecorated
"long long".
> +}
> +
> +static void btf_int128_shift(__u64 *print_num, __u16 left_shift_bits,
> + __u16 right_shift_bits)
> +{
> + __u64 upper_num, lower_num;
> +
> +#ifdef __BIG_ENDIAN_BITFIELD
> + upper_num = print_num[0];
> + lower_num = print_num[1];
> +#else
> + upper_num = print_num[1];
> + lower_num = print_num[0];
> +#endif
> +
> + /* shake out un-needed bits by shift/or operations */
> + if (left_shift_bits >= 64) {
> + upper_num = lower_num << (left_shift_bits - 64);
surprised we don't have BITS_PER_U64 for this ;)
> + lower_num = 0;
> + } else {
> + upper_num = (upper_num << left_shift_bits) |
> + (lower_num >> (64 - left_shift_bits));
> + lower_num = lower_num << left_shift_bits;
> + }
> +
> + if (right_shift_bits >= 64) {
> + lower_num = upper_num >> (right_shift_bits - 64);
> + upper_num = 0;
> + } else {
> + lower_num = (lower_num >> right_shift_bits) |
> + (upper_num << (64 - right_shift_bits));
> + upper_num = upper_num >> right_shift_bits;
> + }
> +
> +#ifdef __BIG_ENDIAN_BITFIELD
> + print_num[0] = upper_num;
> + print_num[1] = lower_num;
> +#else
> + print_num[0] = lower_num;
> + print_num[1] = upper_num;
> +#endif
> +}
> +
> +static void btf_dump_emit_bitfield_data(struct btf_dump *d,
> + void *data,
> + __u8 bits_offset,
> + __u8 nr_bits)
> +{
> + __u16 left_shift_bits, right_shift_bits;
> + __u8 nr_copy_bytes;
> + __u8 nr_copy_bits;
> + __u64 print_num[2] = {};
> +
> + nr_copy_bits = nr_bits + bits_offset;
> + nr_copy_bytes = BITS_ROUNDUP_BYTES(nr_copy_bits);
> +
> + memcpy(print_num, data, nr_copy_bytes);
> +
> +#ifdef __BIG_ENDIAN_BITFIELD
> + left_shift_bits = bits_offset;
> +#else
> + left_shift_bits = BITS_PER_U128 - nr_copy_bits;
> +#endif
> + right_shift_bits = BITS_PER_U128 - nr_bits;
> +
> + btf_int128_shift(print_num, left_shift_bits, right_shift_bits);
I suspect the whole shifting of 128-bit numbers can be less elaborate,
but I'll postpone digging into details for now. I'm also wondering how
realistic it is to have 128-bit bitfields...
> + btf_dump_emit_int128(d, print_num);
> +}
> +
> +static void btf_dump_emit_int_bits(struct btf_dump *d,
> + const struct btf_type *t,
> + void *data,
> + __u8 bits_offset)
> +{
> + __u32 int_data = btf_int(t);
> + __u8 nr_bits = BTF_INT_BITS(int_data);
> + __u8 total_bits_offset;
> +
> + /*
> + * bits_offset is at most 7.
> + * BTF_INT_OFFSET() cannot exceed 128 bits.
> + */
> + total_bits_offset = bits_offset + BTF_INT_OFFSET(int_data);
> + data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
> + bits_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
> + btf_dump_emit_bitfield_data(d, data, bits_offset, nr_bits);
> +}
> +
> +static void btf_dump_emit_int_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 type_id,
> + void *data,
> + __u8 bits_offset)
> +{
> + __u32 int_data = btf_int(t);
> + __u8 encoding = BTF_INT_ENCODING(int_data);
> + bool sign = encoding & BTF_INT_SIGNED;
> + __u8 nr_bits = BTF_INT_BITS(int_data);
> +
> + btf_dump_start_type(d, t, type_id);
> +
> + if (bits_offset || BTF_INT_OFFSET(int_data) ||
> + BITS_PER_BYTE_MASKED(nr_bits)) {
> + btf_dump_emit_int_bits(d, t, data, bits_offset);
> + goto out;
> + }
> +
> + switch (nr_bits) {
> + case 128:
> + btf_dump_emit_int128(d, data);
> + break;
> + case 64:
> + if (sign)
> + btf_dump_emit_type_value(d, "%lld", *(__s64 *)data);
> + else
> + btf_dump_emit_type_value(d, "%llu", *(__u64 *)data);
if I remember correctly, the only bulletproof way to not get warning
about %lld vs %ld on any platform is to cast to (long long), not any
of the typedefs (int64_t, __s64, etc).
> + break;
> + case 32:
> + if (sign)
> + btf_dump_emit_type_value(d, "%d", *(__s32 *)data);
> + else
> + btf_dump_emit_type_value(d, "%u", *(__u32 *)data);
> + break;
> + case 16:
> + if (sign)
> + btf_dump_emit_type_value(d, "%d", *(__s16 *)data);
> + else
> + btf_dump_emit_type_value(d, "%u", *(__u16 *)data);
> + break;
> + case 8:
> + if (d->data.state.array_encoding == BTF_INT_CHAR) {
> + /* check for null terminator */
> + if (d->data.state.array_terminated)
> + break;
> + if (*(char *)data == '\0') {
> + d->data.state.array_terminated = 1;
> + break;
> + }
> + if (isprint(*(char *)data)) {
> + btf_dump_emit_type_value(d, "'%c'",
> + *(char *)data);
> + break;
> + }
> + }
> + if (sign)
> + btf_dump_emit_type_value(d, "%d", *(__s8 *)data);
> + else
> + btf_dump_emit_type_value(d, "%u", *(__u8 *)data);
Nitpicking here. Isn't it more natural to emit char array that
actually represents printable characters as "abracadabra", not { 'a',
'b', 'r', 'a', 'c', 'a', ... }?
> + break;
> + default:
> + btf_dump_emit_int_bits(d, t, data, bits_offset);
> + break;
> + }
> +out:
> + btf_dump_end_type(d);
> +}
> +
> +static void btf_dump_emit_modifier_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + t = skip_mods_and_typedefs(d->btf, id, NULL);
> + __btf_dump_emit_type_data(d, t, id, data, bits_offset);
> +}
> +
> +static void btf_dump_emit_var_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + __u32 linkage = btf_var(t)->linkage;
linkage is an enum btf_func_linkage, please use proper enumerator constants
> +
> + btf_dump_data_printf(d, "%s%s =",
> + linkage ? "" : "static ",
> + btf_name_of(d, t->name_off));
> + t = btf__type_by_id(d->btf, t->type);
> + __btf_dump_emit_type_data(d, t, t->type, data, bits_offset);
> +}
> +
> +static void __btf_dump_emit_array_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + const struct btf_array *array = btf_array(t);
> + const struct btf_type *elem_type;
> + __u32 i, elem_size = 0, elem_type_id;
> + __u16 encoding = 0;
> +
> + elem_type_id = array->type;
> + elem_type = skip_mods_and_typedefs(d->btf, elem_type_id, NULL);
> + if (elem_type && btf_has_size(elem_type))
if (!elem_type) means corrupted BTF, bail out, don't ignore such problems?..
But the bigger problem is btf_has_size() use. What if you have an
array of array (can you add a test for that, btw)? Inner array does
have size, just not immediately available. That's what I meant when I
said btf_has_size() is misleading and you do mis-use it, it seems.
> + elem_size = elem_type->size;
> +
> + if (elem_type && btf_is_int(elem_type)) {
> + __u32 int_type = btf_int(elem_type);
> +
> + encoding = BTF_INT_ENCODING(int_type);
encoding = btf_int_encoding(elem_type);
> +
> + /*
> + * BTF_INT_CHAR encoding never seems to be set for
> + * char arrays, so if size is 1 and element is
> + * printable as a char, we'll do that.
> + */
> + if (elem_size == 1)
> + encoding = BTF_INT_CHAR;
> + }
> +
> + btf_dump_start_array_type(d, t, id, encoding);
> +
> + if (!elem_type)
> + goto out;
> +
> + for (i = 0; i < array->nelems; i++) {
> +
> + btf_dump_start_array_member(d);
> +
> + __btf_dump_emit_type_data(d, elem_type, elem_type_id,
> + data, bits_offset);
> + data += elem_size;
> +
> + btf_dump_end_array_member(d);
> +
> + if (d->data.state.array_terminated)
> + break;
> + }
> +out:
> + btf_dump_end_array_type(d);
> +}
> +
> +static void btf_dump_emit_array_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + const struct btf_member *m = d->data.state.member;
> +
> + /*
> + * First check if any members would be shown (are non-zero).
> + * See comments above "struct btf_dump_data" definition for more
> + * details on how this works at a high-level.
> + */
> + if (d->data.state.depth > 0 && !d->data.zero) {
> + if (!d->data.state.depth_check) {
> + d->data.state.depth_check = d->data.state.depth + 1;
> + d->data.state.depth_to_show = 0;
> + }
> + __btf_dump_emit_array_data(d, t, id, data, bits_offset);
> + d->data.state.member = m;
> +
> + if (d->data.state.depth_check != d->data.state.depth + 1)
> + return;
> + d->data.state.depth_check = 0;
> +
> + if (d->data.state.depth_to_show <= d->data.state.depth)
> + return;
> + /*
> + * Reaching here indicates we have recursed and found
> + * non-zero array member(s).
> + */
> + }
> + __btf_dump_emit_array_data(d, t, id, data, bits_offset);
here and in many other places, if at this point bits_offset is not
zero, we are in big trouble. Why passing this through everywhere? It
has to be zero for everything but terminal bitfield.
> +}
> +
> +#define for_each_member(i, struct_type, member) \
> + for (i = 0, member = btf_members(struct_type); \
> + i < btf_vlen(struct_type); \
> + i++, member++)
same as below about unnecessary macro, more code, harder to follow, no benefit
> +
> +static void __btf_dump_emit_struct_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + const struct btf_member *member;
> + __u32 i;
> +
> + btf_dump_start_struct_type(d, t, id);
> +
> + for_each_member(i, t, member) {
> + const struct btf_type *member_type;
> + __u32 member_offset, bitfield_size;
> + __u32 bytes_offset;
> + __u8 bits8_offset;
> +
> + member_type = btf__type_by_id(d->btf, member->type);
> + btf_dump_start_member(d, member);
> +
> + member_offset = btf_member_bit_offset(t, i);
> + bitfield_size = btf_member_bitfield_size(t, i);
> + bytes_offset = BITS_ROUNDDOWN_BYTES(member_offset);
> + bits8_offset = BITS_PER_BYTE_MASKED(member_offset);
> + if (bitfield_size) {
> + btf_dump_start_type(d, member_type, member->type);
> + btf_dump_emit_bitfield_data(d,
> + data + bytes_offset,
> + bits8_offset,
> + bitfield_size);
> + btf_dump_end_type(d);
> + } else {
> + __btf_dump_emit_type_data(d, member_type, member->type,
> + data + bytes_offset, bits8_offset);
> + }
> + btf_dump_end_member(d);
> + }
> + btf_dump_end_struct_type(d);
> +}
> +
> +static void btf_dump_emit_struct_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + const struct btf_member *m = d->data.state.member;
> +
> + /*
> + * First check if any members would be shown (are non-zero).
> + * See comments above "struct btf_dump_data" definition for more
> + * details on how this works at a high-level.
> + */
> + if (d->data.state.depth > 0 && !d->data.zero) {
> + if (!d->data.state.depth_check) {
> + d->data.state.depth_check = d->data.state.depth + 1;
> + d->data.state.depth_to_show = 0;
> + }
> + __btf_dump_emit_struct_data(d, t, id, data, bits_offset);
> + /* Restore saved member data here */
> + d->data.state.member = m;
> + if (d->data.state.depth_check != d->data.state.depth + 1)
> + return;
> + d->data.state.depth_check = 0;
> +
> + if (d->data.state.depth_to_show <= d->data.state.depth)
> + return;
> + /*
> + * Reaching here indicates we have recursed and found
> + * non-zero child values.
> + */
> + }
> +
> + __btf_dump_emit_struct_data(d, t, id, data, bits_offset);
> +}
> +
> +static void btf_dump_emit_ptr_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + btf_dump_start_type(d, t, id);
> +
> + btf_dump_emit_type_value(d, "%p", *(void **)data);
so, technically, BTF could have different sizeof(void *) from the host
platform. See btf__pointer_size(). Not entirely sure we should care,
but just keep this in mind.
> + btf_dump_end_type(d);
> +}
> +
> +static void btf_dump_emit_enum_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + const struct btf_enum *enums = btf_enum(t);
> + __s64 value;
> + __u16 i;
stick to full sized int, no need to use __u16
also probably better to cache vlen here
> +
> + btf_dump_start_type(d, t, id);
> +
> + switch (t->size) {
> + case 8:
> + value = *(__s64 *)data;
> + break;
> + case 4:
> + value = *(__s32 *)data;
> + break;
> + case 2:
> + value = *(__s16 *)data;
> + break;
> + case 1:
> + value = *(__s8 *)data;
> + break;
> + default:
> + pr_warn("unexpected size %d for enum, id:[%u]\n", t->size,
> + id);
> + d->data.state.err = -EINVAL;
> + return;
> + }
> +
> + for (i = 0; i < btf_vlen(t); i++) {
for consistency with other iteration loops, enums++ (or rather e++) here?
> + if (value == enums[i].val) {
if you invert condition here and continue on !=, then you will reduce
nestedness below and won't need to wrap btf_name_of() invocation
> + btf_dump_emit_type_value(d, "%s",
> + btf_name_of(d,
> + enums[i].name_off));
> + btf_dump_end_type(d);
> + return;
> + }
> + }
> +
> + btf_dump_emit_type_value(d, "%d", value);
> + btf_dump_end_type(d);
> +}
> +
> +#define for_each_vsi(i, struct_type, member) \
> + for (i = 0, member = btf_var_secinfos(struct_type); \
> + i < btf_vlen(struct_type); \
> + i++, member++)
typo: struct_type -> datasec?
but I'd say just drop this macro and write a plain for loop. You are
using more lines of code and harm readability:
const struct btf_var_secinfo *vsi = btf_var_secinfos(t);
__u32 i, vlen = btf_vlen(t);
for (i = 0; i < vlen; i++, vsi++) {
...
}
4 lines of code less and I don't have to jump around to match i, t and
vsi inside the for_each_vsi() macro.
And it's good idea to "cache" vlen, makes for loop less wide (and
probably has some miniscule perf benefits, who knows).
> +
> +static void btf_dump_emit_datasec_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + const struct btf_var_secinfo *vsi;
> + const struct btf_type *var;
> + __u32 i;
> +
> + btf_dump_start_type(d, t, id);
> +
> + btf_dump_emit_type_value(d, "section (\"%s\") = {",
> + btf_name_of(d, t->name_off));
> + for_each_vsi(i, t, vsi) {
> + var = btf__type_by_id(d->btf, vsi->type);
> + if (i)
> + btf_dump_data_printf(d, ",");
> + __btf_dump_emit_type_data(d, var, vsi->type,
> + data + vsi->offset,
> + bits_offset);
> + }
> + btf_dump_end_type(d);
> +}
> +
> +typedef void (*btf_dump_emit_kind_data)(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset);
> +
> +static btf_dump_emit_kind_data dump_emit_kind_data[NR_BTF_KINDS] = {
> + &btf_dump_emit_df_data,
> + &btf_dump_emit_int_data,
> + &btf_dump_emit_ptr_data,
> + &btf_dump_emit_array_data,
> + &btf_dump_emit_struct_data,
> + &btf_dump_emit_struct_data,
> + &btf_dump_emit_enum_data,
> + &btf_dump_emit_df_data,
> + &btf_dump_emit_modifier_data,
> + &btf_dump_emit_modifier_data,
> + &btf_dump_emit_modifier_data,
> + &btf_dump_emit_modifier_data,
> + &btf_dump_emit_df_data,
> + &btf_dump_emit_df_data,
> + &btf_dump_emit_var_data,
> + &btf_dump_emit_datasec_data,
You don't seem to have any tests for DATASEC and VAR... so we don't
really know if everything really works for them
> +};
> +
> +static void __btf_dump_emit_type_data(struct btf_dump *d,
> + const struct btf_type *t,
> + __u32 id,
> + void *data,
> + __u8 bits_offset)
> +{
> + dump_emit_kind_data[BTF_INFO_KIND(t->info)](d, t, id, data,
> + bits_offset);
I really don't like this array lookup approach to handle different BTF
kinds. I know the kernel uses this approach internally, but I think it
just complicates everything unnecessarily. I've written a lot of BTF
handling code and I'm still convinced that explicit switch over
btf_kind(t) (please use that instead of BTF_INFO_KIND(t->info), btw)
is significantly more readable and maintainable.
Besides ability to read and follow the logic, you are not checking for
bad kind whatsoever, while with switch it would be easy and natural to
check and handle that. Some corrupted BTF data would lead to cryptic
failures here.
> +}
> +
> +static void btf_dump_emit_type_data(struct btf_dump *d, __u32 id, void *data)
> +{
It's unclear why you need three functions doing pieces of the same
operation, and that operation is pretty linear in its control flow, so
multiple functions buy you nothing. Why __btf_dump_emit_type_data and
btf_dump_emit_type_data logic are not inlined in
btf_dump__emit_type_data()?
> + const struct btf_type *t = btf__type_by_id(d->btf, id);
> +
> + memset(&d->data.state, 0, sizeof(d->data.state));
> +
> + if (!t) {
you can do this check before all the fussing with the state, right
after OPTS_VALID check below. btf_dump__dump_type() does just that and
it works fine.
> + pr_warn("no type info, id [%u]\n", id);
I'd drop this warning, btf_dump__dump_type() doesn't do it.
> + d->data.state.err = -EINVAL;
> + return;
> + }
> +
> + __btf_dump_emit_type_data(d, t, id, data, 0);
> +}
> +
> +int btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
> + const struct btf_dump_emit_type_data_opts *opts,
> + void *data)
How about we call it in line with btf_dump__dump_type() as
btf_dump__dump_type_data()? Also, please move opts to be the last
argument.
> +{
> + int err;
> +
> + if (!OPTS_VALID(opts, btf_dump_emit_type_data_opts))
> + return -EINVAL;
> +
> + d->data.indent_lvl = OPTS_GET(opts, indent_level, 0);
> + d->data.compact = OPTS_GET(opts, compact, false);
> + d->data.noname = OPTS_GET(opts, noname, false);
> + d->data.zero = OPTS_GET(opts, zero, false);
> + btf_dump_emit_type_data(d, id, data);
> + err = d->data.state.err;
> + memset(&d->data, 0, sizeof(d->data));
> + return err;
> +}
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 1c0fd2d..b81c069 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -350,3 +350,8 @@ LIBBPF_0.3.0 {
> xsk_setup_xdp_prog;
> xsk_socket__update_xskmap;
> } LIBBPF_0.2.0;
> +
> +LIBBPF_0.4.0 {
> + global:
> + btf_dump__emit_type_data;
> +} LIBBPF_0.3.0;
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 bpf-next 4/4] selftests/bpf: add dump type data tests to btf dump tests
2021-01-17 22:16 ` [PATCH v2 bpf-next 4/4] selftests/bpf: add dump type data tests to btf dump tests Alan Maguire
@ 2021-01-21 7:01 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 7:01 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, morbo,
Shuah Khan, bpf, Networking, open list:KERNEL SELFTEST FRAMEWORK,
open list
On Sun, Jan 17, 2021 at 2:21 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Test various type data dumping operations by comparing expected
> format with the dumped string; an snprintf-style printf function
> is used to record the string dumped.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/testing/selftests/bpf/prog_tests/btf_dump.c | 233 ++++++++++++++++++++++
> 1 file changed, 233 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> index c60091e..262561f4 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> @@ -232,6 +232,237 @@ void test_btf_dump_incremental(void)
> btf__free(btf);
> }
>
> +#define STRSIZE 2048
> +#define EXPECTED_STRSIZE 256
> +
> +void btf_dump_snprintf(void *ctx, const char *fmt, va_list args)
> +{
> + char *s = ctx, new[STRSIZE];
> +
> + vsnprintf(new, STRSIZE, fmt, args);
> + strncat(s, new, STRSIZE);
> + vfprintf(ctx, fmt, args);
> +}
> +
> +/* skip "enum "/"struct " prefixes */
> +#define SKIP_PREFIX(_typestr, _prefix) \
> + do { \
> + if (strstr(_typestr, _prefix) == _typestr) \
why not strncmp?
> + _typestr += strlen(_prefix) + 1; \
> + } while (0)
> +
> +int btf_dump_data(struct btf *btf, struct btf_dump *d,
> + char *ptrtype, __u64 flags, void *ptr,
> + char *str, char *expectedval)
> +{
> + struct btf_dump_emit_type_data_opts opts = { 0 };
> + int ret = 0, cmp;
> + __s32 type_id;
> +
> + opts.sz = sizeof(opts);
> + opts.compact = true;
Please use DECLARE_LIBBPF_OPTS(), check other examples in selftests
> + if (flags & BTF_F_NONAME)
> + opts.noname = true;
> + if (flags & BTF_F_ZERO)
> + opts.zero = true;
> + SKIP_PREFIX(ptrtype, "enum");
> + SKIP_PREFIX(ptrtype, "struct");
> + SKIP_PREFIX(ptrtype, "union");
> + type_id = btf__find_by_name(btf, ptrtype);
> + if (CHECK(type_id <= 0, "find type id",
> + "no '%s' in BTF: %d\n", ptrtype, type_id)) {
> + ret = -ENOENT;
> + goto err;
> + }
> + str[0] = '\0';
> + ret = btf_dump__emit_type_data(d, type_id, &opts, ptr);
> + if (CHECK(ret < 0, "btf_dump__emit_type_data",
> + "failed: %d\n", ret))
> + goto err;
> +
> + cmp = strncmp(str, expectedval, EXPECTED_STRSIZE);
> + if (CHECK(cmp, "ensure expected/actual match",
> + "'%s' does not match expected '%s': %d\n",
> + str, expectedval, cmp))
> + ret = -EFAULT;
> +
> +err:
> + if (ret)
> + btf_dump__free(d);
> + return ret;
> +}
> +
> +#define TEST_BTF_DUMP_DATA(_b, _d, _str, _type, _flags, _expected, ...) \
> + do { \
> + char _expectedval[EXPECTED_STRSIZE] = _expected; \
> + char __ptrtype[64] = #_type; \
> + char *_ptrtype = (char *)__ptrtype; \
> + static _type _ptrdata = __VA_ARGS__; \
> + void *_ptr = &_ptrdata; \
> + \
> + if (btf_dump_data(_b, _d, _ptrtype, _flags, _ptr, \
> + _str, _expectedval)) \
> + return; \
> + } while (0)
> +
> +/* Use where expected data string matches its stringified declaration */
> +#define TEST_BTF_DUMP_DATA_C(_b, _d, _str, _type, _opts, ...) \
> + TEST_BTF_DUMP_DATA(_b, _d, _str, _type, _opts, \
> + "(" #_type ")" #__VA_ARGS__, __VA_ARGS__)
> +
> +void test_btf_dump_data(void)
> +{
> + struct btf *btf = libbpf_find_kernel_btf();
> + char str[STRSIZE];
> + struct btf_dump_opts opts = { .ctx = str };
> + struct btf_dump *d;
> +
> + if (CHECK(!btf, "get kernel BTF", "no kernel BTF found"))
> + return;
> +
use libbpf_get_error(), btf won't be NULL on error
> + d = btf_dump__new(btf, NULL, &opts, btf_dump_snprintf);
> +
> + if (CHECK(!d, "new dump", "could not create BTF dump"))
same, d won't be NULL on error
> + return;
> +
> + /* Verify type display for various types. */
> +
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data
2021-01-21 6:56 ` Andrii Nakryiko
@ 2021-01-21 19:51 ` Andrii Nakryiko
2021-01-22 16:31 ` Alan Maguire
0 siblings, 1 reply; 12+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 19:51 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Bill Wendling,
Shuah Khan, bpf, Networking, open list:KERNEL SELFTEST FRAMEWORK,
open list
On Wed, Jan 20, 2021 at 10:56 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sun, Jan 17, 2021 at 2:22 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > Add a BTF dumper for typed data, so that the user can dump a typed
> > version of the data provided.
> >
> > The API is
> >
> > int btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
> > const struct btf_dump_emit_type_data_opts *opts,
> > void *data);
> >
Two more things I realized about this API overnight:
1. It's error-prone to specify only the pointer to data without
specifying the size. If user screws up and scecifies wrong type ID or
if BTF data is corrupted, then this API would start reading and
printing memory outside the bounds. I think it's much better to also
require user to specify the size and bail out with error if we reach
the end of the allowed memory area.
2. This API would be more useful if it also returns the amount of
"consumed" bytes. That way users can do more flexible and powerful
pretty-printing of raw data. So on success we'll have >= 0 number of
bytes used for dumping given BTF type, or <0 on error. WDYT?
> > ...where the id is the BTF id of the data pointed to by the "void *"
> > argument; for example the BTF id of "struct sk_buff" for a
> > "struct skb *" data pointer. Options supported are
> >
> > - a starting indent level (indent_lvl)
> > - a set of boolean options to control dump display, similar to those
> > used for BPF helper bpf_snprintf_btf(). Options are
> > - compact : omit newlines and other indentation
> > - noname: omit member names
> > - zero: show zero-value members
> >
> > Default output format is identical to that dumped by bpf_snprintf_btf(),
> > for example a "struct sk_buff" representation would look like this:
> >
> > struct sk_buff){
> > (union){
> > (struct){
>
> Curious, these explicit anonymous (union) and (struct), is that
> preferred way for explicitness, or is it just because it makes
> implementation simpler and thus was chosen? I.e., if the goal was to
> mimic C-style data initialization, you'd just have plain .next = ...,
> .prev = ..., .dev = ..., .dev_scratch = ..., all on the same level. So
> just checking for myself.
>
> > .next = (struct sk_buff *)0xffffffffffffffff,
> > .prev = (struct sk_buff *)0xffffffffffffffff,
> > (union){
> > .dev = (struct net_device *)0xffffffffffffffff,
> > .dev_scratch = (long unsigned int)18446744073709551615,
> > },
> > },
> > ...
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
>
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data
2021-01-21 19:51 ` Andrii Nakryiko
@ 2021-01-22 16:31 ` Alan Maguire
2021-01-22 20:05 ` Andrii Nakryiko
0 siblings, 1 reply; 12+ messages in thread
From: Alan Maguire @ 2021-01-22 16:31 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alan Maguire, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
john fastabend, KP Singh, Bill Wendling, Shuah Khan, bpf,
Networking, open list:KERNEL SELFTEST FRAMEWORK, open list
On Thu, 21 Jan 2021, Andrii Nakryiko wrote:
> On Wed, Jan 20, 2021 at 10:56 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sun, Jan 17, 2021 at 2:22 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> > >
> > > Add a BTF dumper for typed data, so that the user can dump a typed
> > > version of the data provided.
> > >
> > > The API is
> > >
> > > int btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
> > > const struct btf_dump_emit_type_data_opts *opts,
> > > void *data);
> > >
>
> Two more things I realized about this API overnight:
>
> 1. It's error-prone to specify only the pointer to data without
> specifying the size. If user screws up and scecifies wrong type ID or
> if BTF data is corrupted, then this API would start reading and
> printing memory outside the bounds. I think it's much better to also
> require user to specify the size and bail out with error if we reach
> the end of the allowed memory area.
Yep, good point, especially given in the tracing context we will likely
only have a subset of the data (e.g. part of the 16k representing a
task_struct). The way I was approaching this was to return -E2BIG
and append a "..." to the dumped data denoting the data provided
didn't cover the size needed to fully represent the type. The idea is
the structure is too big for the data provided, hence E2BIG, but maybe
there's a more intuitive way to do this? See below for more...
>
> 2. This API would be more useful if it also returns the amount of
> "consumed" bytes. That way users can do more flexible and powerful
> pretty-printing of raw data. So on success we'll have >= 0 number of
> bytes used for dumping given BTF type, or <0 on error. WDYT?
>
I like it! So
1. if a user provides a too-big data object, we return the amount we used; and
2. if a user provides a too-small data object, we append "..." to the dump
and return -E2BIG (or whatever error code).
However I wonder for case 2 if it'd be better to use a snprintf()-like
semantic rather than an error code, returning the amount we would have
used. That way we easily detect case 1 (size passed in > return value),
case 2 (size passed in < return value), and errors can be treated separately.
Feels to me that dealing with truncated data is going to be sufficiently
frequent it might be good not to classify it as an error. Let me know if
you think that makes sense.
I'm working on v3, and hope to have something early next week, but a quick
reply to a question below...
> > > ...where the id is the BTF id of the data pointed to by the "void *"
> > > argument; for example the BTF id of "struct sk_buff" for a
> > > "struct skb *" data pointer. Options supported are
> > >
> > > - a starting indent level (indent_lvl)
> > > - a set of boolean options to control dump display, similar to those
> > > used for BPF helper bpf_snprintf_btf(). Options are
> > > - compact : omit newlines and other indentation
> > > - noname: omit member names
> > > - zero: show zero-value members
> > >
> > > Default output format is identical to that dumped by bpf_snprintf_btf(),
> > > for example a "struct sk_buff" representation would look like this:
> > >
> > > struct sk_buff){
> > > (union){
> > > (struct){
> >
> > Curious, these explicit anonymous (union) and (struct), is that
> > preferred way for explicitness, or is it just because it makes
> > implementation simpler and thus was chosen? I.e., if the goal was to
> > mimic C-style data initialization, you'd just have plain .next = ...,
> > .prev = ..., .dev = ..., .dev_scratch = ..., all on the same level. So
> > just checking for myself.
The idea here is that we want to clarify if we're dealing with
an anonymous struct or union. I wanted to have things work
like a C-style initializer as closely as possible, but I
realized it's not legit to initialize multiple values in a
union, and more importantly when we're trying to visually interpret
data, we really want to know if an anonymous container of data is
a structure (where all values represent different elements in the
structure) or a union (where we're seeing multiple interpretations of
the same value).
Thanks again for the detailed review!
Alan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data
2021-01-22 16:31 ` Alan Maguire
@ 2021-01-22 20:05 ` Andrii Nakryiko
0 siblings, 0 replies; 12+ messages in thread
From: Andrii Nakryiko @ 2021-01-22 20:05 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Bill Wendling,
Shuah Khan, bpf, Networking, open list:KERNEL SELFTEST FRAMEWORK,
open list
On Fri, Jan 22, 2021 at 8:31 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On Thu, 21 Jan 2021, Andrii Nakryiko wrote:
>
> > On Wed, Jan 20, 2021 at 10:56 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Sun, Jan 17, 2021 at 2:22 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > >
> > > > Add a BTF dumper for typed data, so that the user can dump a typed
> > > > version of the data provided.
> > > >
> > > > The API is
> > > >
> > > > int btf_dump__emit_type_data(struct btf_dump *d, __u32 id,
> > > > const struct btf_dump_emit_type_data_opts *opts,
> > > > void *data);
> > > >
> >
> > Two more things I realized about this API overnight:
> >
> > 1. It's error-prone to specify only the pointer to data without
> > specifying the size. If user screws up and scecifies wrong type ID or
> > if BTF data is corrupted, then this API would start reading and
> > printing memory outside the bounds. I think it's much better to also
> > require user to specify the size and bail out with error if we reach
> > the end of the allowed memory area.
>
> Yep, good point, especially given in the tracing context we will likely
> only have a subset of the data (e.g. part of the 16k representing a
> task_struct). The way I was approaching this was to return -E2BIG
> and append a "..." to the dumped data denoting the data provided
> didn't cover the size needed to fully represent the type. The idea is
> the structure is too big for the data provided, hence E2BIG, but maybe
> there's a more intuitive way to do this? See below for more...
>
Hm... that's an interesting use case for sure, but seems reasonable to
support. "..." seems a bit misleading because it can be interpreted as
"we omitted some output for brevity", no? "<truncated>" or something
like that might be more obvious, but I'm just bikeshedding :)
> >
> > 2. This API would be more useful if it also returns the amount of
> > "consumed" bytes. That way users can do more flexible and powerful
> > pretty-printing of raw data. So on success we'll have >= 0 number of
> > bytes used for dumping given BTF type, or <0 on error. WDYT?
> >
>
> I like it! So
>
> 1. if a user provides a too-big data object, we return the amount we used; and
> 2. if a user provides a too-small data object, we append "..." to the dump
> and return -E2BIG (or whatever error code).
>
> However I wonder for case 2 if it'd be better to use a snprintf()-like
> semantic rather than an error code, returning the amount we would have
> used. That way we easily detect case 1 (size passed in > return value),
> case 2 (size passed in < return value), and errors can be treated separately.
> Feels to me that dealing with truncated data is going to be sufficiently
> frequent it might be good not to classify it as an error. Let me know if
> you think that makes sense.
Hm... Yeah, that would work, I think, and would feel pretty natural.
On the other hand, it's easy to know the total input size needed by
calling btf__resolve_size(btf, type_id), so if user expects to provide
truncated input data and wants to know how much they should have
provided, they can easily do that.
Basically, I don't have strong preference here, though providing
truncated input data still feels more like an error, than a normal
situation... Maybe someone else want to weigh in? And -E2BIG is
distinctive enough in this case. So both would work fine, but not
clear which one is less surprising API.
>
> I'm working on v3, and hope to have something early next week, but a quick
> reply to a question below...
>
> > > > ...where the id is the BTF id of the data pointed to by the "void *"
> > > > argument; for example the BTF id of "struct sk_buff" for a
> > > > "struct skb *" data pointer. Options supported are
> > > >
> > > > - a starting indent level (indent_lvl)
> > > > - a set of boolean options to control dump display, similar to those
> > > > used for BPF helper bpf_snprintf_btf(). Options are
> > > > - compact : omit newlines and other indentation
> > > > - noname: omit member names
> > > > - zero: show zero-value members
> > > >
> > > > Default output format is identical to that dumped by bpf_snprintf_btf(),
> > > > for example a "struct sk_buff" representation would look like this:
> > > >
> > > > struct sk_buff){
> > > > (union){
> > > > (struct){
> > >
> > > Curious, these explicit anonymous (union) and (struct), is that
> > > preferred way for explicitness, or is it just because it makes
> > > implementation simpler and thus was chosen? I.e., if the goal was to
> > > mimic C-style data initialization, you'd just have plain .next = ...,
> > > .prev = ..., .dev = ..., .dev_scratch = ..., all on the same level. So
> > > just checking for myself.
>
> The idea here is that we want to clarify if we're dealing with
> an anonymous struct or union. I wanted to have things work
> like a C-style initializer as closely as possible, but I
> realized it's not legit to initialize multiple values in a
> union, and more importantly when we're trying to visually interpret
> data, we really want to know if an anonymous container of data is
> a structure (where all values represent different elements in the
> structure) or a union (where we're seeing multiple interpretations of
> the same value).
Yeah, fair enough.
>
> Thanks again for the detailed review!
Of course. But it's not clear if you agree with me on everything, so I
still hope to get replies later.
>
> Alan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-01-22 22:15 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-17 22:16 [PATCH v2 bpf-next 0/4] libbpf: BTF dumper support for typed data Alan Maguire
2021-01-17 22:16 ` [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines Alan Maguire
2021-01-21 4:11 ` Andrii Nakryiko
2021-01-17 22:16 ` [PATCH v2 bpf-next 2/4] libbpf: make skip_mods_and_typedefs available internally in libbpf Alan Maguire
2021-01-21 4:13 ` Andrii Nakryiko
2021-01-17 22:16 ` [PATCH v2 bpf-next 3/4] libbpf: BTF dumper support for typed data Alan Maguire
2021-01-21 6:56 ` Andrii Nakryiko
2021-01-21 19:51 ` Andrii Nakryiko
2021-01-22 16:31 ` Alan Maguire
2021-01-22 20:05 ` Andrii Nakryiko
2021-01-17 22:16 ` [PATCH v2 bpf-next 4/4] selftests/bpf: add dump type data tests to btf dump tests Alan Maguire
2021-01-21 7:01 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).