linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] include program header in elf files generated by perf inject
@ 2022-08-25  8:49 Lieven Hey
  2022-08-25 13:51 ` James Clark
  2022-09-14 13:52 ` Leo Yan
  0 siblings, 2 replies; 8+ messages in thread
From: Lieven Hey @ 2022-08-25  8:49 UTC (permalink / raw)
  To: acme, linux-perf-users; +Cc: Lieven Hey

The missing header makes it hard for programs like elfutils to open
these files.
---
 tools/perf/util/genelf.c | 14 ++++++++++++++
 tools/perf/util/genelf.h |  4 ++++
 2 files changed, 18 insertions(+)

diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
index 953338b9e887..02cd9f75e3d2 100644
--- a/tools/perf/util/genelf.c
+++ b/tools/perf/util/genelf.c
@@ -251,6 +251,7 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
 	Elf_Data *d;
 	Elf_Scn *scn;
 	Elf_Ehdr *ehdr;
+	Elf_Phdr *phdr;
 	Elf_Shdr *shdr;
 	uint64_t eh_frame_base_offset;
 	char *strsym = NULL;
@@ -285,6 +286,19 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
 	ehdr->e_version = EV_CURRENT;
 	ehdr->e_shstrndx= unwinding ? 4 : 2; /* shdr index for section name */
 
+	/*
+	 * setup program header
+	 */
+	phdr = elf_newphdr(e, 1);
+	phdr[0].p_type = PT_LOAD;
+	phdr[0].p_offset = 0;
+	phdr[0].p_vaddr = 0;
+	phdr[0].p_paddr = 0;
+	phdr[0].p_filesz = csize;
+	phdr[0].p_memsz = csize;
+	phdr[0].p_flags = PF_X | PF_R;
+	phdr[0].p_align = 8;
+
 	/*
 	 * setup text section
 	 */
diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
index ae138afe6c56..b5c909546e3f 100644
--- a/tools/perf/util/genelf.h
+++ b/tools/perf/util/genelf.h
@@ -53,8 +53,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
 
 #if GEN_ELF_CLASS == ELFCLASS64
 #define elf_newehdr	elf64_newehdr
+#define elf_newphdr	elf64_newphdr
 #define elf_getshdr	elf64_getshdr
 #define Elf_Ehdr	Elf64_Ehdr
+#define Elf_Phdr	Elf64_Phdr
 #define Elf_Shdr	Elf64_Shdr
 #define Elf_Sym		Elf64_Sym
 #define ELF_ST_TYPE(a)	ELF64_ST_TYPE(a)
@@ -62,8 +64,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
 #define ELF_ST_VIS(a)	ELF64_ST_VISIBILITY(a)
 #else
 #define elf_newehdr	elf32_newehdr
+#define elf_newphdr	elf32_newphdr
 #define elf_getshdr	elf32_getshdr
 #define Elf_Ehdr	Elf32_Ehdr
+#define Elf_Phdr	Elf32_Phdr
 #define Elf_Shdr	Elf32_Shdr
 #define Elf_Sym		Elf32_Sym
 #define ELF_ST_TYPE(a)	ELF32_ST_TYPE(a)
-- 
2.37.2

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] include program header in elf files generated by perf inject
  2022-08-25  8:49 [PATCH] include program header in elf files generated by perf inject Lieven Hey
