| Message ID | 20211007110543.564963-1-james.clark@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | perf tools: Enable strict JSON parsing | expand |
On 10/7/2021 4:05 AM, James Clark wrote: > After a discussion on "[PATCH 1/4] perf vendor events: Syntax corrections in Neoverse N1 json", > John Garry suggested that we can just modify the parser to make it more strict. Hopefully this will > remove the need to apply any future JSON comma fixup commits. > > Applies on top of "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" on > perf/core. Looks good to me. The Intel files are already generated by automated tools using the standard python JSON writer, I guess if it's out of sync someone must have edited it by hand. So it should be fine to fix it. Reviewed-by: Andi Kleen <ak@linux.intel.com> -Andi
On 10/7/21 4:35 PM, James Clark wrote: > After a discussion on "[PATCH 1/4] perf vendor events: Syntax corrections in Neoverse N1 json", > John Garry suggested that we can just modify the parser to make it more strict. Hopefully this will > remove the need to apply any future JSON comma fixup commits. > > Applies on top of "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" on > perf/core. > > Also available at: > git clone --branch james-json-parse-fix git@git.gitlab.arm.com:linux-arm/linux-jc.git Hi James, Do we have any dependency patches on top of this patch series. I am reviewing and testing it, but in both powerpc and x86 system I am getting build issue. Not sure if I am missing something. I am trying your changes on top of upstream perf. pmu-events/arch/test/test_soc/sys/uncore.json: json error Invalid character inside JSON string make[3]: *** [pmu-events/Build:18: pmu-events/pmu-events.c] Error 1 make[3]: *** Deleting file 'pmu-events/pmu-events.c' make[2]: *** [Makefile.perf:667: pmu-events/pmu-events-in.o] Error 2 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [Makefile.perf:238: sub-make] Error 2 make: *** [Makefile:70: all] Error 2 Also, Is it possible to add line number along with file name while showing this error `json error Invalid character inside JSON string`. It might make it easy to fix. Thanks, Kajol Jain > > James Clark (3): > perf vendor-events: Fix all remaining invalid JSON files > perf tools: Make the JSON parser more conformant when in strict mode > perf tools: Enable strict JSON parsing > > .../arch/arm64/ampere/emag/bus.json | 2 +- > .../arch/arm64/ampere/emag/cache.json | 20 ++++----- > .../arch/arm64/ampere/emag/clock.json | 2 +- > .../arch/arm64/ampere/emag/exception.json | 4 +- > .../arch/arm64/ampere/emag/instruction.json | 10 ++--- > .../arch/arm64/ampere/emag/memory.json | 4 +- > .../arch/arm64/hisilicon/hip08/metrics.json | 2 +- > .../pmu-events/arch/nds32/n13/atcpmu.json | 2 +- > .../pmu-events/arch/s390/cf_z10/basic.json | 2 +- > .../pmu-events/arch/s390/cf_z10/crypto.json | 2 +- > .../pmu-events/arch/s390/cf_z10/extended.json | 2 +- > .../pmu-events/arch/s390/cf_z13/basic.json | 2 +- > .../pmu-events/arch/s390/cf_z13/crypto.json | 2 +- > .../pmu-events/arch/s390/cf_z13/extended.json | 2 +- > .../pmu-events/arch/s390/cf_z14/basic.json | 2 +- > .../pmu-events/arch/s390/cf_z14/crypto.json | 2 +- > .../pmu-events/arch/s390/cf_z14/extended.json | 2 +- > .../pmu-events/arch/s390/cf_z15/basic.json | 2 +- > .../pmu-events/arch/s390/cf_z15/crypto.json | 2 +- > .../pmu-events/arch/s390/cf_z15/crypto6.json | 2 +- > .../pmu-events/arch/s390/cf_z15/extended.json | 2 +- > .../pmu-events/arch/s390/cf_z196/basic.json | 2 +- > .../pmu-events/arch/s390/cf_z196/crypto.json | 2 +- > .../arch/s390/cf_z196/extended.json | 2 +- > .../pmu-events/arch/s390/cf_zec12/basic.json | 2 +- > .../pmu-events/arch/s390/cf_zec12/crypto.json | 2 +- > .../arch/s390/cf_zec12/extended.json | 2 +- > .../arch/test/test_soc/cpu/uncore.json | 2 +- > .../arch/x86/icelakex/icx-metrics.json | 2 +- > tools/perf/pmu-events/jsmn.c | 43 ++++++++++++++++++- > 30 files changed, 85 insertions(+), 46 deletions(-) >
On 08/10/2021 08:43, kajoljain wrote: > > > On 10/7/21 4:35 PM, James Clark wrote: >> After a discussion on "[PATCH 1/4] perf vendor events: Syntax corrections in Neoverse N1 json", >> John Garry suggested that we can just modify the parser to make it more strict. Hopefully this will >> remove the need to apply any future JSON comma fixup commits. >> >> Applies on top of "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" on >> perf/core. >> >> Also available at: >> git clone --branch james-json-parse-fix git@git.gitlab.arm.com:linux-arm/linux-jc.git > > Hi James, > Do we have any dependency patches on top of this patch series. I am > reviewing and testing it, but in both powerpc and x86 system I am > getting build issue. Not sure if I am missing something> > I am trying your changes on top of upstream perf. > > pmu-events/arch/test/test_soc/sys/uncore.json: json error Invalid > character inside JSON string Hi Kajol, A trailing comma was fixed in this file 3 weeks ago at b8b350a. Can you confirm if you have updated to get this commit on perf core? Alternately you could pull from my branch above which is up to date enough to include it. The file is in pmu-events/arch/test/ so I would expect it to fail on all platforms. > make[3]: *** [pmu-events/Build:18: pmu-events/pmu-events.c] Error 1 > make[3]: *** Deleting file 'pmu-events/pmu-events.c' > make[2]: *** [Makefile.perf:667: pmu-events/pmu-events-in.o] Error 2 > make[2]: *** Waiting for unfinished jobs.... > make[1]: *** [Makefile.perf:238: sub-make] Error 2 > make: *** [Makefile:70: all] Error 2 > > Also, Is it possible to add line number along with file name while > showing this error `json error Invalid character inside JSON string`. > It might make it easy to fix. I can add a character number with the following fix if you think that would be good enough? A line number might be a bigger change and involve keeping track of newline characters. diff --git a/tools/perf/pmu-events/json.c b/tools/perf/pmu-events/json.c index 0544398d6e2d..41a14e1543bf 100644 --- a/tools/perf/pmu-events/json.c +++ b/tools/perf/pmu-events/json.c @@ -99,7 +99,7 @@ jsmntok_t *parse_json(const char *fn, char **map, size_t *size, int *len) res = jsmn_parse(&parser, *map, *size, tokens, sz / sizeof(jsmntok_t)); if (res != JSMN_SUCCESS) { - pr_err("%s: json error %s\n", fn, jsmn_strerror(res)); + pr_err("%s: json error at character %u '%s'\n", fn, parser.pos, jsmn_strerror(res)); goto error_free; } if (len) It prints this for the same error you have above: pmu-events/arch/test/test_soc/sys/uncore.json: json error at character 213 'Invalid character inside JSON string' Although funnily enough after re-introducing that extra comma it doesn't fail the build for me, it just prints the error message. But I may have noticed some dependency tracking issues around the json files. James
On 10/8/21 3:32 PM, James Clark wrote: > > > On 08/10/2021 08:43, kajoljain wrote: >> >> >> On 10/7/21 4:35 PM, James Clark wrote: >>> After a discussion on "[PATCH 1/4] perf vendor events: Syntax corrections in Neoverse N1 json", >>> John Garry suggested that we can just modify the parser to make it more strict. Hopefully this will >>> remove the need to apply any future JSON comma fixup commits. >>> >>> Applies on top of "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" on >>> perf/core. >>> >>> Also available at: >>> git clone --branch james-json-parse-fix git@git.gitlab.arm.com:linux-arm/linux-jc.git >> >> Hi James, >> Do we have any dependency patches on top of this patch series. I am >> reviewing and testing it, but in both powerpc and x86 system I am >> getting build issue. Not sure if I am missing something> >> I am trying your changes on top of upstream perf. >> >> pmu-events/arch/test/test_soc/sys/uncore.json: json error Invalid >> character inside JSON string > > Hi Kajol, > > A trailing comma was fixed in this file 3 weeks ago at b8b350a. Can you > confirm if you have updated to get this commit on perf core? > > Alternately you could pull from my branch above which is up to date enough > to include it. Hi James, Thanks for pointing it. Not getting build issue now. > > The file is in pmu-events/arch/test/ so I would expect it to fail on all platforms. > >> make[3]: *** [pmu-events/Build:18: pmu-events/pmu-events.c] Error 1 >> make[3]: *** Deleting file 'pmu-events/pmu-events.c' >> make[2]: *** [Makefile.perf:667: pmu-events/pmu-events-in.o] Error 2 >> make[2]: *** Waiting for unfinished jobs.... >> make[1]: *** [Makefile.perf:238: sub-make] Error 2 >> make: *** [Makefile:70: all] Error 2 >> >> Also, Is it possible to add line number along with file name while >> showing this error `json error Invalid character inside JSON string`. >> It might make it easy to fix. > > I can add a character number with the following fix if you think that would > be good enough? A line number might be a bigger change and involve keeping > track of newline characters. Sure. I think then we can skip this change. Not sure if character number will be helpful. Patch-set looks good to me. Reviewed-by Kajol Jain<kjain@linux.ibm.com> Thanks, Kajol Jain > > diff --git a/tools/perf/pmu-events/json.c b/tools/perf/pmu-events/json.c > index 0544398d6e2d..41a14e1543bf 100644 > --- a/tools/perf/pmu-events/json.c > +++ b/tools/perf/pmu-events/json.c > @@ -99,7 +99,7 @@ jsmntok_t *parse_json(const char *fn, char **map, size_t *size, int *len) > res = jsmn_parse(&parser, *map, *size, tokens, > sz / sizeof(jsmntok_t)); > if (res != JSMN_SUCCESS) { > - pr_err("%s: json error %s\n", fn, jsmn_strerror(res)); > + pr_err("%s: json error at character %u '%s'\n", fn, parser.pos, jsmn_strerror(res)); > goto error_free; > } > if (len) > > > It prints this for the same error you have above> > pmu-events/arch/test/test_soc/sys/uncore.json: json error at character 213 'Invalid character inside JSON string' > > Although funnily enough after re-introducing that extra comma it doesn't fail the build for me, > it just prints the error message. But I may have noticed some dependency tracking issues around > the json files. > > James >
Em Fri, Oct 08, 2021 at 04:56:55PM +0530, kajoljain escreveu: > > > On 10/8/21 3:32 PM, James Clark wrote: > > > > > > On 08/10/2021 08:43, kajoljain wrote: > >> > >> > >> On 10/7/21 4:35 PM, James Clark wrote: > >>> After a discussion on "[PATCH 1/4] perf vendor events: Syntax corrections in Neoverse N1 json", > >>> John Garry suggested that we can just modify the parser to make it more strict. Hopefully this will > >>> remove the need to apply any future JSON comma fixup commits. > >>> > >>> Applies on top of "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" on > >>> perf/core. > >>> > >>> Also available at: > >>> git clone --branch james-json-parse-fix git@git.gitlab.arm.com:linux-arm/linux-jc.git > >> > >> Hi James, > >> Do we have any dependency patches on top of this patch series. I am > >> reviewing and testing it, but in both powerpc and x86 system I am > >> getting build issue. Not sure if I am missing something> > >> I am trying your changes on top of upstream perf. > >> > >> pmu-events/arch/test/test_soc/sys/uncore.json: json error Invalid > >> character inside JSON string > > > > Hi Kajol, > > > > A trailing comma was fixed in this file 3 weeks ago at b8b350a. Can you > > confirm if you have updated to get this commit on perf core? > > > > Alternately you could pull from my branch above which is up to date enough > > to include it. > > Hi James, > Thanks for pointing it. Not getting build issue now. > > > > The file is in pmu-events/arch/test/ so I would expect it to fail on all platforms. > > > >> make[3]: *** [pmu-events/Build:18: pmu-events/pmu-events.c] Error 1 > >> make[3]: *** Deleting file 'pmu-events/pmu-events.c' > >> make[2]: *** [Makefile.perf:667: pmu-events/pmu-events-in.o] Error 2 > >> make[2]: *** Waiting for unfinished jobs.... > >> make[1]: *** [Makefile.perf:238: sub-make] Error 2 > >> make: *** [Makefile:70: all] Error 2 > >> > >> Also, Is it possible to add line number along with file name while > >> showing this error `json error Invalid character inside JSON string`. > >> It might make it easy to fix. > > > > I can add a character number with the following fix if you think that would > > be good enough? A line number might be a bigger change and involve keeping > > track of newline characters. > > Sure. I think then we can skip this change. Not sure if character > number will be helpful. > > Patch-set looks good to me. > > Reviewed-by Kajol Jain<kjain@linux.ibm.com> Applied ok as-is to my perf/core branch, applied and added your Reviewed-by, thanks. - Arnaldo > Thanks, > Kajol Jain > > > > > diff --git a/tools/perf/pmu-events/json.c b/tools/perf/pmu-events/json.c > > index 0544398d6e2d..41a14e1543bf 100644 > > --- a/tools/perf/pmu-events/json.c > > +++ b/tools/perf/pmu-events/json.c > > @@ -99,7 +99,7 @@ jsmntok_t *parse_json(const char *fn, char **map, size_t *size, int *len) > > res = jsmn_parse(&parser, *map, *size, tokens, > > sz / sizeof(jsmntok_t)); > > if (res != JSMN_SUCCESS) { > > - pr_err("%s: json error %s\n", fn, jsmn_strerror(res)); > > + pr_err("%s: json error at character %u '%s'\n", fn, parser.pos, jsmn_strerror(res)); > > goto error_free; > > } > > if (len) > > > > > > It prints this for the same error you have above> > > pmu-events/arch/test/test_soc/sys/uncore.json: json error at character 213 'Invalid character inside JSON string' > > > > Although funnily enough after re-introducing that extra comma it doesn't fail the build for me, > > it just prints the error message. But I may have noticed some dependency tracking issues around > > the json files. > > > > James > >
On 08/10/2021 20:00, Arnaldo Carvalho de Melo wrote: > Em Fri, Oct 08, 2021 at 04:56:55PM +0530, kajoljain escreveu: >> >> >> On 10/8/21 3:32 PM, James Clark wrote: >>> >>> >>> On 08/10/2021 08:43, kajoljain wrote: >>>> >>>> >>>> On 10/7/21 4:35 PM, James Clark wrote: >>>>> After a discussion on "[PATCH 1/4] perf vendor events: Syntax corrections in Neoverse N1 json", >>>>> John Garry suggested that we can just modify the parser to make it more strict. Hopefully this will >>>>> remove the need to apply any future JSON comma fixup commits. >>>>> >>>>> Applies on top of "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" on >>>>> perf/core. >>>>> >>>>> Also available at: >>>>> git clone --branch james-json-parse-fix git@git.gitlab.arm.com:linux-arm/linux-jc.git >>>> >>>> Hi James, >>>> Do we have any dependency patches on top of this patch series. I am >>>> reviewing and testing it, but in both powerpc and x86 system I am >>>> getting build issue. Not sure if I am missing something> >>>> I am trying your changes on top of upstream perf. >>>> >>>> pmu-events/arch/test/test_soc/sys/uncore.json: json error Invalid >>>> character inside JSON string >>> >>> Hi Kajol, >>> >>> A trailing comma was fixed in this file 3 weeks ago at b8b350a. Can you >>> confirm if you have updated to get this commit on perf core? >>> >>> Alternately you could pull from my branch above which is up to date enough >>> to include it. >> >> Hi James, >> Thanks for pointing it. Not getting build issue now. >>> >>> The file is in pmu-events/arch/test/ so I would expect it to fail on all platforms. >>> >>>> make[3]: *** [pmu-events/Build:18: pmu-events/pmu-events.c] Error 1 >>>> make[3]: *** Deleting file 'pmu-events/pmu-events.c' >>>> make[2]: *** [Makefile.perf:667: pmu-events/pmu-events-in.o] Error 2 >>>> make[2]: *** Waiting for unfinished jobs.... >>>> make[1]: *** [Makefile.perf:238: sub-make] Error 2 >>>> make: *** [Makefile:70: all] Error 2 >>>> >>>> Also, Is it possible to add line number along with file name while >>>> showing this error `json error Invalid character inside JSON string`. >>>> It might make it easy to fix. >>> >>> I can add a character number with the following fix if you think that would >>> be good enough? A line number might be a bigger change and involve keeping >>> track of newline characters. >> >> Sure. I think then we can skip this change. Not sure if character >> number will be helpful. >> >> Patch-set looks good to me. >> >> Reviewed-by Kajol Jain<kjain@linux.ibm.com> > > Applied ok as-is to my perf/core branch, applied and added your > Reviewed-by, thanks. > Thanks Arnaldo. This does mean that the arm64 build will fail until "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" is applied. I think there is also an arm64 build issue with "[PATCH 02/21] perf pmu: Add const to pmu_events_map." which Andrew Kilroy has replied to. James
On October 12, 2021 10:30:51 AM GMT-03:00, James Clark <james.clark@arm.com> wrote: > > >On 08/10/2021 20:00, Arnaldo Carvalho de Melo wrote: >> Em Fri, Oct 08, 2021 at 04:56:55PM +0530, kajoljain escreveu: >>> >>> >>> On 10/8/21 3:32 PM, James Clark wrote: >>>> >>>> >>>> On 08/10/2021 08:43, kajoljain wrote: >>>>> >>>>> >>>>> On 10/7/21 4:35 PM, James Clark wrote: >>>>>> After a discussion on "[PATCH 1/4] perf vendor events: Syntax corrections in Neoverse N1 json", >>>>>> John Garry suggested that we can just modify the parser to make it more strict. Hopefully this will >>>>>> remove the need to apply any future JSON comma fixup commits. >>>>>> >>>>>> Applies on top of "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" on >>>>>> perf/core. >>>>>> >>>>>> Also available at: >>>>>> git clone --branch james-json-parse-fix git@git.gitlab.arm.com:linux-arm/linux-jc.git >>>>> >>>>> Hi James, >>>>> Do we have any dependency patches on top of this patch series. I am >>>>> reviewing and testing it, but in both powerpc and x86 system I am >>>>> getting build issue. Not sure if I am missing something> >>>>> I am trying your changes on top of upstream perf. >>>>> >>>>> pmu-events/arch/test/test_soc/sys/uncore.json: json error Invalid >>>>> character inside JSON string >>>> >>>> Hi Kajol, >>>> >>>> A trailing comma was fixed in this file 3 weeks ago at b8b350a. Can you >>>> confirm if you have updated to get this commit on perf core? >>>> >>>> Alternately you could pull from my branch above which is up to date enough >>>> to include it. >>> >>> Hi James, >>> Thanks for pointing it. Not getting build issue now. >>>> >>>> The file is in pmu-events/arch/test/ so I would expect it to fail on all platforms. >>>> >>>>> make[3]: *** [pmu-events/Build:18: pmu-events/pmu-events.c] Error 1 >>>>> make[3]: *** Deleting file 'pmu-events/pmu-events.c' >>>>> make[2]: *** [Makefile.perf:667: pmu-events/pmu-events-in.o] Error 2 >>>>> make[2]: *** Waiting for unfinished jobs.... >>>>> make[1]: *** [Makefile.perf:238: sub-make] Error 2 >>>>> make: *** [Makefile:70: all] Error 2 >>>>> >>>>> Also, Is it possible to add line number along with file name while >>>>> showing this error `json error Invalid character inside JSON string`. >>>>> It might make it easy to fix. >>>> >>>> I can add a character number with the following fix if you think that would >>>> be good enough? A line number might be a bigger change and involve keeping >>>> track of newline characters. >>> >>> Sure. I think then we can skip this change. Not sure if character >>> number will be helpful. >>> >>> Patch-set looks good to me. >>> >>> Reviewed-by Kajol Jain<kjain@linux.ibm.com> >> >> Applied ok as-is to my perf/core branch, applied and added your >> Reviewed-by, thanks. >> > >Thanks Arnaldo. This does mean that the arm64 build will fail until >"[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" is >applied. I think there is also an arm64 build issue with "[PATCH 02/21] perf >pmu: Add const to pmu_events_map." which Andrew Kilroy has replied to. > Ok, this will hopefully change tomorrow after today's Brazilian holiday when I get to process that series, Thanks, - Arnaldo
Em Tue, Oct 12, 2021 at 02:30:51PM +0100, James Clark escreveu: > On 08/10/2021 20:00, Arnaldo Carvalho de Melo wrote: > > Em Fri, Oct 08, 2021 at 04:56:55PM +0530, kajoljain escreveu: > >> On 10/8/21 3:32 PM, James Clark wrote: > >>> On 08/10/2021 08:43, kajoljain wrote: > >> Sure. I think then we can skip this change. Not sure if character > >> number will be helpful. > >> Patch-set looks good to me. > >> Reviewed-by Kajol Jain<kjain@linux.ibm.com> > > Applied ok as-is to my perf/core branch, applied and added your > > Reviewed-by, thanks. > Thanks Arnaldo. This does mean that the arm64 build will fail until > "[PATCH v2 1/3] perf vendor events: Syntax corrections in Neoverse N1 json" is > applied. I think there is also an arm64 build issue with "[PATCH 02/21] perf > pmu: Add const to pmu_events_map." which Andrew Kilroy has replied to. Its all in: 19f8408a634c9515 (HEAD -> perf/core) perf intel-pt: Add support for PERF_RECORD_AUX_OUTPUT_HW_ID 69125e749c006b4f perf tools: Add support for PERF_RECORD_AUX_OUTPUT_HW_ID 1b1d9560a61f1e4e perf vendor events arm64: Categorise the Neoverse V1 counters abe8733bc3575869 perf vendor events arm64: Add new armv8 pmu events d211e9e76a466cce perf vendor events: Syntax corrections in Neoverse N1 json c067335fcbfc67c3 (quaco/perf/core, acme/tmp.perf/core) perf metric: Allow modifiers on metrics. acf2cb9cf242e9ab perf parse-events: Identify broken modifiers fb193eca0ae8ddae perf metric: Switch fprintf() to pr_err() fb8c3a06943cc3c7 perf metrics: Modify setup and deduplication 4965bb2e71371d5f perf expr: Add subset utility c1d7cd1b36fce16b perf metric: Encode and use metric-id as qualifier 3743f880b3856971 perf parse-events: Allow config on kernel PMU events 844f07a5ddcd46c5 perf parse-events: Add new "metric-id" term e68f07424b8b3f00 perf parse-events: Add const to evsel name ace154d9f5dc3331 perf metric: Simplify metric_refs calculation eeffd53b41dc7077 perf metric: Document the internal 'struct metric' 9aa64400defa07fb perf metric: Comment data structures 353ce4ed869635c7 perf metric: Modify resolution and recursion check 0bffecb0ac304bb2 perf metric: Only add a referenced metric once 937323c22db4cb1e perf metric: Add metric new and free 176b9da84871449d perf metric: Add documentation and rename a variable. cc6803c12cef80f1 perf metric: Move runtime value to the expr context 9610bca8f117d963 perf pmu: Make pmu_event tables const 95ed79343835656d perf pmu: Make pmu_sys_event_tables const 05335f28549c7cc5 perf pmu: Add const to pmu_events_map. cac98c8aca3c7dd2 tools lib: Adopt list_sort from the kernel sources f792cf8a094eac29 perf kmem: Improve man page for record options eda1a84cb4e93759 perf tools: Enable strict JSON parsing 21813684e46df1c9 perf tools: Make the JSON parser more conformant when in strict mode 08f3e0873ac20344 perf vendor-events: Fix all remaining invalid JSON files - Arnaldo