On 5/18/20 2:46 AM, Alan Maguire wrote:
On Wed, 13 May 2020, Yonghong Song wrote:
+struct btf_show {
+ u64 flags;
+ void *target; /* target of show operation (seq file, buffer) */
+ void (*showfn)(struct btf_show *show, const char *fmt, ...);
+ const struct btf *btf;
+ /* below are used during iteration */
+ struct {
+ u8 depth;
+ u8 depth_shown;
+ u8 depth_check;
I have some difficulties to understand the relationship between
the above three variables. Could you add some comments here?
Will do; sorry the code got a bit confusing. The goal is to track
which sub-components in a data structure we need to display. The
"depth" variable tracks where we are currently; "depth_shown"
is the depth at which we have something nonzer to display (perhaps
"depth_to_show" would be a better name?). "depth_check" tells
"depth_to_show" is indeed better.
us whether we are currently checking depth or doing printing.
If we're checking, we don't actually print anything, we merely note
if we hit a non-zero value, and if so, we set "depth_shown"
to the depth at which we hit that value.
When we show a struct, union or array, we will only display an
object has one or more non-zero members. But because
the struct can in turn nest a struct or array etc, we need
to recurse into the object. When we are doing that, depth_check
is set, and this tells us not to do any actual display. When
that recursion is complete, we check if "depth_shown" (depth
to show) is > depth (i.e. we found something) and if it is
we go on to display the object (setting depth_check to 0).
Thanks for the explanation. Putting them in the comments
will be great.
There may be a better way to solve this problem of course,
but I wanted to avoid storing values where possible as
deeply-nested data structures might overrun such storage.
[...]
+
+ /*
+ * 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.
+ */
+ for (i = 0; i < BTF_SHOW_MAX_ITER; i++) {
+
+ switch (BTF_INFO_KIND(t->info)) {
+ case BTF_KIND_TYPEDEF:
+ if (!type_name)
+ type_name = btf_name_by_offset(show->btf,
+ t->name_off);
type_name should never be NULL for valid vmlinux BTF.
+ kinds |= BTF_KIND_BIT(BTF_KIND_TYPEDEF);
+ id = t->type;
+ break;
+ case BTF_KIND_ARRAY:
+ kinds |= BTF_KIND_BIT(BTF_KIND_ARRAY);
+ parens = "[";
+ array = btf_type_array(t);
+ if (!array)
array will never be NULL here.
+ return show->state.type_name;
+ if (!t)
t will never be NULL here.
+ return show->state.type_name;
+ 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 = btf_type_skip_qualifiers(show->btf, id);
t should never be NULL here.
+ if (!t)
+ return show->state.type_name;
+ }
Do we do pointer tracing here? For example
struct t {
int *m[5];
}
When trying to access memory, the above code may go through
ptr->array and out of loop when hitting array element type "int"?
I'm not totally sure I understand the question so I'll
try and describe how the above is supposed to work. I
think there's a bug here alright.
In the above case, when we reach the "m" field of "struct t",
the code should start with the BTF_KIND_ARRAY, set up
the array suffix, then get the array type which is a PTR
and we will set up the ptr suffix to be "*" and we set
the id to the id associated with "int", and
btf_type_skip_qualifiers() will use that id to look up
the new value for the type used in btf_name_by_offset().
So on the next iteration we hit the int itself and bail from
the loop, having noted that we've got a _PTR and _ARRAY set in
the "kinds" bitfield.
Then we look up the int type using "t" with btf_name_by_offset,
so we end up displaying "(int *m[])" as the type.
Thanks for explanation. Previously I thought this somehow
may be related to tracing data. Looks it is only for
*constructing* type names. So it largely looks fine though.
However the code assumes we don't need the parentheses for
the array if we have encountered a pointer; that's never
the case. We only should eliminate the opening parens
for a struct or union "{" in such cases, as in those cases
we have a pointer to the struct rather than a nested struct.
So that needs to be fixed. Are the other problems here you're
seeing that the above doesn't cover?
A few minor comments in the above.
+ /* We may not be able to represent this type; bail to be safe */
+ if (i == BTF_SHOW_MAX_ITER)
+ return show->state.type_name;
+
+ if (!type_name)
+ type_name = btf_name_by_offset(show->btf, 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:
+ allow_anon = false;
[...]
+ if (elem_type && btf_type_is_int(elem_type)) {
+ u32 int_type = btf_type_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;
Some char array may be printable and some may not be printable,
how did you differentiate this?
I should probably change the logic to ensure all chars
(before a \0) are printable. I'll do that for v2. We will always
have cases (e.g. the skb cb[] field) where the char[] is not
intended as a string, but I think the utility of showing them as
strings where possible is worthwhile.
Make sense. Thanks!
Thanks again for reviewing!
Alan
