Re: [PATCH 2/4] perf-probe: Require '@' prefix for filename always

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Mon, 4 Nov 2024 17:10:41 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> On Tue, Nov 05, 2024 at 01:17:26AM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>  
> > Currently perf probe allows user to specify probing place without '@'
> > prefix, for example, both `-V file:line` and `-V function:line` are
> > allowed. But this makes a problem if a (demangled) function name is
> > hard to be distinguished from a file name.
>  
> > This changes the perf-probe to require '@' prefix for filename even
> > without function name. For example, `-V @file:line` and
> > `-V function:line` are acceptable.
>  
> > With this change, users can specify filename or function correctly.
> 
> Well, this will break scripts that use perf probe for a given file,
> probably the right thing not to break backwards compatibility is to
> continue accepting and when there is a real conflict, an ambiguity that
> makes differentiating from file to function names, then refuse it,
> stating that it is ambiguous, probably spelling out the names of the
> files and functions that match so and stating that to make it
> unambiguoius, prefix file names with @.

The problem is that the ambiguous function name. For example, Go's
main routine is `main.main`, and this is not likely to the C function
name, so currently perf probe treats it as a filename and failed to
find that.

I think one possible solution is to run search loop twice internally
(search it as file name, if fails, search it as function name) if it
looks like a file name but it does not start from `@`.
This takes costs a bit but can keep backward compatibility.

Thank you,

> 
> - Arnaldo
>  
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  tools/perf/util/probe-event.c |   31 +++++++++----------------------
> >  1 file changed, 9 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 665dcce482e1..913a27cbb5d9 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -1341,7 +1341,7 @@ static bool is_c_func_name(const char *name)
> >   * Stuff 'lr' according to the line range described by 'arg'.
> >   * The line range syntax is described by:
> >   *
> > - *         SRC[:SLN[+NUM|-ELN]]
> > + *         @SRC[:SLN[+NUM|-ELN]]
> >   *         FNC[@SRC][:SLN[+NUM|-ELN]]
> >   */
> >  int parse_line_range_desc(const char *arg, struct line_range *lr)
> > @@ -1404,16 +1404,10 @@ int parse_line_range_desc(const char *arg, struct line_range *lr)
> >  			err = -ENOMEM;
> >  			goto err;
> >  		}
> > +		if (*name != '\0')
> > +			lr->function = name;
> > +	} else
> >  		lr->function = name;
> > -	} else if (strpbrk_esc(name, "/."))
> > -		lr->file = name;
> > -	else if (is_c_func_name(name))/* We reuse it for checking funcname */
> > -		lr->function = name;
> > -	else {	/* Invalid name */
> > -		semantic_error("'%s' is not a valid function name.\n", name);
> > -		err = -EINVAL;
> > -		goto err;
> > -	}
> >  
> >  	return 0;
> >  err:
> > @@ -1463,7 +1457,7 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  
> >  	/*
> >  	 * <Syntax>
> > -	 * perf probe [GRP:][EVENT=]SRC[:LN|;PTN]
> > +	 * perf probe [GRP:][EVENT=]@SRC[:LN|;PTN]
> >  	 * perf probe [GRP:][EVENT=]FUNC[@SRC][+OFFS|%return|:LN|;PAT]
> >  	 * perf probe %[GRP:]SDT_EVENT
> >  	 */
> > @@ -1516,19 +1510,12 @@ static int parse_perf_probe_point(char *arg, struct perf_probe_event *pev)
> >  	/*
> >  	 * Check arg is function or file name and copy it.
> >  	 *
> > -	 * We consider arg to be a file spec if and only if it satisfies
> > -	 * all of the below criteria::
> > -	 * - it does not include any of "+@%",
> > -	 * - it includes one of ":;", and
> > -	 * - it has a period '.' in the name.
> > -	 *
> > +	 * We consider arg to be a file spec if it starts with '@'.
> >  	 * Otherwise, we consider arg to be a function specification.
> >  	 */
> > -	if (!strpbrk_esc(arg, "+@%")) {
> > -		ptr = strpbrk_esc(arg, ";:");
> > -		/* This is a file spec if it includes a '.' before ; or : */
> > -		if (ptr && memchr(arg, '.', ptr - arg))
> > -			file_spec = true;
> > +	if (*arg == '@') {
> > +		file_spec = true;
> > +		arg++;
> >  	}
> >  
> >  	ptr = strpbrk_esc(arg, ";:+@%");


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>