[PATCH 1/2] perf annotate: Fallback to objdump when capstone fails

[PATCH 1/2] perf annotate: Fallback to objdump when capstone fails

[Namhyung Kim]

I found some cases that capstone failed to disassemble.  Probably my
capstone is an old version but anyway there's a chance it can fail.  And
then it silently stopped in the middle.  In my case, it didn't
understand "RDPKRU" instruction.

Let's check if the capstone disassemble reached to the end of the
function.  And fallback to objdump if not.

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

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 92937809be85..412101f2cf2a 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1542,6 +1542,20 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
 		offset += insn[i].size;
 	}
 
+	/* It failed in the middle: probably due to unknown instructions */
+	if (offset != len) {
+		struct list_head *list = &notes->src->source;
+
+		/* Discard all lines and fallback to objdump */
+		while (!list_empty(list)) {
+			dl = list_first_entry(list, struct disasm_line, al.node);
+
+			list_del_init(&dl->al.node);
+			disasm_line__free(dl);
+		}
+		count = -1;
+	}
+
 out:
 	if (needs_cs_close)
 		cs_close(&handle);
-- 
2.44.0.769.g3c40516874-goog

[PATCH 2/2] perf annotate: Update dso binary type when try build-id

[Namhyung Kim]

dso__disassemble_filename() tries to get the filename for objdump (or
capstone) using build-id.  But I found sometimes it didn't disassemble
some functions.  It turned out that those functions belong to a dso
which has no binary type set.  It seems it sets the binary type for some
special files only - like kernel (kallsyms or kcore) or BPF images.  And
there's a logic to skip dso with DSO_BINARY_TYPE__NOT_FOUND.

As it's checked the build-id cache linke, it should set the binary type
as DSO_BINARY_TYPE__BUILD_ID_CACHE.

Fixes: 873a83731f1c ("perf annotate: Skip DSOs not found")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/disasm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 412101f2cf2a..6d1125e687b7 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 			}
 		}
 		mutex_unlock(&dso->lock);
+	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
+		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
 	}
 
 	free(build_id_path);
-- 
2.44.0.769.g3c40516874-goog

Re: [PATCH 2/2] perf annotate: Update dso binary type when try build-id

[Arnaldo Carvalho de Melo]

On Wed, Apr 24, 2024 at 05:51:57PM -0700, Namhyung Kim wrote:
> dso__disassemble_filename() tries to get the filename for objdump (or
> capstone) using build-id.  But I found sometimes it didn't disassemble
> some functions.  It turned out that those functions belong to a dso
> which has no binary type set.  It seems it sets the binary type for some
> special files only - like kernel (kallsyms or kcore) or BPF images.  And
> there's a logic to skip dso with DSO_BINARY_TYPE__NOT_FOUND.
> 
> As it's checked the build-id cache linke, it should set the binary type
> as DSO_BINARY_TYPE__BUILD_ID_CACHE.
> 
> Fixes: 873a83731f1c ("perf annotate: Skip DSOs not found")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/disasm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 412101f2cf2a..6d1125e687b7 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  			}
>  		}
>  		mutex_unlock(&dso->lock);
> +	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
> +		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
>  	}
>  
>  	free(build_id_path);

Fixed up to take into account a recent patch by Ian that turned that
&dso->lock into dso__lock(dso):

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 70650808e2e7bf88..2921b32357705a02 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 			}
 		}
 		mutex_unlock(dso__lock(dso));
+	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
+		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
 	}
 
 	free(build_id_path);

Re: [PATCH 2/2] perf annotate: Update dso binary type when try build-id

[Arnaldo Carvalho de Melo]

On Thu, Apr 25, 2024 at 11:12:40AM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Apr 24, 2024 at 05:51:57PM -0700, Namhyung Kim wrote:
> > +++ b/tools/perf/util/disasm.c
> > @@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> >  		mutex_unlock(&dso->lock);
> > +	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
> > +		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
> >  	}
 
> Fixed up to take into account a recent patch by Ian that turned that
> &dso->lock into dso__lock(dso):
 
> +++ b/tools/perf/util/disasm.c
> @@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>  			}
>  		}
>  		mutex_unlock(dso__lock(dso));
> +	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
> +		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
>  	}

Nah, I forgot some more stuff, this is what I have now:

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 2921b32357705a02..72aec8f61b944a7a 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1156,8 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
 			}
 		}
 		mutex_unlock(dso__lock(dso));
-	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
-		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
+	} else if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
+		dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
 	}
 
 	free(build_id_path);

Re: [PATCH 2/2] perf annotate: Update dso binary type when try build-id

[Arnaldo Carvalho de Melo]

On Thu, Apr 25, 2024 at 11:49:49AM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Apr 25, 2024 at 11:12:40AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Wed, Apr 24, 2024 at 05:51:57PM -0700, Namhyung Kim wrote:
> > > +++ b/tools/perf/util/disasm.c
> > > @@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> > >  		mutex_unlock(&dso->lock);
> > > +	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
> > > +		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
> > >  	}
>  
> > Fixed up to take into account a recent patch by Ian that turned that
> > &dso->lock into dso__lock(dso):
>  
> > +++ b/tools/perf/util/disasm.c
> > @@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> >  			}
> >  		}
> >  		mutex_unlock(dso__lock(dso));
> > +	} else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
> > +		dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
> >  	}
> 
> Nah, I forgot some more stuff, this is what I have now:

Nah², I had to remove all these:

