[PATCH v3 dwarves 6/8] btf_encoder: support delaying function addition to check for function prototype inconsistencies

[Alan Maguire <alan.maguire@oracle.com>]

There are multiple sources of inconsistency that can result in functions
of the same name having multiple prototypes:

- multiple static functions in different CUs share the same name
- static and external functions share the same name

In addition a function may have optimized-out parameters in some
CUs but not others.

Here we attempt to catch such cases by finding inconsistencies across
CUs using the save/compare/merge mechanisms that were previously
introduced to handle optimized-out parameters.

For two instances of a function to be considered consistent:

- number of parameters must match
- parameter names must match

The latter is a less strong method than a full type comparison but
suffices to match functions.

To enable inconsistency checking, the --skip_encoding_btf_inconsistent_proto
option is introduced.

With it, and the --btf_gen_optimized options in place:

- 285 functions are omitted due to inconsistent function prototypes
- 2495 functions are omitted due to optimized-out parameters, of these
  803 are functions with optimization-related suffixes ".isra", etc,
  leaving 1692 other functions without such suffixes.

Below performance effects and variability in encoded BTF are detailed.
It can be seen that due to the approach used - where functions are
marked "generated" on a per-encoder basis, we see quite variable
numbers of multiply-defined functions in the baseline case, some
with inconsistent prototypes.  With --skip_encoding_btf_inconsistent_proto
specified, this variability disappears, at the cost of a longer time
to carry out encoding due to the need to compare representations across
encoders at thread collection time.

Baseline:

Single-threaded:

$ time LLVM_OBJCOPY=objcopy pahole -J vmlinux

real    0m17.534s
user    0m17.019s
sys     0m0.514s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
51529
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
51529

2 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j2 vmlinux

real    0m10.942s
user    0m17.309s
sys     0m0.592s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
51798
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
51529

4 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j4 vmlinux

real    0m7.890s
user    0m18.067s
sys     0m0.661s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
52028
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
51529

Test:

Single-threaded:

$ time LLVM_OBJCOPY=objcopy pahole -J --skip_encoding_btf_inconsistent_proto --btf_gen_optimized vmlinux

real    0m19.216s
user    0m17.590s
sys     0m1.624s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
50246
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
50246

2 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j2 --skip_encoding_btf_inconsistent_proto --btf_gen_optimized vmlinux

real    0m13.147s
user    0m18.179s
sys     0m3.486s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
50246
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
50246

4 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j4 --skip_encoding_btf_inconsistent_proto --btf_gen_optimized vmlinux

real    0m11.090s
user    0m19.613s
sys     0m5.895s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
50246
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
50246

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 btf_encoder.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-----
 dwarves.h     |  2 ++
 pahole.c      |  8 +++++
 3 files changed, 96 insertions(+), 9 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index cb50401..35fb60a 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -33,9 +33,13 @@
 #include <search.h> /* for tsearch(), tfind() and tdestroy() */
 #include <pthread.h>
 
+#define BTF_ENCODER_MAX_PARAMETERS	12
+
 /* state used to do later encoding of saved functions */
 struct btf_encoder_state {
 	uint32_t type_id_off;
+	bool got_parameter_names;
+	const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
 };
 
 struct elf_function {
@@ -800,6 +804,66 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
 	return id;
 }
 
