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
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,
using it for all functions.
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.
With these changes, we see 278 functions removed due to
protoype inconsistency. For example, wakeup_show()
has two distinct prototypes:
static ssize_t wakeup_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
(from kernel/irq/irqdesc.c)
static ssize_t wakeup_show(struct device *dev, struct device_attribute *attr,
char *buf)
(from drivers/base/power/sysfs.c)
In some other cases, the parameter comparisons weed out additional
inconsistencies in "."-suffixed functions across CUs.
We also see a large number of functions eliminated due to
optimized-out parameters; 2542 functions are eliminated for this
reason, both "."-suffixed (1007) and otherwise (1535).
Because the save/compare/merge process occurs for all functions
it is important to assess performance effects. In addition,
prior to these changes the number of functions ultimately
represented in BTF was non-deterministic when pahole was
run with multiple threads. This was due to the fact that
functions were marked as generated on a per-encoder basis
when first added, and as such the same function could
be added multiple times for different encoders, and if they
encountered inconsistent function prototypes, deduplication
could leave multiple entries in place for the same name.
When run in a single thread, the "generated" state associated
with the name would prevent this.
Here we assess both BTF encoding performance and determinism
of the function representation in baseline compared to with
these changes. Determinism is assessed by counting the
number of functions in BTF. Comparisons are done for 1,
4 and 8 threads.
Baseline
$ time LLVM_OBJCOPY=objcopy pahole -J vmlinux
real 0m18.160s
user 0m17.179s
sys 0m0.757s
$ grep " FUNC " /tmp/vmlinux.btf.base |awk '{print $3}'|sort|wc -l
51150
$ grep " FUNC " /tmp/vmlinux.btf.base |awk '{print $3}'|sort|uniq|wc -l
51150
$ time LLVM_OBJCOPY=objcopy pahole -J -j4 vmlinux
real 0m8.078s
user 0m17.978s
sys 0m0.732s
$ grep " FUNC " /tmp/vmlinux.btf.base |awk '{print $3}'|sort|wc -l
51592
$ grep " FUNC " /tmp/vmlinux.btf.base |awk '{print $3}'|sort|uniq|wc -l
51150
$ time LLVM_OBJCOPY=objcopy pahole -J -j8 vmlinux
real 0m7.075s
user 0m19.010s
sys 0m0.587s
$ grep " FUNC " /tmp/vmlinux.btf.base |awk '{print $3}'|sort|wc -l
51683
$ grep " FUNC " /tmp/vmlinux.btf.base |awk '{print $3}'|sort|uniq|wc -l
51150
Test:
$ time LLVM_OBJCOPY=objcopy pahole -J vmlinux
real 0m19.039s
user 0m17.617s
sys 0m1.419s
$ bpftool btf dump file vmlinux | grep ' FUNC ' |sort|wc -l
49871
$ bpftool btf dump file vmlinux | grep ' FUNC ' |sort|uniq|wc -l
49871
$ time LLVM_OBJCOPY=objcopy pahole -J -j4 vmlinux
real 0m8.482s
user 0m18.233s
sys 0m2.412s
$ bpftool btf dump file vmlinux | grep ' FUNC ' |sort|wc -l
49871
$ bpftool btf dump file vmlinux | grep ' FUNC ' |sort|uniq|wc -l
49871
$ time LLVM_OBJCOPY=objcopy pahole -J -j8 vmlinux
real 0m7.614s
user 0m19.384s
sys 0m3.739s
$ bpftool btf dump file vmlinux | grep ' FUNC ' |sort|wc -l
49871
$ bpftool btf dump file vmlinux | grep ' FUNC ' |sort|uniq|wc -l
So there is a small cost in performance, but we improve determinism
and the consistency of representation.
Future work could support maintaining multiple inconsistent
prototypes; having a way to associate the function BTF representation
with the function site would be needed however, and the BPF
infrastructure would need to ensure that an fentry program was
attached to the right site with the right prototype for example.
BTF declaration tags with specifying the function address(es)
a prototype referred to could help here, but edge cases like
KASLR (where addresses change dynamically at boot-time) would
have to be considered to make this work well.
Similarly, future work could potentially accommodate function
prototypes with optimized-out parameters, similarly using
tagging to identify them. Again the kernel would have to
be aware of such tagging and handle it.
For now it is better to have an incomplete representation
that more accurately reflects the actual function parameters
used, removing inconsistencies that could otherwise do harm.
Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
---
btf_encoder.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++------------
dwarves.h | 1 +
2 files changed, 81 insertions(+), 20 deletions(-)
diff --git a/btf_encoder.c b/btf_encoder.c
index f36150e..44739a9 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -35,7 +35,6 @@
struct elf_function {
const char *name;
- bool generated;
size_t prefixlen;
};
@@ -708,6 +707,9 @@ int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder
int32_t i, id;
struct btf_var_secinfo *vsi;
+ /* saved functions are added to each encoder's BTF prior to it
+ * being merged with the parent encoder.
+ */
btf_encoder__add_saved_funcs(other);
if (encoder == other)
return 0;
@@ -795,11 +797,72 @@ static int function__compare(const void *a, const void *b)
return strcmp(function__name(fa), function__name(fb));
}
+#define BTF_ENCODER_MAX_PARAMETERS 12
+
struct btf_encoder_state {
struct btf_encoder *encoder;
uint32_t type_id_off;
+ bool got_parameter_names;
+ const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
};
+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)
+ break;
+ parameter_names[i++] = parameter__name(parameter);
+ }
+}
+
+static bool funcs__match(struct function *f1, struct function *f2)
+{
+
+ const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
+ struct btf_encoder_state *state = f1->priv;
+ const char *name = function__name(f1);
+ int i;
+
+ if (!state)
+ return false;
+
+ if (f1->proto.nr_parms != f2->proto.nr_parms) {
+ if (state->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 (!state->got_parameter_names) {
+ parameter_names__get(&f1->proto, BTF_ENCODER_MAX_PARAMETERS,
+ state->parameter_names);
+ 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 (!state->parameter_names[i]) {
+ if (!parameter_names[i])
+ continue;
+ } else if (parameter_names[i]) {
+ if (strcmp(state->parameter_names[i], parameter_names[i]) == 0)
+ continue;
+ }
+ if (state->encoder->verbose)
+ printf("function mismatch for '%s'(%s): parameter #%d '%s' != '%s'\n",
+ name, f1->alias ?: name, i,
+ state->parameter_names[i] ?: "<null>",
+ parameter_names[i] ?: "<null>");
+
+ return false;
+ }
+ return true;
+}
+
static void btf_encoder__merge_func(struct btf_encoder *encoder, struct function *fn)
{
struct function **nodep;
@@ -812,6 +875,9 @@ static void btf_encoder__merge_func(struct btf_encoder *encoder, struct function
*/
fn->proto.optimized_parms |= (*nodep)->proto.optimized_parms;
(*nodep)->proto.optimized_parms |= fn->proto.optimized_parms;
+ if ((fn->proto.inconsistent_proto || (*nodep)->proto.inconsistent_proto) ||
+ !funcs__match(fn, *nodep))
+ (*nodep)->proto.inconsistent_proto = fn->proto.inconsistent_proto = 1;
(*nodep)->proto.processed = 1;
}
@@ -822,19 +888,22 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
nodep = tsearch(fn, &encoder->saved_func_tree, function__compare);
if (nodep == NULL) {
- fprintf(stderr, "error: out of memory adding static function '%s'\n",
+ fprintf(stderr, "error: out of memory adding function '%s'\n",
name);
return -1;
}
/* 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 our tree entry.
+ * "seen optimized parameters" and inconsistent prototype
+ * status is reflected in our tree 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.
*/
if (*nodep != fn) {
(*nodep)->proto.optimized_parms |= fn->proto.optimized_parms;
+ if (!funcs__match(*nodep, fn))
+ (*nodep)->proto.inconsistent_proto = fn->proto.inconsistent_proto = 1;
} else {
struct btf_encoder_state *state = zalloc(sizeof(*state));
@@ -905,12 +974,14 @@ static void btf_encoder__add_saved_func(const void *nodep, const VISIT which,
btf_encoder__merge_func(other_encoder, fn);
}
- 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 {
btf_encoder__add_func(encoder, fn);
@@ -991,7 +1062,6 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
encoder->functions.suffix_cnt++;
encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
}
- encoder->functions.entries[encoder->functions.cnt].generated = false;
encoder->functions.cnt++;
return 0;
}
@@ -1779,8 +1849,6 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
}
cu__for_each_function(cu, core_id, fn) {
- bool save = false;
-
/*
* Skip functions that:
* - are marked as declarations
@@ -1802,11 +1870,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
/* prefer exact function name match... */
func = btf_encoder__find_function(encoder, name, 0);
- if (func) {
- if (func->generated)
- continue;
- func->generated = true;
- } else if (encoder->functions.suffix_cnt) {
+ if (!func && encoder->functions.suffix_cnt) {
/* falling back to name.isra.0 match if no exact
* match is found; only bother if we found any
* .suffix function names. The function
@@ -1817,7 +1881,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
func = btf_encoder__find_function(encoder, name,
strlen(name));
if (func) {
- save = true;
+ fn->alias = func->name;
if (encoder->verbose)
printf("matched function '%s' with '%s'%s\n",
name, func->name,
@@ -1827,16 +1891,12 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
}
if (!func)
continue;
- fn->alias = func->name;
} else {
if (!fn->external)
continue;
}
- if (save)
- err = btf_encoder__save_func(encoder, fn);
- else
- err = btf_encoder__add_func(encoder, fn);
+ err = btf_encoder__save_func(encoder, fn);
if (err)
goto out;
}
diff --git a/dwarves.h b/dwarves.h
index 64c7c56..ba94573 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -832,6 +832,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)
--
1.8.3.1
