[PATCH v1 0/3] 2 memory fixes and a build fix

[PATCH v1 0/3] 2 memory fixes and a build fix

[Ian Rogers]

I was looking into some lsan regressions and a latent issue with
libdw, creating these fixes.

A thought, we should probably simplify the libdw logic but rather than
do it here I'll do it as a separate series on top of these. The issues
I see are:

1) dwfl_thread_getframes is used to test for the presence of
   libdw-dwarf-unwind. The blame date on this function is
   2013-05-30. As the function is 10 years old I think having libdw
   implies having dwfl_thread_getframes and so we can just merge the
   two pieces of logic instead of having different feature tests and
   ifdefs.

2) similarly, dwarf_getlocations has a blame date of 2013-08-23 so
   let's just make libdw tests test for this and make having libdw
   imply dwarf_getlocations support.

3) similarly, dwarf_getcfi has a blame date of 2009-06-24 so let's
   just make libdw tests test for this and make having libdw imply
   dwarf_getcfi support.

4) in Makefie.config feature-dwarf is a synonym for libdw support. I
   think using the name libdw is more intention revealing as dwarf can
   mean multiple things. Let's change HAVE_DWARF_SUPPORT to
   HAVE_LIBDW_SUPPORT and all similar dwarf vs libdw names.

5) We have "#if _ELFUTILS_PREREQ(0, 142)" testing for elfutils version
   0.142. Elfutils 0.142 was released around 2009-06-13 (via git blame
   on the NEWS file). Let's remove the #if and ensure elfutils feature
   tests for at least 0.142. If someone were using an incredibly old
   version then they'd lose some elfutils support, but given the 15
   year old age of the library I find it unlikely anyone is doing
   this. They can also just move to a newer version.

From the mailing list I notice also overlap with the last patch and
this series:
https://lore.kernel.org/lkml/20240919013513.118527-1-yangjihong@bytedance.com/
Simplifying the libdw support will address some of those issues too.

Ian Rogers (3):
  perf disasm: Fix capstone memory leak
  perf probe: Fix libdw memory leak
  perf build: Fix !HAVE_DWARF_GETLOCATIONS_SUPPORT

 tools/perf/Makefile.config     |  6 ++++++
 tools/perf/util/disasm.c       | 11 +++++++----
 tools/perf/util/dwarf-aux.h    |  1 +
 tools/perf/util/probe-finder.c |  5 +++++
 4 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.46.0.792.g87dc391469-goog

[PATCH v1 1/3] perf disasm: Fix capstone memory leak

[Ian Rogers]

The insn argument passed to cs_disasm needs freeing. To support
accurately having count, add an additional free_count variable.

Fixes: c5d60de1813a ("perf annotate: Add support to use libcapstone in powerpc")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/disasm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index f05ba7739c1e..2c8063660f2e 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1627,12 +1627,12 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
 	u64 start = map__rip_2objdump(map, sym->start);
 	u64 len;
 	u64 offset;
-	int i, count;
+	int i, count, free_count;
 	bool is_64bit = false;
 	bool needs_cs_close = false;
 	u8 *buf = NULL;
 	csh handle;
-	cs_insn *insn;
+	cs_insn *insn = NULL;
 	char disasm_buf[512];
 	struct disasm_line *dl;
 
@@ -1664,7 +1664,7 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
 
 	needs_cs_close = true;
 
-	count = cs_disasm(handle, buf, len, start, len, &insn);
+	free_count = count = cs_disasm(handle, buf, len, start, len, &insn);
 	for (i = 0, offset = 0; i < count; i++) {
 		int printed;
 
@@ -1702,8 +1702,11 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
 	}
 
 out:
-	if (needs_cs_close)
+	if (needs_cs_close) {
 		cs_close(&handle);
+		if (free_count > 0)
+			cs_free(insn, free_count);
+	}
 	free(buf);
 	return count < 0 ? count : 0;
 
-- 
2.46.0.792.g87dc391469-goog

[PATCH v1 2/3] perf probe: Fix libdw memory leak

[Ian Rogers]

Add missing dwarf_cfi_end to free memory associated with probe_finder
cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
tools/perf/tests/shell/test_uprobe_from_different_cu.sh

Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/probe-finder.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 630e16c54ed5..78f34fa0c391 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
 	if (ret >= 0 && tf.pf.skip_empty_arg)
 		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
 
