Messages in this thread | | | Subject | Re: [PATCH] perf expr: Fix return value of ids__new | From | German Gomez <> | Date | Mon, 13 Dec 2021 13:13:47 +0000 |
| |
Hi Miaoqian,
Fails to build due to missing import: "#import <linux/err.h>".
Could you please verify?
Other than that, it looks good to me. I shared the testing below:
On 13/12/2021 07:09, Miaoqian Lin wrote: > callers of ids__new() function only do NULL checking for the return > value. ids__new() calles hashmap__new(), which may return > ERR_PTR(-ENOMEM). Instead of changing the checking one-by-one. > return NULL instead of ERR_PTR(-ENOMEM) to keep > consistent. > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > --- > tools/perf/util/expr.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c > index 1d532b9fed29..aabdc112300c 100644 > --- a/tools/perf/util/expr.c > +++ b/tools/perf/util/expr.c > @@ -65,7 +65,13 @@ static bool key_equal(const void *key1, const void *key2, > > struct hashmap *ids__new(void) > { > - return hashmap__new(key_hash, key_equal, NULL); > + struct hashmap *hash; > + > + hash = hashmap__new(key_hash, key_equal, NULL); > + if (IS_ERR(hash)) > + return NULL; > + else > + return hash; > } > > void ids__free(struct hashmap *ids) Before this patch, perf-test was segfaulting instead of a graceful fail. I think this could have been an issue in the perf tool as well.
(I forced hashmap__new in "tools/perf/util/hashmap.c" to always return the error for the purposes of the test).
$ make DEBUG=1 NO_LIBBPF=1 # builds with tools/perf/util/hashmap.c $ ./perf test 7 -v Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc 7: Simple expression parser : --- start --- test child forked, pid 1953536 perf: Segmentation fault Obtained 16 stack frames. ./perf(dump_stack+0x31) [0x559222b3ae48] ./perf(sighandler_dump_stack+0x33) [0x559222b3af30] /lib/x86_64-linux-gnu/libc.so.6(+0x4620f) [0x7ff81df6220f] ./perf(hashmap__size+0x23) [0x559222bf9f47] ./perf(ids__union+0x5f) [0x559222be1f0e] ./perf(+0x2d9ba8) [0x559222ac7ba8] ./perf(+0x2da223) [0x559222ac8223] ./perf(+0x2a5fe9) [0x559222a93fe9] ./perf(+0x2a6119) [0x559222a94119] ./perf(+0x2a6de7) [0x559222a94de7] ./perf(cmd_test+0x25f) [0x559222a95686] ./perf(+0x2e4d25) [0x559222ad2d25] ./perf(+0x2e4faa) [0x559222ad2faa] ./perf(+0x2e50fd) [0x559222ad30fd] ./perf(main+0x29d) [0x559222ad3500] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf2) [0x7ff81df430b2] test child interrupted ---- end ---- Simple expression parser: FAILED!
After the patch:
$ make DEBUG=1 NO_LIBBPF=1 $ ./perf test 7 -v Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc 7: Simple expression parser : --- start --- test child forked, pid 1960026 FAILED tests/expr.c:16 ids__new FAILED tests/expr.c:16 ids__new FAILED tests/expr.c:73 ids_union (-1 != 0) test child finished with -1 ---- end ----
Simple expression parser: FAILED!
so with the missing import fixex:
Tested-by: German Gomez <german.gomez@arm.com> Reviewed-by: German Gomez <german.gomez@arm.com>
| |