Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Jiri Olsa]

On Mon, Jul 24, 2017 at 05:16:38PM -0700, Andi Kleen wrote:
> From: Andi Kleen 
> 
> Today, when a JSON file fails parsing the build continues,
> but there are no json files built in, which is difficult to debug later.
> Make the build stop on a parse error instead.
> 
> v2: Add fixes from Sukadev. Now we handle architectures
> with no JSON events correctly. And fix some stale comments.
> Cc: suka...@linux.vnet.ibm.com
> Signed-off-by: Andi Kleen 

Acked-by: Jiri Olsa 

thanks,
jirka

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Jiri Olsa]

On Mon, Jul 24, 2017 at 05:16:38PM -0700, Andi Kleen wrote:
> From: Andi Kleen 
> 
> Today, when a JSON file fails parsing the build continues,
> but there are no json files built in, which is difficult to debug later.
> Make the build stop on a parse error instead.
> 
> v2: Add fixes from Sukadev. Now we handle architectures
> with no JSON events correctly. And fix some stale comments.
> Cc: suka...@linux.vnet.ibm.com
> Signed-off-by: Andi Kleen 

Acked-by: Jiri Olsa 

thanks,
jirka

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Sukadev Bhattiprolu]

Andi Kleen [a...@firstfloor.org] wrote:
> On Mon, Jul 24, 2017 at 02:42:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jul 24, 2017 at 04:16:49PM +0200, Jiri Olsa escreveu:
> > > On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> > > > Andi Kleen [a...@firstfloor.org] wrote:
> > > > > From: Andi Kleen 
> > > > > 
> > > > > Today, when a JSON file fails parsing the build continues,
> > > > > but there are no json files built in, which is difficult to debug 
> > > > > later.
> > > > > Make the build stop on a parse error instead.
> > > > 
> > > > I see the problem and we were being defensive to not break the build
> > > > on architectures that don't yet have the PMU event lists. It will be
> > > > good to check build on an architecture other than x86/powerpc.
> > > > 
> > > > Also, following comments may no longer be applicable?
> > > 
> > > Isn't the Andi's change only to fail in case there's
> > > a real error in process_one_file? I think you can still
> > > have empty events dir.
> > 
> > That explains why all the cross builds failed when I added that cset?
> 
> Hmm, let me test. It was supposed to only fail when there
> is a file, but it cannot be parsed.

Wonder if adding this check for event lists helps.

---
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index baa073f..4de5c87 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -840,6 +840,7 @@ int main(int argc, char *argv[])
const char *arch;
const char *output_file;
const char *start_dirname;
+   struct stat stbuf;
 
prog = basename(argv[0]);
if (argc < 4) {
@@ -861,11 +862,17 @@ int main(int argc, char *argv[])
return 2;
}
 
+   sprintf(ldirname, "%s/%s", start_dirname, arch);
+
+   /* If architecture does not have any event lists, bail out */
+   if (stat(ldirname, ) < 0) {
+   pr_info("%s: Arch %s has no PMU event lists\n", prog, arch);
+   goto empty_map;
+   }
+
/* Include pmu-events.h first */
fprintf(eventsfp, "#include \"../../pmu-events/pmu-events.h\"\n");
 
-   sprintf(ldirname, "%s/%s", start_dirname, arch);
-
/*
 * The mapfile allows multiple CPUids to point to the same JSON file,
 * so, not sure if there is a need for symlinks within the pmu-events
@@ -882,6 +889,9 @@ int main(int argc, char *argv[])
if (rc && verbose) {
pr_info("%s: Error walking file tree %s\n", prog, ldirname);
goto empty_map;
+   } else if (rc < 0) {
+   /* Make build fail */
+   return 1;
} else if (rc) {
goto empty_map;
}
@@ -896,7 +906,8 @@ int main(int argc, char *argv[])
 
if (process_mapfile(eventsfp, mapfile)) {
pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
-   goto empty_map;
+   /* Make build fail */
+   return 1;
}
 
return 0;

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Sukadev Bhattiprolu]