+static void parameter_names__get(struct ftype *ftype, size_t nr_parameters,
+				 const char **parameter_names)
+{
+	struct parameter *parameter;
+	int i = 0;
+
+	ftype__for_each_parameter(ftype, parameter) {
+		if (i >= nr_parameters)
+			return;
+		parameter_names[i++] = parameter__name(parameter);
+	}
+}
+
+static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func, struct function *f2)
+{
+	const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
+	struct function *f1 = func->function;
+	const char *name;
+	int i;
+
+	if (!f1)
+		return false;
+
+	name = function__name(f1);
+
+	if (f1->proto.nr_parms != f2->proto.nr_parms) {
+		if (encoder->verbose)
+			printf("function mismatch for '%s'(%s): %d params != %d params\n",
+			       name, f1->alias ?: name,
+			       f1->proto.nr_parms, f2->proto.nr_parms);
+		return false;
+	}
+	if (f1->proto.nr_parms == 0)
+		return true;
+
+	if (!func->state.got_parameter_names) {
+		parameter_names__get(&f1->proto, BTF_ENCODER_MAX_PARAMETERS,
+				     func->state.parameter_names);
+		func->state.got_parameter_names = true;
+	}
+	parameter_names__get(&f2->proto, BTF_ENCODER_MAX_PARAMETERS, parameter_names);
+	for (i = 0; i < f1->proto.nr_parms && i < BTF_ENCODER_MAX_PARAMETERS; i++) {
+		if (!func->state.parameter_names[i]) {
+			if (!parameter_names[i])
+				continue;
+		} else if (parameter_names[i]) {
+			if (strcmp(func->state.parameter_names[i], parameter_names[i]) == 0)
+				continue;
+		}
+		if (encoder->verbose) {
+			printf("function mismatch for '%s'(%s): parameter #%d '%s' != '%s'\n",
+			       name, f1->alias ?: name, i,
+			       func->state.parameter_names[i] ?: "<null>",
+			       parameter_names[i] ?: "<null>");
+		}
+		return false;
+	}
+	return true;
+}
+
 static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
 {
 	if (func->function) {
@@ -807,12 +871,16 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
 
 		/* If saving and we find an existing entry, we want to merge
 		 * observations across both functions, checking that the
-		 * "seen optimized parameters" status is reflected in the func
-		 * entry. If the entry is new, record encoder state required
+		 * "seen optimized parameters" and "inconsistent prototype"
+		 * status is reflected in the func entry.
+		 * If the entry is new, record encoder state required
 		 * to add the local function later (encoder + type_id_off)
 		 * such that we can add the function later.
 		 */
 		existing->proto.optimized_parms |= fn->proto.optimized_parms;
+		if (!existing->proto.optimized_parms && !existing->proto.inconsistent_proto &&
+		     !funcs__match(encoder, func, fn))
+			existing->proto.inconsistent_proto = 1;
 	} else {
 		func->state.type_id_off = encoder->type_id_off;
 		func->function = fn;
@@ -851,7 +919,8 @@ static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 	int i;
 
 	for (i = 0; i < encoder->functions.cnt; i++) {
-		struct function *fn = encoder->functions.entries[i].function;
+		struct elf_function *func = &encoder->functions.entries[i];
+		struct function *fn = func->function;
 		struct btf_encoder *other_encoder;
 
 		if (!fn || fn->proto.processed)
@@ -871,18 +940,23 @@ static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 			if (!other_fn)
 				continue;
 			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
+			if (other_fn->proto.inconsistent_proto)
+				fn->proto.inconsistent_proto = 1;
+			if (!fn->proto.optimized_parms && !fn->proto.inconsistent_proto &&
+			    !funcs__match(encoder, func, other_fn))
+				fn->proto.inconsistent_proto = 1;
 			other_fn->proto.processed = 1;
 		}
-		if (fn->proto.optimized_parms) {
+		if (fn->proto.optimized_parms || fn->proto.inconsistent_proto) {
 			if (encoder->verbose) {
 				const char *name = function__name(fn);
 
-				printf("skipping addition of '%s'(%s) due to optimized-out parameters\n",
-				       name, fn->alias ?: name);
+				printf("skipping addition of '%s'(%s) due to %s\n",
+				       name, fn->alias ?: name,
+				       fn->proto.optimized_parms ? "optimized-out parameters" :
+								   "multiple inconsistent function prototypes");
 			}
 		} else {
-			struct elf_function *func = &encoder->functions.entries[i];
-
 			encoder->type_id_off = func->state.type_id_off;
 			btf_encoder__add_func(encoder, fn);
 		}
@@ -1759,7 +1833,10 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 			if (func) {
 				if (func->generated)
 					continue;
-				func->generated = true;
+				if (conf_load->skip_encoding_btf_inconsistent_proto)
+					save = true;
+				else
+					func->generated = true;
 			} else if (encoder->functions.suffix_cnt &&
 				   conf_load->btf_gen_optimized) {
 				/* falling back to name.isra.0 match if no exact
diff --git a/dwarves.h b/dwarves.h
index bb2c3bb..c9d2bf9 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -67,6 +67,7 @@ struct conf_load {
 	bool			skip_encoding_btf_type_tag;
 	bool			skip_encoding_btf_enum64;
 	bool			btf_gen_optimized;
+	bool			skip_encoding_btf_inconsistent_proto;
 	uint8_t			hashtable_bits;
 	uint8_t			max_hashtable_bits;
 	uint16_t		kabi_prefix_len;
@@ -834,6 +835,7 @@ struct ftype {
 	uint8_t		 unspec_parms:1; /* just one bit is needed */
 	uint8_t		 optimized_parms:1;
 	uint8_t		 processed:1;
+	uint8_t		 inconsistent_proto:1;
 };
 
 static inline struct ftype *tag__ftype(const struct tag *tag)
diff --git a/pahole.c b/pahole.c
index f48b66d..2992f43 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1222,6 +1222,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_skip_encoding_btf_enum64 337
 #define ARGP_skip_emitting_atomic_typedefs 338
 #define ARGP_btf_gen_optimized  339
+#define ARGP_skip_encoding_btf_inconsistent_proto 340
 
 static const struct argp_option pahole__options[] = {
 	{
@@ -1639,6 +1640,11 @@ static const struct argp_option pahole__options[] = {
 		.key  = ARGP_btf_gen_optimized,
 		.doc  = "Generate BTF for functions with optimization-related suffixes (.isra, .constprop).  BTF will only be generated if a function does not optimize out parameters."
 	},
+	{
+		.name = "skip_encoding_btf_inconsistent_proto",
+		.key = ARGP_skip_encoding_btf_inconsistent_proto,
+		.doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or have optimized-out parameters."
+	},
 	{
 		.name = NULL,
 	}
@@ -1810,6 +1816,8 @@ static error_t pahole__options_parser(int key, char *arg,
 		conf.skip_emitting_atomic_typedefs = true;	break;
 	case ARGP_btf_gen_optimized:
 		conf_load.btf_gen_optimized = true;		break;
+	case ARGP_skip_encoding_btf_inconsistent_proto:
+		conf_load.skip_encoding_btf_inconsistent_proto = true; break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.31.1