diff mbox series

[v2,11/19] gendwarfksyms: Limit structure expansion

Message ID 20240815173903.4172139-32-samitolvanen@google.com (mailing list archive)
State New
Headers show
Series Implement DWARF modversions | expand

Commit Message

Sami Tolvanen Aug. 15, 2024, 5:39 p.m. UTC
Expand each structure type only once per exported symbol. This
is necessary to support self-referential structures, which would
otherwise result in infinite recursion, but is still sufficient for
catching ABI changes.

For pointers to structure types, limit type expansion inside the
pointer to two levels. This should be plenty for detecting ABI
differences, but it stops us from pulling in half the kernel for
types that contain pointers to large kernel data structures, like
task_struct, for example.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 scripts/gendwarfksyms/Makefile        |   1 +
 scripts/gendwarfksyms/cache.c         |  51 ++++++++++++
 scripts/gendwarfksyms/dwarf.c         | 108 ++++++++++++++++++++++++--
 scripts/gendwarfksyms/gendwarfksyms.h |  38 ++++++++-
 4 files changed, 189 insertions(+), 9 deletions(-)
 create mode 100644 scripts/gendwarfksyms/cache.c

Comments

Petr Pavlu Sept. 3, 2024, 3:15 p.m. UTC | #1
On 8/15/24 19:39, Sami Tolvanen wrote:
> Expand each structure type only once per exported symbol. This
> is necessary to support self-referential structures, which would
> otherwise result in infinite recursion, but is still sufficient for
> catching ABI changes.
> 
> For pointers to structure types, limit type expansion inside the
> pointer to two levels. This should be plenty for detecting ABI
> differences, but it stops us from pulling in half the kernel for
> types that contain pointers to large kernel data structures, like
> task_struct, for example.

I'm quite worried about this optimization for pointer types. It could
result in some kABI changes not being recognized.

I assume the goal of the optimization is to speed up the tool's runtime.
How much does it improve the processing time and is there any other way
how it could be done?

> diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
> index 92b6ca4c5c91..2f1601015c4e 100644
> --- a/scripts/gendwarfksyms/dwarf.c
> +++ b/scripts/gendwarfksyms/dwarf.c
> [...]
> @@ -651,6 +742,7 @@ static int process_exported_symbols(struct state *state, struct die *cache,
>  		else
>  			check(process_variable(state, &state->die));
>  
> +		cache_clear_expanded(&state->expansion_cache);
>  		return 0;
>  	default:
>  		return 0;

I wonder if it would make sense to share the cache between processing
individual exported symbols.

The hard case looks to be the following:
s#A struct A { int ; }
s#B struct B { s#A ; }
foo void foo ( s#B )
bar void bar ( s#A , s#B )

When processing foo, the code would cache s#B with expanded s#A.
However, when processing bar and reaching s#B, the cache should report
a miss because s#B with unexpanded s#A is required.

So the code would need to track which types were already expanded and
have each cache entry accordingly tagged with similar data.

Hm, it might be that doing all this additional tracking would then be
actually slower than processing the types repeatedly for each symbol.
I'm not sure.
Sami Tolvanen Sept. 5, 2024, 6:15 p.m. UTC | #2
On Tue, Sep 3, 2024 at 8:15 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> On 8/15/24 19:39, Sami Tolvanen wrote:
> > Expand each structure type only once per exported symbol. This
> > is necessary to support self-referential structures, which would
> > otherwise result in infinite recursion, but is still sufficient for
> > catching ABI changes.
> >
> > For pointers to structure types, limit type expansion inside the
> > pointer to two levels. This should be plenty for detecting ABI
> > differences, but it stops us from pulling in half the kernel for
> > types that contain pointers to large kernel data structures, like
> > task_struct, for example.
>
> I'm quite worried about this optimization for pointer types. It could
> result in some kABI changes not being recognized.
>
> I assume the goal of the optimization is to speed up the tool's runtime.
> How much does it improve the processing time and is there any other way
> how it could be done?

It’s mostly a matter of how deep it makes sense to go. For example,
queue_delayed_work_on accepts a pointer to s#workqueue_struct, which
points to s#worker, which points to s#task_struct, which points to
s#mm_struct etc. Does a change to an internal kernel data structure
several references deep change the ABI of the function?

If we traverse through the DWARF without limits, during a defconfig
build the highest pointer expansion depth I see is 70 levels (!), with
~5k symbols going 30+ levels deep. We would end up pulling in a lot of
major internal data structures at that point, and a change to any of
them would change thousands of symbol versions, which feels
undesirable.

I'm fine with increasing the limit to something more reasonable
though, the impact on performance doesn't seem to be significant in
parallel builds. Of course, this might impact vmlinux.o processing
more, if we end up doing that, since the DWARF at that point contains
information about all the data structures.

I do wonder if there's a better way to figure out where to stop than a
hard limit. Any thoughts?

> > diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
> > index 92b6ca4c5c91..2f1601015c4e 100644
> > --- a/scripts/gendwarfksyms/dwarf.c
> > +++ b/scripts/gendwarfksyms/dwarf.c
> > [...]
> > @@ -651,6 +742,7 @@ static int process_exported_symbols(struct state *state, struct die *cache,
> >               else
> >                       check(process_variable(state, &state->die));
> >
> > +             cache_clear_expanded(&state->expansion_cache);
> >               return 0;
> >       default:
> >               return 0;
>
> I wonder if it would make sense to share the cache between processing
> individual exported symbols.

The actual DIE caching happens in die_map, which is already shared
between symbols. The expansion cache keeps track of which DIEs we have
processed per symbol, so we don't process the same thing twice and end
up in a loop, for example.

Sami
diff mbox series

Patch

diff --git a/scripts/gendwarfksyms/Makefile b/scripts/gendwarfksyms/Makefile
index fcbac52ca00a..681b42441840 100644
--- a/scripts/gendwarfksyms/Makefile
+++ b/scripts/gendwarfksyms/Makefile
@@ -1,6 +1,7 @@ 
 hostprogs-always-y += gendwarfksyms
 
 gendwarfksyms-objs += gendwarfksyms.o
+gendwarfksyms-objs += cache.o
 gendwarfksyms-objs += die.o
 gendwarfksyms-objs += dwarf.o
 gendwarfksyms-objs += symbols.o
diff --git a/scripts/gendwarfksyms/cache.c b/scripts/gendwarfksyms/cache.c
new file mode 100644
index 000000000000..0a4efdcb8cda
--- /dev/null
+++ b/scripts/gendwarfksyms/cache.c
@@ -0,0 +1,51 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Google LLC
+ */
+
+#include "gendwarfksyms.h"
+
+struct expanded {
+	uintptr_t addr;
+	struct hlist_node hash;
+};
+
+int __cache_mark_expanded(struct expansion_cache *ec, uintptr_t addr)
+{
+	struct expanded *es;
+
+	es = malloc(sizeof(struct expanded));
+	if (!es) {
+		error("malloc failed");
+		return -1;
+	}
+
+	es->addr = addr;
+	hash_add(ec->cache, &es->hash, addr_hash(addr));
+	return 0;
+}
+
+bool __cache_was_expanded(struct expansion_cache *ec, uintptr_t addr)
+{
+	struct expanded *es;
+
+	hash_for_each_possible(ec->cache, es, hash, addr_hash(addr)) {
+		if (es->addr == addr)
+			return true;
+	}
+
+	return false;
+}
+
+void cache_clear_expanded(struct expansion_cache *ec)
+{
+	struct hlist_node *tmp;
+	struct expanded *es;
+	int i;
+
+	hash_for_each_safe(ec->cache, i, tmp, es, hash) {
+		free(es);
+	}
+
+	hash_init(ec->cache);
+}
diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
index 92b6ca4c5c91..2f1601015c4e 100644
--- a/scripts/gendwarfksyms/dwarf.c
+++ b/scripts/gendwarfksyms/dwarf.c
@@ -25,6 +25,7 @@  static int process_linebreak(struct die *cache, int n)
 		       !dwarf_form##attr(&da, value);                  \
 	}
 
+DEFINE_GET_ATTR(flag, bool)
 DEFINE_GET_ATTR(udata, Dwarf_Word)
 
 static bool get_ref_die_attr(Dwarf_Die *die, unsigned int id, Dwarf_Die *value)
@@ -69,6 +70,13 @@  static bool is_export_symbol(struct state *state, Dwarf_Die *die)
 	return !!state->sym;
 }
 
+static bool is_declaration(Dwarf_Die *die)
+{
+	bool value;
+
+	return get_flag_attr(die, DW_AT_declaration, &value) && value;
+}
+
 /*
  * Type string processing
  */
@@ -421,19 +429,26 @@  static int __process_structure_type(struct state *state, struct die *cache,
 				    die_callback_t process_func,
 				    die_match_callback_t match_func)
 {
+	bool is_decl = is_declaration(die);
+
 	check(process(state, cache, type));
 	check(process_fqn(state, cache, die));
 	check(process(state, cache, " {"));
 	check(process_linebreak(cache, 1));
 
-	check(process_die_container(state, cache, die, process_func,
-				    match_func));
+	if (!is_decl && state->expand.expand) {
+		check(cache_mark_expanded(&state->expansion_cache, die->addr));
+		check(process_die_container(state, cache, die, process_func,
+					    match_func));
+	}
 
 	check(process_linebreak(cache, -1));
 	check(process(state, cache, "}"));
 
-	check(process_byte_size_attr(state, cache, die));
-	check(process_alignment_attr(state, cache, die));
+	if (!is_decl && state->expand.expand) {
+		check(process_byte_size_attr(state, cache, die));
+		check(process_alignment_attr(state, cache, die));
+	}
 
 	return 0;
 }
@@ -519,6 +534,42 @@  static int process_cached(struct state *state, struct die *cache,
 	return 0;
 }
 
+static void state_init(struct state *state)
+{
+	state->expand.expand = true;
+	state->expand.in_pointer_type = false;
+	state->expand.ptr_expansion_depth = 0;
+	hash_init(state->expansion_cache.cache);
+}
+
+static void expansion_state_restore(struct expansion_state *state,
+				    struct expansion_state *saved)
+{
+	state->ptr_expansion_depth = saved->ptr_expansion_depth;
+	state->in_pointer_type = saved->in_pointer_type;
+	state->expand = saved->expand;
+}
+
+static void expansion_state_save(struct expansion_state *state,
+				 struct expansion_state *saved)
+{
+	expansion_state_restore(saved, state);
+}
+
+static bool is_pointer_type(int tag)
+{
+	return tag == DW_TAG_pointer_type || tag == DW_TAG_reference_type;
+}
+
+static bool is_expanded_type(int tag)
+{
+	return tag == DW_TAG_class_type || tag == DW_TAG_structure_type ||
+	       tag == DW_TAG_union_type || tag == DW_TAG_enumeration_type;
+}
+
+/* The maximum depth for expanding structures in pointers */
+#define MAX_POINTER_EXPANSION_DEPTH 2
+
 #define PROCESS_TYPE(type)                                       \
 	case DW_TAG_##type##_type:                               \
 		check(process_##type##_type(state, cache, die)); \
@@ -526,18 +577,56 @@  static int process_cached(struct state *state, struct die *cache,
 
 static int process_type(struct state *state, struct die *parent, Dwarf_Die *die)
 {
+	enum die_state want_state = COMPLETE;
 	struct die *cache = NULL;
+	struct expansion_state saved;
 	int tag = dwarf_tag(die);
 
+	expansion_state_save(&state->expand, &saved);
+
 	/*
-	 * If we have the DIE already cached, use it instead of walking
+	 * Structures and enumeration types are expanded only once per
+	 * exported symbol. This is sufficient for detecting ABI changes
+	 * within the structure.
+	 *
+	 * If the exported symbol contains a pointer to a structure,
+	 * at most MAX_POINTER_EXPANSION_DEPTH levels are expanded into
+	 * the referenced structure.
+	 */
+	state->expand.in_pointer_type = saved.in_pointer_type ||
+					is_pointer_type(tag);
+
+	if (state->expand.in_pointer_type &&
+	    state->expand.ptr_expansion_depth >= MAX_POINTER_EXPANSION_DEPTH)
+		state->expand.expand = false;
+	else
+		state->expand.expand =
+			saved.expand &&
+			!cache_was_expanded(&state->expansion_cache, die->addr);
+
+	/* Keep track of pointer expansion depth */
+	if (state->expand.expand && state->expand.in_pointer_type &&
+	    is_expanded_type(tag))
+		state->expand.ptr_expansion_depth++;
+
+	/*
+	 * If we have want_state already cached, use it instead of walking
 	 * through DWARF.
 	 */
-	check(die_map_get(die, COMPLETE, &cache));
+	if (!state->expand.expand && is_expanded_type(tag))
+		want_state = UNEXPANDED;
+
+	check(die_map_get(die, want_state, &cache));
+
+	if (cache->state == want_state) {
+		if (want_state == COMPLETE && is_expanded_type(tag))
+			check(cache_mark_expanded(&state->expansion_cache,
+						  die->addr));
 
-	if (cache->state == COMPLETE) {
 		check(process_cached(state, cache, die));
 		check(die_map_add_die(parent, cache));
+
+		expansion_state_restore(&state->expand, &saved);
 		return 0;
 	}
 
@@ -578,9 +667,10 @@  static int process_type(struct state *state, struct die *parent, Dwarf_Die *die)
 
 	/* Update cache state and append to the parent (if any) */
 	cache->tag = tag;
-	cache->state = COMPLETE;
+	cache->state = want_state;
 	check(die_map_add_die(parent, cache));
 
+	expansion_state_restore(&state->expand, &saved);
 	return 0;
 }
 
@@ -643,6 +733,7 @@  static int process_exported_symbols(struct state *state, struct die *cache,
 			return 0;
 
 		debug("%s", state->sym->name);
+		state_init(state);
 
 		if (is_symbol_ptr(get_name(&state->die)))
 			check(process_symbol_ptr(state, &state->die));
@@ -651,6 +742,7 @@  static int process_exported_symbols(struct state *state, struct die *cache,
 		else
 			check(process_variable(state, &state->die));
 
+		cache_clear_expanded(&state->expansion_cache);
 		return 0;
 	default:
 		return 0;
diff --git a/scripts/gendwarfksyms/gendwarfksyms.h b/scripts/gendwarfksyms/gendwarfksyms.h
index 7d32ccd590f8..6482503e7d6e 100644
--- a/scripts/gendwarfksyms/gendwarfksyms.h
+++ b/scripts/gendwarfksyms/gendwarfksyms.h
@@ -106,7 +106,7 @@  extern struct symbol *symbol_get(const char *name);
  * die.c
  */
 
-enum die_state { INCOMPLETE, COMPLETE, LAST = COMPLETE };
+enum die_state { INCOMPLETE, UNEXPANDED, COMPLETE, LAST = COMPLETE };
 enum die_fragment_type { EMPTY, STRING, LINEBREAK, DIE };
 
 struct die_fragment {
@@ -128,6 +128,7 @@  static inline const char *die_state_name(enum die_state state)
 	switch (state) {
 	default:
 	CASE_CONST_TO_STR(INCOMPLETE)
+	CASE_CONST_TO_STR(UNEXPANDED)
 	CASE_CONST_TO_STR(COMPLETE)
 	}
 }
@@ -150,15 +151,50 @@  extern int die_map_add_linebreak(struct die *pd, int linebreak);
 extern int die_map_add_die(struct die *pd, struct die *child);
 extern void die_map_free(void);
 
+/*
+ * cache.c
+ */
+
+#define EXPANSION_CACHE_HASH_BITS 11
+
+/* A cache for addresses we've already seen. */
+struct expansion_cache {
+	DECLARE_HASHTABLE(cache, EXPANSION_CACHE_HASH_BITS);
+};
+
+extern int __cache_mark_expanded(struct expansion_cache *ec, uintptr_t addr);
+extern bool __cache_was_expanded(struct expansion_cache *ec, uintptr_t addr);
+
+static inline int cache_mark_expanded(struct expansion_cache *ec, void *addr)
+{
+	return __cache_mark_expanded(ec, (uintptr_t)addr);
+}
+
+static inline bool cache_was_expanded(struct expansion_cache *ec, void *addr)
+{
+	return __cache_was_expanded(ec, (uintptr_t)addr);
+}
+
+extern void cache_clear_expanded(struct expansion_cache *ec);
+
 /*
  * dwarf.c
  */
+struct expansion_state {
+	bool expand;
+	bool in_pointer_type;
+	unsigned int ptr_expansion_depth;
+};
 
 struct state {
 	Dwfl_Module *mod;
 	Dwarf *dbg;
 	struct symbol *sym;
 	Dwarf_Die die;
+
+	/* Structure expansion */
+	struct expansion_state expand;
+	struct expansion_cache expansion_cache;
 };
 
 typedef int (*die_callback_t)(struct state *state, struct die *cache,