Expected 0x

Expected 0x

[Dave Jones]

Linus' current tree seems to trigger this in perf_event_open on startup:

                        if (value[ptr]!='0') fprintf(stderr,"Expected 0x\n");
                        ptr++;
                        if (value[ptr]!='x') fprintf(stderr,"Expected 0x\n");
                        ptr++;

I think it's from this..

/sys/bus/event_source/devices/cpu/events/mem-loads
event=0xcd,umask=0x1,ldlat=3

Looks like that field isn't always hex.

	Dave

Re: Expected 0x

[Vince Weaver]

On Wed, 3 Jul 2013, Dave Jones wrote:

> Linus' current tree seems to trigger this in perf_event_open on startup:
> 
>                         if (value[ptr]!='0') fprintf(stderr,"Expected 0x\n");
>                         ptr++;
>                         if (value[ptr]!='x') fprintf(stderr,"Expected 0x\n");
>                         ptr++;
> 
> I think it's from this..
> 
> /sys/bus/event_source/devices/cpu/events/mem-loads
> event=0xcd,umask=0x1,ldlat=3
> 
> Looks like that field isn't always hex.

Ugh, I would have thought they were using sprintf() or something
to generate these, but they just hard-code the string in the kernel.

  EVENT_ATTR_STR(mem-loads, mem_ld_snb, "event=0xcd,umask=0x1,ldlat=3");

The only documentation for what to expect are the lex/yacc files in
tools/perf/util/parse-events.*

I can sort of muddle my way through lex/yacc enough to see that it looks 
like both hex and decimal are allowed, so I'll update the parser for 
trinity.

I guess I should reverse-engineer what the lex/yacc is doing and document 
it in the manpage since this is an official ABI.  Fun times.


Vince Weaver
vincent.weaver@maine.edu
http://www.eece.maine.edu/~vweaver/

Re: Expected 0x

[Michael Ellerman]

On Wed, 2013-07-03 at 14:43 -0400, Vince Weaver wrote:
> On Wed, 3 Jul 2013, Dave Jones wrote:
> 
> > Linus' current tree seems to trigger this in perf_event_open on startup:
> > 
> >                         if (value[ptr]!='0') fprintf(stderr,"Expected 0x\n");
> >                         ptr++;
> >                         if (value[ptr]!='x') fprintf(stderr,"Expected 0x\n");
> >                         ptr++;
> > 
> > I think it's from this..
> > 
> > /sys/bus/event_source/devices/cpu/events/mem-loads
> > event=0xcd,umask=0x1,ldlat=3
> > 
> > Looks like that field isn't always hex.
> 
> Ugh, I would have thought they were using sprintf() or something
> to generate these, but they just hard-code the string in the kernel.
> 
>   EVENT_ATTR_STR(mem-loads, mem_ld_snb, "event=0xcd,umask=0x1,ldlat=3");
> 
> The only documentation for what to expect are the lex/yacc files in
> tools/perf/util/parse-events.*

It's sort of documented here:

Documentation/ABI/testing/sysfs-bus-event_source-devices-events


Though strictly speaking that only applies to the events listed in that
file, which doesn't include mem-loads.

And it says it should be hex.

cheers

Re: Expected 0x

[Vince Weaver]

On Thu, 4 Jul 2013, Michael Ellerman wrote:

> On Wed, 2013-07-03 at 14:43 -0400, Vince Weaver wrote:
> > 
> > The only documentation for what to expect are the lex/yacc files in
> > tools/perf/util/parse-events.*
> 
> It's sort of documented here:
> 
> Documentation/ABI/testing/sysfs-bus-event_source-devices-events
>  
> Though strictly speaking that only applies to the events listed in that
> file, which doesn't include mem-loads.
> 
> And it says it should be hex.

now don't I look silly, I had just sent a patch to linux-kernel 
documenting the files but putting it in 
    Documentation/ABI/stable/sysfs-bus-event_source-devices
rather than testing.  My search through git log somehow missed when this 
got added.

Oh I see, it came through the Power people and not through perf_event, 
that's how I missed it.

I guess I should complain about the non-hex value.  I'm more worried that 
the perf people are going to start leaking more complex perf-tool specific 
syntax into the files.

Vince

Re: Expected 0x

[Michael Ellerman]