+#if _ELFUTILS_PREREQ(0, 142)
+	dwarf_cfi_end(tf.pf.cfi_eh);
+	dwarf_cfi_end(tf.pf.cfi_dbg);
+#endif
+
 	if (ret < 0 || tf.ntevs == 0) {
 		for (i = 0; i < tf.ntevs; i++)
 			clear_probe_trace_event(&tf.tevs[i]);
-- 
2.46.0.792.g87dc391469-goog

[PATCH v1 3/3] perf build: Fix !HAVE_DWARF_GETLOCATIONS_SUPPORT

[Ian Rogers]

Testing with a LIBDW_DIR showed some issues. In dwarf-aux.h if
HAVE_DWARF_GETLOCATIONS_SUPPORT isn't defined then the code uses an
undefined errno value, so add errno.h.
In Makefile.config the dwarf feature tests need the LIBDW_DIR setting
in the CFLAGS/LDFLAGS.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Makefile.config  | 6 ++++++
 tools/perf/util/dwarf-aux.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 4dcf7a0fd235..5e26d3a91b36 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -159,8 +159,14 @@ ifeq ($(findstring -static,${LDFLAGS}),-static)
     DWARFLIBS += -lebl
   endif
 endif
+FEATURE_CHECK_CFLAGS-dwarf := $(LIBDW_CFLAGS)
+FEATURE_CHECK_LDFLAGS-dwarf := $(LIBDW_LDFLAGS) $(DWARFLIBS)
 FEATURE_CHECK_CFLAGS-libdw-dwarf-unwind := $(LIBDW_CFLAGS)
 FEATURE_CHECK_LDFLAGS-libdw-dwarf-unwind := $(LIBDW_LDFLAGS) $(DWARFLIBS)
+FEATURE_CHECK_CFLAGS-dwarf_getlocations := $(LIBDW_CFLAGS)
+FEATURE_CHECK_LDFLAGS-dwarf_getlocations := $(LIBDW_LDFLAGS) $(DWARFLIBS)
+FEATURE_CHECK_CFLAGS-dwarf_getcfi := $(LIBDW_CFLAGS)
+FEATURE_CHECK_LDFLAGS-dwarf_getcfi := $(LIBDW_LDFLAGS) $(DWARFLIBS)
 
 # for linking with debug library, run like:
 # make DEBUG=1 LIBBABELTRACE_DIR=/opt/libbabeltrace/
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index 336a3a183a78..925a9bb9fb15 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -177,6 +177,7 @@ void die_collect_vars(Dwarf_Die *sc_die, struct die_var_type **var_types);
 void die_collect_global_vars(Dwarf_Die *cu_die, struct die_var_type **var_types);
 
 #else /*  HAVE_DWARF_GETLOCATIONS_SUPPORT */
+#include <errno.h>
 
 static inline int die_get_var_range(Dwarf_Die *sp_die __maybe_unused,
 				    Dwarf_Die *vr_die __maybe_unused,
-- 
2.46.0.792.g87dc391469-goog

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

[James Clark]

On 24/09/2024 1:37 am, Ian Rogers wrote:
> Add missing dwarf_cfi_end to free memory associated with probe_finder
> cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> 
> Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/util/probe-finder.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 630e16c54ed5..78f34fa0c391 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>   	if (ret >= 0 && tf.pf.skip_empty_arg)
>   		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
>   
> +#if _ELFUTILS_PREREQ(0, 142)
> +	dwarf_cfi_end(tf.pf.cfi_eh);
> +	dwarf_cfi_end(tf.pf.cfi_dbg);
> +#endif
> +

I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into 
HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code 
(unrelated to this patch). The commit message doesn't say why it is 
better, just that it could be changed, so I'm not sure which one is right.

Re: [PATCH v1 0/3] 2 memory fixes and a build fix

[James Clark]

On 24/09/2024 1:37 am, Ian Rogers wrote:
> I was looking into some lsan regressions and a latent issue with
> libdw, creating these fixes.
> 
> A thought, we should probably simplify the libdw logic but rather than
> do it here I'll do it as a separate series on top of these. The issues
> I see are:
> 
> 1) dwfl_thread_getframes is used to test for the presence of
>     libdw-dwarf-unwind. The blame date on this function is
>     2013-05-30. As the function is 10 years old I think having libdw
>     implies having dwfl_thread_getframes and so we can just merge the
>     two pieces of logic instead of having different feature tests and
>     ifdefs.
> 
> 2) similarly, dwarf_getlocations has a blame date of 2013-08-23 so
>     let's just make libdw tests test for this and make having libdw
>     imply dwarf_getlocations support.
> 
> 3) similarly, dwarf_getcfi has a blame date of 2009-06-24 so let's
>     just make libdw tests test for this and make having libdw imply
>     dwarf_getcfi support.
> 
> 4) in Makefie.config feature-dwarf is a synonym for libdw support. I
>     think using the name libdw is more intention revealing as dwarf can
>     mean multiple things. Let's change HAVE_DWARF_SUPPORT to
>     HAVE_LIBDW_SUPPORT and all similar dwarf vs libdw names.
> 
> 5) We have "#if _ELFUTILS_PREREQ(0, 142)" testing for elfutils version
>     0.142. Elfutils 0.142 was released around 2009-06-13 (via git blame
>     on the NEWS file). Let's remove the #if and ensure elfutils feature
>     tests for at least 0.142. If someone were using an incredibly old
>     version then they'd lose some elfutils support, but given the 15
>     year old age of the library I find it unlikely anyone is doing
>     this. They can also just move to a newer version.
> 
>  From the mailing list I notice also overlap with the last patch and
> this series:
> https://lore.kernel.org/lkml/20240919013513.118527-1-yangjihong@bytedance.com/
> Simplifying the libdw support will address some of those issues too.
> 
> Ian Rogers (3):
>    perf disasm: Fix capstone memory leak
>    perf probe: Fix libdw memory leak
>    perf build: Fix !HAVE_DWARF_GETLOCATIONS_SUPPORT
> 
>   tools/perf/Makefile.config     |  6 ++++++
>   tools/perf/util/disasm.c       | 11 +++++++----
>   tools/perf/util/dwarf-aux.h    |  1 +
>   tools/perf/util/probe-finder.c |  5 +++++
>   4 files changed, 19 insertions(+), 4 deletions(-)
> 