Andi Kleen [a...@firstfloor.org] wrote:
> On Mon, Jul 24, 2017 at 02:42:09PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jul 24, 2017 at 04:16:49PM +0200, Jiri Olsa escreveu:
> > > On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> > > > Andi Kleen [a...@firstfloor.org] wrote:
> > > > > From: Andi Kleen 
> > > > > 
> > > > > Today, when a JSON file fails parsing the build continues,
> > > > > but there are no json files built in, which is difficult to debug 
> > > > > later.
> > > > > Make the build stop on a parse error instead.
> > > > 
> > > > I see the problem and we were being defensive to not break the build
> > > > on architectures that don't yet have the PMU event lists. It will be
> > > > good to check build on an architecture other than x86/powerpc.
> > > > 
> > > > Also, following comments may no longer be applicable?
> > > 
> > > Isn't the Andi's change only to fail in case there's
> > > a real error in process_one_file? I think you can still
> > > have empty events dir.
> > 
> > That explains why all the cross builds failed when I added that cset?
> 
> Hmm, let me test. It was supposed to only fail when there
> is a file, but it cannot be parsed.

Wonder if adding this check for event lists helps.

---
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index baa073f..4de5c87 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -840,6 +840,7 @@ int main(int argc, char *argv[])
const char *arch;
const char *output_file;
const char *start_dirname;
+   struct stat stbuf;
 
prog = basename(argv[0]);
if (argc < 4) {
@@ -861,11 +862,17 @@ int main(int argc, char *argv[])
return 2;
}
 
+   sprintf(ldirname, "%s/%s", start_dirname, arch);
+
+   /* If architecture does not have any event lists, bail out */
+   if (stat(ldirname, ) < 0) {
+   pr_info("%s: Arch %s has no PMU event lists\n", prog, arch);
+   goto empty_map;
+   }
+
/* Include pmu-events.h first */
fprintf(eventsfp, "#include \"../../pmu-events/pmu-events.h\"\n");
 
-   sprintf(ldirname, "%s/%s", start_dirname, arch);
-
/*
 * The mapfile allows multiple CPUids to point to the same JSON file,
 * so, not sure if there is a need for symlinks within the pmu-events
@@ -882,6 +889,9 @@ int main(int argc, char *argv[])
if (rc && verbose) {
pr_info("%s: Error walking file tree %s\n", prog, ldirname);
goto empty_map;
+   } else if (rc < 0) {
+   /* Make build fail */
+   return 1;
} else if (rc) {
goto empty_map;
}
@@ -896,7 +906,8 @@ int main(int argc, char *argv[])
 
if (process_mapfile(eventsfp, mapfile)) {
pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
-   goto empty_map;
+   /* Make build fail */
+   return 1;
}
 
return 0;

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Andi Kleen]

On Mon, Jul 24, 2017 at 02:42:09PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 24, 2017 at 04:16:49PM +0200, Jiri Olsa escreveu:
> > On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> > > Andi Kleen [a...@firstfloor.org] wrote:
> > > > From: Andi Kleen 
> > > > 
> > > > Today, when a JSON file fails parsing the build continues,
> > > > but there are no json files built in, which is difficult to debug later.
> > > > Make the build stop on a parse error instead.
> > > 
> > > I see the problem and we were being defensive to not break the build
> > > on architectures that don't yet have the PMU event lists. It will be
> > > good to check build on an architecture other than x86/powerpc.
> > > 
> > > Also, following comments may no longer be applicable?
> > 
> > Isn't the Andi's change only to fail in case there's
> > a real error in process_one_file? I think you can still
> > have empty events dir.
> 
> That explains why all the cross builds failed when I added that cset?

Hmm, let me test. It was supposed to only fail when there
is a file, but it cannot be parsed.

-Andi

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Andi Kleen]

On Mon, Jul 24, 2017 at 02:42:09PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 24, 2017 at 04:16:49PM +0200, Jiri Olsa escreveu:
> > On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> > > Andi Kleen [a...@firstfloor.org] wrote:
> > > > From: Andi Kleen 
> > > > 
> > > > Today, when a JSON file fails parsing the build continues,
> > > > but there are no json files built in, which is difficult to debug later.
> > > > Make the build stop on a parse error instead.
> > > 
> > > I see the problem and we were being defensive to not break the build
> > > on architectures that don't yet have the PMU event lists. It will be
> > > good to check build on an architecture other than x86/powerpc.
> > > 
> > > Also, following comments may no longer be applicable?
> > 
> > Isn't the Andi's change only to fail in case there's
> > a real error in process_one_file? I think you can still
> > have empty events dir.
> 
> That explains why all the cross builds failed when I added that cset?