@ 2022-08-25 13:51 ` James Clark
       [not found]   ` <6e0ebfb6-9fbf-073c-94b0-a799b9ec657f@kdab.com>
  2022-09-14 13:52 ` Leo Yan
  1 sibling, 1 reply; 8+ messages in thread
From: James Clark @ 2022-08-25 13:51 UTC (permalink / raw)
  To: Lieven Hey; +Cc: acme, linux-perf-users



On 25/08/2022 09:49, Lieven Hey wrote:
> The missing header makes it hard for programs like elfutils to open
> these files.

Hi Lieven,

Do you have an example command that demonstrates this? I'm interested in
trying it out.

I was also only aware of the elf files put in ~/.debug after perf
record, but not perf inject.

Thanks
James

> ---
>  tools/perf/util/genelf.c | 14 ++++++++++++++
>  tools/perf/util/genelf.h |  4 ++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> index 953338b9e887..02cd9f75e3d2 100644
> --- a/tools/perf/util/genelf.c
> +++ b/tools/perf/util/genelf.c
> @@ -251,6 +251,7 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>  	Elf_Data *d;
>  	Elf_Scn *scn;
>  	Elf_Ehdr *ehdr;
> +	Elf_Phdr *phdr;
>  	Elf_Shdr *shdr;
>  	uint64_t eh_frame_base_offset;
>  	char *strsym = NULL;
> @@ -285,6 +286,19 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>  	ehdr->e_version = EV_CURRENT;
>  	ehdr->e_shstrndx= unwinding ? 4 : 2; /* shdr index for section name */
>  
> +	/*
> +	 * setup program header
> +	 */
> +	phdr = elf_newphdr(e, 1);
> +	phdr[0].p_type = PT_LOAD;
> +	phdr[0].p_offset = 0;
> +	phdr[0].p_vaddr = 0;
> +	phdr[0].p_paddr = 0;
> +	phdr[0].p_filesz = csize;
> +	phdr[0].p_memsz = csize;
> +	phdr[0].p_flags = PF_X | PF_R;
> +	phdr[0].p_align = 8;
> +
>  	/*
>  	 * setup text section
>  	 */
> diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
> index ae138afe6c56..b5c909546e3f 100644
> --- a/tools/perf/util/genelf.h
> +++ b/tools/perf/util/genelf.h
> @@ -53,8 +53,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
>  
>  #if GEN_ELF_CLASS == ELFCLASS64
>  #define elf_newehdr	elf64_newehdr
> +#define elf_newphdr	elf64_newphdr
>  #define elf_getshdr	elf64_getshdr
>  #define Elf_Ehdr	Elf64_Ehdr
> +#define Elf_Phdr	Elf64_Phdr
>  #define Elf_Shdr	Elf64_Shdr
>  #define Elf_Sym		Elf64_Sym
>  #define ELF_ST_TYPE(a)	ELF64_ST_TYPE(a)
> @@ -62,8 +64,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
>  #define ELF_ST_VIS(a)	ELF64_ST_VISIBILITY(a)
>  #else
>  #define elf_newehdr	elf32_newehdr
> +#define elf_newphdr	elf32_newphdr
>  #define elf_getshdr	elf32_getshdr
>  #define Elf_Ehdr	Elf32_Ehdr
> +#define Elf_Phdr	Elf32_Phdr
>  #define Elf_Shdr	Elf32_Shdr
>  #define Elf_Sym		Elf32_Sym
>  #define ELF_ST_TYPE(a)	ELF32_ST_TYPE(a)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] include program header in elf files generated by perf inject
       [not found]         ` <cf2f3f27-1002-dbd0-d851-465b169c2031@arm.com>
@ 2022-09-13 13:34           ` Lieven Hey
  2022-09-13 13:49             ` Leo Yan
  0 siblings, 1 reply; 8+ messages in thread
From: Lieven Hey @ 2022-09-13 13:34 UTC (permalink / raw)
  To: James Clark, leo.yan; +Cc: linux-perf-users, acme

Hi,

perf is unable to open elf files generated by perf inject.

Example:
Create a file named test.dart with the following content:

void main() {
     print("Hello World");
}

and then run

perf record --call-graph dwarf -k 1 dart --generate-perf-jitdump test.dart
perf inject -j -i perf.data -o perf.data.jitted

The problem exists since 2d86612aacb78. The commit added some code that 
now requires an elf file to have a valid program header.

Cheers
Lieven