Reviewed-by: James Clark <james.clark@linaro.org>

Re: [PATCH v1 0/3] 2 memory fixes and a build fix

[Namhyung Kim]

On Mon, Sep 23, 2024 at 05:37:17PM -0700, Ian Rogers wrote:
> I was looking into some lsan regressions and a latent issue with
> libdw, creating these fixes.
> 
> A thought, we should probably simplify the libdw logic but rather than
> do it here I'll do it as a separate series on top of these. The issues
> I see are:
> 
> 1) dwfl_thread_getframes is used to test for the presence of
>    libdw-dwarf-unwind. The blame date on this function is
>    2013-05-30. As the function is 10 years old I think having libdw
>    implies having dwfl_thread_getframes and so we can just merge the
>    two pieces of logic instead of having different feature tests and
>    ifdefs.
> 
> 2) similarly, dwarf_getlocations has a blame date of 2013-08-23 so
>    let's just make libdw tests test for this and make having libdw
>    imply dwarf_getlocations support.
> 
> 3) similarly, dwarf_getcfi has a blame date of 2009-06-24 so let's
>    just make libdw tests test for this and make having libdw imply
>    dwarf_getcfi support.
> 
> 4) in Makefie.config feature-dwarf is a synonym for libdw support. I
>    think using the name libdw is more intention revealing as dwarf can
>    mean multiple things. Let's change HAVE_DWARF_SUPPORT to
>    HAVE_LIBDW_SUPPORT and all similar dwarf vs libdw names.
> 
> 5) We have "#if _ELFUTILS_PREREQ(0, 142)" testing for elfutils version
>    0.142. Elfutils 0.142 was released around 2009-06-13 (via git blame
>    on the NEWS file). Let's remove the #if and ensure elfutils feature
>    tests for at least 0.142. If someone were using an incredibly old
>    version then they'd lose some elfutils support, but given the 15
>    year old age of the library I find it unlikely anyone is doing
>    this. They can also just move to a newer version.

Looking at the map file in libdw, the latest addition was 0.158 for
dwfl_thread_getframes().  Probably we can add the version check to the
feature test to make sure if it has all the required APIs.

https://sourceware.org/git/?p=elfutils.git;a=blob;f=libdw/libdw.map;h=552588a94c0c1a1f2fd5b973553c784026e6de14;hb=HEAD#l274

> 
> From the mailing list I notice also overlap with the last patch and
> this series:
> https://lore.kernel.org/lkml/20240919013513.118527-1-yangjihong@bytedance.com/
> Simplifying the libdw support will address some of those issues too.

Yeah I noticed that too and feel like it should go to perf-tools tree.
Probably it doesn't clash with this so I think it's ok to have this in
perf-tools-next.

Thanks,
Namhyung

