To aid debugging why it fails. Also, combine the loops for reading a
line for the llvm/binutils cases.
Signed-off-by: Ian Rogers <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Ian Rogers <[email protected]>
---
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
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 <[email protected]>
Changbin,
Did this fix the problem you reported? If so can I have your
Tested-by?
Thanks,
- Arnaldo
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> 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
On Mon, Jun 12, 2023 at 12:11 PM Arnaldo Carvalho de Melo
<[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> ---
> 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