* [PATCH 2/4] perf report: Load PE files from debug cache only
@ 2021-02-10 19:17 Nicholas Fraser
2021-02-12 12:28 ` Arnaldo Carvalho de Melo
2021-02-15 14:36 ` Jiri Olsa
0 siblings, 2 replies; 6+ messages in thread
From: Nicholas Fraser @ 2021-02-10 19:17 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Frank Ch. Eigler, Song Liu, Adrian Hunter,
Kim Phillips, Tommi Rantala, Remi Bernon, linux-kernel
Cc: Ulrich Czekalla, Huw Davies
dso__load_bfd_symbols() attempts to load a DSO at its original path,
then closes it and loads the file in the debug cache. This is incorrect.
It should ignore the original file and work with only the debug cache.
The original file may have changed or may not even exist, for example if
the debug cache has been transferred to another machine via "perf
archive".
This fix makes it only load the file in the debug cache.
Signed-off-by: Nicholas Fraser <nfraser@codeweavers.com>
---
tools/perf/util/symbol.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 64a039cbba1b..aa9ae875b995 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1569,7 +1569,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
u_int i;
u64 start, len;
- abfd = bfd_openr(dso->long_name, NULL);
+ abfd = bfd_openr(debugfile, NULL);
if (!abfd)
return -1;
@@ -1586,12 +1586,6 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
if (section)
dso->text_offset = section->vma - section->filepos;
- bfd_close(abfd);
-
- abfd = bfd_openr(debugfile, NULL);
- if (!abfd)
- return -1;
-
if (!bfd_check_format(abfd, bfd_object)) {
pr_debug2("%s: cannot read %s bfd file.\n", __func__,
debugfile);
--
2.30.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] perf report: Load PE files from debug cache only
2021-02-10 19:17 [PATCH 2/4] perf report: Load PE files from debug cache only Nicholas Fraser
@ 2021-02-12 12:28 ` Arnaldo Carvalho de Melo
2021-02-12 16:34 ` Nicholas Fraser
2021-02-15 14:36 ` Jiri Olsa
1 sibling, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-12 12:28 UTC (permalink / raw)
To: Nicholas Fraser
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Frank Ch. Eigler, Song Liu,
Adrian Hunter, Kim Phillips, Tommi Rantala, Remi Bernon,
linux-kernel, Ulrich Czekalla, Huw Davies
Em Wed, Feb 10, 2021 at 02:17:38PM -0500, Nicholas Fraser escreveu:
> dso__load_bfd_symbols() attempts to load a DSO at its original path,
> then closes it and loads the file in the debug cache. This is incorrect.
> It should ignore the original file and work with only the debug cache.
> The original file may have changed or may not even exist, for example if
> the debug cache has been transferred to another machine via "perf
> archive".
>
> This fix makes it only load the file in the debug cache.
Well this improves your current use case and only affects PE files, so I
am applying, but consider a slightly different workflow:
1. perf record ./foo.exe
2. perf report # works, finds the file in the ~/.debug cache, as stored
# by 'perf record'
3. rm -rf ~/.debug # I need more space
4. perf report # Fails, as it looks only in the ~/.debug cache, that
# was nuked
So at 4 it should look at the original pathname, and hope for the best.
All this is moot if we have something like a build-id in PE files,
where we can look in any order since we'll verify the unique ID to see
if it is the one we need, right?
- Arnaldo
> Signed-off-by: Nicholas Fraser <nfraser@codeweavers.com>
> ---
> tools/perf/util/symbol.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 64a039cbba1b..aa9ae875b995 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1569,7 +1569,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
> u_int i;
> u64 start, len;
>
> - abfd = bfd_openr(dso->long_name, NULL);
> + abfd = bfd_openr(debugfile, NULL);
> if (!abfd)
> return -1;
>
> @@ -1586,12 +1586,6 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
> if (section)
> dso->text_offset = section->vma - section->filepos;
>
> - bfd_close(abfd);
> -
> - abfd = bfd_openr(debugfile, NULL);
> - if (!abfd)
> - return -1;
> -
> if (!bfd_check_format(abfd, bfd_object)) {
> pr_debug2("%s: cannot read %s bfd file.\n", __func__,
> debugfile);
> --
> 2.30.0
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] perf report: Load PE files from debug cache only
2021-02-12 12:28 ` Arnaldo Carvalho de Melo
@ 2021-02-12 16:34 ` Nicholas Fraser
2021-02-12 21:17 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Nicholas Fraser @ 2021-02-12 16:34 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Frank Ch. Eigler, Song Liu,
Adrian Hunter, Kim Phillips, Tommi Rantala, Remi Bernon,
linux-kernel, Ulrich Czekalla, Huw Davies
Sorry, I should have been more clear in the commit message. The use case
you outlined still works even with this patch.
dso__load_bfd_symbols() is called in a loop from dso__load() for a variety
of paths. These are generated by the various DSO_BINARY_TYPEs in the
binary_type_symtab list at the top of util/symbol.c. In each case the
debugfile passed to dso__load_bfd_symbols() is the path to try.
One of those iterations (the first one I believe) passes the original path
as the debugfile. If the file still exists at the original path, this is
the one that ends up being used in case the debugcache was deleted or the
PE file doesn't have a build-id.
A later iteration (BUILD_ID_CACHE) passes debugfile as the file in the
debugcache if it has a build-id. Even if the file was previously loaded at
its original path, (if I understand correctly) this load will override it
so the debugcache file ends up being used.
Nick
On 2021-02-12 7:28 a.m., Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 10, 2021 at 02:17:38PM -0500, Nicholas Fraser escreveu:
>> dso__load_bfd_symbols() attempts to load a DSO at its original path,
>> then closes it and loads the file in the debug cache. This is incorrect.
>> It should ignore the original file and work with only the debug cache.
>> The original file may have changed or may not even exist, for example if
>> the debug cache has been transferred to another machine via "perf
>> archive".
>>
>> This fix makes it only load the file in the debug cache.
>
> Well this improves your current use case and only affects PE files, so I
> am applying, but consider a slightly different workflow:
>
> 1. perf record ./foo.exe
> 2. perf report # works, finds the file in the ~/.debug cache, as stored
> # by 'perf record'
> 3. rm -rf ~/.debug # I need more space
> 4. perf report # Fails, as it looks only in the ~/.debug cache, that
> # was nuked
>
>
> So at 4 it should look at the original pathname, and hope for the best.
>
> All this is moot if we have something like a build-id in PE files,
> where we can look in any order since we'll verify the unique ID to see
> if it is the one we need, right?
>
> - Arnaldo
>
>> Signed-off-by: Nicholas Fraser <nfraser@codeweavers.com>
>> ---
>> tools/perf/util/symbol.c | 8 +-------
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 64a039cbba1b..aa9ae875b995 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1569,7 +1569,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
>> u_int i;
>> u64 start, len;
>>
>> - abfd = bfd_openr(dso->long_name, NULL);
>> + abfd = bfd_openr(debugfile, NULL);
>> if (!abfd)
>> return -1;
>>
>> @@ -1586,12 +1586,6 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
>> if (section)
>> dso->text_offset = section->vma - section->filepos;
>>
>> - bfd_close(abfd);
>> -
>> - abfd = bfd_openr(debugfile, NULL);
>> - if (!abfd)
>> - return -1;
>> -
>> if (!bfd_check_format(abfd, bfd_object)) {
>> pr_debug2("%s: cannot read %s bfd file.\n", __func__,
>> debugfile);
>> --
>> 2.30.0
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] perf report: Load PE files from debug cache only
2021-02-12 16:34 ` Nicholas Fraser
@ 2021-02-12 21:17 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-12 21:17 UTC (permalink / raw)
To: Nicholas Fraser
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Frank Ch. Eigler, Song Liu,
Adrian Hunter, Kim Phillips, Tommi Rantala, Remi Bernon,
linux-kernel, Ulrich Czekalla, Huw Davies
Em Fri, Feb 12, 2021 at 11:34:24AM -0500, Nicholas Fraser escreveu:
> Sorry, I should have been more clear in the commit message. The use case
> you outlined still works even with this patch.
Ok, I'll clarify that in the commit log then.
- Arnaldo
> dso__load_bfd_symbols() is called in a loop from dso__load() for a variety
> of paths. These are generated by the various DSO_BINARY_TYPEs in the
> binary_type_symtab list at the top of util/symbol.c. In each case the
> debugfile passed to dso__load_bfd_symbols() is the path to try.
>
> One of those iterations (the first one I believe) passes the original path
> as the debugfile. If the file still exists at the original path, this is
> the one that ends up being used in case the debugcache was deleted or the
> PE file doesn't have a build-id.
>
> A later iteration (BUILD_ID_CACHE) passes debugfile as the file in the
> debugcache if it has a build-id. Even if the file was previously loaded at
> its original path, (if I understand correctly) this load will override it
> so the debugcache file ends up being used.
>
> Nick
>
>
> On 2021-02-12 7:28 a.m., Arnaldo Carvalho de Melo wrote:
> > Em Wed, Feb 10, 2021 at 02:17:38PM -0500, Nicholas Fraser escreveu:
> >> dso__load_bfd_symbols() attempts to load a DSO at its original path,
> >> then closes it and loads the file in the debug cache. This is incorrect.
> >> It should ignore the original file and work with only the debug cache.
> >> The original file may have changed or may not even exist, for example if
> >> the debug cache has been transferred to another machine via "perf
> >> archive".
> >>
> >> This fix makes it only load the file in the debug cache.
> >
> > Well this improves your current use case and only affects PE files, so I
> > am applying, but consider a slightly different workflow:
> >
> > 1. perf record ./foo.exe
> > 2. perf report # works, finds the file in the ~/.debug cache, as stored
> > # by 'perf record'
> > 3. rm -rf ~/.debug # I need more space
> > 4. perf report # Fails, as it looks only in the ~/.debug cache, that
> > # was nuked
> >
> >
> > So at 4 it should look at the original pathname, and hope for the best.
> >
> > All this is moot if we have something like a build-id in PE files,
> > where we can look in any order since we'll verify the unique ID to see
> > if it is the one we need, right?
> >
> > - Arnaldo
> >
> >> Signed-off-by: Nicholas Fraser <nfraser@codeweavers.com>
> >> ---
> >> tools/perf/util/symbol.c | 8 +-------
> >> 1 file changed, 1 insertion(+), 7 deletions(-)
> >>
> >> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> >> index 64a039cbba1b..aa9ae875b995 100644
> >> --- a/tools/perf/util/symbol.c
> >> +++ b/tools/perf/util/symbol.c
> >> @@ -1569,7 +1569,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
> >> u_int i;
> >> u64 start, len;
> >>
> >> - abfd = bfd_openr(dso->long_name, NULL);
> >> + abfd = bfd_openr(debugfile, NULL);
> >> if (!abfd)
> >> return -1;
> >>
> >> @@ -1586,12 +1586,6 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
> >> if (section)
> >> dso->text_offset = section->vma - section->filepos;
> >>
> >> - bfd_close(abfd);
> >> -
> >> - abfd = bfd_openr(debugfile, NULL);
> >> - if (!abfd)
> >> - return -1;
> >> -
> >> if (!bfd_check_format(abfd, bfd_object)) {
> >> pr_debug2("%s: cannot read %s bfd file.\n", __func__,
> >> debugfile);
> >> --
> >> 2.30.0
> >>
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] perf report: Load PE files from debug cache only
2021-02-10 19:17 [PATCH 2/4] perf report: Load PE files from debug cache only Nicholas Fraser
2021-02-12 12:28 ` Arnaldo Carvalho de Melo
@ 2021-02-15 14:36 ` Jiri Olsa
2021-02-16 15:55 ` Nicholas Fraser
1 sibling, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2021-02-15 14:36 UTC (permalink / raw)
To: Nicholas Fraser
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Ian Rogers,
Frank Ch. Eigler, Song Liu, Adrian Hunter, Kim Phillips,
Tommi Rantala, Remi Bernon, linux-kernel, Ulrich Czekalla,
Huw Davies
On Wed, Feb 10, 2021 at 02:17:38PM -0500, Nicholas Fraser wrote:
> dso__load_bfd_symbols() attempts to load a DSO at its original path,
> then closes it and loads the file in the debug cache. This is incorrect.
> It should ignore the original file and work with only the debug cache.
> The original file may have changed or may not even exist, for example if
> the debug cache has been transferred to another machine via "perf
> archive".
>
> This fix makes it only load the file in the debug cache.
>
> Signed-off-by: Nicholas Fraser <nfraser@codeweavers.com>
> ---
> tools/perf/util/symbol.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 64a039cbba1b..aa9ae875b995 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1569,7 +1569,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
> u_int i;
> u64 start, len;
>
> - abfd = bfd_openr(dso->long_name, NULL);
> + abfd = bfd_openr(debugfile, NULL);
> if (!abfd)
> return -1;
>
> @@ -1586,12 +1586,6 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
> if (section)
> dso->text_offset = section->vma - section->filepos;
>
> - bfd_close(abfd);
> -
> - abfd = bfd_openr(debugfile, NULL);
> - if (!abfd)
> - return -1;
> -
hum, should you also remove the following code?
if (!bfd_check_format(abfd, bfd_object)) {
pr_debug2("%s: cannot read %s bfd file.\n", __func__,
debugfile);
goto out_close;
}
if (bfd_get_flavour(abfd) == bfd_target_elf_flavour)
goto out_close;
it seems to be called already above
thanks,
jirka
> if (!bfd_check_format(abfd, bfd_object)) {
> pr_debug2("%s: cannot read %s bfd file.\n", __func__,
> debugfile);
> --
> 2.30.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] perf report: Load PE files from debug cache only
2021-02-15 14:36 ` Jiri Olsa
@ 2021-02-16 15:55 ` Nicholas Fraser
0 siblings, 0 replies; 6+ messages in thread
From: Nicholas Fraser @ 2021-02-16 15:55 UTC (permalink / raw)
To: Jiri Olsa
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Namhyung Kim, Ian Rogers,
Frank Ch. Eigler, Song Liu, Adrian Hunter, Kim Phillips,
Tommi Rantala, Remi Bernon, linux-kernel, Ulrich Czekalla,
Huw Davies
Oops, you are correct; it is doing these checks twice. Not sure how I missed that. I'll send another patch to fix it.
Nick
On 2021-02-15 9:36 a.m., Jiri Olsa wrote:
> On Wed, Feb 10, 2021 at 02:17:38PM -0500, Nicholas Fraser wrote:
>> dso__load_bfd_symbols() attempts to load a DSO at its original path,
>> then closes it and loads the file in the debug cache. This is incorrect.
>> It should ignore the original file and work with only the debug cache.
>> The original file may have changed or may not even exist, for example if
>> the debug cache has been transferred to another machine via "perf
>> archive".
>>
>> This fix makes it only load the file in the debug cache.
>>
>> Signed-off-by: Nicholas Fraser <nfraser@codeweavers.com>
>> ---
>> tools/perf/util/symbol.c | 8 +-------
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 64a039cbba1b..aa9ae875b995 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1569,7 +1569,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
>> u_int i;
>> u64 start, len;
>>
>> - abfd = bfd_openr(dso->long_name, NULL);
>> + abfd = bfd_openr(debugfile, NULL);
>> if (!abfd)
>> return -1;
>>
>> @@ -1586,12 +1586,6 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
>> if (section)
>> dso->text_offset = section->vma - section->filepos;
>>
>> - bfd_close(abfd);
>> -
>> - abfd = bfd_openr(debugfile, NULL);
>> - if (!abfd)
>> - return -1;
>> -
>
> hum, should you also remove the following code?
>
> if (!bfd_check_format(abfd, bfd_object)) {
> pr_debug2("%s: cannot read %s bfd file.\n", __func__,
> debugfile);
> goto out_close;
> }
>
> if (bfd_get_flavour(abfd) == bfd_target_elf_flavour)
> goto out_close;
>
> it seems to be called already above
>
> thanks,
> jirka
>
>> if (!bfd_check_format(abfd, bfd_object)) {
>> pr_debug2("%s: cannot read %s bfd file.\n", __func__,
>> debugfile);
>> --
>> 2.30.0
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-16 15:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10 19:17 [PATCH 2/4] perf report: Load PE files from debug cache only Nicholas Fraser
2021-02-12 12:28 ` Arnaldo Carvalho de Melo
2021-02-12 16:34 ` Nicholas Fraser
2021-02-12 21:17 ` Arnaldo Carvalho de Melo
2021-02-15 14:36 ` Jiri Olsa
2021-02-16 15:55 ` Nicholas Fraser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.