Re: [PATCH v1 01/10] perf: Avoid buffer overflow in python register interface

Re: [PATCH v1 01/10] perf: Avoid buffer overflow in python register interface

[Arnaldo Carvalho de Melo]

On Wed, Sep 04, 2024 at 06:50:09PM -0700, Andi Kleen wrote:
> Running a script that processes PEBS records gives buffer overflows
> in valgrind. The problem is that the allocation of the register
> string doesn't include the terminating 0 byte. Fix this. I also replaced
> the very magic "28" with a more reasonable larger buffer that should
> fit all registers. There's no need to conserve memory here.

I applied this one already.

But you used the wrong list address, perf-tools-users@vger, I'm fixing
this up now to linux-perf-users@vger so that the message reaches the
mailing list.

I waited a bit for reviewers but then realized the problem with the list
address when trying to use b4 to fetch it from lore :-\

- Arnaldo
 
> ==2106591== Memcheck, a memory error detector
> ==2106591== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
> ==2106591== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
> ==2106591== Command: ../perf script -i tcall.data gcov.py tcall.gcov
> ==2106591==
> ==2106591== Invalid write of size 1
> ==2106591==    at 0x713354: regs_map (trace-event-python.c:748)
> ==2106591==    by 0x7134EB: set_regs_in_dict (trace-event-python.c:784)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==    by 0x630E7A: __ordered_events__flush (ordered-events.c:324)
> ==2106591==  Address 0x7186fe0 is 0 bytes after a block of size 0 alloc'd
> ==2106591==    at 0x484280F: malloc (vg_replace_malloc.c:442)
> ==2106591==    by 0x7134AD: set_regs_in_dict (trace-event-python.c:780)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==    by 0x630E7A: __ordered_events__flush (ordered-events.c:324)
> ==2106591==
> ==2106591== Invalid read of size 1
> ==2106591==    at 0x484B6C6: strlen (vg_replace_strmem.c:502)
> ==2106591==    by 0x555D494: PyUnicode_FromString (unicodeobject.c:1899)
> ==2106591==    by 0x7134F7: set_regs_in_dict (trace-event-python.c:786)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==  Address 0x7186fe0 is 0 bytes after a block of size 0 alloc'd
> ==2106591==    at 0x484280F: malloc (vg_replace_malloc.c:442)
> ==2106591==    by 0x7134AD: set_regs_in_dict (trace-event-python.c:780)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==    by 0x630E7A: __ordered_events__flush (ordered-events.c:324)
> ==2106591==
> ==2106591== Invalid write of size 1
> ==2106591==    at 0x713354: regs_map (trace-event-python.c:748)
> ==2106591==    by 0x713539: set_regs_in_dict (trace-event-python.c:789)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==    by 0x630E7A: __ordered_events__flush (ordered-events.c:324)
> ==2106591==  Address 0x7186fe0 is 0 bytes after a block of size 0 alloc'd
> ==2106591==    at 0x484280F: malloc (vg_replace_malloc.c:442)
> ==2106591==    by 0x7134AD: set_regs_in_dict (trace-event-python.c:780)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==    by 0x630E7A: __ordered_events__flush (ordered-events.c:324)
> ==2106591==
> ==2106591== Invalid read of size 1
> ==2106591==    at 0x484B6C6: strlen (vg_replace_strmem.c:502)
> ==2106591==    by 0x555D494: PyUnicode_FromString (unicodeobject.c:1899)
> ==2106591==    by 0x713545: set_regs_in_dict (trace-event-python.c:791)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==  Address 0x7186fe0 is 0 bytes after a block of size 0 alloc'd
> ==2106591==    at 0x484280F: malloc (vg_replace_malloc.c:442)
> ==2106591==    by 0x7134AD: set_regs_in_dict (trace-event-python.c:780)
> ==2106591==    by 0x713E58: get_perf_sample_dict (trace-event-python.c:940)
> ==2106591==    by 0x716327: python_process_general_event (trace-event-python.c:1499)
> ==2106591==    by 0x7164E1: python_process_event (trace-event-python.c:1531)
> ==2106591==    by 0x44F9AF: process_sample_event (builtin-script.c:2549)
> ==2106591==    by 0x6294DC: evlist__deliver_sample (session.c:1534)
> ==2106591==    by 0x6296D0: machines__deliver_event (session.c:1573)
> ==2106591==    by 0x629C39: perf_session__deliver_event (session.c:1655)
> ==2106591==    by 0x625830: ordered_events__deliver_event (session.c:193)
> ==2106591==    by 0x630B23: do_flush (ordered-events.c:245)
> ==2106591==    by 0x630E7A: __ordered_events__flush (ordered-events.c:324)
> ==2106591==
> 73056 total, 29 ignored
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  .../perf/util/scripting-engines/trace-event-python.c  | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 6971dd6c231f..d7183134b669 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -762,6 +762,8 @@ static void regs_map(struct regs_dump *regs, uint64_t mask, const char *arch, ch
>  	}
>  }
>  
> +#define MAX_REG_SIZE 128
> +
>  static int set_regs_in_dict(PyObject *dict,
>  			     struct perf_sample *sample,
>  			     struct evsel *evsel)
> @@ -769,14 +771,7 @@ static int set_regs_in_dict(PyObject *dict,
>  	struct perf_event_attr *attr = &evsel->core.attr;
>  	const char *arch = perf_env__arch(evsel__env(evsel));
>  
> -	/*
> -	 * Here value 28 is a constant size which can be used to print
> -	 * one register value and its corresponds to:
> -	 * 16 chars is to specify 64 bit register in hexadecimal.
> -	 * 2 chars is for appending "0x" to the hexadecimal value and
> -	 * 10 chars is for register name.
> -	 */
> -	int size = __sw_hweight64(attr->sample_regs_intr) * 28;
> +	int size = (__sw_hweight64(attr->sample_regs_intr) * MAX_REG_SIZE) + 1;
>  	char *bf = malloc(size);
>  	if (!bf)
>  		return -1;
> -- 
> 2.45.2

Re: [PATCH v1 01/10] perf: Avoid buffer overflow in python register interface

[Andi Kleen]

On Thu, Sep 12, 2024 at 09:41:55AM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Sep 04, 2024 at 06:50:09PM -0700, Andi Kleen wrote:
> > Running a script that processes PEBS records gives buffer overflows
> > in valgrind. The problem is that the allocation of the register
> > string doesn't include the terminating 0 byte. Fix this. I also replaced
> > the very magic "28" with a more reasonable larger buffer that should
> > fit all registers. There's no need to conserve memory here.
> 
> I applied this one already.
> 
> But you used the wrong list address, perf-tools-users@vger, I'm fixing
> this up now to linux-perf-users@vger so that the message reaches the
> mailing list.
> 
> I waited a bit for reviewers but then realized the problem with the list
> address when trying to use b4 to fetch it from lore :-\

I sent it twice. The second version had the right address.

But you were in cc and got both versions, sorry.

-Andi

Re: [PATCH v1 01/10] perf: Avoid buffer overflow in python register interface

[Arnaldo Carvalho de Melo]

On Thu, Sep 12, 2024 at 07:38:21AM -0700, Andi Kleen wrote:
> On Thu, Sep 12, 2024 at 09:41:55AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Wed, Sep 04, 2024 at 06:50:09PM -0700, Andi Kleen wrote:
> > > Running a script that processes PEBS records gives buffer overflows
> > > in valgrind. The problem is that the allocation of the register
> > > string doesn't include the terminating 0 byte. Fix this. I also replaced
> > > the very magic "28" with a more reasonable larger buffer that should
> > > fit all registers. There's no need to conserve memory here.
> > 
> > I applied this one already.
> > 
> > But you used the wrong list address, perf-tools-users@vger, I'm fixing
> > this up now to linux-perf-users@vger so that the message reaches the
> > mailing list.
> > 
> > I waited a bit for reviewers but then realized the problem with the list
> > address when trying to use b4 to fetch it from lore :-\
> 
> I sent it twice. The second version had the right address.
 
I have tried to continue reviewing/processing the patch series from the
one without linux-perf-users@ on the CC :-\

> But you were in cc and got both versions, sorry.

Lets try to continue from here, I made some comments,

Thanks a lot for your work,

- Arnaldo

Re: [PATCH v1 02/10] perf: Support discriminator in addr2line

[Arnaldo Carvalho de Melo]

On Wed, Sep 04, 2024 at 06:50:10PM -0700, Andi Kleen wrote:
> Dwarf has the concept of a discriminator to distinguish multiple
> compiler generated statements in a line. Add support
> for retrieving the discriminator in addr2line. Will be used
> in later patches to pass to python scripts.

So I left the whole message quoted because originally it went to the
wrong/non-existent mailing list (perf-tools-users@vger.kernel.org), so I
couldn't refer it to Steinar.

More comments below.
 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/builtin-script.c                   |  6 ++-
>  .../scripts/python/Perf-Trace-Util/Context.c  |  4 +-
>  tools/perf/util/dlfilter.c                    |  4 +-
>  tools/perf/util/srcline.c                     | 42 ++++++++++++-------
>  tools/perf/util/srcline.h                     |  2 +-
>  5 files changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 206b08426555..dc7e5406dae9 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1131,7 +1131,7 @@ static int map__fprintf_srccode(struct map *map, u64 addr, FILE *fp, struct srcc
>  {
>  	char *srcfile;
>  	int ret = 0;
> -	unsigned line;
> +	unsigned line, disc;
>  	int len;
>  	char *srccode;
>  	struct dso *dso;
> @@ -1140,7 +1140,7 @@ static int map__fprintf_srccode(struct map *map, u64 addr, FILE *fp, struct srcc
>  		return 0;
>  	srcfile = get_srcline_split(dso,
>  				    map__rip_2objdump(map, addr),
> -				    &line);
> +				    &line, &disc);
>  	if (!srcfile)
>  		return 0;
>  
> @@ -1157,6 +1157,8 @@ static int map__fprintf_srccode(struct map *map, u64 addr, FILE *fp, struct srcc
>  	if (!srccode)
>  		goto out_free_line;
>  
> +	/* Print discriminator too? Maybe with a new option */
> +
>  	ret = fprintf(fp, "|%-8d %.*s", line, len, srccode);
>  
>  	if (state) {
> diff --git a/tools/perf/scripts/python/Perf-Trace-Util/Context.c b/tools/perf/scripts/python/Perf-Trace-Util/Context.c
> index 3954bd1587ce..3aa5e6385c1e 100644
> --- a/tools/perf/scripts/python/Perf-Trace-Util/Context.c
> +++ b/tools/perf/scripts/python/Perf-Trace-Util/Context.c
> @@ -140,7 +140,7 @@ static PyObject *perf_set_itrace_options(PyObject *obj, PyObject *args)
>  static PyObject *perf_sample_src(PyObject *obj, PyObject *args, bool get_srccode)
>  {
>  	struct scripting_context *c = get_scripting_context(args);
> -	unsigned int line = 0;
> +	unsigned int line = 0, disc = 0;
>  	char *srcfile = NULL;
>  	char *srccode = NULL;
>  	PyObject *result;
> @@ -157,7 +157,7 @@ static PyObject *perf_sample_src(PyObject *obj, PyObject *args, bool get_srccode
>  	dso = map ? map__dso(map) : NULL;
>  
>  	if (dso)
> -		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line);
> +		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line, &disc);
>  
>  	if (get_srccode) {
>  		if (srcfile)
> diff --git a/tools/perf/util/dlfilter.c b/tools/perf/util/dlfilter.c
> index 7d180bdaedbc..1a3b5ba92313 100644
> --- a/tools/perf/util/dlfilter.c
> +++ b/tools/perf/util/dlfilter.c
> @@ -250,7 +250,7 @@ static const char *dlfilter__srcline(void *ctx, __u32 *line_no)
>  {
>  	struct dlfilter *d = (struct dlfilter *)ctx;
>  	struct addr_location *al;
> -	unsigned int line = 0;
> +	unsigned int line = 0, disc = 0;
>  	char *srcfile = NULL;
>  	struct map *map;
>  	struct dso *dso;
> @@ -268,7 +268,7 @@ static const char *dlfilter__srcline(void *ctx, __u32 *line_no)
>  	dso = map ? map__dso(map) : NULL;
>  
>  	if (dso)
> -		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line);
> +		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line, &disc);
>  
>  	*line_no = line;
>  	return srcfile;
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index 760742fd4a7d..07df410b52e4 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -146,6 +146,7 @@ struct a2l_data {
>  	const char 	*filename;
>  	const char 	*funcname;
>  	unsigned 	line;
> +	unsigned	discriminator;
>  
>  	bfd 		*abfd;
>  	asymbol 	**syms;
> @@ -232,9 +233,10 @@ static void find_address_in_section(bfd *abfd, asection *section, void *data)
>  	if (pc < vma || pc >= vma + size)
>  		return;
>  
> -	a2l->found = bfd_find_nearest_line(abfd, section, a2l->syms, pc - vma,
> -					   &a2l->filename, &a2l->funcname,
> -					   &a2l->line);
> +	a2l->found = bfd_find_nearest_line_discriminator(abfd,
> +							 section, a2l->syms, pc - vma,
> +							 &a2l->filename, &a2l->funcname,
> +							 &a2l->line, &a2l->discriminator);

I noticed that this is under:

#elif defined(HAVE_LIBBFD_SUPPORT)

That isn't built by default due to licensing issues, i.e. we only build
it if BUILD_NONDISTRO=1 is selected on the make command line, see:

tools/perf/Makefile.config +943

And we have the patch below now, can we try to use it instead so that at
some point we can remove the libbpf support?

Steinar?

Thanks,

- Arnaldo

⬢[acme@toolbox perf-tools-next]$ git show c3f8644c21df9b7db97eb70e08e2826368aaafa0
commit c3f8644c21df9b7db97eb70e08e2826368aaafa0
Author: Steinar H. Gunderson <sesse@google.com>
Date:   Sat Aug 3 17:20:06 2024 +0200

    perf report: Support LLVM for addr2line()
    
    In addition to the existing support for libbfd and calling out to
    an external addr2line command, add support for using libllvm directly.
    
    This is both faster than libbfd, and can be enabled in distro builds
    (the LLVM license has an explicit provision for GPLv2 compatibility).
    
    Thus, it is set as the primary choice if available.
    
    As an example, running 'perf report' on a medium-size profile with
    DWARF-based backtraces took 58 seconds with LLVM, 78 seconds with
    libbfd, 153 seconds with external llvm-addr2line, and I got tired and
    aborted the test after waiting for 55 minutes with external bfd
    addr2line (which is the default for perf as compiled by distributions
    today).
    
    Evidently, for this case, the bfd addr2line process needs 18 seconds (on
    a 5.2 GHz Zen 3) to load the .debug ELF in question, hits the 1-second
    timeout and gets killed during initialization, getting restarted anew
    every time. Having an in-process addr2line makes this much more robust.


  
>  	if (a2l->filename && !strlen(a2l->filename))
>  		a2l->filename = NULL;
> @@ -301,7 +303,7 @@ static int inline_list__append_dso_a2l(struct dso *dso,
>  static int addr2line(const char *dso_name, u64 addr,
>  		     char **file, unsigned int *line, struct dso *dso,
>  		     bool unwind_inlines, struct inline_node *node,
> -		     struct symbol *sym)
> +		     struct symbol *sym, unsigned *disc)
>  {
>  	int ret = 0;
>  	struct a2l_data *a2l = dso__a2l(dso);
> @@ -354,6 +356,8 @@ static int addr2line(const char *dso_name, u64 addr,
>  
>  	if (line)
>  		*line = a2l->line;
> +	if (disc)
> +		*disc = a2l->discriminator;
>  
>  	return ret;
>  }
> @@ -506,7 +510,8 @@ static int read_addr2line_record(struct io *io,
>  				 bool first,
>  				 char **function,
>  				 char **filename,
> -				 unsigned int *line_nr)
> +				 unsigned int *line_nr,
> +				 unsigned *disc)
>  {
>  	/*
>  	 * Returns:
> @@ -518,6 +523,9 @@ static int read_addr2line_record(struct io *io,
>  	size_t line_len = 0;
>  	unsigned int dummy_line_nr = 0;
>  	int ret = -1;
> +	char *p;
> +
> +	*disc = 0;
>  
>  	if (function != NULL)
>  		zfree(function);
> @@ -602,6 +610,10 @@ static int read_addr2line_record(struct io *io,
>  		goto error;
>  	}
>  
> +	p = strstr(line, "discriminator");
> +	if (p && disc)
> +		*disc = strtoul(p + sizeof("discriminator"), NULL, 0);
> +
>  	if (filename != NULL)
>  		*filename = strdup(line);
>  
> @@ -636,7 +648,8 @@ static int addr2line(const char *dso_name, u64 addr,
>  		     struct dso *dso,
>  		     bool unwind_inlines,
>  		     struct inline_node *node,
> -		     struct symbol *sym __maybe_unused)
> +		     struct symbol *sym __maybe_unused,
> +		     unsigned *disc)
>  {
>  	struct child_process *a2l = dso__a2l(dso);
>  	char *record_function = NULL;
> @@ -688,7 +701,8 @@ static int addr2line(const char *dso_name, u64 addr,
>  	io__init(&io, a2l->out, buf, sizeof(buf));
>  	io.timeout_ms = addr2line_timeout_ms;
>  	switch (read_addr2line_record(&io, a2l_style, dso_name, addr, /*first=*/true,
> -				      &record_function, &record_filename, &record_line_nr)) {
> +				      &record_function, &record_filename, &record_line_nr,
> +				      &disc)) {
>  	case -1:
>  		if (!symbol_conf.disable_add2line_warn)
>  			pr_warning("%s %s: could not read first record\n", __func__, dso_name);
> @@ -704,7 +718,7 @@ static int addr2line(const char *dso_name, u64 addr,
>  		 */
>  		switch (read_addr2line_record(&io, a2l_style, dso_name,
>  					      /*addr=*/1, /*first=*/true,
> -					      NULL, NULL, NULL)) {
> +					      NULL, NULL, NULL, NULL)) {
>  		case -1:
>  			if (!symbol_conf.disable_add2line_warn)
>  				pr_warning("%s %s: could not read sentinel record\n",
> @@ -754,7 +768,7 @@ static int addr2line(const char *dso_name, u64 addr,
>  						      /*first=*/false,
>  						      &record_function,
>  						      &record_filename,
> -						      &record_line_nr)) == 1) {
> +						      &record_line_nr, NULL)) == 1) {
>  		if (unwind_inlines && node && inline_count++ < MAX_INLINE_NEST) {
>  			if (inline_list__append_record(dso, node, sym,
>  						       record_function,
> @@ -805,7 +819,7 @@ static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
>  	INIT_LIST_HEAD(&node->val);
>  	node->addr = addr;
>  
> -	addr2line(dso_name, addr, NULL, NULL, dso, true, node, sym);
> +	addr2line(dso_name, addr, NULL, NULL, dso, true, node, sym, NULL);
>  	return node;
>  }
>  
> @@ -832,7 +846,7 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
>  		goto out_err;
>  
>  	if (!addr2line(dso_name, addr, &file, &line, dso,
> -		       unwind_inlines, NULL, sym))
> +		       unwind_inlines, NULL, sym, NULL))
>  		goto out_err;
>  
>  	srcline = srcline_from_fileline(file, line);
> @@ -865,8 +879,8 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
>  	return srcline;
>  }
>  
> -/* Returns filename and fills in line number in line */
> -char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line)
> +/* Returns filename and fills in line number in line and discriminator in disc */
> +char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line, unsigned *disc)
>  {
>  	char *file = NULL;
>  	const char *dso_name;
> @@ -878,7 +892,7 @@ char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line)
>  	if (dso_name == NULL)
>  		goto out_err;
>  
> -	if (!addr2line(dso_name, addr, &file, line, dso, true, NULL, NULL))
> +	if (!addr2line(dso_name, addr, &file, line, dso, true, NULL, NULL, disc))
>  		goto out_err;
>  
>  	dso__set_a2l_fails(dso, 0);
> diff --git a/tools/perf/util/srcline.h b/tools/perf/util/srcline.h
> index 75010d39ea28..d2cb90b1177e 100644
> --- a/tools/perf/util/srcline.h
> +++ b/tools/perf/util/srcline.h
> @@ -17,7 +17,7 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
>  		  bool show_sym, bool show_addr, bool unwind_inlines,
>  		  u64 ip);
>  void zfree_srcline(char **srcline);
> -char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line);
> +char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line, unsigned *disc);
>  
>  /* insert the srcline into the DSO, which will take ownership */
>  void srcline__tree_insert(struct rb_root_cached *tree, u64 addr, char *srcline);
> -- 
> 2.45.2

