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@xxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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
>>
>