On Wed, 2013-07-03 at 23:07 -0400, Vince Weaver wrote:
> On Thu, 4 Jul 2013, Michael Ellerman wrote:
> 
> > On Wed, 2013-07-03 at 14:43 -0400, Vince Weaver wrote:
> > > 
> > > The only documentation for what to expect are the lex/yacc files in
> > > tools/perf/util/parse-events.*
> > 
> > It's sort of documented here:
> > 
> > Documentation/ABI/testing/sysfs-bus-event_source-devices-events
> >  
> > Though strictly speaking that only applies to the events listed in that
> > file, which doesn't include mem-loads.
> > 
> > And it says it should be hex.
> 
> now don't I look silly, I had just sent a patch to linux-kernel 
> documenting the files but putting it in 
>     Documentation/ABI/stable/sysfs-bus-event_source-devices
> rather than testing.  My search through git log somehow missed when this 
> got added.
> 
> Oh I see, it came through the Power people and not through perf_event, 
> that's how I missed it.

It still went through acme's tree AFAIK, but yeah easy enough to miss.

> I guess I should complain about the non-hex value.  I'm more worried that 
> the perf people are going to start leaking more complex perf-tool specific 
> syntax into the files.

Nothing in there currently is specific to the perf tool, though
obviously the perf tool accepts decimal and so didn't spot the problem.

Still the best way to ensure that doesn't happen is to have a second
consumer of the API, which can spot these sort of issues.

cheers

Re: Expected 0x

[Vince Weaver]

The following patch should properly handle decimal values in
   /sys/bus/event_source/devices/cpu/events/
files.

Feel free to run lindent on the whole perf_event_open.c sometime.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>

diff --git a/syscalls/perf_event_open.c b/syscalls/perf_event_open.c
index ef8cb4f..5c8decd 100644
--- a/syscalls/perf_event_open.c
+++ b/syscalls/perf_event_open.c
@@ -16,6 +16,7 @@
 #include "maps.h"
 #include "shm.h"
 
