[PATCH v1 1/2] perf srcline: Make addr2line configuration failure more verbose

[PATCH v1 1/2] perf srcline: Make addr2line configuration failure more verbose

[Ian Rogers]

To aid debugging why it fails. Also, combine the loops for reading a
line for the llvm/binutils cases.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/srcline.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index b8e596528d7e..fc85cdd6c8f9 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -441,7 +441,7 @@ enum a2l_style {
 	LLVM,
 };
 
-static enum a2l_style addr2line_configure(struct child_process *a2l)
+static enum a2l_style addr2line_configure(struct child_process *a2l, const char *dso_name)
 {
 	static bool cached;
 	static enum a2l_style style;
@@ -450,6 +450,7 @@ static enum a2l_style addr2line_configure(struct child_process *a2l)
 		char buf[128];
 		struct io io;
 		int ch;
+		int lines;
 
 		if (write(a2l->in, ",\n", 2) != 2)
 			return BROKEN;
@@ -459,19 +460,29 @@ static enum a2l_style addr2line_configure(struct child_process *a2l)
 		if (ch == ',') {
 			style = LLVM;
 			cached = true;
+			lines = 1;
 		} else if (ch == '?') {
 			style = GNU_BINUTILS;
 			cached = true;
+			lines = 2;
 		} else {
-			style = BROKEN;
+			if (!symbol_conf.disable_add2line_warn) {
+				char *output;
+				size_t output_len;
+
+				io__getline(&io, &output, &output_len);
+				pr_warning("%s %s: addr2line configuration failed\n",
+					   __func__, dso_name);
+				pr_warning("\t%c%s\n", ch, output);
+			}
+			return BROKEN;
 		}
-		do {
+		while (lines) {
 			ch = io__get_char(&io);
-		} while (ch > 0 && ch != '\n');
-		if (style == GNU_BINUTILS) {
-			do {
-				ch = io__get_char(&io);
-			} while (ch > 0 && ch != '\n');
+			if (ch <= 0)
+				break;
+			if (ch == '\n')
+				lines--;
 		}
 		/* Ignore SIGPIPE in the event addr2line exits. */
 		signal(SIGPIPE, SIG_IGN);
@@ -591,12 +602,9 @@ static int addr2line(const char *dso_name, u64 addr,
 			pr_warning("%s %s: addr2line_subprocess_init failed\n", __func__, dso_name);
 		goto out;
 	}
-	a2l_style = addr2line_configure(a2l);
-	if (a2l_style == BROKEN) {
-		if (!symbol_conf.disable_add2line_warn)
-			pr_warning("%s: addr2line configuration failed\n", __func__);
+	a2l_style = addr2line_configure(a2l, dso_name);
+	if (a2l_style == BROKEN)
 		goto out;
-	}
 
 	/*
 	 * Send our request and then *deliberately* send something that can't be interpreted as
-- 
2.41.0.162.gfafddb0af9-goog

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

[Ian Rogers]

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@huawei.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

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@huawei.com>

Changbin,

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

Thanks,

- Arnaldo

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  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

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

[Ian Rogers]

On Mon, Jun 12, 2023 at 12:11 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> 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@huawei.com>
>
> Changbin,
>
>         Did this fix the problem you reported? If so can I have your
> Tested-by?

There are ongoing issues that Changbin has found, the thread is here:
https://lore.kernel.org/linux-perf-users/CAP-5=fWdvMVOiZDhqiz1R9hucz+6+ho_L3jBixqP3YLcJD7Lew@mail.gmail.com/

Thanks,
Ian

> Thanks,
>
> - Arnaldo
>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  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

Re: [PATCH v1 1/2] perf srcline: Make addr2line configuration failure more verbose

[Changbin Du]

On Fri, Jun 09, 2023 at 04:54:18PM -0700, Ian Rogers wrote:
> To aid debugging why it fails. Also, combine the loops for reading a
> line for the llvm/binutils cases.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/srcline.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index b8e596528d7e..fc85cdd6c8f9 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -441,7 +441,7 @@ enum a2l_style {
>  	LLVM,
>  };
>  
> -static enum a2l_style addr2line_configure(struct child_process *a2l)
> +static enum a2l_style addr2line_configure(struct child_process *a2l, const char *dso_name)
>  {
>  	static bool cached;
>  	static enum a2l_style style;
> @@ -450,6 +450,7 @@ static enum a2l_style addr2line_configure(struct child_process *a2l)
>  		char buf[128];
>  		struct io io;
>  		int ch;
> +		int lines;
>  
>  		if (write(a2l->in, ",\n", 2) != 2)
>  			return BROKEN;
> @@ -459,19 +460,29 @@ static enum a2l_style addr2line_configure(struct child_process *a2l)
>  		if (ch == ',') {
>  			style = LLVM;
>  			cached = true;
> +			lines = 1;
>  		} else if (ch == '?') {
>  			style = GNU_BINUTILS;
>  			cached = true;
> +			lines = 2;
>  		} else {
> -			style = BROKEN;
> +			if (!symbol_conf.disable_add2line_warn) {
> +				char *output;
This 'output' should be initialized to NULL.

In file included from util/srcline.c:13:
util/srcline.c: In function ‘addr2line’:
/work/linux/tools/perf/libapi/include/api/io.h:130:2: error: ‘output’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  130 |  free(*line_out);
      |  ^~~~~~~~~~~~~~~
util/srcline.c:472:11: note: ‘output’ was declared here
  472 |     char *output;
      |           ^~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/work/linux/tools/build/Makefile.build:97: util/srcline.o] Error 1


> +				size_t output_len;
> +
> +				io__getline(&io, &output, &output_len);
> +				pr_warning("%s %s: addr2line configuration failed\n",
> +					   __func__, dso_name);
> +				pr_warning("\t%c%s\n", ch, output);
> +			}
> +			return BROKEN;
>  		}
> -		do {
> +		while (lines) {
>  			ch = io__get_char(&io);
> -		} while (ch > 0 && ch != '\n');
> -		if (style == GNU_BINUTILS) {
> -			do {
> -				ch = io__get_char(&io);
> -			} while (ch > 0 && ch != '\n');
> +			if (ch <= 0)
> +				break;
> +			if (ch == '\n')
> +				lines--;
>  		}
>  		/* Ignore SIGPIPE in the event addr2line exits. */
>  		signal(SIGPIPE, SIG_IGN);
> @@ -591,12 +602,9 @@ static int addr2line(const char *dso_name, u64 addr,
>  			pr_warning("%s %s: addr2line_subprocess_init failed\n", __func__, dso_name);
>  		goto out;
>  	}
> -	a2l_style = addr2line_configure(a2l);
> -	if (a2l_style == BROKEN) {
> -		if (!symbol_conf.disable_add2line_warn)
> -			pr_warning("%s: addr2line configuration failed\n", __func__);
> +	a2l_style = addr2line_configure(a2l, dso_name);
> +	if (a2l_style == BROKEN)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Send our request and then *deliberately* send something that can't be interpreted as
> -- 
> 2.41.0.162.gfafddb0af9-goog
> 

-- 
Cheers,
Changbin Du