pick a58b4da77b40920f perf dsos: Switch backing storage to array from rbtree/list 
pick 7d91cefd1fb63068 perf dsos: Remove __dsos__addnew()
pick 80c3ccf05199dbb6 perf dsos: Remove __dsos__findnew_link_by_longname_id()
pick af3f8dea24f47802 perf dsos: Switch hand code to bsearch()
pick 7537b92b48318834 perf dso: Add reference count checking and accessor functions
pick 9bd7c6fe8de22b37 perf dso: Reference counting related fixes
pick 4de57b46a0cb2027 perf dso: Use container_of() to avoid a pointer in 'struct dso_data'

Due to a bisect that pointed "perf dsos: Switch backing storage to array
from rbtree/list" as the one where:

 
root@x1:~# perf test "kernel lock contention analysis test"
 87: kernel lock contention analysis test                            : FAILED!

- Arnaldo

Re: [PATCH 2/2] perf annotate: Update dso binary type when try build-id

[Ian Rogers]

On Thu, Apr 25, 2024 at 7:12 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Wed, Apr 24, 2024 at 05:51:57PM -0700, Namhyung Kim wrote:
> > dso__disassemble_filename() tries to get the filename for objdump (or
> > capstone) using build-id.  But I found sometimes it didn't disassemble
> > some functions.  It turned out that those functions belong to a dso
> > which has no binary type set.  It seems it sets the binary type for some
> > special files only - like kernel (kallsyms or kcore) or BPF images.  And
> > there's a logic to skip dso with DSO_BINARY_TYPE__NOT_FOUND.
> >
> > As it's checked the build-id cache linke, it should set the binary type
> > as DSO_BINARY_TYPE__BUILD_ID_CACHE.
> >
> > Fixes: 873a83731f1c ("perf annotate: Skip DSOs not found")
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/disasm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > index 412101f2cf2a..6d1125e687b7 100644
> > --- a/tools/perf/util/disasm.c
> > +++ b/tools/perf/util/disasm.c
> > @@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> >                       }
> >               }
> >               mutex_unlock(&dso->lock);
> > +     } else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {
> > +             dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
> >       }
> >
> >       free(build_id_path);
>
> Fixed up to take into account a recent patch by Ian that turned that
> &dso->lock into dso__lock(dso):
>
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 70650808e2e7bf88..2921b32357705a02 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1156,6 +1156,8 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>                         }
>                 }
>                 mutex_unlock(dso__lock(dso));
> +       } else if (dso->binary_type == DSO_BINARY_TYPE__NOT_FOUND) {

With reference count checking on dsos, this will need to be:

dso__binary_type(dso)

too.

Thanks,
Ian

> +               dso->binary_type = DSO_BINARY_TYPE__BUILD_ID_CACHE;
>         }
>
>         free(build_id_path);

Re: [PATCH 1/2] perf annotate: Fallback to objdump when capstone fails

[Arnaldo Carvalho de Melo]

On Wed, Apr 24, 2024 at 05:51:56PM -0700, Namhyung Kim wrote:
> I found some cases that capstone failed to disassemble.  Probably my
> capstone is an old version but anyway there's a chance it can fail.  And
> then it silently stopped in the middle.  In my case, it didn't
> understand "RDPKRU" instruction.
> 
> Let's check if the capstone disassemble reached to the end of the
> function.  And fallback to objdump if not.

Thanks, applied to perf-tools-next,

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/disasm.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 92937809be85..412101f2cf2a 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1542,6 +1542,20 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>  		offset += insn[i].size;
>  	}
>  
> +	/* It failed in the middle: probably due to unknown instructions */
> +	if (offset != len) {
> +		struct list_head *list = &notes->src->source;
> +
> +		/* Discard all lines and fallback to objdump */
> +		while (!list_empty(list)) {
> +			dl = list_first_entry(list, struct disasm_line, al.node);
> +
> +			list_del_init(&dl->al.node);
> +			disasm_line__free(dl);
> +		}
> +		count = -1;
> +	}
> +
>  out:
>  	if (needs_cs_close)
>  		cs_close(&handle);
> -- 
> 2.44.0.769.g3c40516874-goog

Re: [PATCH 1/2] perf annotate: Fallback to objdump when capstone fails

[Arnaldo Carvalho de Melo]

On Wed, Apr 24, 2024 at 05:51:56PM -0700, Namhyung Kim wrote:
> I found some cases that capstone failed to disassemble.  Probably my
> capstone is an old version but anyway there's a chance it can fail.  And
> then it silently stopped in the middle.  In my case, it didn't
> understand "RDPKRU" instruction.
> 
> Let's check if the capstone disassemble reached to the end of the
> function.  And fallback to objdump if not.

Humm:

⬢[acme@toolbox perf-tools-next]$        git am ./20240424_namhyung_perf_annotate_fallback_to_objdump_when_capstone_fails.mbx
Applying: perf annotate: Fallback to objdump when capstone fails
Applying: perf annotate: Update dso binary type when try build-id
error: patch failed: tools/perf/util/disasm.c:1156
error: tools/perf/util/disasm.c: patch does not apply
Patch failed at 0002 perf annotate: Update dso binary type when try build-id
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
⬢[acme@toolbox perf-tools-next]$ git am --abort
⬢[acme@toolbox perf-tools-next]$

checking...

- Arnado
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/disasm.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 92937809be85..412101f2cf2a 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1542,6 +1542,20 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>  		offset += insn[i].size;
>  	}
>  
> +	/* It failed in the middle: probably due to unknown instructions */
> +	if (offset != len) {
> +		struct list_head *list = &notes->src->source;
> +
> +		/* Discard all lines and fallback to objdump */
> +		while (!list_empty(list)) {
> +			dl = list_first_entry(list, struct disasm_line, al.node);
> +
> +			list_del_init(&dl->al.node);
> +			disasm_line__free(dl);
> +		}
> +		count = -1;
> +	}
> +
>  out:
>  	if (needs_cs_close)
>  		cs_close(&handle);
> -- 
> 2.44.0.769.g3c40516874-goog