[PATCH v1 0/4] Support for llvm-addr2line

[PATCH v1 0/4] Support for llvm-addr2line

[Ian Rogers]

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

[PATCH v1 1/4] tools api: Add io__getline

[Ian Rogers]

Reads a line to allocated memory up to a newline following the getline
API.

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

[PATCH v1 2/4] perf srcline: Simplify addr2line subprocess

[Ian Rogers]

Don't wrap stdin and stdout of subprocess with streams, use the api/io
library for buffering.

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

[PATCH v1 3/4] perf srcline: Support for llvm-addr2line

[Ian Rogers]

The sentinel value differs for llvm-addr2line. Configure this once and
then detect when reading records.

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

[PATCH v1 4/4] perf srcline: Avoid addr2line SIGPIPEs

[Ian Rogers]

Ignore SIGPIPEs when addr2line is configured.

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

Re: [PATCH v1 1/4] tools api: Add io__getline

[Arnaldo Carvalho de Melo]

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 <irogers@google.com>
> ---
>  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

Re: [PATCH v1 1/4] tools api: Add io__getline

[Namhyung Kim]

Hi Ian,

On Thu, Mar 30, 2023 at 5:49 PM Ian Rogers <irogers@google.com> wrote:
>
> Reads a line to allocated memory up to a newline following the getline
> API.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  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
>

Re: [PATCH v1 3/4] perf srcline: Support for llvm-addr2line

[Namhyung Kim]

On Thu, Mar 30, 2023 at 5:49 PM Ian Rogers <irogers@google.com> 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 <irogers@google.com>
> ---
>  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
>