Re: [PATCH v1 02/10] perf: Support discriminator in addr2line

[Andi Kleen]

Arnaldo Carvalho de Melo <acme@kernel.org> writes:
>
> I noticed that this is under:
>
> #elif defined(HAVE_LIBBFD_SUPPORT)
>
> That isn't built by default due to licensing issues, i.e. we only build
> it if BUILD_NONDISTRO=1 is selected on the make command line, see:
>
> tools/perf/Makefile.config +943

Yes I know. I ran into this when my perf report runs were suddenly incredibly
slow until I figured out how to set this flag.

It's sad how theoretical lawyerneering breaks perfectly fine code.

At least I'm fine with whatever legal risk this causes.


>
> And we have the patch below now, can we try to use it instead so that at
> some point we can remove the libbpf support?

I also support the external addr2line.

But yes need to support the LLVM version too. I'll look into that.

As a side note. I always disliked the LLVM dependencies, these
libraries/packages are gigantic and traditionally unstable in
interface. It would be better to use a sane small library as a
replacement like https://github.com/ianlancetaylor/libbacktrace

-Andi

[RESEND] More dwarf support in python interface

[Andi Kleen]

[resend because I typoed the mailing list address in the first try.
Apologies if you see it twice]

This patch kit adds better support for resolving DWARF from
perf script python scripts:

- Add new perf_brstack_srcline / perf_ip_srcline functions
to resolve full brstacks or individual IPs.
- Support the DWARF discriminator in addition to the line number
- Report the inline stack to python
- Report the build-id and executable name.

Most of the patch is just plumbing to pass all this information
around.

The first patch is a bug fix and can be applied independently.

-andi

[PATCH v1 02/10] perf: Support discriminator in addr2line

[Andi Kleen]

Dwarf has the concept of a discriminator to distinguish multiple
compiler generated statements in a line. Add support
for retrieving the discriminator in addr2line. Will be used
in later patches to pass to python scripts.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-script.c                   |  6 ++-
 .../scripts/python/Perf-Trace-Util/Context.c  |  4 +-
 tools/perf/util/dlfilter.c                    |  4 +-
 tools/perf/util/srcline.c                     | 42 ++++++++++++-------
 tools/perf/util/srcline.h                     |  2 +-
 5 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 206b08426555..dc7e5406dae9 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1131,7 +1131,7 @@ static int map__fprintf_srccode(struct map *map, u64 addr, FILE *fp, struct srcc
 {
 	char *srcfile;
 	int ret = 0;
-	unsigned line;
+	unsigned line, disc;
 	int len;
 	char *srccode;
 	struct dso *dso;
@@ -1140,7 +1140,7 @@ static int map__fprintf_srccode(struct map *map, u64 addr, FILE *fp, struct srcc
 		return 0;
 	srcfile = get_srcline_split(dso,
 				    map__rip_2objdump(map, addr),
-				    &line);
+				    &line, &disc);
 	if (!srcfile)
 		return 0;
 
@@ -1157,6 +1157,8 @@ static int map__fprintf_srccode(struct map *map, u64 addr, FILE *fp, struct srcc
 	if (!srccode)
 		goto out_free_line;
 
+	/* Print discriminator too? Maybe with a new option */
+
 	ret = fprintf(fp, "|%-8d %.*s", line, len, srccode);
 
 	if (state) {
diff --git a/tools/perf/scripts/python/Perf-Trace-Util/Context.c b/tools/perf/scripts/python/Perf-Trace-Util/Context.c
index 3954bd1587ce..3aa5e6385c1e 100644
--- a/tools/perf/scripts/python/Perf-Trace-Util/Context.c
+++ b/tools/perf/scripts/python/Perf-Trace-Util/Context.c
@@ -140,7 +140,7 @@ static PyObject *perf_set_itrace_options(PyObject *obj, PyObject *args)
 static PyObject *perf_sample_src(PyObject *obj, PyObject *args, bool get_srccode)
 {
 	struct scripting_context *c = get_scripting_context(args);
-	unsigned int line = 0;
+	unsigned int line = 0, disc = 0;
 	char *srcfile = NULL;
 	char *srccode = NULL;
 	PyObject *result;
@@ -157,7 +157,7 @@ static PyObject *perf_sample_src(PyObject *obj, PyObject *args, bool get_srccode
 	dso = map ? map__dso(map) : NULL;
 
 	if (dso)
-		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line);
+		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line, &disc);
 
 	if (get_srccode) {
 		if (srcfile)
diff --git a/tools/perf/util/dlfilter.c b/tools/perf/util/dlfilter.c
index 7d180bdaedbc..1a3b5ba92313 100644
--- a/tools/perf/util/dlfilter.c
+++ b/tools/perf/util/dlfilter.c
@@ -250,7 +250,7 @@ static const char *dlfilter__srcline(void *ctx, __u32 *line_no)
 {
 	struct dlfilter *d = (struct dlfilter *)ctx;
 	struct addr_location *al;
-	unsigned int line = 0;
+	unsigned int line = 0, disc = 0;
 	char *srcfile = NULL;
 	struct map *map;
 	struct dso *dso;
@@ -268,7 +268,7 @@ static const char *dlfilter__srcline(void *ctx, __u32 *line_no)
 	dso = map ? map__dso(map) : NULL;
 
 	if (dso)
-		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line);
+		srcfile = get_srcline_split(dso, map__rip_2objdump(map, addr), &line, &disc);
 
 	*line_no = line;
 	return srcfile;
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 760742fd4a7d..07df410b52e4 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -146,6 +146,7 @@ struct a2l_data {
 	const char 	*filename;
 	const char 	*funcname;
 	unsigned 	line;
+	unsigned	discriminator;
 
 	bfd 		*abfd;
 	asymbol 	**syms;
@@ -232,9 +233,10 @@ static void find_address_in_section(bfd *abfd, asection *section, void *data)
 	if (pc < vma || pc >= vma + size)
 		return;
 
-	a2l->found = bfd_find_nearest_line(abfd, section, a2l->syms, pc - vma,
-					   &a2l->filename, &a2l->funcname,
-					   &a2l->line);
+	a2l->found = bfd_find_nearest_line_discriminator(abfd,
+							 section, a2l->syms, pc - vma,
+							 &a2l->filename, &a2l->funcname,
+							 &a2l->line, &a2l->discriminator);
 
 	if (a2l->filename && !strlen(a2l->filename))
 		a2l->filename = NULL;
@@ -301,7 +303,7 @@ static int inline_list__append_dso_a2l(struct dso *dso,
 static int addr2line(const char *dso_name, u64 addr,
 		     char **file, unsigned int *line, struct dso *dso,
 		     bool unwind_inlines, struct inline_node *node,
-		     struct symbol *sym)
+		     struct symbol *sym, unsigned *disc)
 {
 	int ret = 0;
 	struct a2l_data *a2l = dso__a2l(dso);
@@ -354,6 +356,8 @@ static int addr2line(const char *dso_name, u64 addr,
 
 	if (line)
 		*line = a2l->line;
+	if (disc)
+		*disc = a2l->discriminator;
 
 	return ret;
 }
@@ -506,7 +510,8 @@ static int read_addr2line_record(struct io *io,
 				 bool first,
 				 char **function,
 				 char **filename,
-				 unsigned int *line_nr)
+				 unsigned int *line_nr,
+				 unsigned *disc)
 {
 	/*
 	 * Returns:
@@ -518,6 +523,9 @@ static int read_addr2line_record(struct io *io,
 	size_t line_len = 0;
 	unsigned int dummy_line_nr = 0;
 	int ret = -1;
+	char *p;
+
+	*disc = 0;
 
 	if (function != NULL)
 		zfree(function);
@@ -602,6 +610,10 @@ static int read_addr2line_record(struct io *io,
 		goto error;
 	}
 
+	p = strstr(line, "discriminator");
+	if (p && disc)
+		*disc = strtoul(p + sizeof("discriminator"), NULL, 0);
+
 	if (filename != NULL)
 		*filename = strdup(line);
 
@@ -636,7 +648,8 @@ static int addr2line(const char *dso_name, u64 addr,
 		     struct dso *dso,
 		     bool unwind_inlines,
 		     struct inline_node *node,
-		     struct symbol *sym __maybe_unused)
+		     struct symbol *sym __maybe_unused,
+		     unsigned *disc)
 {
 	struct child_process *a2l = dso__a2l(dso);
 	char *record_function = NULL;
@@ -688,7 +701,8 @@ static int addr2line(const char *dso_name, u64 addr,
 	io__init(&io, a2l->out, buf, sizeof(buf));
 	io.timeout_ms = addr2line_timeout_ms;
 	switch (read_addr2line_record(&io, a2l_style, dso_name, addr, /*first=*/true,
-				      &record_function, &record_filename, &record_line_nr)) {
+				      &record_function, &record_filename, &record_line_nr,
+				      &disc)) {
 	case -1:
 		if (!symbol_conf.disable_add2line_warn)
 			pr_warning("%s %s: could not read first record\n", __func__, dso_name);
@@ -704,7 +718,7 @@ static int addr2line(const char *dso_name, u64 addr,
 		 */
 		switch (read_addr2line_record(&io, a2l_style, dso_name,
 					      /*addr=*/1, /*first=*/true,
-					      NULL, NULL, NULL)) {
+					      NULL, NULL, NULL, NULL)) {
 		case -1:
 			if (!symbol_conf.disable_add2line_warn)
 				pr_warning("%s %s: could not read sentinel record\n",
@@ -754,7 +768,7 @@ static int addr2line(const char *dso_name, u64 addr,
 						      /*first=*/false,
 						      &record_function,
 						      &record_filename,
-						      &record_line_nr)) == 1) {
+						      &record_line_nr, NULL)) == 1) {
 		if (unwind_inlines && node && inline_count++ < MAX_INLINE_NEST) {
 			if (inline_list__append_record(dso, node, sym,
 						       record_function,
@@ -805,7 +819,7 @@ static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
 	INIT_LIST_HEAD(&node->val);
 	node->addr = addr;
 
-	addr2line(dso_name, addr, NULL, NULL, dso, true, node, sym);
+	addr2line(dso_name, addr, NULL, NULL, dso, true, node, sym, NULL);
 	return node;
 }
 
@@ -832,7 +846,7 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 		goto out_err;
 
 	if (!addr2line(dso_name, addr, &file, &line, dso,
-		       unwind_inlines, NULL, sym))
+		       unwind_inlines, NULL, sym, NULL))
 		goto out_err;
 
 	srcline = srcline_from_fileline(file, line);
@@ -865,8 +879,8 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 	return srcline;
 }
 
-/* Returns filename and fills in line number in line */
-char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line)
+/* Returns filename and fills in line number in line and discriminator in disc */
+char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line, unsigned *disc)
 {
 	char *file = NULL;
 	const char *dso_name;
@@ -878,7 +892,7 @@ char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line)
 	if (dso_name == NULL)
 		goto out_err;
 
-	if (!addr2line(dso_name, addr, &file, line, dso, true, NULL, NULL))
+	if (!addr2line(dso_name, addr, &file, line, dso, true, NULL, NULL, disc))
 		goto out_err;
 
 	dso__set_a2l_fails(dso, 0);
diff --git a/tools/perf/util/srcline.h b/tools/perf/util/srcline.h
index 75010d39ea28..d2cb90b1177e 100644
--- a/tools/perf/util/srcline.h
+++ b/tools/perf/util/srcline.h
@@ -17,7 +17,7 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 		  bool show_sym, bool show_addr, bool unwind_inlines,
 		  u64 ip);
 void zfree_srcline(char **srcline);
-char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line);
+char *get_srcline_split(struct dso *dso, u64 addr, unsigned *line, unsigned *disc);
 
 /* insert the srcline into the DSO, which will take ownership */
 void srcline__tree_insert(struct rb_root_cached *tree, u64 addr, char *srcline);
-- 
2.45.2