+#define SYSFS "/sys/bus/event_source/devices/"
 
 struct generic_event_type {
 	char *name;
@@ -150,6 +151,7 @@ static int parse_generic(int pmu,char *value, long long *config, long long *conf
 	long long c=0,c1=0,temp;
 	char field[BUFSIZ];
 	int i,ptr=0;
+	int base=10;
 
 	while(1) {
 		i=0;
@@ -165,18 +167,24 @@ static int parse_generic(int pmu,char *value, long long *config, long long *conf
 		}
 
 		/* if at end, was parameter w/o value */
+		/* So it is a flag with a value of 1  */
 		if ((value[ptr]==',') || (value[ptr]==0)) {
 			temp=0x1;
 		}
 		else {
 			/* get number */
 
-			ptr++;
+			base=10;
 
-			if (value[ptr]!='0') fprintf(stderr,"Expected 0x\n");
-			ptr++;
-			if (value[ptr]!='x') fprintf(stderr,"Expected 0x\n");
 			ptr++;
+
+			if (value[ptr]=='0') {
+				if (value[ptr+1]=='x') {
+					ptr++;
+					ptr++;
+					base=16;
+				}
+			}
 			temp=0x0;
 			while(1) {
 
@@ -186,7 +194,7 @@ static int parse_generic(int pmu,char *value, long long *config, long long *conf
                    			|| ((value[ptr]>='a') && (value[ptr]<='f'))) ) {
 					fprintf(stderr,"Unexpected char %c\n",value[ptr]);
 				}
-				temp*=16;
+				temp*=base;
 				if ((value[ptr]>='0') && (value[ptr]<='9')) {
 					temp+=value[ptr]-'0';
 				}
@@ -221,7 +229,7 @@ static int init_pmus(void) {
 	/* Count number of PMUs */
 	/* This may break if PMUs are ever added/removed on the fly? */
 
-	dir=opendir("/sys/bus/event_source/devices");
+	dir=opendir(SYSFS);
 	if (dir==NULL) {
 		return -1;
 	}
@@ -255,7 +263,7 @@ static int init_pmus(void) {
 
 		/* read name */
 		pmus[pmu_num].name=strdup(entry->d_name);
-		sprintf(dir_name,"/sys/bus/event_source/devices/%s",
+		sprintf(dir_name,SYSFS"/%s",
 			entry->d_name);
 
 		/* read type */

Re: Expected 0x

[Dave Jones]

On Wed, Jul 03, 2013 at 05:07:38PM -0400, Vince Weaver wrote:
 > 
 > The following patch should properly handle decimal values in
 >    /sys/bus/event_source/devices/cpu/events/
 > files.

applied and pushed. thanks.

 > Feel free to run lindent on the whole perf_event_open.c sometime.

maybe next week ;)

btw, here's something curious that llvm finds when you build trinity with clang.
I'm not sure if this is a real problem, or a bug in the compiler, but you
might want to take a look anyway:

syscalls/perf_event_open.c:66:19: warning: Assigned value is garbage or undefined
                format_string[i]=string[i];
                                ^~~~~~~~~~
syscalls/perf_event_open.c:159:12: warning: Assigned value is garbage or undefined
                        field[i]=value[ptr];
                                ^~~~~~~~~~~

In case you're not setup for llvm, I've put the decision trees it generated at
http://codemonkey.org.uk/junk/1.html
http://codemonkey.org.uk/junk/2.html

It may be that some of those combinations it took will never happen, but
I'd like to know for sure. (and if possible somehow silence the warnings
as long as it doesn't ugly stuff up too much)

	Dave

Re: Expected 0x

[Vince Weaver]

On Wed, 3 Jul 2013, Dave Jones wrote:

> btw, here's something curious that llvm finds when you build trinity with clang.
> I'm not sure if this is a real problem, or a bug in the compiler, but you
> might want to take a look anyway:
> 
> syscalls/perf_event_open.c:66:19: warning: Assigned value is garbage or undefined
>                 format_string[i]=string[i];
>                                 ^~~~~~~~~~
> syscalls/perf_event_open.c:159:12: warning: Assigned value is garbage or undefined
>                         field[i]=value[ptr];
>                                 ^~~~~~~~~~~
> 
> In case you're not setup for llvm, I've put the decision trees it generated at
> http://codemonkey.org.uk/junk/1.html
> http://codemonkey.org.uk/junk/2.html

I'll see if I can figure out llvm, I've been wanting an excuse to look 
into it.

The values that it is complaining about are gathered via a fscanf followed 
by a strdup, so in theory if the fscanf fails (due to the file being 
empty) or if strdup() fails (due to OOM) it is indeed possible the string 
is garbage.  I can add handling for those conditions to see if it helps.

Vince

Re: Expected 0x

[Vince Weaver]

On Wed, 3 Jul 2013, Vince Weaver wrote:

> I'll see if I can figure out llvm, I've been wanting an excuse to look 
> into it.

well it turns out it was as simple as "apt-get install clang"
"make scan".  Too easy.

> The values that it is complaining about are gathered via a fscanf followed 
> by a strdup, so in theory if the fscanf fails (due to the file being 
> empty) or if strdup() fails (due to OOM) it is indeed possible the string 
> is garbage.  I can add handling for those conditions to see if it helps.

I was slightly off, the problem turned out to be if we fail at fopen()
we never got to the fscanf(), but we'd then still call parse_format()
with an uninitialized format_value.  So good catch by llvm.

The following should fix things.

Signed-off-by: Vince Weaver <vincent.weaver@maine.edu>

diff --git a/syscalls/perf_event_open.c b/syscalls/perf_event_open.c
index 5df6bed..0835de7 100644
--- a/syscalls/perf_event_open.c
+++ b/syscalls/perf_event_open.c
@@ -323,18 +323,19 @@ static int init_pmus(void) {
 					pmus[pmu_num].formats[format_num].value=
 						strdup(format_value);
 					fclose(fff);
-				}
-				parse_format(format_value,
+
+					parse_format(format_value,
 						&pmus[pmu_num].formats[format_num].field,
 						&pmus[pmu_num].formats[format_num].shift,
 						&pmus[pmu_num].formats[format_num].bits);
-				if (pmus[pmu_num].formats[format_num].bits==64) {
-					pmus[pmu_num].formats[format_num].mask=0xffffffffffffffffULL;
-				} else {
-					pmus[pmu_num].formats[format_num].mask=
-						(1ULL<<pmus[pmu_num].formats[format_num].bits)-1;
+					if (pmus[pmu_num].formats[format_num].bits==64) {
+						pmus[pmu_num].formats[format_num].mask=0xffffffffffffffffULL;
+					} else {
+						pmus[pmu_num].formats[format_num].mask=
+							(1ULL<<pmus[pmu_num].formats[format_num].bits)-1;
+					}
+					format_num++;
 				}
-				format_num++;
 			}
 			closedir(format_dir);
 		}