Re: [PATCH 2/2] perf, tools: Don't force MetricExprs to lower case

[Jiri Olsa]

On Wed, Oct 04, 2017 at 09:27:11AM -0700, Andi Kleen wrote:
> On Wed, Oct 04, 2017 at 12:30:52PM +0200, Jiri Olsa wrote:
> > On Tue, Oct 03, 2017 at 01:06:05PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Sep 12, 2017 at 12:56:43PM -0700, Andi Kleen escreveu:
> > > > From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > > > 
> > > > There are still problems with BPF misinterpreting some events
> > > > that include .c. An earlier fix made it work for stand alone
> > > > aliases, but it still fails for more complex constructs.
> > > 
> > > Hi Wang, Jiri,
> > > 
> > > 	Can you please take a look at this and see if there is something
> > > we can do to help Andi?
> > > 
> > > - Arnaldo
> > >  
> > > > REJECT keeps trying and trying a shorter string until
> > > > .c is matched and it appears like a valid BPF path.
> > > > 
> > > > % perf stat -e cpu/uops_executed.core,cmask=1/ true
> > > > bpf: builtin compilation failed: -95, try external compiler
> > > > ERROR: problems with path cpu/uops_executed.c: No such file or directory
> > > > event syntax error: 'cpu/uops_executed.core,cmask=1/'
> > > >                      \___ Failed to load cpu/uops_executed.c from source: Error when compiling BPF scriptlet
> > > > 
> > > > I tried to fix it, but it exceeds my flex knowledge, because
> > > > REJECT does not interact well with BEGIN states.
> > > > 
> > > > The BPF syntax in its current form really causes an ambigious
> > > > grammar.
> > 
> > right, it looks like we allow whole path (including / char)
> > for BPF file, which messes up with out pmu/.../ syntax
> > 
> > do we need that? (Cc-ed some bpf folks)
> > 
> > if not attached patch seems to fix things.. otherwise
> > we need to come up with another fix
> 
> I tried similar patches, but I always ran into more complex
> situations where it still matched incorrectly.
> 
> e.g. try it with cpu/uops_executed.core,... vs uops_executed.core

hm, both works for me with the change:

  perf stat -e cpu/uops_executed.core/ ls
  perf stat -e uops_executed.core ls

> The only real fix would be probably to add some unique 
> prefix for BPF, but that would break all existing users.


yea, there was no response from bpf folks, but it's  probably not an option

how about checking if the file exist like below..

jirka


---
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index ea2426daf7e8..38a42bdf1492 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -8,6 +8,9 @@
 
 %{
 #include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 #include "../perf.h"
 #include "parse-events.h"
 #include "parse-events-bison.h"
@@ -53,9 +56,8 @@ static int str(yyscan_t scanner, int token)
 	return token;
 }
 
-static bool isbpf(yyscan_t scanner)
+static bool isbpf_suffix(char *text)
 {
-	char *text = parse_events_get_text(scanner);
 	int len = strlen(text);
 
 	if (len < 2)
@@ -68,6 +70,17 @@ static bool isbpf(yyscan_t scanner)
 	return false;
 }
 
+static bool isbpf(yyscan_t scanner)
+{
+	char *text = parse_events_get_text(scanner);
+	struct stat st;
+
+	if (!isbpf_suffix(text))
+		return false;
+
+	return stat(text, &st) == 0;
+}
+
 /*
  * This function is called when the parser gets two kind of input:
  *