> 
> Ian Rogers (3):
>   perf disasm: Fix capstone memory leak
>   perf probe: Fix libdw memory leak
>   perf build: Fix !HAVE_DWARF_GETLOCATIONS_SUPPORT
> 
>  tools/perf/Makefile.config     |  6 ++++++
>  tools/perf/util/disasm.c       | 11 +++++++----
>  tools/perf/util/dwarf-aux.h    |  1 +
>  tools/perf/util/probe-finder.c |  5 +++++
>  4 files changed, 19 insertions(+), 4 deletions(-)
> 
> -- 
> 2.46.0.792.g87dc391469-goog
> 

Re: [PATCH v1 1/3] perf disasm: Fix capstone memory leak

[Namhyung Kim]

On Mon, Sep 23, 2024 at 05:37:18PM -0700, Ian Rogers wrote:
> The insn argument passed to cs_disasm needs freeing. To support
> accurately having count, add an additional free_count variable.
> 
> Fixes: c5d60de1813a ("perf annotate: Add support to use libcapstone in powerpc")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/disasm.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index f05ba7739c1e..2c8063660f2e 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1627,12 +1627,12 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>  	u64 start = map__rip_2objdump(map, sym->start);
>  	u64 len;
>  	u64 offset;
> -	int i, count;
> +	int i, count, free_count;
>  	bool is_64bit = false;
>  	bool needs_cs_close = false;
>  	u8 *buf = NULL;
>  	csh handle;
> -	cs_insn *insn;
> +	cs_insn *insn = NULL;
>  	char disasm_buf[512];
>  	struct disasm_line *dl;
>  
> @@ -1664,7 +1664,7 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>  
>  	needs_cs_close = true;
>  
> -	count = cs_disasm(handle, buf, len, start, len, &insn);
> +	free_count = count = cs_disasm(handle, buf, len, start, len, &insn);
>  	for (i = 0, offset = 0; i < count; i++) {
>  		int printed;
>  
> @@ -1702,8 +1702,11 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>  	}
>  
>  out:
> -	if (needs_cs_close)
> +	if (needs_cs_close) {
>  		cs_close(&handle);
> +		if (free_count > 0)
> +			cs_free(insn, free_count);

It seems cs_free() can handle NULL insn and 0 free_count (like regular free)
so we can call it unconditionally.

Thanks,
Namhyung


> +	}
>  	free(buf);
>  	return count < 0 ? count : 0;
>  
> -- 
> 2.46.0.792.g87dc391469-goog
> 

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

[Namhyung Kim]

On Tue, Sep 24, 2024 at 10:17:08AM +0100, James Clark wrote:
> 
> 
> On 24/09/2024 1:37 am, Ian Rogers wrote:
> > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > 
> > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/util/probe-finder.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 630e16c54ed5..78f34fa0c391 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> >   	if (ret >= 0 && tf.pf.skip_empty_arg)
> >   		ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > +#if _ELFUTILS_PREREQ(0, 142)
> > +	dwarf_cfi_end(tf.pf.cfi_eh);
> > +	dwarf_cfi_end(tf.pf.cfi_dbg);
> > +#endif
> > +
> 
> I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into
> HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code
> (unrelated to this patch). The commit message doesn't say why it is better,
> just that it could be changed, so I'm not sure which one is right.

I think HAVE_DWARF_CFI_SUPPORT is better since it reveals the intention
clearly.

Thanks,
Namhyung

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

[Ian Rogers]

On Tue, Sep 24, 2024 at 11:40 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Sep 24, 2024 at 10:17:08AM +0100, James Clark wrote:
> >
> >
> > On 24/09/2024 1:37 am, Ian Rogers wrote:
> > > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > >
> > > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >   tools/perf/util/probe-finder.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > index 630e16c54ed5..78f34fa0c391 100644
> > > --- a/tools/perf/util/probe-finder.c
> > > +++ b/tools/perf/util/probe-finder.c
> > > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> > >     if (ret >= 0 && tf.pf.skip_empty_arg)
> > >             ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > > +#if _ELFUTILS_PREREQ(0, 142)
> > > +   dwarf_cfi_end(tf.pf.cfi_eh);
> > > +   dwarf_cfi_end(tf.pf.cfi_dbg);
> > > +#endif
> > > +
> >
> > I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into
> > HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code
> > (unrelated to this patch). The commit message doesn't say why it is better,
> > just that it could be changed, so I'm not sure which one is right.
>
> I think HAVE_DWARF_CFI_SUPPORT is better since it reveals the intention
> clearly.

Let's just nuke the conditional compilation, the CFI support is in
libdw (calling it dwarf is just actively confusing) is 15 years old:
https://lore.kernel.org/lkml/20240924160418.1391100-7-irogers@google.com/
https://lore.kernel.org/lkml/20240924160418.1391100-8-irogers@google.com/

Thanks,
Ian