Hmm, let me test. It was supposed to only fail when there
is a file, but it cannot be parsed.

-Andi

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Arnaldo Carvalho de Melo]

Em Mon, Jul 24, 2017 at 04:16:49PM +0200, Jiri Olsa escreveu:
> On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> > Andi Kleen [a...@firstfloor.org] wrote:
> > > From: Andi Kleen 
> > > 
> > > Today, when a JSON file fails parsing the build continues,
> > > but there are no json files built in, which is difficult to debug later.
> > > Make the build stop on a parse error instead.
> > 
> > I see the problem and we were being defensive to not break the build
> > on architectures that don't yet have the PMU event lists. It will be
> > good to check build on an architecture other than x86/powerpc.
> > 
> > Also, following comments may no longer be applicable?
> 
> Isn't the Andi's change only to fail in case there's
> a real error in process_one_file? I think you can still
> have empty events dir.

That explains why all the cross builds failed when I added that cset?

- Arnaldo

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Arnaldo Carvalho de Melo]

Em Mon, Jul 24, 2017 at 04:16:49PM +0200, Jiri Olsa escreveu:
> On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> > Andi Kleen [a...@firstfloor.org] wrote:
> > > From: Andi Kleen 
> > > 
> > > Today, when a JSON file fails parsing the build continues,
> > > but there are no json files built in, which is difficult to debug later.
> > > Make the build stop on a parse error instead.
> > 
> > I see the problem and we were being defensive to not break the build
> > on architectures that don't yet have the PMU event lists. It will be
> > good to check build on an architecture other than x86/powerpc.
> > 
> > Also, following comments may no longer be applicable?
> 
> Isn't the Andi's change only to fail in case there's
> a real error in process_one_file? I think you can still
> have empty events dir.

That explains why all the cross builds failed when I added that cset?

- Arnaldo

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Jiri Olsa]

On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> Andi Kleen [a...@firstfloor.org] wrote:
> > From: Andi Kleen 
> > 
> > Today, when a JSON file fails parsing the build continues,
> > but there are no json files built in, which is difficult to debug later.
> > Make the build stop on a parse error instead.
> 
> I see the problem and we were being defensive to not break the build
> on architectures that don't yet have the PMU event lists. It will be
> good to check build on an architecture other than x86/powerpc.
> 
> Also, following comments may no longer be applicable?

Isn't the Andi's change only to fail in case there's
a real error in process_one_file? I think you can still
have empty events dir.

jirka

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Jiri Olsa]

On Fri, Jul 21, 2017 at 05:31:59PM -0700, Sukadev Bhattiprolu wrote:
> Andi Kleen [a...@firstfloor.org] wrote:
> > From: Andi Kleen 
> > 
> > Today, when a JSON file fails parsing the build continues,
> > but there are no json files built in, which is difficult to debug later.
> > Make the build stop on a parse error instead.
> 
> I see the problem and we were being defensive to not break the build
> on architectures that don't yet have the PMU event lists. It will be
> good to check build on an architecture other than x86/powerpc.
> 
> Also, following comments may no longer be applicable?

Isn't the Andi's change only to fail in case there's
a real error in process_one_file? I think you can still
have empty events dir.

jirka

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Sukadev Bhattiprolu]

Andi Kleen [a...@firstfloor.org] wrote:
> From: Andi Kleen 
> 
> Today, when a JSON file fails parsing the build continues,
> but there are no json files built in, which is difficult to debug later.
> Make the build stop on a parse error instead.

I see the problem and we were being defensive to not break the build
on architectures that don't yet have the PMU event lists. It will be
good to check build on an architecture other than x86/powerpc.

Also, following comments may no longer be applicable?

diff --git a/tools/perf/pmu-events/README b/tools/perf/pmu-events/README
index 1408ade0d773..c2ee3e4417fe 100644
--- a/tools/perf/pmu-events/README
+++ b/tools/perf/pmu-events/README
@@ -85,10 +85,6 @@ users to specify events by their name:
 
 where 'pm_1plus_ppc_cmpl' is a Power8 PMU event.
 
