[PATCH 1/2] perf symbol: Set binary_type of dso when loading

[PATCH 1/2] perf symbol: Set binary_type of dso when loading

[Namhyung Kim]

For the kernel dso, it sets the binary type of dso when loading the
symbol table.  But it seems not to do that for user DSOs.  Actually
it sets the symtab type only.  It's not clear why we want to maintain
the two separately but it uses the binary type info before getting
the disassembly.

Let's use the symtab type as binary type too if it's not set.  I think
it's ok to set the binary type when it founds a symsrc whether or not
it has actual symbols.

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

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 7772a4d3e66c..63936269ec15 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1927,6 +1927,9 @@ int dso__load(struct dso *dso, struct map *map)
 		if (next_slot) {
 			ss_pos++;
 
+			if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
+				dso__set_binary_type(dso, symtab_type);
+
 			if (syms_ss && runtime_ss)
 				break;
 		} else {
-- 
2.44.0.769.g3c40516874-goog

[PATCH 2/2] perf symbol: Simplify kernel module checking

[Namhyung Kim]

In dso__load(), it checks if the dso is a kernel module by looking the
symtab type.  Actually dso has 'is_kmod' field to check that easily and
dso__set_module_info() set the symtab type and the is_kmod bit.  So it
should have the same result to check the is_kmod bit.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 63936269ec15..50969c687135 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1811,10 +1811,7 @@ int dso__load(struct dso *dso, struct map *map)
 		goto out;
 	}
 
-	kmod = dso__symtab_type(dso) == DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE ||
-		dso__symtab_type(dso) == DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP ||
-		dso__symtab_type(dso) == DSO_BINARY_TYPE__GUEST_KMODULE ||
-		dso__symtab_type(dso) == DSO_BINARY_TYPE__GUEST_KMODULE_COMP;
+	kmod = dso__is_kmod(dso);
 
 	if (dso__kernel(dso) && !kmod) {
 		if (dso__kernel(dso) == DSO_SPACE__KERNEL)
-- 
2.44.0.769.g3c40516874-goog

Re: [PATCH 1/2] perf symbol: Set binary_type of dso when loading

[Alexander Monakov]

Hi,

On Fri, 26 Apr 2024, Namhyung Kim wrote:

> For the kernel dso, it sets the binary type of dso when loading the
> symbol table.  But it seems not to do that for user DSOs.  Actually
> it sets the symtab type only.  It's not clear why we want to maintain
> the two separately but it uses the binary type info before getting
> the disassembly.
> 
> Let's use the symtab type as binary type too if it's not set.  I think
> it's ok to set the binary type when it founds a symsrc whether or not
> it has actual symbols.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Any chance this can be applied and propagated to stable? Annotating
functions outside of the main executable is broken otherwise (regressed
since 873a83731f1c ("perf annotate: Skip DSOs not found")).

Tested-by: Alexander Monakov <amonakov@ispras.ru>

> ---
>  tools/perf/util/symbol.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 7772a4d3e66c..63936269ec15 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1927,6 +1927,9 @@ int dso__load(struct dso *dso, struct map *map)
>  		if (next_slot) {
>  			ss_pos++;
>  
> +			if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
> +				dso__set_binary_type(dso, symtab_type);
> +
>  			if (syms_ss && runtime_ss)
>  				break;
>  		} else {
> 

Re: [PATCH 1/2] perf symbol: Set binary_type of dso when loading

[Namhyung Kim]

Hello,

On Fri, Sep 20, 2024 at 08:04:00PM +0300, Alexander Monakov wrote:
> Hi,
> 
> On Fri, 26 Apr 2024, Namhyung Kim wrote:
> 
> > For the kernel dso, it sets the binary type of dso when loading the
> > symbol table.  But it seems not to do that for user DSOs.  Actually
> > it sets the symtab type only.  It's not clear why we want to maintain
> > the two separately but it uses the binary type info before getting
> > the disassembly.
> > 
> > Let's use the symtab type as binary type too if it's not set.  I think
> > it's ok to set the binary type when it founds a symsrc whether or not
> > it has actual symbols.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Any chance this can be applied and propagated to stable? Annotating
> functions outside of the main executable is broken otherwise (regressed
> since 873a83731f1c ("perf annotate: Skip DSOs not found")).
> 
> Tested-by: Alexander Monakov <amonakov@ispras.ru>

Thanks for the test.  For some reason, it doesn't seem to be merged
while patch 2/2 is.  I'll add this with above information.

Thanks,
Namhyung

> 
> > ---
> >  tools/perf/util/symbol.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index 7772a4d3e66c..63936269ec15 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1927,6 +1927,9 @@ int dso__load(struct dso *dso, struct map *map)
> >  		if (next_slot) {
> >  			ss_pos++;
> >  
> > +			if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
> > +				dso__set_binary_type(dso, symtab_type);
> > +
> >  			if (syms_ss && runtime_ss)
> >  				break;
> >  		} else {
> > 

Re: [PATCH 1/2] perf symbol: Set binary_type of dso when loading

[Namhyung Kim]

On Tue, Sep 24, 2024 at 10:50:07PM -0700, Namhyung Kim wrote:
> Hello,
> 
> On Fri, Sep 20, 2024 at 08:04:00PM +0300, Alexander Monakov wrote:
> > Hi,
> > 
> > On Fri, 26 Apr 2024, Namhyung Kim wrote:
> > 
> > > For the kernel dso, it sets the binary type of dso when loading the
> > > symbol table.  But it seems not to do that for user DSOs.  Actually
> > > it sets the symtab type only.  It's not clear why we want to maintain
> > > the two separately but it uses the binary type info before getting
> > > the disassembly.
> > > 
> > > Let's use the symtab type as binary type too if it's not set.  I think
> > > it's ok to set the binary type when it founds a symsrc whether or not
> > > it has actual symbols.
> > > 
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > 
> > Any chance this can be applied and propagated to stable? Annotating
> > functions outside of the main executable is broken otherwise (regressed
> > since 873a83731f1c ("perf annotate: Skip DSOs not found")).
> > 
> > Tested-by: Alexander Monakov <amonakov@ispras.ru>
> 
> Thanks for the test.  For some reason, it doesn't seem to be merged
> while patch 2/2 is.  I'll add this with above information.
 
Well, it actually needs to go to perf-tools tree.  Arnaldo, can you
please pick this up?  It should have this too.

Fixes: 873a83731f1c ("perf annotate: Skip DSOs not found"))

Thanks,
Namhyung

> > 
> > > ---
> > >  tools/perf/util/symbol.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > > index 7772a4d3e66c..63936269ec15 100644
> > > --- a/tools/perf/util/symbol.c
> > > +++ b/tools/perf/util/symbol.c
> > > @@ -1927,6 +1927,9 @@ int dso__load(struct dso *dso, struct map *map)
> > >  		if (next_slot) {
> > >  			ss_pos++;
> > >  
> > > +			if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND)
> > > +				dso__set_binary_type(dso, symtab_type);
> > > +
> > >  			if (syms_ss && runtime_ss)
> > >  				break;
> > >  		} else {
> > >