Re: [PATCH v1 1/3] perf disasm: Fix capstone memory leak

[Ian Rogers]

On Tue, Sep 24, 2024 at 11:38 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Sep 23, 2024 at 05:37:18PM -0700, Ian Rogers wrote:
> > The insn argument passed to cs_disasm needs freeing. To support
> > accurately having count, add an additional free_count variable.
> >
> > Fixes: c5d60de1813a ("perf annotate: Add support to use libcapstone in powerpc")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/disasm.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > index f05ba7739c1e..2c8063660f2e 100644
> > --- a/tools/perf/util/disasm.c
> > +++ b/tools/perf/util/disasm.c
> > @@ -1627,12 +1627,12 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> >       u64 start = map__rip_2objdump(map, sym->start);
> >       u64 len;
> >       u64 offset;
> > -     int i, count;
> > +     int i, count, free_count;
> >       bool is_64bit = false;
> >       bool needs_cs_close = false;
> >       u8 *buf = NULL;
> >       csh handle;
> > -     cs_insn *insn;
> > +     cs_insn *insn = NULL;
> >       char disasm_buf[512];
> >       struct disasm_line *dl;
> >
> > @@ -1664,7 +1664,7 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> >
> >       needs_cs_close = true;
> >
> > -     count = cs_disasm(handle, buf, len, start, len, &insn);
> > +     free_count = count = cs_disasm(handle, buf, len, start, len, &insn);
> >       for (i = 0, offset = 0; i < count; i++) {
> >               int printed;
> >
> > @@ -1702,8 +1702,11 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> >       }
> >
> >  out:
> > -     if (needs_cs_close)
> > +     if (needs_cs_close) {
> >               cs_close(&handle);
> > +             if (free_count > 0)
> > +                     cs_free(insn, free_count);
>
> It seems cs_free() can handle NULL insn and 0 free_count (like regular free)
> so we can call it unconditionally.

No, on error from cs_disasm free_count gets assigned -1 and my
experience was things crashing.

Thanks,
Ian

>
> > +     }
> >       free(buf);
> >       return count < 0 ? count : 0;
> >
> > --
> > 2.46.0.792.g87dc391469-goog
> >

Re: [PATCH v1 1/3] perf disasm: Fix capstone memory leak

[Namhyung Kim]

On Tue, Sep 24, 2024 at 12:51:20PM -0700, Ian Rogers wrote:
> On Tue, Sep 24, 2024 at 11:38 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Sep 23, 2024 at 05:37:18PM -0700, Ian Rogers wrote:
> > > The insn argument passed to cs_disasm needs freeing. To support
> > > accurately having count, add an additional free_count variable.
> > >
> > > Fixes: c5d60de1813a ("perf annotate: Add support to use libcapstone in powerpc")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/disasm.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> > > index f05ba7739c1e..2c8063660f2e 100644
> > > --- a/tools/perf/util/disasm.c
> > > +++ b/tools/perf/util/disasm.c
> > > @@ -1627,12 +1627,12 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> > >       u64 start = map__rip_2objdump(map, sym->start);
> > >       u64 len;
> > >       u64 offset;
> > > -     int i, count;
> > > +     int i, count, free_count;
> > >       bool is_64bit = false;
> > >       bool needs_cs_close = false;
> > >       u8 *buf = NULL;
> > >       csh handle;
> > > -     cs_insn *insn;
> > > +     cs_insn *insn = NULL;
> > >       char disasm_buf[512];
> > >       struct disasm_line *dl;
> > >
> > > @@ -1664,7 +1664,7 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> > >
> > >       needs_cs_close = true;
> > >
> > > -     count = cs_disasm(handle, buf, len, start, len, &insn);
> > > +     free_count = count = cs_disasm(handle, buf, len, start, len, &insn);
> > >       for (i = 0, offset = 0; i < count; i++) {
> > >               int printed;
> > >
> > > @@ -1702,8 +1702,11 @@ static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
> > >       }
> > >
> > >  out:
> > > -     if (needs_cs_close)
> > > +     if (needs_cs_close) {
> > >               cs_close(&handle);
> > > +             if (free_count > 0)
> > > +                     cs_free(insn, free_count);
> >
> > It seems cs_free() can handle NULL insn and 0 free_count (like regular free)
> > so we can call it unconditionally.
> 
> No, on error from cs_disasm free_count gets assigned -1 and my
> experience was things crashing.
 
Ok then, I thought it returns 0 on error.

Thanks,
Namhyung

> >
> > > +     }
> > >       free(buf);
> > >       return count < 0 ? count : 0;
> > >
> > > --
> > > 2.46.0.792.g87dc391469-goog
> > >

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