-In case of errors when processing files in the tools/perf/pmu-events/arch
-directory, 'jevents' tries to create an empty mapping file to allow the perf
-build to succeed even if the PMU event aliases cannot be used.
-
 However some errors in processing may cause the perf build to fail.
 
 Mapfile format
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index baa073f38334..f6fb0eebf488 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -826,10 +826,6 @@ static int process_one_file(const char *fpath, const 
struct stat *sb,
  * PMU event tables (see struct pmu_events_map).
  *
  * Write out the PMU events tables and the mapping table to pmu-event.c.
- *
- * If unable to process the JSON or arch files, create an empty mapping
- * table so we can continue to build/use  perf even if we cannot use the
- * PMU event aliases.
  */
 int main(int argc, char *argv[])
 {





> 
> Cc: suka...@linux.vnet.ibm.com
> Signed-off-by: Andi Kleen 
> ---
>  tools/perf/pmu-events/jevents.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 70cbd5bc4819..58b42508c333 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -890,6 +890,9 @@ int main(int argc, char *argv[])
>   if (rc && verbose) {
>   pr_info("%s: Error walking file tree %s\n", prog, ldirname);
>   goto empty_map;
> + } else if (rc < 0) {
> + /* Make build fail */
> + return 1;
>   } else if (rc) {
>   goto empty_map;
>   }
> @@ -904,7 +907,8 @@ int main(int argc, char *argv[])
> 
>   if (process_mapfile(eventsfp, mapfile)) {
>   pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
> - goto empty_map;
> + /* Make build fail */
> + return 1;
>   }
> 
>   return 0;
> -- 
> 2.9.4

Re: [PATCH] perf, tools: Make build fail on JSON parse error

[Sukadev Bhattiprolu]

Andi Kleen [a...@firstfloor.org] wrote:
> From: Andi Kleen 
> 
> Today, when a JSON file fails parsing the build continues,
> but there are no json files built in, which is difficult to debug later.
> Make the build stop on a parse error instead.

I see the problem and we were being defensive to not break the build
on architectures that don't yet have the PMU event lists. It will be
good to check build on an architecture other than x86/powerpc.

Also, following comments may no longer be applicable?

diff --git a/tools/perf/pmu-events/README b/tools/perf/pmu-events/README
index 1408ade0d773..c2ee3e4417fe 100644
--- a/tools/perf/pmu-events/README
+++ b/tools/perf/pmu-events/README
@@ -85,10 +85,6 @@ users to specify events by their name:
 
 where 'pm_1plus_ppc_cmpl' is a Power8 PMU event.
 
-In case of errors when processing files in the tools/perf/pmu-events/arch
-directory, 'jevents' tries to create an empty mapping file to allow the perf
-build to succeed even if the PMU event aliases cannot be used.
-
 However some errors in processing may cause the perf build to fail.
 
 Mapfile format
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index baa073f38334..f6fb0eebf488 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -826,10 +826,6 @@ static int process_one_file(const char *fpath, const 
struct stat *sb,
  * PMU event tables (see struct pmu_events_map).
  *
  * Write out the PMU events tables and the mapping table to pmu-event.c.
- *
- * If unable to process the JSON or arch files, create an empty mapping
- * table so we can continue to build/use  perf even if we cannot use the
- * PMU event aliases.
  */
 int main(int argc, char *argv[])
 {





> 
> Cc: suka...@linux.vnet.ibm.com
> Signed-off-by: Andi Kleen 
> ---
>  tools/perf/pmu-events/jevents.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 70cbd5bc4819..58b42508c333 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -890,6 +890,9 @@ int main(int argc, char *argv[])
>   if (rc && verbose) {
>   pr_info("%s: Error walking file tree %s\n", prog, ldirname);
>   goto empty_map;
> + } else if (rc < 0) {
> + /* Make build fail */
> + return 1;
>   } else if (rc) {
>   goto empty_map;
>   }
> @@ -904,7 +907,8 @@ int main(int argc, char *argv[])
> 
>   if (process_mapfile(eventsfp, mapfile)) {
>   pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
> - goto empty_map;
> + /* Make build fail */
> + return 1;
>   }
> 
>   return 0;
> -- 
> 2.9.4