Re: [PATCH v1 2/2] perf srcline: Make sentinel reading for binutils addr2line more robust

[Arnaldo Carvalho de Melo]

Em Fri, Jun 09, 2023 at 04:54:19PM -0700, Ian Rogers escreveu:
> The addr2line process is sent an address then multiple function,
> filename:line "records" are read. To detect the end of output a ',' is
> sent and for llvm-addr2line a ',' is then read back showing the end of
> addrline's output. For binutils addr2line the ',' translates to
> address 0 and we expect the bogus filename marker "??:0" (see
> filename_split) to be sent from addr2line. For some kernels address 0
> may have a mapping and so a seemingly valid inline output is given and
> breaking the sentinel discovery:
> 
> ```
> $ addr2line -e vmlinux -f -i
> ,
> __per_cpu_start
> ./arch/x86/kernel/cpu/common.c:1850
> ```
> 
> To avoid this problem enable the address dumping for addr2line (the -a
> option). If an address of 0x0000000000000000 is read then this is the
> sentinel value working around the problem above. The filename_split
> still needs to check for "??:0" as bogus non-zero addresses also need
> handling.
> 
> Reported-by: Changbin Du <changbin.du@xxxxxxxxxx>

Changbin,

	Did this fix the problem you reported? If so can I have your
Tested-by?

Thanks,

- Arnaldo

> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/util/srcline.c | 61 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index fc85cdd6c8f9..c99a001453b4 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -406,7 +406,7 @@ static struct child_process *addr2line_subprocess_init(const char *addr2line_pat
>  	const char *argv[] = {
>  		addr2line_path ?: "addr2line",
>  		"-e", binary_path,
> -		"-i", "-f", NULL
> +		"-a", "-i", "-f", NULL
>  	};
>  	struct child_process *a2l = zalloc(sizeof(*a2l));
>  	int start_command_status = 0;
> @@ -461,10 +461,10 @@ static enum a2l_style addr2line_configure(struct child_process *a2l, const char
>  			style = LLVM;
>  			cached = true;
>  			lines = 1;
> -		} else if (ch == '?') {
> +		} else if (ch == '0') {
>  			style = GNU_BINUTILS;
>  			cached = true;
> -			lines = 2;
> +			lines = 3;
>  		} else {
>  			if (!symbol_conf.disable_add2line_warn) {
>  				char *output;
> @@ -516,20 +516,64 @@ static int read_addr2line_record(struct io *io,
>  	if (line_nr != NULL)
>  		*line_nr = 0;
>  
> +	/*
> +	 * Read the first line. Without an error this will be either an address
> +	 * like 0x1234 or for llvm-addr2line the sentinal ',' character.
> +	 */
>  	if (io__getline(io, &line, &line_len) < 0 || !line_len)
>  		goto error;
>  
> -	if (style == LLVM && line_len == 2 && line[0] == ',') {
> -		zfree(&line);
> -		return 0;
> +	if (style == LLVM) {
> +		if (line_len == 2 && line[0] == ',') {
> +			zfree(&line);
> +			return 0;
> +		}
> +	} else {
> +		int zero_count = 0, non_zero_count = 0;
> +
> +		/* The address should always start 0x. */
> +		if (line_len < 2 || line[0] != '0' || line[1] != 'x')
> +			goto error;
> +
> +		for (size_t i = 2; i < line_len; i++) {
> +			if (line[i] == '0')
> +				zero_count++;
> +			else
> +				non_zero_count++;
> +		}
> +		if (!non_zero_count) {
> +			int ch;
> +
> +			if (!zero_count) {
> +				/* Line was erroneous just '0x'. */
> +				goto error;
> +			}
> +			/*
> +			 * Line was 0x0..0, the sentinel for binutils. Remove
> +			 * the function and filename lines.
> +			 */
> +			zfree(&line);
> +			do {
> +				ch = io__get_char(io);
> +			} while (ch > 0 && ch != '\n');
> +			do {
> +				ch = io__get_char(io);
> +			} while (ch > 0 && ch != '\n');
> +			return 0;
> +		}
>  	}
>  
> +	/* Read the second function name line. */
> +	if (io__getline(io, &line, &line_len) < 0 || !line_len)
> +		goto error;
> +
>  	if (function != NULL)
>  		*function = strdup(strim(line));
>  
>  	zfree(&line);
>  	line_len = 0;
>  
> +	/* Read the third filename and line number line. */
>  	if (io__getline(io, &line, &line_len) < 0 || !line_len)
>  		goto error;
>  
> @@ -633,8 +677,9 @@ static int addr2line(const char *dso_name, u64 addr,
>  		goto out;
>  	case 0:
>  		/*
> -		 * The first record was invalid, so return failure, but first read another
> -		 * record, since we asked a junk question and have to clear the answer out.
> +		 * The first record was invalid, so return failure, but first
> +		 * read another record, since we sent a sentinel ',' for the
> +		 * sake of detected the last inlined function.
>  		 */
>  		switch (read_addr2line_record(&io, a2l_style, NULL, NULL, NULL)) {
>  		case -1:
> -- 
> 2.41.0.162.gfafddb0af9-goog
> 

-- 

- Arnaldo