[PATCH v2 1/3] perf tools: Introduce dso__decompress_kmodule_{fd,path}

[PATCH v2 1/3] perf tools: Introduce dso__decompress_kmodule_{fd,path}

[Namhyung Kim]

Move decompress_kmodule() to util/dso.c and split it to two functions
returning fd and (decompressed) file path.  Existing user only wants the
fd version but the path version will be used soon.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c        | 51 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/dso.h        |  3 +++
 tools/perf/util/symbol-elf.c | 36 +------------------------------
 3 files changed, 55 insertions(+), 35 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index b27d127cdf68..fc4747a8c283 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -248,6 +248,57 @@ bool dso__needs_decompress(struct dso *dso)
 		dso->symtab_type == DSO_BINARY_TYPE__GUEST_KMODULE_COMP;
 }
 
+static int decompress_kmodule(struct dso *dso, const char *name, char *tmpbuf)
+{
+	int fd = -1;
+	struct kmod_path m;
+
+	if (!dso__needs_decompress(dso))
+		return -1;
+
+	if (kmod_path__parse_ext(&m, dso->long_name) || !m.comp)
+		return -1;
+
+	fd = mkstemp(tmpbuf);
+	if (fd < 0) {
+		dso->load_errno = errno;
+		goto out;
+	}
+
+	if (!decompress_to_file(m.ext, name, fd)) {
+		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
+		close(fd);
+		fd = -1;
+	}
+
+out:
+	free(m.ext);
+	return fd;
+}
+
+int dso__decompress_kmodule_fd(struct dso *dso, const char *name)
+{
+	char tmpbuf[] = "/tmp/perf-kmod-XXXXXX";
+	int fd;
+
+	fd = decompress_kmodule(dso, name, tmpbuf);
+	unlink(tmpbuf);
+	return fd;
+}
+
+int dso__decompress_kmodule_path(struct dso *dso, const char *name,
+				 char *pathname, size_t len)
+{
+	char tmpbuf[] = "/tmp/perf-kmod-XXXXXX";
+	int fd;
+
+	fd = decompress_kmodule(dso, name, tmpbuf);
+	close(fd);
+
+	strncpy(pathname, tmpbuf, len);
+	return fd >= 0 ? 0 : fd;
+}
+
 /*
  * Parses kernel module specified in @path and updates
  * @m argument like:
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 5fe2ab5877bd..a2f544990c19 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -244,6 +244,9 @@ bool is_supported_compression(const char *ext);
 bool is_kernel_module(const char *pathname, int cpumode);
 bool decompress_to_file(const char *ext, const char *filename, int output_fd);
 bool dso__needs_decompress(struct dso *dso);
+int dso__decompress_kmodule_fd(struct dso *dso, const char *name);
+int dso__decompress_kmodule_path(struct dso *dso, const char *name,
+				 char *pathname, size_t len);
 
 struct kmod_path {
 	char *name;
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 1fb2efae4f02..d342e771dbad 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -637,40 +637,6 @@ static int dso__swap_init(struct dso *dso, unsigned char eidata)
 	return 0;
 }
 
-static int decompress_kmodule(struct dso *dso, const char *name,
-			      enum dso_binary_type type)
-{
-	int fd = -1;
-	char tmpbuf[] = "/tmp/perf-kmod-XXXXXX";
-	struct kmod_path m;
-
-	if (type != DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__GUEST_KMODULE_COMP &&
-	    type != DSO_BINARY_TYPE__BUILD_ID_CACHE)
-		return -1;
-
-	if (kmod_path__parse_ext(&m, dso->long_name) || !m.comp)
-		return -1;
-
-	fd = mkstemp(tmpbuf);
-	if (fd < 0) {
-		dso->load_errno = errno;
-		goto out;
-	}
-
-	if (!decompress_to_file(m.ext, name, fd)) {
-		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
-		close(fd);
-		fd = -1;
-	}
-
-	unlink(tmpbuf);
-
-out:
-	free(m.ext);
-	return fd;
-}
-
 bool symsrc__possibly_runtime(struct symsrc *ss)
 {
 	return ss->dynsym || ss->opdsec;
@@ -702,7 +668,7 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 	int fd;
 
 	if (dso__needs_decompress(dso)) {
-		fd = decompress_kmodule(dso, name, type);
+		fd = dso__decompress_kmodule_fd(dso, name);
 		if (fd < 0)
 			return -1;
 	} else {
-- 
2.13.0

[PATCH v2 2/3] perf tools: Decompress kernel module when reading DSO data

[Namhyung Kim]

Currently perf decompresses kernel modules when loading symbol table but
it missed to do it when reading raw data.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index fc4747a8c283..77e5701e38b9 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -463,8 +463,22 @@ static int __open_dso(struct dso *dso, struct machine *machine)
 		return -EINVAL;
 	}
 
-	if (!is_regular_file(name))
+	if (!is_regular_file(name)) {
+		free(name);
 		return -EINVAL;
+	}
+
+	if (dso__needs_decompress(dso)) {
+		char newpath[32];
+		size_t len = sizeof(newpath);
+
+		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
+			free(name);
+			return -1;
+		}
+
+		strcpy(name, newpath);
+	}
 
 	fd = do_open(name);
 	free(name);
-- 
2.13.0

[PATCH v2 3/3] perf test: Decompress kernel module before objdump

[Namhyung Kim]

If a kernel modules is compressed, it should be decompressed before
running objdump to parse binary data correctly.  This fixes a failure of
object code reading test for me.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/code-reading.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index 1f14e7612cbb..fe6bd34d68b1 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -229,6 +229,7 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 	unsigned char buf2[BUFSZ];
 	size_t ret_len;
 	u64 objdump_addr;
+	const char *objdump_name;
 	int ret;
 
 	pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
@@ -289,9 +290,32 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
 		state->done[state->done_cnt++] = al.map->start;
 	}
 
+	objdump_name = al.map->dso->long_name;
+	if (dso__needs_decompress(al.map->dso)) {
+		char buf[32];  /* for "/tmp/perf-kmod-XXXXXX" */
+
+		if (dso__decompress_kmodule_path(al.map->dso, objdump_name,
+						 buf, sizeof(buf)) < 0) {
+			pr_debug("decompression failed\n");
+			return -1;
+		}
+
+		objdump_name = strdup(buf);
+		if (objdump_name == NULL) {
+			pr_debug("memory allocation failed\n");
+			return -1;
+		}
+	}
+
 	/* Read the object code using objdump */
 	objdump_addr = map__rip_2objdump(al.map, al.addr);