Am 13.09.22 um 11:56 schrieb James Clark:
> 
> 
> On 09/09/2022 14:47, Lieven Hey wrote:
>> Hi James,
>>
>> I looked into this again. Commit 2d86612aacb78 changed the elf loading
>> code so that a program header is now required.
>>
> 
> Can you reply-all on the original mailing list thread with this and a
> 'to' to Leo. It's probably best to continue the discussion there so Leo
> is aware.
> 
> Thanks
> 
>> Am 25.08.22 um 17:24 schrieb James Clark:
>>>
>>>
>>> On 25/08/2022 14:56, Lieven Hey wrote:
>>>> Hi James,
>>>>
>>>> I created a file named test.dart with the following content:
>>>>
>>>> void main() {
>>>>     print("Hello World");
>>>> }
>>>>
>>>> and then run
>>>>
>>>> perf record --call-graph dwarf -k 1 dart --generate-perf-jitdump
>>>> test.dart
>>>> perf inject -j -i perf.data -o perf.data.jitted
>>>>
>>>> If you now open perf.data.jitted with perf report you can see lots of
>>>> errors regarding the missing program header.
>>>
>>> Do I need a debug build of dart or some extra dependency linked into
>>> perf? I don't see any errors but I do see a load of addresses without
>>> symbols:
>>>
>>>       13.27%     0.00%  dart          ld-2.31.so          [.] dl_main
>>>               |
>>>               ---dl_main
>>>                  |
>>>                  |--11.82%--_dl_relocate_object
>>>                  |          |
>>>                  |          |--9.56%--0xffffffff85c01284
>>>                  |          |          0xffffffff8507b04c
>>>                  |          |          0xffffffff8507afe8
>>>                  |          |          0xffffffff8507aa59
>>>                  |          |          0xffffffff85253dda
>>>                  |          |          0xffffffff85253a35
>>>                  |          |          |
>>>                  |          |          |--3.81%--0xffffffff85a7831a
>>>                  |          |          |
>>>                  |          |          |--2.49%--0xffffffff85252e72
>>>
>>>
>>>
>>>>
>>>> Cheers,
>>>> Lieven
>>>>
>>>> Am 25.08.22 um 15:51 schrieb James Clark:
>>>>>
>>>>>
>>>>> On 25/08/2022 09:49, Lieven Hey wrote:
>>>>>> The missing header makes it hard for programs like elfutils to open
>>>>>> these files.
>>>>>
>>>>> Hi Lieven,
>>>>>
>>>>> Do you have an example command that demonstrates this? I'm
>>>>> interested in
>>>>> trying it out.
>>>>>
>>>>> I was also only aware of the elf files put in ~/.debug after perf
>>>>> record, but not perf inject.
>>>>>
>>>>> Thanks
>>>>> James
>>>>>
>>>>>> ---
>>>>>>     tools/perf/util/genelf.c | 14 ++++++++++++++
>>>>>>     tools/perf/util/genelf.h |  4 ++++
>>>>>>     2 files changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
>>>>>> index 953338b9e887..02cd9f75e3d2 100644
>>>>>> --- a/tools/perf/util/genelf.c
>>>>>> +++ b/tools/perf/util/genelf.c
>>>>>> @@ -251,6 +251,7 @@ jit_write_elf(int fd, uint64_t load_addr, const
>>>>>> char *sym,
>>>>>>         Elf_Data *d;
>>>>>>         Elf_Scn *scn;
>>>>>>         Elf_Ehdr *ehdr;
>>>>>> +    Elf_Phdr *phdr;
>>>>>>         Elf_Shdr *shdr;
>>>>>>         uint64_t eh_frame_base_offset;
>>>>>>         char *strsym = NULL;
>>>>>> @@ -285,6 +286,19 @@ jit_write_elf(int fd, uint64_t load_addr, const
>>>>>> char *sym,
>>>>>>         ehdr->e_version = EV_CURRENT;
>>>>>>         ehdr->e_shstrndx= unwinding ? 4 : 2; /* shdr index for section
>>>>>> name */
>>>>>>     +    /*
>>>>>> +     * setup program header
>>>>>> +     */
>>>>>> +    phdr = elf_newphdr(e, 1);
>>>>>> +    phdr[0].p_type = PT_LOAD;
>>>>>> +    phdr[0].p_offset = 0;
>>>>>> +    phdr[0].p_vaddr = 0;
>>>>>> +    phdr[0].p_paddr = 0;
>>>>>> +    phdr[0].p_filesz = csize;
>>>>>> +    phdr[0].p_memsz = csize;
>>>>>> +    phdr[0].p_flags = PF_X | PF_R;
>>>>>> +    phdr[0].p_align = 8;
>>>>>> +
>>>>>>         /*
>>>>>>          * setup text section
>>>>>>          */
>>>>>> diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
>>>>>> index ae138afe6c56..b5c909546e3f 100644
>>>>>> --- a/tools/perf/util/genelf.h
>>>>>> +++ b/tools/perf/util/genelf.h
>>>>>> @@ -53,8 +53,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr,
>>>>>> void *debug, int nr_debug_ent
>>>>>>       #if GEN_ELF_CLASS == ELFCLASS64
>>>>>>     #define elf_newehdr    elf64_newehdr
>>>>>> +#define elf_newphdr    elf64_newphdr
>>>>>>     #define elf_getshdr    elf64_getshdr
>>>>>>     #define Elf_Ehdr    Elf64_Ehdr
>>>>>> +#define Elf_Phdr    Elf64_Phdr
>>>>>>     #define Elf_Shdr    Elf64_Shdr
>>>>>>     #define Elf_Sym        Elf64_Sym
>>>>>>     #define ELF_ST_TYPE(a)    ELF64_ST_TYPE(a)
>>>>>> @@ -62,8 +64,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr,
>>>>>> void *debug, int nr_debug_ent
>>>>>>     #define ELF_ST_VIS(a)    ELF64_ST_VISIBILITY(a)
>>>>>>     #else
>>>>>>     #define elf_newehdr    elf32_newehdr
>>>>>> +#define elf_newphdr    elf32_newphdr
>>>>>>     #define elf_getshdr    elf32_getshdr
>>>>>>     #define Elf_Ehdr    Elf32_Ehdr
>>>>>> +#define Elf_Phdr    Elf32_Phdr
>>>>>>     #define Elf_Shdr    Elf32_Shdr
>>>>>>     #define Elf_Sym        Elf32_Sym
>>>>>>     #define ELF_ST_TYPE(a)    ELF32_ST_TYPE(a)
>>>>
>>

-- 
Lieven Hey | lieven.hey@kdab.com | Software Developer
KDAB (Deutschland) GmbH, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt, C++ and OpenGL Experts

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] include program header in elf files generated by perf inject
  2022-09-13 13:34           ` Lieven Hey
@ 2022-09-13 13:49             ` Leo Yan
  2022-09-13 14:34               ` Lieven Hey
  0 siblings, 1 reply; 8+ messages in thread
From: Leo Yan @ 2022-09-13 13:49 UTC (permalink / raw)
  To: Lieven Hey; +Cc: James Clark, linux-perf-users, acme

Hi Lieven,

On Tue, Sep 13, 2022 at 03:34:18PM +0200, Lieven Hey wrote:
> Hi,
> 
> perf is unable to open elf files generated by perf inject.
> 
> Example:
> Create a file named test.dart with the following content:
> 
> void main() {
>     print("Hello World");
> }
> 
> and then run
> 
> perf record --call-graph dwarf -k 1 dart --generate-perf-jitdump test.dart
> perf inject -j -i perf.data -o perf.data.jitted
> 
> The problem exists since 2d86612aacb78. The commit added some code that now
> requires an elf file to have a valid program header.

Sorry I missed this patch.

Could you confirm if the commit 6d518ac7be62 ("perf symbol: Fail to read
phdr workaround") is included in your local code base?

After a quick look for this patch, I think it is doing right thing.
Just want to check if this is the same issue with the one fixed by the
commit 6d518ac7be62.  I will review it in details in my next 1~2 days.

> > Can you reply-all on the original mailing list thread with this and a
> > 'to' to Leo. It's probably best to continue the discussion there so Leo
> > is aware.

Thanks a lot, James.

Leo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] include program header in elf files generated by perf inject
  2022-09-13 13:49             ` Leo Yan
@ 2022-09-13 14:34               ` Lieven Hey
  0 siblings, 0 replies; 8+ messages in thread
From: Lieven Hey @ 2022-09-13 14:34 UTC (permalink / raw)
  To: Leo Yan; +Cc: James Clark, linux-perf-users, acme

Hi Leo,

6d518ac7be62 was not in my code base. It fixes the issue, but not in my 
usecase. In my usecase I open the elf files with elfutils, but elfutils 
requires a program header to be present.

Cheers,
Lieven

Am 13.09.22 um 15:49 schrieb Leo Yan:
> Hi Lieven,
> 
> On Tue, Sep 13, 2022 at 03:34:18PM +0200, Lieven Hey wrote:
>> Hi,
>>
>> perf is unable to open elf files generated by perf inject.
>>
>> Example:
>> Create a file named test.dart with the following content:
>>
>> void main() {
>>      print("Hello World");
>> }
>>
>> and then run
>>
>> perf record --call-graph dwarf -k 1 dart --generate-perf-jitdump test.dart
>> perf inject -j -i perf.data -o perf.data.jitted
>>
>> The problem exists since 2d86612aacb78. The commit added some code that now
>> requires an elf file to have a valid program header.
> 
> Sorry I missed this patch.
> 
> Could you confirm if the commit 6d518ac7be62 ("perf symbol: Fail to read
> phdr workaround") is included in your local code base?
> 
> After a quick look for this patch, I think it is doing right thing.
> Just want to check if this is the same issue with the one fixed by the
> commit 6d518ac7be62.  I will review it in details in my next 1~2 days.
> 
>>> Can you reply-all on the original mailing list thread with this and a
>>> 'to' to Leo. It's probably best to continue the discussion there so Leo
>>> is aware.
> 
> Thanks a lot, James.
> 
> Leo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] include program header in elf files generated by perf inject
  2022-08-25  8:49 [PATCH] include program header in elf files generated by perf inject Lieven Hey
  2022-08-25 13:51 ` James Clark
@ 2022-09-14 13:52 ` Leo Yan
  2022-09-15  8:17   ` Lieven Hey
  1 sibling, 1 reply; 8+ messages in thread
From: Leo Yan @ 2022-09-14 13:52 UTC (permalink / raw)
  To: Lieven Hey, James Clark, Ian Rogers; +Cc: acme, linux-perf-users

Hi Lieven,

On Thu, Aug 25, 2022 at 10:49:23AM +0200, Lieven Hey wrote:
> The missing header makes it hard for programs like elfutils to open
> these files.

Several minor suggestions:

- I would suggest to change the subject as:
  perf jit: Include program header in elf files
- Please add your Signed-off-by tag in the commit log.
- I confirmed this patch works for Java jitted symbols with the test
  case [1], this is a real fixing for 2d86612aacb7805f, it's good to
  add the fix tag:
  Fixes: 2d86612aacb7805f ("perf symbol: Correct address for bss symbols")

[1] https://lore.kernel.org/lkml/20220806033434.121908-1-leo.yan@linaro.org/

I also followed you suggested commands to verify dart jitted symbols:

  # perf record --call-graph dwarf -k 1 dart --generate-perf-jitdump test.dart
  # perf inject -j -i perf.data -o perf.data.jitted
  # perf --debug verbose=4 report -i perf.data.jitted

Before change:

  unwind: find_proc_info dso /tmp/jitted-287756-38.so
  dso__load_sym_internal: failed to find program header for symbol: CallToRuntime st_value: 0x40
  dso__load_sym_internal: adjusting symbol: st_value: 0x40 sh_addr: 0x40 sh_offset: 0x40
  symbol__new: CallToRuntime 0x40-0xc0

After change:

  unwind: find_proc_info dso /tmp/jitted-287756-38.so
  dso__load_sym_internal: adjusting symbol: st_value: 0x40 p_vaddr: 0 p_offset: 0
  symbol__new: CallToRuntime 0x40-0xc0

Which proves the program header has been added for jitted symbols in
the generated elf file.

So with addressing above comments:

Reviewed-by: Leo Yan <leo.yan@linaro.org>
Tested-by: Leo Yan <leo.yan@linaro.org>


> ---
>  tools/perf/util/genelf.c | 14 ++++++++++++++
>  tools/perf/util/genelf.h |  4 ++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> index 953338b9e887..02cd9f75e3d2 100644
> --- a/tools/perf/util/genelf.c
> +++ b/tools/perf/util/genelf.c
> @@ -251,6 +251,7 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>  	Elf_Data *d;
>  	Elf_Scn *scn;
>  	Elf_Ehdr *ehdr;
> +	Elf_Phdr *phdr;
>  	Elf_Shdr *shdr;
>  	uint64_t eh_frame_base_offset;
>  	char *strsym = NULL;
> @@ -285,6 +286,19 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>  	ehdr->e_version = EV_CURRENT;
>  	ehdr->e_shstrndx= unwinding ? 4 : 2; /* shdr index for section name */
>  
> +	/*
> +	 * setup program header
> +	 */
> +	phdr = elf_newphdr(e, 1);
> +	phdr[0].p_type = PT_LOAD;
> +	phdr[0].p_offset = 0;
> +	phdr[0].p_vaddr = 0;
> +	phdr[0].p_paddr = 0;
> +	phdr[0].p_filesz = csize;
> +	phdr[0].p_memsz = csize;
> +	phdr[0].p_flags = PF_X | PF_R;
> +	phdr[0].p_align = 8;
> +
>  	/*
>  	 * setup text section
>  	 */
> diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
> index ae138afe6c56..b5c909546e3f 100644
> --- a/tools/perf/util/genelf.h
> +++ b/tools/perf/util/genelf.h
> @@ -53,8 +53,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
>  
>  #if GEN_ELF_CLASS == ELFCLASS64
>  #define elf_newehdr	elf64_newehdr
> +#define elf_newphdr	elf64_newphdr
>  #define elf_getshdr	elf64_getshdr
>  #define Elf_Ehdr	Elf64_Ehdr
> +#define Elf_Phdr	Elf64_Phdr
>  #define Elf_Shdr	Elf64_Shdr
>  #define Elf_Sym		Elf64_Sym
>  #define ELF_ST_TYPE(a)	ELF64_ST_TYPE(a)
> @@ -62,8 +64,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
>  #define ELF_ST_VIS(a)	ELF64_ST_VISIBILITY(a)
>  #else
>  #define elf_newehdr	elf32_newehdr
> +#define elf_newphdr	elf32_newphdr
>  #define elf_getshdr	elf32_getshdr
>  #define Elf_Ehdr	Elf32_Ehdr
> +#define Elf_Phdr	Elf32_Phdr
>  #define Elf_Shdr	Elf32_Shdr
>  #define Elf_Sym		Elf32_Sym
>  #define ELF_ST_TYPE(a)	ELF32_ST_TYPE(a)
> -- 
> 2.37.2

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] include program header in elf files generated by perf inject
  2022-09-14 13:52 ` Leo Yan
@ 2022-09-15  8:17   ` Lieven Hey
  2022-09-15  8:35     ` Leo Yan
  0 siblings, 1 reply; 8+ messages in thread
From: Lieven Hey @ 2022-09-15  8:17 UTC (permalink / raw)
  To: Leo Yan, James Clark, Ian Rogers; +Cc: acme, linux-perf-users

Hi Leo,

can I simply call git send-email after amending my commit or do I need 
to add some kind of id or reference to the old patch?

Am 14.09.22 um 15:52 schrieb Leo Yan:
> Hi Lieven,
> 
> On Thu, Aug 25, 2022 at 10:49:23AM +0200, Lieven Hey wrote:
>> The missing header makes it hard for programs like elfutils to open
>> these files.
> 
> Several minor suggestions:
> 
> - I would suggest to change the subject as:
>    perf jit: Include program header in elf files
> - Please add your Signed-off-by tag in the commit log.
> - I confirmed this patch works for Java jitted symbols with the test
>    case [1], this is a real fixing for 2d86612aacb7805f, it's good to
>    add the fix tag:
>    Fixes: 2d86612aacb7805f ("perf symbol: Correct address for bss symbols")
> 
> [1] https://lore.kernel.org/lkml/20220806033434.121908-1-leo.yan@linaro.org/
> 
> I also followed you suggested commands to verify dart jitted symbols:
> 
>    # perf record --call-graph dwarf -k 1 dart --generate-perf-jitdump test.dart
>    # perf inject -j -i perf.data -o perf.data.jitted
>    # perf --debug verbose=4 report -i perf.data.jitted
> 
> Before change:
> 
>    unwind: find_proc_info dso /tmp/jitted-287756-38.so
>    dso__load_sym_internal: failed to find program header for symbol: CallToRuntime st_value: 0x40
>    dso__load_sym_internal: adjusting symbol: st_value: 0x40 sh_addr: 0x40 sh_offset: 0x40
>    symbol__new: CallToRuntime 0x40-0xc0
> 
> After change:
> 
>    unwind: find_proc_info dso /tmp/jitted-287756-38.so
>    dso__load_sym_internal: adjusting symbol: st_value: 0x40 p_vaddr: 0 p_offset: 0
>    symbol__new: CallToRuntime 0x40-0xc0
> 
> Which proves the program header has been added for jitted symbols in
> the generated elf file.
> 
> So with addressing above comments:
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> Tested-by: Leo Yan <leo.yan@linaro.org>
> 
> 
>> ---
>>   tools/perf/util/genelf.c | 14 ++++++++++++++
>>   tools/perf/util/genelf.h |  4 ++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
>> index 953338b9e887..02cd9f75e3d2 100644
>> --- a/tools/perf/util/genelf.c
>> +++ b/tools/perf/util/genelf.c
>> @@ -251,6 +251,7 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>>   	Elf_Data *d;
>>   	Elf_Scn *scn;
>>   	Elf_Ehdr *ehdr;
>> +	Elf_Phdr *phdr;
>>   	Elf_Shdr *shdr;
>>   	uint64_t eh_frame_base_offset;
>>   	char *strsym = NULL;
>> @@ -285,6 +286,19 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>>   	ehdr->e_version = EV_CURRENT;
>>   	ehdr->e_shstrndx= unwinding ? 4 : 2; /* shdr index for section name */
>>   
>> +	/*
>> +	 * setup program header
>> +	 */
>> +	phdr = elf_newphdr(e, 1);
>> +	phdr[0].p_type = PT_LOAD;
>> +	phdr[0].p_offset = 0;
>> +	phdr[0].p_vaddr = 0;
>> +	phdr[0].p_paddr = 0;
>> +	phdr[0].p_filesz = csize;
>> +	phdr[0].p_memsz = csize;
>> +	phdr[0].p_flags = PF_X | PF_R;
>> +	phdr[0].p_align = 8;
>> +
>>   	/*
>>   	 * setup text section
>>   	 */
>> diff --git a/tools/perf/util/genelf.h b/tools/perf/util/genelf.h
>> index ae138afe6c56..b5c909546e3f 100644
>> --- a/tools/perf/util/genelf.h
>> +++ b/tools/perf/util/genelf.h
>> @@ -53,8 +53,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
>>   
>>   #if GEN_ELF_CLASS == ELFCLASS64
>>   #define elf_newehdr	elf64_newehdr
>> +#define elf_newphdr	elf64_newphdr
>>   #define elf_getshdr	elf64_getshdr
>>   #define Elf_Ehdr	Elf64_Ehdr
>> +#define Elf_Phdr	Elf64_Phdr
>>   #define Elf_Shdr	Elf64_Shdr
>>   #define Elf_Sym		Elf64_Sym
>>   #define ELF_ST_TYPE(a)	ELF64_ST_TYPE(a)
>> @@ -62,8 +64,10 @@ int jit_add_debug_info(Elf *e, uint64_t code_addr, void *debug, int nr_debug_ent
>>   #define ELF_ST_VIS(a)	ELF64_ST_VISIBILITY(a)
>>   #else
>>   #define elf_newehdr	elf32_newehdr
>> +#define elf_newphdr	elf32_newphdr
>>   #define elf_getshdr	elf32_getshdr
>>   #define Elf_Ehdr	Elf32_Ehdr
>> +#define Elf_Phdr	Elf32_Phdr
>>   #define Elf_Shdr	Elf32_Shdr
>>   #define Elf_Sym		Elf32_Sym
>>   #define ELF_ST_TYPE(a)	ELF32_ST_TYPE(a)
>> -- 
>> 2.37.2

-- 
Lieven Hey | lieven.hey@kdab.com | Software Developer
KDAB (Deutschland) GmbH, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt, C++ and OpenGL Experts

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] include program header in elf files generated by perf inject
  2022-09-15  8:17   ` Lieven Hey