[Namhyung Kim]

On Tue, Sep 24, 2024 at 12:47:33PM -0700, Ian Rogers wrote:
> On Tue, Sep 24, 2024 at 11:40 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Tue, Sep 24, 2024 at 10:17:08AM +0100, James Clark wrote:
> > >
> > >
> > > On 24/09/2024 1:37 am, Ian Rogers wrote:
> > > > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > > > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > > > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > > >
> > > > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >   tools/perf/util/probe-finder.c | 5 +++++
> > > >   1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > > index 630e16c54ed5..78f34fa0c391 100644
> > > > --- a/tools/perf/util/probe-finder.c
> > > > +++ b/tools/perf/util/probe-finder.c
> > > > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> > > >     if (ret >= 0 && tf.pf.skip_empty_arg)
> > > >             ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > > > +#if _ELFUTILS_PREREQ(0, 142)
> > > > +   dwarf_cfi_end(tf.pf.cfi_eh);
> > > > +   dwarf_cfi_end(tf.pf.cfi_dbg);
> > > > +#endif
> > > > +
> > >
> > > I noticed that c06547d converted an _ELFUTILS_PREREQ(0, 142) into
> > > HAVE_DWARF_CFI_SUPPORT. But there is still a mixture of both in the code
> > > (unrelated to this patch). The commit message doesn't say why it is better,
> > > just that it could be changed, so I'm not sure which one is right.
> >
> > I think HAVE_DWARF_CFI_SUPPORT is better since it reveals the intention
> > clearly.
> 
> Let's just nuke the conditional compilation, the CFI support is in
> libdw (calling it dwarf is just actively confusing) is 15 years old:
> https://lore.kernel.org/lkml/20240924160418.1391100-7-irogers@google.com/
> https://lore.kernel.org/lkml/20240924160418.1391100-8-irogers@google.com/

Sounds good as long as we checks the version or the API in the feature
test.

Thanks,
Namhyung

Re: [PATCH v1 0/3] 2 memory fixes and a build fix

[Ian Rogers]

On Tue, Sep 24, 2024 at 11:25 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Sep 23, 2024 at 05:37:17PM -0700, Ian Rogers wrote:
> > I was looking into some lsan regressions and a latent issue with
> > libdw, creating these fixes.
> >
> > A thought, we should probably simplify the libdw logic but rather than
> > do it here I'll do it as a separate series on top of these. The issues
> > I see are:
> >
> > 1) dwfl_thread_getframes is used to test for the presence of
> >    libdw-dwarf-unwind. The blame date on this function is
> >    2013-05-30. As the function is 10 years old I think having libdw
> >    implies having dwfl_thread_getframes and so we can just merge the
> >    two pieces of logic instead of having different feature tests and
> >    ifdefs.
> >
> > 2) similarly, dwarf_getlocations has a blame date of 2013-08-23 so
> >    let's just make libdw tests test for this and make having libdw
> >    imply dwarf_getlocations support.
> >
> > 3) similarly, dwarf_getcfi has a blame date of 2009-06-24 so let's
> >    just make libdw tests test for this and make having libdw imply
> >    dwarf_getcfi support.
> >
> > 4) in Makefie.config feature-dwarf is a synonym for libdw support. I
> >    think using the name libdw is more intention revealing as dwarf can
> >    mean multiple things. Let's change HAVE_DWARF_SUPPORT to
> >    HAVE_LIBDW_SUPPORT and all similar dwarf vs libdw names.
> >
> > 5) We have "#if _ELFUTILS_PREREQ(0, 142)" testing for elfutils version
> >    0.142. Elfutils 0.142 was released around 2009-06-13 (via git blame
> >    on the NEWS file). Let's remove the #if and ensure elfutils feature
> >    tests for at least 0.142. If someone were using an incredibly old
> >    version then they'd lose some elfutils support, but given the 15
> >    year old age of the library I find it unlikely anyone is doing
> >    this. They can also just move to a newer version.
>
> Looking at the map file in libdw, the latest addition was 0.158 for
> dwfl_thread_getframes().  Probably we can add the version check to the
> feature test to make sure if it has all the required APIs.
>
> https://sourceware.org/git/?p=elfutils.git;a=blob;f=libdw/libdw.map;h=552588a94c0c1a1f2fd5b973553c784026e6de14;hb=HEAD#l274
>
> >
> > From the mailing list I notice also overlap with the last patch and
> > this series:
> > https://lore.kernel.org/lkml/20240919013513.118527-1-yangjihong@bytedance.com/
> > Simplifying the libdw support will address some of those issues too.
>
> Yeah I noticed that too and feel like it should go to perf-tools tree.
> Probably it doesn't clash with this so I think it's ok to have this in
> perf-tools-next.

