Re: [PATCH 2/2] perf probe: Fix the incorrect line number display in 'perf probe -l'

[Li Huafei <lihuafei1@huawei.com>]

Hi Masami,

On 2024/11/11 20:05, Masami Hiramatsu (Google) wrote:
> Hi Li,
> 
> On Mon, 11 Nov 2024 17:05:49 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
>> Currently debuginfo__find_probe_point() does
>>
>>  (1) Get the line and file from CU's lineinfo
>>  (2) Get the real function(function instance) of the address
>>      (use this function's decl_line/decl_file as basement)
>>  (2-1) Search the inlined function scope in the real function
>>      for the given address.
>>  (2-2) if there is inlined function, update basement line/file.
>>  (2-3) verify the filename is same as basement filename.
>>  (3) calculate the relative line number from basement.
>>
>> The problem is in (1). Since we have no basement file/line info,
>> we can not verify that the file/line info from CU's lineinfo.
>> As Li shown above, the lineinfo may have several different lines
>> for one address. We need to find most appropriate one based on
>> the basement file/line.
>>
>> Thus what we need are
>>
>>  - Introduce cu_find_lineinfo_at() which gives basement file/line
>>    information so that it can choose correct one. (hopefully)
>>  - Swap the order of (1) and (2*) so that we can pass the basement
>>    file/line when searching lineinfo. (Also, (2-3) should be right
>>    before (3))
>>
> 
> Can you check below change fixes your issue?
> 

Thank you for the detailed explanation in your previous email. I tested
your patch, and the results are as follows:

  # perf probe -l
    probe:schedule       (on schedule:5@kernel/sched/core.c)

Is this not as expected? Should our expectation be:

  probe:schedule       (on schedule:6780@kernel/sched/core.c)

Thanks,
Huafei

> Thank you,
> 
>>From 3027c4d3c8be874a7c84a98e73fe0837fd135129 Mon Sep 17 00:00:00 2001
> From: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
> Date: Mon, 11 Nov 2024 20:56:00 +0900
> Subject: [PATCH] perf-probe: Fix --line option to show correct offset line
>  number from function
> 
> Fix --line option to show correct offset if the DWARF line info of the
> probe address has the statement lines in differnt functions.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  tools/perf/util/dwarf-aux.c    | 78 ++++++++++++++++++++++++++++------
>  tools/perf/util/dwarf-aux.h    |  7 +++
>  tools/perf/util/probe-finder.c | 44 +++++++++++++------
>  3 files changed, 105 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 92eb9c8dc3e5..0bffec6962b8 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -60,14 +60,33 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die)
>  	return dwarf_formstring(&attr);
>  }
>  
> +static Dwarf_Line *get_next_statement_line(Dwarf_Lines *lines, size_t *idx, Dwarf_Addr addr)
> +{
> +	bool is_statement = false;
> +	Dwarf_Line *line;
> +	Dwarf_Addr laddr;
> +	size_t l = *idx;
> +
> +	while (!is_statement) {
> +		line = dwarf_onesrcline(lines, ++l);
> +		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
> +			dwarf_linebeginstatement(line, &is_statement) != 0)
> +				return NULL;
> +		if (laddr > addr)
> +			return NULL;
> +	}
> +	*idx = l;
> +	return line;
> +}
> +
>  /* Unlike dwarf_getsrc_die(), cu_getsrc_die() only returns statement line */
> -static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
> +static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr,
> +								 Dwarf_Lines **lines_p, size_t *idx)
>  {
>  	Dwarf_Addr laddr;
>  	Dwarf_Lines *lines;
>  	Dwarf_Line *line;
>  	size_t nlines, l, u, n;
> -	bool flag;
>  
>  	if (dwarf_getsrclines(cu_die, &lines, &nlines) != 0 ||
>  	    nlines == 0)
> @@ -91,16 +110,14 @@ static Dwarf_Line *cu_getsrc_die(Dwarf_Die *cu_die, Dwarf_Addr addr)
>  		if (!line || dwarf_lineaddr(line, &laddr) != 0)
>  			return NULL;
>  	} while (laddr == addr);
> -	l++;
>  	/* Going forward to find the statement line */
> -	do {
> -		line = dwarf_onesrcline(lines, l++);
> -		if (!line || dwarf_lineaddr(line, &laddr) != 0 ||
> -		    dwarf_linebeginstatement(line, &flag) != 0)
> -			return NULL;
> -		if (laddr > addr)
> -			return NULL;
> -	} while (!flag);
> +	line = get_next_statement_line(lines, &l, addr);
> +	if (!line)
> +		return NULL;
> +	if (lines_p)
> +		*lines_p = lines;
> +	if (idx)
> +		*idx = l;
>  
>  	return line;
>  }
> @@ -129,7 +146,7 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
>  		goto out;
>  	}
>  
> -	line = cu_getsrc_die(cu_die, addr);
> +	line = cu_getsrc_die(cu_die, addr, NULL, NULL);
>  	if (line && dwarf_lineno(line, lineno) == 0) {
>  		*fname = dwarf_linesrc(line, NULL, NULL);
>  		if (!*fname)
> @@ -141,6 +158,43 @@ int cu_find_lineinfo(Dwarf_Die *cu_die, Dwarf_Addr addr,
>  	return (*lineno && *fname) ? *lineno : -ENOENT;
>  }
>  
> +/**
> + * cu_find_lineinfo_after - Get a line number after file:line for given address
> + * @cu_die: a CU DIE
> + * @addr: An address
> + * @lineno: a pointer which returns the line number
> + * @fname: the filename where searching the line
> + * @baseline: the basement line number
> + *
> + * Find a line number after @baseline in @fname for @addr in @cu_die.
> + * Return the found line number, or -ENOENT if not found.
> + */
> +int cu_find_lineinfo_after(Dwarf_Die *cu_die, Dwarf_Addr addr,
> +					int *lineno, const char *fname, int baseline)
> +{
> +	const char *line_fname;
> +	Dwarf_Lines *lines;
> +	Dwarf_Line *line;
> +	size_t idx = 0;
> +
> +	if (cu_find_lineinfo(cu_die, addr, &line_fname, lineno) < 0)
> +		return -ENOENT;
> +
> +	if (!strcmp(line_fname, fname) && baseline <= *lineno)
> +		return *lineno;
> +
> +	line = cu_getsrc_die(cu_die, addr, &lines, &idx);
> +
> +	while (line && dwarf_lineno(line, lineno) == 0) {
> +		line_fname = dwarf_linesrc(line, NULL, NULL);
> +		if (line_fname && !strcmp(line_fname, fname) && baseline <= *lineno)
> +			return *lineno;
> +		line = get_next_statement_line(lines, &idx, addr);
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  static int __die_find_inline_cb(Dwarf_Die *die_mem, void *data);
>  
>  /**
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index bd7505812569..19edf21e2f78 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -23,6 +23,13 @@ const char *cu_get_comp_dir(Dwarf_Die *cu_die);
>  int cu_find_lineinfo(Dwarf_Die *cudie, Dwarf_Addr addr,
>  		     const char **fname, int *lineno);
>  
> +/*
> + * Get the most likely line number for given address in given filename
> + * and basement line number.
> + */
> +int cu_find_lineinfo_after(Dwarf_Die *cudie, Dwarf_Addr addr,
> +					int *lineno, const char *fname, int baseline);
> +
>  /* Walk on functions at given address */
>  int cu_walk_functions_at(Dwarf_Die *cu_die, Dwarf_Addr addr,
>  			 int (*callback)(Dwarf_Die *, void *), void *data);
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 13ff45d3d6a4..efcacb5568e5 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1578,7 +1578,8 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  {
>  	Dwarf_Die cudie, spdie, indie;
>  	Dwarf_Addr _addr = 0, baseaddr = 0;
> -	const char *fname = NULL, *func = NULL, *basefunc = NULL, *tmp;
> +	const char *fname = NULL, *func = NULL, *basefunc = NULL;
> +	const char *basefname = NULL, *tmp;
>  	int baseline = 0, lineno = 0, ret = 0;
>  
>  	/* We always need to relocate the address for aranges */
> @@ -1592,11 +1593,7 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  		goto end;
>  	}
>  
> -	/* Find a corresponding line (filename and lineno) */
> -	cu_find_lineinfo(&cudie, (Dwarf_Addr)addr, &fname, &lineno);
> -	/* Don't care whether it failed or not */
> -
> -	/* Find a corresponding function (name, baseline and baseaddr) */
> +	/* Find the basement function (name, baseline and baseaddr) */
>  	if (die_find_realfunc(&cudie, (Dwarf_Addr)addr, &spdie)) {
>  		/* Get function entry information */
>  		func = basefunc = dwarf_diename(&spdie);
> @@ -1607,10 +1604,16 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  			goto post;
>  		}
>  
> -		fname = die_get_decl_file(&spdie);
> +		basefname = die_get_decl_file(&spdie);
> +		if (!basefname) {
> +			lineno = 0;
> +			goto post;
> +		}
> +
>  		if (addr == baseaddr) {
>  			/* Function entry - Relative line number is 0 */
>  			lineno = baseline;
> +			fname = basefname;
>  			goto post;
>  		}
>  
> @@ -1627,7 +1630,9 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  				 */
>  				lineno = die_get_call_lineno(&indie);
>  				fname = die_get_call_file(&indie);
> -				break;
> +				if (!fname || strcmp(fname, basefname))
> +					lineno = 0;
> +				goto post;
>  			} else {
>  				/*
>  				 * addr is in an inline function body.
> @@ -1636,20 +1641,35 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, u64 addr,
>  				 * be the entry line of the inline function.
>  				 */
>  				tmp = dwarf_diename(&indie);
> -				if (!tmp ||
> -				    dwarf_decl_line(&indie, &baseline) != 0)
> -					break;
> +				basefname = die_get_decl_file(&indie);
> +				if (!tmp || !basefname ||
> +				    dwarf_decl_line(&indie, &baseline) != 0) {
> +					lineno = 0;
> +					goto post;
> +				}
>  				func = tmp;
>  				spdie = indie;
>  			}
>  		}
> +	}
> +
> +	if (!lineno) {
> +		/* Find a corresponding line (filename and lineno) */
> +		if (cu_find_lineinfo_after(&cudie, (Dwarf_Addr)addr, &lineno,
> +								   basefname, baseline) < 0)
> +			lineno = 0;
> +		else
> +			fname = basefname;
> +	}
> +
> +post:
> +	if (lineno) {
>  		/* Verify the lineno and baseline are in a same file */
>  		tmp = die_get_decl_file(&spdie);
>  		if (!tmp || (fname && strcmp(tmp, fname) != 0))
>  			lineno = 0;
>  	}
>  
> -post:
>  	/* Make a relative line number or an offset */
>  	if (lineno)
>  		ppt->line = lineno - baseline;
>