@ 2022-09-15  8:35     ` Leo Yan
  0 siblings, 0 replies; 8+ messages in thread
From: Leo Yan @ 2022-09-15  8:35 UTC (permalink / raw)
  To: Lieven Hey; +Cc: James Clark, Ian Rogers, acme, linux-perf-users

Hi Lieven,

On Thu, Sep 15, 2022 at 10:17:17AM +0200, Lieven Hey wrote:
> Hi Leo,
> 
> can I simply call git send-email after amending my commit or do I need to
> add some kind of id or reference to the old patch?

Yeah, you just need to use "git send-email" to send the patch; so
far we doesn't need something like gerrit's SHA ID.

Some tips: it's good to use b4 tool and generate patch with increment
version number, this would be easier for maintainers to track your
patch.  Something like:

  $ cd $LINUX_SRC

  # Download the patch v1 and append review / test tags automatically
  $ b4 am 20220825084923.167140-1-lieven.hey@kdab.com
  $ git am 20220825_lieven_hey_include_program_header_in_elf_files_generated_by_perf_inject.mbx

  # Do yourself's modificantion for the change ....

  # ... Generate the patch v2
  $ git format-patch -v2 HEAD~1
  $ git send-email v2-...patch

Thanks,
Leo

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-09-15  8:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-25  8:49 [PATCH] include program header in elf files generated by perf inject Lieven Hey
2022-08-25 13:51 ` James Clark
     [not found]   ` <6e0ebfb6-9fbf-073c-94b0-a799b9ec657f@kdab.com>
     [not found]     ` <e3c57763-eb3d-e8cc-c3b7-36a33e86ad6a@arm.com>
     [not found]       ` <1cc13a5d-bb41-edbb-ade7-62d4a678c0f9@kdab.com>
     [not found]         ` <cf2f3f27-1002-dbd0-d851-465b169c2031@arm.com>
2022-09-13 13:34           ` Lieven Hey
2022-09-13 13:49             ` Leo Yan
2022-09-13 14:34               ` Lieven Hey
2022-09-14 13:52 ` Leo Yan
2022-09-15  8:17   ` Lieven Hey
2022-09-15  8:35     ` Leo Yan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).