I think the comments wrt libdw are covered in the series cleaning up libdw:
https://lore.kernel.org/lkml/20240924160418.1391100-1-irogers@google.com/
so these fixes should be good to land?

Thanks,
Ian

Re: [PATCH v1 0/3] 2 memory fixes and a build fix

[Namhyung Kim]

On Mon, Sep 30, 2024 at 10:11:34PM -0700, Ian Rogers wrote:
> On Tue, Sep 24, 2024 at 11:25 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Sep 23, 2024 at 05:37:17PM -0700, Ian Rogers wrote:
> > > I was looking into some lsan regressions and a latent issue with
> > > libdw, creating these fixes.
> > >
> > > A thought, we should probably simplify the libdw logic but rather than
> > > do it here I'll do it as a separate series on top of these. The issues
> > > I see are:
> > >
> > > 1) dwfl_thread_getframes is used to test for the presence of
> > >    libdw-dwarf-unwind. The blame date on this function is
> > >    2013-05-30. As the function is 10 years old I think having libdw
> > >    implies having dwfl_thread_getframes and so we can just merge the
> > >    two pieces of logic instead of having different feature tests and
> > >    ifdefs.
> > >
> > > 2) similarly, dwarf_getlocations has a blame date of 2013-08-23 so
> > >    let's just make libdw tests test for this and make having libdw
> > >    imply dwarf_getlocations support.
> > >
> > > 3) similarly, dwarf_getcfi has a blame date of 2009-06-24 so let's
> > >    just make libdw tests test for this and make having libdw imply
> > >    dwarf_getcfi support.
> > >
> > > 4) in Makefie.config feature-dwarf is a synonym for libdw support. I
> > >    think using the name libdw is more intention revealing as dwarf can
> > >    mean multiple things. Let's change HAVE_DWARF_SUPPORT to
> > >    HAVE_LIBDW_SUPPORT and all similar dwarf vs libdw names.
> > >
> > > 5) We have "#if _ELFUTILS_PREREQ(0, 142)" testing for elfutils version
> > >    0.142. Elfutils 0.142 was released around 2009-06-13 (via git blame
> > >    on the NEWS file). Let's remove the #if and ensure elfutils feature
> > >    tests for at least 0.142. If someone were using an incredibly old
> > >    version then they'd lose some elfutils support, but given the 15
> > >    year old age of the library I find it unlikely anyone is doing
> > >    this. They can also just move to a newer version.
> >
> > Looking at the map file in libdw, the latest addition was 0.158 for
> > dwfl_thread_getframes().  Probably we can add the version check to the
> > feature test to make sure if it has all the required APIs.
> >
> > https://sourceware.org/git/?p=elfutils.git;a=blob;f=libdw/libdw.map;h=552588a94c0c1a1f2fd5b973553c784026e6de14;hb=HEAD#l274
> >
> > >
> > > From the mailing list I notice also overlap with the last patch and
> > > this series:
> > > https://lore.kernel.org/lkml/20240919013513.118527-1-yangjihong@bytedance.com/
> > > Simplifying the libdw support will address some of those issues too.
> >
> > Yeah I noticed that too and feel like it should go to perf-tools tree.
> > Probably it doesn't clash with this so I think it's ok to have this in
> > perf-tools-next.
> 
> I think the comments wrt libdw are covered in the series cleaning up libdw:
> https://lore.kernel.org/lkml/20240924160418.1391100-1-irogers@google.com/
> so these fixes should be good to land?

Sure, I'll just adjust the errno.h part as it's picked up by Arnaldo
to the perf-tools already.  I'll merge the branch later.

Thanks,
Namhyung

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

[Namhyung Kim]

On Mon, Sep 23, 2024 at 5:37 PM Ian Rogers <irogers@google.com> wrote:
>
> Add missing dwarf_cfi_end to free memory associated with probe_finder
> cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> tools/perf/tests/shell/test_uprobe_from_different_cu.sh
>
> Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/probe-finder.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index 630e16c54ed5..78f34fa0c391 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
>         if (ret >= 0 && tf.pf.skip_empty_arg)
>                 ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
>
> +#if _ELFUTILS_PREREQ(0, 142)
> +       dwarf_cfi_end(tf.pf.cfi_eh);
> +       dwarf_cfi_end(tf.pf.cfi_dbg);
> +#endif

This is causing another problem.  Now vfs_getname tests are
failing because perf probe aborts.

