Re: [PATCH v4 3/6] perf libdw: Fix libdw API contract violations

[Namhyung Kim <namhyung@kernel.org>]

On Sun, May 03, 2026 at 10:10:29AM -0700, Ian Rogers wrote:
> Check return values of `dwfl_report_end` and `dwfl_module_addrdie`.
> Check `die_get_call_lineno` for errors.
> 
> Additionally:
>  - Introduce `inline_node__clear_frames()` to clean up partial
>    allocations.

Sounds like it should be a separate commit.


>  - Ensure `*file` is freed and inline frames are cleared on error in
>    `libdw__addr2line()` to prevent memory leaks and duplicated
>    callchains when falling back to other unwinders.
>  - Allow DWARF line 0 in `libdw_a2l_cb()`, as it is a valid
>    reference for compiler-generated code.
>  - Fix the parent srcline lookup in `libdw_a2l_cb()` to target the
>    correct parent node depending on the callchain order
>    (ORDER_CALLER/ORDER_CALLEE).

Also it looks like an independent fix.  While you could put other fixes
into separate commits (up to you), we can merge simple things.

> 
> Fixes: b7a2b011e962 ("perf powerpc: Unify the skip-callchain-idx libdw with that for addr2line")
> Fixes: 88c51002d06f ("perf addr2line: Add a libdw implementation")
> Assisted-by: Gemini-CLI:Google Gemini 3
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> v4:
>  - Fix memory leaks and duplicated callchains on unwinding errors.
>  - Support DWARF line 0 in inline list.
>  - Fix callchain parent update in ORDER_CALLER mode.
> ---
>  tools/perf/util/libdw.c   | 84 +++++++++++++++++++++++++++++++--------
>  tools/perf/util/srcline.c |  9 ++++-
>  tools/perf/util/srcline.h |  1 +
>  3 files changed, 77 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/util/libdw.c b/tools/perf/util/libdw.c
> index 216977884103..e69ff4cea856 100644
> --- a/tools/perf/util/libdw.c
> +++ b/tools/perf/util/libdw.c
> @@ -4,6 +4,7 @@
>  #include "srcline.h"
>  #include "symbol.h"
>  #include "dwarf-aux.h"
> +#include "callchain.h"
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <elfutils/libdwfl.h>
> @@ -60,7 +61,11 @@ struct Dwfl *dso__libdw_dwfl(struct dso *dso)
>  		return NULL;
>  	}
>  
> -	dwfl_report_end(dwfl, /*removed=*/NULL, /*arg=*/NULL);
> +	if (dwfl_report_end(dwfl, NULL, NULL) != 0) {

Better to keep the comments?


> +		dwfl_end(dwfl);
> +		return NULL;
> +	}
> +
>  	dso__set_libdw(dso, dwfl);
>  
>  	return dwfl;
> @@ -72,41 +77,70 @@ struct libdw_a2l_cb_args {
>  	struct inline_node *node;
>  	char *leaf_srcline;
>  	bool leaf_srcline_used;
> +	int err;
>  };
>  
>  static int libdw_a2l_cb(Dwarf_Die *die, void *_args)
>  {
>  	struct libdw_a2l_cb_args *args  = _args;
> -	struct symbol *inline_sym = new_inline_sym(args->dso, args->sym, dwarf_diename(die));
> +	const char *name = dwarf_diename(die);
> +	struct symbol *inline_sym = new_inline_sym(args->dso, args->sym, name ?: "unknown");

Use die_getname() here?


>  	const char *call_fname = die_get_call_file(die);
> +	int call_lineno = die_get_call_lineno(die);
>  	char *call_srcline = srcline__unknown;
> -	struct inline_list *ilist;
>  
> -	if (!inline_sym)
> -		return -ENOMEM;
> +	if (!inline_sym) {
> +		args->err = -ENOMEM;
> +		return DWARF_CB_ABORT;
> +	}
>  
>  	/* Assign caller information to the parent. */
> -	if (call_fname)
> -		call_srcline = srcline_from_fileline(call_fname, die_get_call_lineno(die));
> +	if (call_fname && call_lineno >= 0)
> +		call_srcline = srcline_from_fileline(call_fname, call_lineno);
>  
> -	list_for_each_entry(ilist, &args->node->val, list) {
> -		if (args->leaf_srcline == ilist->srcline)
> +	if (!list_empty(&args->node->val)) {
> +		struct inline_list *parent;
> +
> +		if (callchain_param.order == ORDER_CALLEE)
> +			parent = list_first_entry(&args->node->val, struct inline_list, list);
> +		else
> +			parent = list_last_entry(&args->node->val, struct inline_list, list);
> +
> +		if (args->leaf_srcline == parent->srcline)
>  			args->leaf_srcline_used = false;
> -		else if (ilist->srcline != srcline__unknown)
> -			free(ilist->srcline);
> -		ilist->srcline =  call_srcline;
> +		else if (parent->srcline != srcline__unknown)
> +			free(parent->srcline);
> +		parent->srcline = call_srcline;
>  		call_srcline = NULL;
> -		break;
>  	}
>  	if (call_srcline && call_srcline != srcline__unknown)
>  		free(call_srcline);
>  
>  	/* Add this symbol to the chain as the leaf. */
>  	if (!args->leaf_srcline_used) {
> -		inline_list__append_tail(inline_sym, args->leaf_srcline, args->node);
> +		if (inline_list__append_tail(inline_sym, args->leaf_srcline, args->node) != 0) {
> +			args->err = -ENOMEM;
> +			if (inline_sym->inlined)
> +				symbol__delete(inline_sym);
> +			return DWARF_CB_ABORT;
> +		}
>  		args->leaf_srcline_used = true;
>  	} else {
> -		inline_list__append_tail(inline_sym, strdup(args->leaf_srcline), args->node);
> +		char *srcline = strdup(args->leaf_srcline);
> +
> +		if (!srcline) {
> +			args->err = -ENOMEM;
> +			if (inline_sym->inlined)
> +				symbol__delete(inline_sym);
> +			return DWARF_CB_ABORT;
> +		}
> +		if (inline_list__append_tail(inline_sym, srcline, args->node) != 0) {
> +			free(srcline);
> +			args->err = -ENOMEM;
> +			if (inline_sym->inlined)
> +				symbol__delete(inline_sym);
> +			return DWARF_CB_ABORT;

Looks like the pattern is repeated.  My rule is to factor it out when
it repeats more than twice. :)


> +		}
>  	}
>  	return 0;
>  }
> @@ -162,11 +196,29 @@ int libdw__addr2line(u64 addr, char **file, unsigned int *line_nr,
>  			.leaf_srcline = srcline_from_fileline(src ?: "<unknown>", lineno),
>  		};
>  
> +		if (!args.leaf_srcline) {
> +			if (file && *file) {
> +				free(*file);
> +				*file = NULL;
> +			}
> +			return 0;
> +		}
> +
>  		/* Walk from the parent down to the leaf. */
> -		cu_walk_functions_at(cudie, addr, libdw_a2l_cb, &args);
> +		if (cudie)
> +			cu_walk_functions_at(cudie, addr, libdw_a2l_cb, &args);
>  
>  		if (!args.leaf_srcline_used)
>  			free(args.leaf_srcline);
> +
> +		if (args.err) {
> +			if (file && *file) {
> +				free(*file);
> +				*file = NULL;
> +			}
> +			inline_node__clear_frames(node);
> +			return 0;
> +		}
>  	}
>  	return 1;
>  }
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index db164d258163..62884428fb5a 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -429,10 +429,13 @@ struct inline_node *dso__parse_addr_inlines(struct dso *dso, u64 addr,
>  	return addr2inlines(dso_name, addr, dso, sym);
>  }
>  
> -void inline_node__delete(struct inline_node *node)
> +void inline_node__clear_frames(struct inline_node *node)
>  {
>  	struct inline_list *ilist, *tmp;
>  
> +	if (node == NULL)
> +		return;
> +
>  	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
>  		list_del_init(&ilist->list);
>  		zfree_srcline(&ilist->srcline);
> @@ -441,7 +444,11 @@ void inline_node__delete(struct inline_node *node)
>  			symbol__delete(ilist->symbol);
>  		free(ilist);
>  	}
> +}
>  
> +void inline_node__delete(struct inline_node *node)
> +{
> +	inline_node__clear_frames(node);
>  	free(node);
>  }
>  
> diff --git a/tools/perf/util/srcline.h b/tools/perf/util/srcline.h
> index 7c37b3bf9ce7..1018cbc886d6 100644
> --- a/tools/perf/util/srcline.h
> +++ b/tools/perf/util/srcline.h
> @@ -47,6 +47,7 @@ struct inline_node *dso__parse_addr_inlines(struct dso *dso, u64 addr,
>  					    struct symbol *sym);
>  /* free resources associated to the inline node list */
>  void inline_node__delete(struct inline_node *node);
> +void inline_node__clear_frames(struct inline_node *node);
>  
>  /* insert the inline node list into the DSO, which will take ownership */
>  void inlines__tree_insert(struct rb_root_cached *tree,
> -- 
> 2.54.0.545.g6539524ca2-goog
>