The addr2line command is started and then addresses piped to it. In
order to determine the end of a addr2lines output a ',' it output with
an expectation to get '??\n??:0\n' as a reply. llvm-addr2line differs
in that ',' generates a reply of ','.
The approach detects and then caches the addr2line style. When records
are read the sentinel is detected appropriately.
Comparing the output there is a little more inline data on my machine
with llvm-addr2line:
$ sudo perf record -a -g sleep 1
$ sudo perf report --addr2line=addr2line > a.txt
$ sudo perf report --addr2line=llvm-addr2line > b.txt
$ wc -l a.txt b.txt
12386 a.txt
12477 b.txt
Some other small changes, switching to the api/io code to avoid file
streams wrapping the command's stdin/stdout. Ignore SIGPIPE for when
addr2line exits and writes fail.
Ian Rogers (4):
tools api: Add io__getline
perf srcline: Simplify addr2line subprocess
perf srcline: Support for llvm-addr2line
perf srcline: Avoid addr2line SIGPIPEs
tools/lib/api/io.h | 40 ++++++++++
tools/perf/tests/api-io.c | 36 +++++++++
tools/perf/util/srcline.c | 162 +++++++++++++++++++++++---------------
3 files changed, 174 insertions(+), 64 deletions(-)
--
2.40.0.348.gf938b09366-goog
Reads a line to allocated memory up to a newline following the getline
API.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/lib/api/io.h | 40 +++++++++++++++++++++++++++++++++++++++
tools/perf/tests/api-io.c | 36 +++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+)
diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
index 777c20f6b604..d874e8fa8b07 100644
--- a/tools/lib/api/io.h
+++ b/tools/lib/api/io.h
@@ -7,7 +7,9 @@
#ifndef __API_IO__
#define __API_IO__
+#include <errno.h>
#include <stdlib.h>
+#include <string.h>
#include <unistd.h>
struct io {
@@ -112,4 +114,42 @@ static inline int io__get_dec(struct io *io, __u64 *dec)
}
}
+/* Read up to and including the first newline following the pattern of getline. */
+static inline ssize_t io__getline(char **line_out, size_t *line_len_out, struct io *io)
+{
+ char buf[128];
+ int buf_pos = 0;
+ char *line = NULL;
+ size_t line_len = 0;
+ int ch = 0;
+
+ /* TODO: reuse previously allocated memory. */
+ free(*line_out);
+ while (ch != '\n') {
+ ch = io__get_char(io);
+
+ if (ch < 0)
+ break;
+
+ if (buf_pos == sizeof(buf)) {
+ line = realloc(line, line_len + sizeof(buf));
+ if (!line)
+ return -ENOMEM;
+ memcpy(&line[line_len], buf, sizeof(buf));
+ line_len += sizeof(buf);
+ buf_pos = 0;
+ }
+ buf[buf_pos++] = (char)ch;
+ }
+ line = realloc(line, line_len + buf_pos + 1);
+ if (!line)
+ return -ENOMEM;
+ memcpy(&line[line_len], buf, buf_pos);
+ line[line_len + buf_pos] = '\0';
+ line_len += buf_pos;
+ *line_out = line;
+ *line_len_out = line_len;
+ return line_len;
+}
+
#endif /* __API_IO__ */
diff --git a/tools/perf/tests/api-io.c b/tools/perf/tests/api-io.c
index e91cf2c127f1..0ff39cdfcb01 100644
--- a/tools/perf/tests/api-io.c
+++ b/tools/perf/tests/api-io.c
@@ -289,6 +289,40 @@ static int test_get_dec(void)
return ret;
}
+static int test_get_line(void)
+{
+ char path[PATH_MAX];
+ struct io io;
+ char test_string[1024];
+ char *line = NULL;
+ size_t i, line_len = 0;
+ size_t buf_size = 128;
+ int ret = 0;
+
+ for (i = 0; i < 512; i++)
+ test_string[i] = 'a';
+ test_string[512] = '\n';
+ for (i = 513; i < 1023; i++)
+ test_string[i] = 'b';
+ test_string[1023] = '\0';
+
+ if (setup_test(path, test_string, buf_size, &io))
+ return -1;
+
+ EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 513);
+ EXPECT_EQUAL((int)strlen(line), 513);
+ for (i = 0; i < 512; i++)
+ EXPECT_EQUAL(line[i], 'a');
+ EXPECT_EQUAL(line[512], '\n');
+ EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 510);
+ for (i = 0; i < 510; i++)
+ EXPECT_EQUAL(line[i], 'b');
+
+ free(line);
+ cleanup_test(path, &io);
+ return ret;
+}
+
static int test__api_io(struct test_suite *test __maybe_unused,
int subtest __maybe_unused)
{
@@ -300,6 +334,8 @@ static int test__api_io(struct test_suite *test __maybe_unused,
ret = TEST_FAIL;
if (test_get_dec())
ret = TEST_FAIL;
+ if (test_get_line())
+ ret = TEST_FAIL;
return ret;
}
--
2.40.0.348.gf938b09366-goog
Ignore SIGPIPEs when addr2line is configured.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/srcline.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 423ddf3967b0..bc3e48afe054 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -473,6 +473,8 @@ static enum a2l_style addr2line_configure(struct child_process *a2l)
ch = io__get_char(&io);
} while (ch > 0 && ch != '\n');
}
+ /* Ignore SIGPIPE in the event addr2line exits. */
+ signal(SIGPIPE, SIG_IGN);
}
return style;
}
--
2.40.0.348.gf938b09366-goog
The sentinel value differs for llvm-addr2line. Configure this once and
then detect when reading records.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/srcline.c | 65 +++++++++++++++++++++++++++++++++++++--
1 file changed, 62 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 2da86e57215d..423ddf3967b0 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -435,7 +435,50 @@ static struct child_process *addr2line_subprocess_init(const char *addr2line_pat
return a2l;
}
+enum a2l_style {
+ BROKEN,
+ GNU_BINUTILS,
+ LLVM,
+};
+
+static enum a2l_style addr2line_configure(struct child_process *a2l)
+{
+ static bool cached;
+ static enum a2l_style style;
+
+ if (!cached) {
+ char buf[128];
+ struct io io;
+ int ch;
+
+ if (write(a2l->in, ",\n", 2) != 2)
+ return BROKEN;
+
+ io__init(&io, a2l->out, buf, sizeof(buf));
+ ch = io__get_char(&io);
+ if (ch == ',') {
+ style = LLVM;
+ cached = true;
+ } else if (ch == '?') {
+ style = GNU_BINUTILS;
+ cached = true;
+ } else {
+ style = BROKEN;
+ }
+ do {
+ 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');
+ }
+ }
+ return style;
+}
+
static int read_addr2line_record(struct io *io,
+ enum a2l_style style,
char **function,
char **filename,
unsigned int *line_nr)
@@ -462,6 +505,12 @@ static int read_addr2line_record(struct io *io,
if (io__getline(&line, &line_len, io) < 0 || !line_len)
goto error;
+
+ if (style == LLVM && line_len == 2 && line[0] == ',') {
+ zfree(&line);
+ return 0;
+ }
+
if (function != NULL)
*function = strdup(strim(line));
@@ -471,7 +520,8 @@ static int read_addr2line_record(struct io *io,
if (io__getline(&line, &line_len, io) < 0 || !line_len)
goto error;
- if (filename_split(line, line_nr == NULL ? &dummy_line_nr : line_nr) == 0) {
+ if (filename_split(line, line_nr == NULL ? &dummy_line_nr : line_nr) == 0 &&
+ style == GNU_BINUTILS) {
ret = 0;
goto error;
}
@@ -523,6 +573,7 @@ static int addr2line(const char *dso_name, u64 addr,
char buf[128];
ssize_t written;
struct io io;
+ enum a2l_style a2l_style;
if (!a2l) {
if (!filename__has_section(dso_name, ".debug_line"))
@@ -538,6 +589,12 @@ 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__);
+ goto out;
+ }
/*
* Send our request and then *deliberately* send something that can't be interpreted as
@@ -557,7 +614,8 @@ static int addr2line(const char *dso_name, u64 addr,
}
io__init(&io, a2l->out, buf, sizeof(buf));
- switch (read_addr2line_record(&io, &record_function, &record_filename, &record_line_nr)) {
+ switch (read_addr2line_record(&io, a2l_style,
+ &record_function, &record_filename, &record_line_nr)) {
case -1:
if (!symbol_conf.disable_add2line_warn)
pr_warning("%s %s: could not read first record\n", __func__, dso_name);
@@ -567,7 +625,7 @@ static int addr2line(const char *dso_name, u64 addr,
* 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.
*/
- switch (read_addr2line_record(&io, NULL, NULL, NULL)) {
+ switch (read_addr2line_record(&io, a2l_style, NULL, NULL, NULL)) {
case -1:
if (!symbol_conf.disable_add2line_warn)
pr_warning("%s %s: could not read delimiter record\n",
@@ -606,6 +664,7 @@ static int addr2line(const char *dso_name, u64 addr,
/* We have to read the records even if we don't care about the inline info. */
while ((record_status = read_addr2line_record(&io,
+ a2l_style,
&record_function,
&record_filename,
&record_line_nr)) == 1) {
--
2.40.0.348.gf938b09366-goog
Don't wrap stdin and stdout of subprocess with streams, use the api/io
library for buffering.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/srcline.c | 103 ++++++++++++++------------------------
1 file changed, 38 insertions(+), 65 deletions(-)
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index f0a96a834e4b..2da86e57215d 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -10,6 +10,8 @@
#include <linux/string.h>
#include <linux/zalloc.h>
+#include <api/io.h>
+
#include "util/dso.h"
#include "util/debug.h"
#include "util/callchain.h"
@@ -366,12 +368,6 @@ void dso__free_a2l(struct dso *dso)
#else /* HAVE_LIBBFD_SUPPORT */
-struct a2l_subprocess {
- struct child_process addr2line;
- FILE *to_child;
- FILE *from_child;
-};
-
static int filename_split(char *filename, unsigned int *line_nr)
{
char *sep;
@@ -393,28 +389,18 @@ static int filename_split(char *filename, unsigned int *line_nr)
return 0;
}
-static void addr2line_subprocess_cleanup(struct a2l_subprocess *a2l)
+static void addr2line_subprocess_cleanup(struct child_process *a2l)
{
- if (a2l->addr2line.pid != -1) {
- kill(a2l->addr2line.pid, SIGKILL);
- finish_command(&a2l->addr2line); /* ignore result, we don't care */
- a2l->addr2line.pid = -1;
- }
-
- if (a2l->to_child != NULL) {
- fclose(a2l->to_child);
- a2l->to_child = NULL;
- }
-
- if (a2l->from_child != NULL) {
- fclose(a2l->from_child);
- a2l->from_child = NULL;
+ if (a2l->pid != -1) {
+ kill(a2l->pid, SIGKILL);
+ finish_command(a2l); /* ignore result, we don't care */
+ a2l->pid = -1;
}
free(a2l);
}
-static struct a2l_subprocess *addr2line_subprocess_init(const char *addr2line_path,
+static struct child_process *addr2line_subprocess_init(const char *addr2line_path,
const char *binary_path)
{
const char *argv[] = {
@@ -422,54 +408,34 @@ static struct a2l_subprocess *addr2line_subprocess_init(const char *addr2line_pa
"-e", binary_path,
"-i", "-f", NULL
};
- struct a2l_subprocess *a2l = zalloc(sizeof(*a2l));
+ struct child_process *a2l = zalloc(sizeof(*a2l));
int start_command_status = 0;
- if (a2l == NULL)
- goto out;
-
- a2l->to_child = NULL;
- a2l->from_child = NULL;
+ if (a2l == NULL) {
+ pr_err("Failed to allocate memory for addr2line");
+ return NULL;
+ }
- a2l->addr2line.pid = -1;
- a2l->addr2line.in = -1;
- a2l->addr2line.out = -1;
- a2l->addr2line.no_stderr = 1;
+ a2l->pid = -1;
+ a2l->in = -1;
+ a2l->out = -1;
+ a2l->no_stderr = 1;
- a2l->addr2line.argv = argv;
- start_command_status = start_command(&a2l->addr2line);
- a2l->addr2line.argv = NULL; /* it's not used after start_command; avoid dangling pointers */
+ a2l->argv = argv;
+ start_command_status = start_command(a2l);
+ a2l->argv = NULL; /* it's not used after start_command; avoid dangling pointers */
if (start_command_status != 0) {
pr_warning("could not start addr2line (%s) for %s: start_command return code %d\n",
addr2line_path, binary_path, start_command_status);
- goto out;
- }
-
- a2l->to_child = fdopen(a2l->addr2line.in, "w");
- if (a2l->to_child == NULL) {
- pr_warning("could not open write-stream to addr2line (%s) of %s\n",
- addr2line_path, binary_path);
- goto out;
- }
-
- a2l->from_child = fdopen(a2l->addr2line.out, "r");
- if (a2l->from_child == NULL) {
- pr_warning("could not open read-stream from addr2line (%s) of %s\n",
- addr2line_path, binary_path);
- goto out;
+ addr2line_subprocess_cleanup(a2l);
+ return NULL;
}
return a2l;
-
-out:
- if (a2l)
- addr2line_subprocess_cleanup(a2l);
-
- return NULL;
}
-static int read_addr2line_record(struct a2l_subprocess *a2l,
+static int read_addr2line_record(struct io *io,
char **function,
char **filename,
unsigned int *line_nr)
@@ -494,7 +460,7 @@ static int read_addr2line_record(struct a2l_subprocess *a2l,
if (line_nr != NULL)
*line_nr = 0;
- if (getline(&line, &line_len, a2l->from_child) < 0 || !line_len)
+ if (io__getline(&line, &line_len, io) < 0 || !line_len)
goto error;
if (function != NULL)
*function = strdup(strim(line));
@@ -502,7 +468,7 @@ static int read_addr2line_record(struct a2l_subprocess *a2l,
zfree(&line);
line_len = 0;
- if (getline(&line, &line_len, a2l->from_child) < 0 || !line_len)
+ if (io__getline(&line, &line_len, io) < 0 || !line_len)
goto error;
if (filename_split(line, line_nr == NULL ? &dummy_line_nr : line_nr) == 0) {
@@ -546,13 +512,17 @@ static int addr2line(const char *dso_name, u64 addr,
struct inline_node *node,
struct symbol *sym __maybe_unused)
{
- struct a2l_subprocess *a2l = dso->a2l;
+ struct child_process *a2l = dso->a2l;
char *record_function = NULL;
char *record_filename = NULL;
unsigned int record_line_nr = 0;
int record_status = -1;
int ret = 0;
size_t inline_count = 0;
+ int len;
+ char buf[128];
+ ssize_t written;
+ struct io io;
if (!a2l) {
if (!filename__has_section(dso_name, ".debug_line"))
@@ -578,13 +548,16 @@ static int addr2line(const char *dso_name, u64 addr,
* though, because it may be genuinely unknown, in which case we'll get two sets of
* "??"/"??:0" lines.
*/
- if (fprintf(a2l->to_child, "%016"PRIx64"\n,\n", addr) < 0 || fflush(a2l->to_child) != 0) {
+ len = snprintf(buf, sizeof(buf), "%016"PRIx64"\n,\n", addr);
+ written = len > 0 ? write(a2l->in, buf, len) : -1;
+ if (written != len) {
if (!symbol_conf.disable_add2line_warn)
pr_warning("%s %s: could not send request\n", __func__, dso_name);
goto out;
}
+ io__init(&io, a2l->out, buf, sizeof(buf));
- switch (read_addr2line_record(a2l, &record_function, &record_filename, &record_line_nr)) {
+ switch (read_addr2line_record(&io, &record_function, &record_filename, &record_line_nr)) {
case -1:
if (!symbol_conf.disable_add2line_warn)
pr_warning("%s %s: could not read first record\n", __func__, dso_name);
@@ -594,7 +567,7 @@ static int addr2line(const char *dso_name, u64 addr,
* 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.
*/
- switch (read_addr2line_record(a2l, NULL, NULL, NULL)) {
+ switch (read_addr2line_record(&io, NULL, NULL, NULL)) {
case -1:
if (!symbol_conf.disable_add2line_warn)
pr_warning("%s %s: could not read delimiter record\n",
@@ -632,7 +605,7 @@ static int addr2line(const char *dso_name, u64 addr,
}
/* We have to read the records even if we don't care about the inline info. */
- while ((record_status = read_addr2line_record(a2l,
+ while ((record_status = read_addr2line_record(&io,
&record_function,
&record_filename,
&record_line_nr)) == 1) {
@@ -656,7 +629,7 @@ static int addr2line(const char *dso_name, u64 addr,
void dso__free_a2l(struct dso *dso)
{
- struct a2l_subprocess *a2l = dso->a2l;
+ struct child_process *a2l = dso->a2l;
if (!a2l)
return;
--
2.40.0.348.gf938b09366-goog
Em Thu, Mar 30, 2023 at 05:48:41PM -0700, Ian Rogers escreveu:
> Reads a line to allocated memory up to a newline following the getline
> API.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/lib/api/io.h | 40 +++++++++++++++++++++++++++++++++++++++
> tools/perf/tests/api-io.c | 36 +++++++++++++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> index 777c20f6b604..d874e8fa8b07 100644
> --- a/tools/lib/api/io.h
> +++ b/tools/lib/api/io.h
> @@ -7,7 +7,9 @@
> #ifndef __API_IO__
> #define __API_IO__
>
> +#include <errno.h>
> #include <stdlib.h>
> +#include <string.h>
> #include <unistd.h>
>
> struct io {
> @@ -112,4 +114,42 @@ static inline int io__get_dec(struct io *io, __u64 *dec)
> }
> }
>
> +/* Read up to and including the first newline following the pattern of getline. */
> +static inline ssize_t io__getline(char **line_out, size_t *line_len_out, struct io *io)
Can we have io be the first arg? To be consistent with the other
functons here and elsewhere in perf.
- Arnaldo
> +{
> + char buf[128];
> + int buf_pos = 0;
> + char *line = NULL;
> + size_t line_len = 0;
> + int ch = 0;
> +
> + /* TODO: reuse previously allocated memory. */
> + free(*line_out);
> + while (ch != '\n') {
> + ch = io__get_char(io);
> +
> + if (ch < 0)
> + break;
> +
> + if (buf_pos == sizeof(buf)) {
> + line = realloc(line, line_len + sizeof(buf));
> + if (!line)
> + return -ENOMEM;
> + memcpy(&line[line_len], buf, sizeof(buf));
> + line_len += sizeof(buf);
> + buf_pos = 0;
> + }
> + buf[buf_pos++] = (char)ch;
> + }
> + line = realloc(line, line_len + buf_pos + 1);
> + if (!line)
> + return -ENOMEM;
> + memcpy(&line[line_len], buf, buf_pos);
> + line[line_len + buf_pos] = '\0';
> + line_len += buf_pos;
> + *line_out = line;
> + *line_len_out = line_len;
> + return line_len;
> +}
> +
> #endif /* __API_IO__ */
> diff --git a/tools/perf/tests/api-io.c b/tools/perf/tests/api-io.c
> index e91cf2c127f1..0ff39cdfcb01 100644
> --- a/tools/perf/tests/api-io.c
> +++ b/tools/perf/tests/api-io.c
> @@ -289,6 +289,40 @@ static int test_get_dec(void)
> return ret;
> }
>
> +static int test_get_line(void)
> +{
> + char path[PATH_MAX];
> + struct io io;
> + char test_string[1024];
> + char *line = NULL;
> + size_t i, line_len = 0;
> + size_t buf_size = 128;
> + int ret = 0;
> +
> + for (i = 0; i < 512; i++)
> + test_string[i] = 'a';
> + test_string[512] = '\n';
> + for (i = 513; i < 1023; i++)
> + test_string[i] = 'b';
> + test_string[1023] = '\0';
> +
> + if (setup_test(path, test_string, buf_size, &io))
> + return -1;
> +
> + EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 513);
> + EXPECT_EQUAL((int)strlen(line), 513);
> + for (i = 0; i < 512; i++)
> + EXPECT_EQUAL(line[i], 'a');
> + EXPECT_EQUAL(line[512], '\n');
> + EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 510);
> + for (i = 0; i < 510; i++)
> + EXPECT_EQUAL(line[i], 'b');
> +
> + free(line);
> + cleanup_test(path, &io);
> + return ret;
> +}
> +
> static int test__api_io(struct test_suite *test __maybe_unused,
> int subtest __maybe_unused)
> {
> @@ -300,6 +334,8 @@ static int test__api_io(struct test_suite *test __maybe_unused,
> ret = TEST_FAIL;
> if (test_get_dec())
> ret = TEST_FAIL;
> + if (test_get_line())
> + ret = TEST_FAIL;
> return ret;
> }
>
> --
> 2.40.0.348.gf938b09366-goog
>
--
- Arnaldo
Hi Ian,
On Thu, Mar 30, 2023 at 5:49 PM Ian Rogers <[email protected]> wrote:
>
> Reads a line to allocated memory up to a newline following the getline
> API.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/lib/api/io.h | 40 +++++++++++++++++++++++++++++++++++++++
> tools/perf/tests/api-io.c | 36 +++++++++++++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> index 777c20f6b604..d874e8fa8b07 100644
> --- a/tools/lib/api/io.h
> +++ b/tools/lib/api/io.h
> @@ -7,7 +7,9 @@
> #ifndef __API_IO__
> #define __API_IO__
>
> +#include <errno.h>
> #include <stdlib.h>
> +#include <string.h>
> #include <unistd.h>
>
> struct io {
> @@ -112,4 +114,42 @@ static inline int io__get_dec(struct io *io, __u64 *dec)
> }
> }
>
> +/* Read up to and including the first newline following the pattern of getline. */
> +static inline ssize_t io__getline(char **line_out, size_t *line_len_out, struct io *io)
> +{
> + char buf[128];
> + int buf_pos = 0;
> + char *line = NULL;
> + size_t line_len = 0;
> + int ch = 0;
> +
> + /* TODO: reuse previously allocated memory. */
> + free(*line_out);
> + while (ch != '\n') {
> + ch = io__get_char(io);
> +
> + if (ch < 0)
> + break;
> +
> + if (buf_pos == sizeof(buf)) {
> + line = realloc(line, line_len + sizeof(buf));
> + if (!line)
> + return -ENOMEM;
We can use a temp variable for the failure case, and also free
the original line before returning the error.
> + memcpy(&line[line_len], buf, sizeof(buf));
> + line_len += sizeof(buf);
> + buf_pos = 0;
> + }
> + buf[buf_pos++] = (char)ch;
> + }
> + line = realloc(line, line_len + buf_pos + 1);
> + if (!line)
> + return -ENOMEM;
Ditto.
Thanks,
Namhyung
> + memcpy(&line[line_len], buf, buf_pos);
> + line[line_len + buf_pos] = '\0';
> + line_len += buf_pos;
> + *line_out = line;
> + *line_len_out = line_len;
> + return line_len;
> +}
> +
> #endif /* __API_IO__ */
> diff --git a/tools/perf/tests/api-io.c b/tools/perf/tests/api-io.c
> index e91cf2c127f1..0ff39cdfcb01 100644
> --- a/tools/perf/tests/api-io.c
> +++ b/tools/perf/tests/api-io.c
> @@ -289,6 +289,40 @@ static int test_get_dec(void)
> return ret;
> }
>
> +static int test_get_line(void)
> +{
> + char path[PATH_MAX];
> + struct io io;
> + char test_string[1024];
> + char *line = NULL;
> + size_t i, line_len = 0;
> + size_t buf_size = 128;
> + int ret = 0;
> +
> + for (i = 0; i < 512; i++)
> + test_string[i] = 'a';
> + test_string[512] = '\n';
> + for (i = 513; i < 1023; i++)
> + test_string[i] = 'b';
> + test_string[1023] = '\0';
> +
> + if (setup_test(path, test_string, buf_size, &io))
> + return -1;
> +
> + EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 513);
> + EXPECT_EQUAL((int)strlen(line), 513);
> + for (i = 0; i < 512; i++)
> + EXPECT_EQUAL(line[i], 'a');
> + EXPECT_EQUAL(line[512], '\n');
> + EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 510);
> + for (i = 0; i < 510; i++)
> + EXPECT_EQUAL(line[i], 'b');
> +
> + free(line);
> + cleanup_test(path, &io);
> + return ret;
> +}
> +
> static int test__api_io(struct test_suite *test __maybe_unused,
> int subtest __maybe_unused)
> {
> @@ -300,6 +334,8 @@ static int test__api_io(struct test_suite *test __maybe_unused,
> ret = TEST_FAIL;
> if (test_get_dec())
> ret = TEST_FAIL;
> + if (test_get_line())
> + ret = TEST_FAIL;
> return ret;
> }
>
> --
> 2.40.0.348.gf938b09366-goog
>
On Thu, Mar 30, 2023 at 5:49 PM Ian Rogers <[email protected]> wrote:
>
> The sentinel value differs for llvm-addr2line. Configure this once and
> then detect when reading records.
Thanks for fixing this!
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/srcline.c | 65 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index 2da86e57215d..423ddf3967b0 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -435,7 +435,50 @@ static struct child_process *addr2line_subprocess_init(const char *addr2line_pat
> return a2l;
> }
>
> +enum a2l_style {
> + BROKEN,
> + GNU_BINUTILS,
> + LLVM,
> +};
> +
> +static enum a2l_style addr2line_configure(struct child_process *a2l)
> +{
> + static bool cached;
> + static enum a2l_style style;
> +
> + if (!cached) {
> + char buf[128];
> + struct io io;
> + int ch;
> +
> + if (write(a2l->in, ",\n", 2) != 2)
> + return BROKEN;
> +
> + io__init(&io, a2l->out, buf, sizeof(buf));
> + ch = io__get_char(&io);
> + if (ch == ',') {
> + style = LLVM;
> + cached = true;
> + } else if (ch == '?') {
> + style = GNU_BINUTILS;
> + cached = true;
> + } else {
> + style = BROKEN;
> + }
> + do {
> + ch = io__get_char(&io);
> + } while (ch > 0 && ch != '\n');
Maybe better to io__getline() ?
> + if (style == GNU_BINUTILS) {
> + do {
> + ch = io__get_char(&io);
> + } while (ch > 0 && ch != '\n');
> + }
> + }
> + return style;
> +}
> +
> static int read_addr2line_record(struct io *io,
> + enum a2l_style style,
> char **function,
> char **filename,
> unsigned int *line_nr)
> @@ -462,6 +505,12 @@ static int read_addr2line_record(struct io *io,
>
> if (io__getline(&line, &line_len, io) < 0 || !line_len)
> goto error;
> +
> + if (style == LLVM && line_len == 2 && line[0] == ',') {
> + zfree(&line);
> + return 0;
> + }
> +
> if (function != NULL)
> *function = strdup(strim(line));
>
> @@ -471,7 +520,8 @@ static int read_addr2line_record(struct io *io,
> if (io__getline(&line, &line_len, io) < 0 || !line_len)
> goto error;
>
> - if (filename_split(line, line_nr == NULL ? &dummy_line_nr : line_nr) == 0) {
> + if (filename_split(line, line_nr == NULL ? &dummy_line_nr : line_nr) == 0 &&
> + style == GNU_BINUTILS) {
> ret = 0;
> goto error;
> }
> @@ -523,6 +573,7 @@ static int addr2line(const char *dso_name, u64 addr,
> char buf[128];
> ssize_t written;
> struct io io;
> + enum a2l_style a2l_style;
>
> if (!a2l) {
> if (!filename__has_section(dso_name, ".debug_line"))
> @@ -538,6 +589,12 @@ 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__);
> + goto out;
> + }
>
> /*
> * Send our request and then *deliberately* send something that can't be interpreted as
Can you please update the comment about the LLVM behavior?
Thanks,
Namhyung
> @@ -557,7 +614,8 @@ static int addr2line(const char *dso_name, u64 addr,
> }
> io__init(&io, a2l->out, buf, sizeof(buf));
>
> - switch (read_addr2line_record(&io, &record_function, &record_filename, &record_line_nr)) {
> + switch (read_addr2line_record(&io, a2l_style,
> + &record_function, &record_filename, &record_line_nr)) {
> case -1:
> if (!symbol_conf.disable_add2line_warn)
> pr_warning("%s %s: could not read first record\n", __func__, dso_name);
> @@ -567,7 +625,7 @@ static int addr2line(const char *dso_name, u64 addr,
> * 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.
> */
> - switch (read_addr2line_record(&io, NULL, NULL, NULL)) {
> + switch (read_addr2line_record(&io, a2l_style, NULL, NULL, NULL)) {
> case -1:
> if (!symbol_conf.disable_add2line_warn)
> pr_warning("%s %s: could not read delimiter record\n",
> @@ -606,6 +664,7 @@ static int addr2line(const char *dso_name, u64 addr,
>
> /* We have to read the records even if we don't care about the inline info. */
> while ((record_status = read_addr2line_record(&io,
> + a2l_style,
> &record_function,
> &record_filename,
> &record_line_nr)) == 1) {
> --
> 2.40.0.348.gf938b09366-goog
>