-	ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2, len);
+	ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
+
+	if (objdump_name != al.map->dso->long_name) {
+		unlink(objdump_name);
+		free((void *)objdump_name);
+	}
+
 	if (ret > 0) {
 		/*
 		 * The kernel maps are inaccurate - assume objdump is right in
-- 
2.13.0

Re: [PATCH v2 1/3] perf tools: Introduce dso__decompress_kmodule_{fd,path}

[Jiri Olsa]

On Sat, Jun 03, 2017 at 11:00:31AM +0900, Namhyung Kim wrote:
> Move decompress_kmodule() to util/dso.c and split it to two functions
> returning fd and (decompressed) file path.  Existing user only wants the
> fd version but the path version will be used soon.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/dso.c        | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/dso.h        |  3 +++
>  tools/perf/util/symbol-elf.c | 36 +------------------------------
>  3 files changed, 55 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index b27d127cdf68..fc4747a8c283 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -248,6 +248,57 @@ bool dso__needs_decompress(struct dso *dso)
>  		dso->symtab_type == DSO_BINARY_TYPE__GUEST_KMODULE_COMP;
>  }
>  
> +static int decompress_kmodule(struct dso *dso, const char *name, char *tmpbuf)
> +{
> +	int fd = -1;
> +	struct kmod_path m;
> +
> +	if (!dso__needs_decompress(dso))
> +		return -1;

the original code had also DSO_BINARY_TYPE__BUILD_ID_CACHE check,
not sure why though.. ;-)

jirka

Re: [PATCH v2 3/3] perf test: Decompress kernel module before objdump

[Adrian Hunter]

On 03/06/17 05:00, Namhyung Kim wrote:
> If a kernel modules is compressed, it should be decompressed before
> running objdump to parse binary data correctly.  This fixes a failure of
> object code reading test for me.

Makes sense to me although I have a few small comments below.

> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/tests/code-reading.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> index 1f14e7612cbb..fe6bd34d68b1 100644
> --- a/tools/perf/tests/code-reading.c
> +++ b/tools/perf/tests/code-reading.c
> @@ -229,6 +229,7 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
>  	unsigned char buf2[BUFSZ];
>  	size_t ret_len;
>  	u64 objdump_addr;
> +	const char *objdump_name;
>  	int ret;
>  
>  	pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
> @@ -289,9 +290,32 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
>  		state->done[state->done_cnt++] = al.map->start;
>  	}
>  
> +	objdump_name = al.map->dso->long_name;
> +	if (dso__needs_decompress(al.map->dso)) {
> +		char buf[32];  /* for "/tmp/perf-kmod-XXXXXX" */

We should be able to set the temporary file name buffer size without
reference to the format of the name i.e. #define it or use PATH_MAX

> +
> +		if (dso__decompress_kmodule_path(al.map->dso, objdump_name,
> +						 buf, sizeof(buf)) < 0) {

Do we care that it is kmodule i.e. why isn't this function called just
dso__decompress_path().

> +			pr_debug("decompression failed\n");
> +			return -1;
> +		}
> +
> +		objdump_name = strdup(buf);

Why not just put the buf in the outer scope instead of using strdup().

> +		if (objdump_name == NULL) {
> +			pr_debug("memory allocation failed\n");
> +			return -1;
> +		}
> +	}
> +
>  	/* Read the object code using objdump */
>  	objdump_addr = map__rip_2objdump(al.map, al.addr);
> -	ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2, len);
> +	ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
> +
> +	if (objdump_name != al.map->dso->long_name) {
> +		unlink(objdump_name);
> +		free((void *)objdump_name);
> +	}
> +
>  	if (ret > 0) {
>  		/*
>  		 * The kernel maps are inaccurate - assume objdump is right in
> 

Re: [PATCH v2 1/3] perf tools: Introduce dso__decompress_kmodule_{fd,path}

[Namhyung Kim]

Hi Jiri,

On Mon, Jun 5, 2017 at 6:58 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Sat, Jun 03, 2017 at 11:00:31AM +0900, Namhyung Kim wrote:
>> Move decompress_kmodule() to util/dso.c and split it to two functions
>> returning fd and (decompressed) file path.  Existing user only wants the
>> fd version but the path version will be used soon.
>>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/util/dso.c        | 51 ++++++++++++++++++++++++++++++++++++++++++++
>>  tools/perf/util/dso.h        |  3 +++
>>  tools/perf/util/symbol-elf.c | 36 +------------------------------
>>  3 files changed, 55 insertions(+), 35 deletions(-)
>>
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index b27d127cdf68..fc4747a8c283 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -248,6 +248,57 @@ bool dso__needs_decompress(struct dso *dso)
>>               dso->symtab_type == DSO_BINARY_TYPE__GUEST_KMODULE_COMP;
>>  }
>>
>> +static int decompress_kmodule(struct dso *dso, const char *name, char *tmpbuf)
>> +{
>> +     int fd = -1;
>> +     struct kmod_path m;
>> +
>> +     if (!dso__needs_decompress(dso))
>> +             return -1;
>
> the original code had also DSO_BINARY_TYPE__BUILD_ID_CACHE check,
> not sure why though.. ;-)

It's because decompressed modules which were found in the build-id
cache didn't set dso->symtab_type properly.  It was fixed by my
previous patch so checking symtab_type will cover the (compressed)
build-id cache case too now.

Thanks,
Namhyung

Re: [PATCH v2 3/3] perf test: Decompress kernel module before objdump

[Namhyung Kim]

Hi Adrian,

On Mon, Jun 5, 2017 at 8:13 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 03/06/17 05:00, Namhyung Kim wrote:
>> If a kernel modules is compressed, it should be decompressed before
>> running objdump to parse binary data correctly.  This fixes a failure of
>> object code reading test for me.
>
> Makes sense to me although I have a few small comments below.

Thanks!

>
>>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/tests/code-reading.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
>> index 1f14e7612cbb..fe6bd34d68b1 100644
>> --- a/tools/perf/tests/code-reading.c
>> +++ b/tools/perf/tests/code-reading.c
>> @@ -229,6 +229,7 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
>>       unsigned char buf2[BUFSZ];
>>       size_t ret_len;
>>       u64 objdump_addr;
>> +     const char *objdump_name;
>>       int ret;
>>
>>       pr_debug("Reading object code for memory address: %#"PRIx64"\n", addr);
>> @@ -289,9 +290,32 @@ static int read_object_code(u64 addr, size_t len, u8 cpumode,
>>               state->done[state->done_cnt++] = al.map->start;
>>       }
>>
>> +     objdump_name = al.map->dso->long_name;
>> +     if (dso__needs_decompress(al.map->dso)) {
>> +             char buf[32];  /* for "/tmp/perf-kmod-XXXXXX" */
>
> We should be able to set the temporary file name buffer size without
> reference to the format of the name i.e. #define it or use PATH_MAX

OK, will define KMOD_DECOMP_LEN then..

>
>> +
>> +             if (dso__decompress_kmodule_path(al.map->dso, objdump_name,
>> +                                              buf, sizeof(buf)) < 0) {
>
> Do we care that it is kmodule i.e. why isn't this function called just
> dso__decompress_path().

Well, I'm not sure loading a compressed DSO is making sense.
Decompressing is needed for kernel modules which supports loading
compressed modules.

>
>> +                     pr_debug("decompression failed\n");
>> +                     return -1;
>> +             }
>> +
>> +             objdump_name = strdup(buf);
>
> Why not just put the buf in the outer scope instead of using strdup().

Will change.

Thanks
Namhyung


>
>> +             if (objdump_name == NULL) {
>> +                     pr_debug("memory allocation failed\n");
>> +                     return -1;
>> +             }
>> +     }
>> +
>>       /* Read the object code using objdump */
>>       objdump_addr = map__rip_2objdump(al.map, al.addr);
>> -     ret = read_via_objdump(al.map->dso->long_name, objdump_addr, buf2, len);
>> +     ret = read_via_objdump(objdump_name, objdump_addr, buf2, len);
>> +
>> +     if (objdump_name != al.map->dso->long_name) {
>> +             unlink(objdump_name);
>> +             free((void *)objdump_name);
>> +     }
>> +
>>       if (ret > 0) {
>>               /*
>>                * The kernel maps are inaccurate - assume objdump is right in
>>
>