Re: [PATCH] perf expr: Fix return value of ids__new

[German Gomez <german.gomez@arm.com>]

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>