Thanks,
Namhyung

> +
>         if (ret < 0 || tf.ntevs == 0) {
>                 for (i = 0; i < tf.ntevs; i++)
>                         clear_probe_trace_event(&tf.tevs[i]);
> --
> 2.46.0.792.g87dc391469-goog
>

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

[Ian Rogers]

On Wed, Oct 2, 2024 at 10:44 AM Namhyung Kim <namhyung@gmail.com> wrote:
>
> On Mon, Sep 23, 2024 at 5:37 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> >
> > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/probe-finder.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 630e16c54ed5..78f34fa0c391 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> >         if (ret >= 0 && tf.pf.skip_empty_arg)
> >                 ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> >
> > +#if _ELFUTILS_PREREQ(0, 142)
> > +       dwarf_cfi_end(tf.pf.cfi_eh);
> > +       dwarf_cfi_end(tf.pf.cfi_dbg);
> > +#endif
>
> This is causing another problem.  Now vfs_getname tests are
> failing because perf probe aborts.

I wasn't able to reproduce but largely as the test skips. The variable
is out of scope after the function so I'm struggling to see what the
issue is.

Thanks,
Ian

Re: [PATCH v1 2/3] perf probe: Fix libdw memory leak

[Namhyung Kim]

On Wed, Oct 02, 2024 at 12:08:30PM -0700, Ian Rogers wrote:
> On Wed, Oct 2, 2024 at 10:44 AM Namhyung Kim <namhyung@gmail.com> wrote:
> >
> > On Mon, Sep 23, 2024 at 5:37 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > Add missing dwarf_cfi_end to free memory associated with probe_finder
> > > cfi_eh or cfi_dbg. This addresses leak sanitizer issues seen in:
> > > tools/perf/tests/shell/test_uprobe_from_different_cu.sh
> > >
> > > Fixes: 270bde1e76f4 ("perf probe: Search both .eh_frame and .debug_frame sections for probe location")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/probe-finder.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > > index 630e16c54ed5..78f34fa0c391 100644
> > > --- a/tools/perf/util/probe-finder.c
> > > +++ b/tools/perf/util/probe-finder.c
> > > @@ -1379,6 +1379,11 @@ int debuginfo__find_trace_events(struct debuginfo *dbg,
> > >         if (ret >= 0 && tf.pf.skip_empty_arg)
> > >                 ret = fill_empty_trace_arg(pev, tf.tevs, tf.ntevs);
> > >
> > > +#if _ELFUTILS_PREREQ(0, 142)
> > > +       dwarf_cfi_end(tf.pf.cfi_eh);
> > > +       dwarf_cfi_end(tf.pf.cfi_dbg);
> > > +#endif
> >
> > This is causing another problem.  Now vfs_getname tests are
> > failing because perf probe aborts.
> 
> I wasn't able to reproduce but largely as the test skips. The variable
> is out of scope after the function so I'm struggling to see what the
> issue is.

I'm seeing this.

  $ sudo ./perf test -v vfs
   91: Add vfs_getname probe to get syscall args filenames:
  --- start ---
  test child forked, pid 3013362
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013380 Aborted                 perf probe -q "vfs_getname=getname_flags:${line} pathname=result->name:string"
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013381 Aborted                 perf probe $add_probe_verbose "vfs_getname=getname_flags:${line} pathname=filename:ustring"
  ---- end(-1) ----
   91: Add vfs_getname probe to get syscall args filenames             : FAILED!
   93: Use vfs_getname probe to get syscall args filenames:
  --- start ---
  test child forked, pid 3013479
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013502 Aborted                 perf probe -q "vfs_getname=getname_flags:${line} pathname=result->name:string"
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013514 Aborted                 perf probe $add_probe_verbose "vfs_getname=getname_flags:${line} pathname=filename:ustring"
  ---- end(-1) ----
   93: Use vfs_getname probe to get syscall args filenames             : FAILED!
  127: Check open filename arg using perf trace + vfs_getname:
  --- start ---
  test child forked, pid 3013528
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013547 Aborted                 perf probe -q "vfs_getname=getname_flags:${line} pathname=result->name:string"
  free(): invalid pointer
  linux/tools/perf/tests/shell/lib/probe_vfs_getname.sh: line 13: 3013548 Aborted                 perf probe $add_probe_verbose "vfs_getname=getname_flags:${line} pathname=filename:ustring"
  ---- end(-1) ----
  127: Check open filename arg using perf trace + vfs_getname          : FAILED!
  
Dropping the series from tmp.perf-tools-next for now.

Thanks,
Namhyung