| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <055803d6-b483-f839-22fb-d84fad404bcf@huawei.com>
Date: Wed, 25 Aug 2021 17:58:31 +0100
From: John Garry <john.garry@...wei.com>
To: Ian Rogers <irogers@...gle.com>
CC: Jiri Olsa <jolsa@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
"Peter Zijlstra" <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Namhyung Kim <namhyung@...nel.org>
Subject: Re: [Question] perf tools: lex parsing issue
On 25/08/2021 17:47, Ian Rogers wrote:
> On Wed, Aug 25, 2021 at 7:32 AM John Garry<john.garry@...wei.com> wrote:
>> On 25/08/2021 01:13, Ian Rogers wrote:
>>
>> Thanks for getting back to me
>>
>>
>>>> + if (term->type_term == PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD) {
>>>> + term->config = strdup("period");
>>>> + }
>>>> +
>>>> if (!head) {
>>>> parse_events_term__delete(term);
>>>> YYABORT;
>>>> --
>>>>
>>>> ----8-----
>>> Agreed this is hacky,
>> This might be a*bit* better:
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index e5eae23cfceb..e597beaaa179 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1058,6 +1058,13 @@ static const char
>> *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
>> [PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE] = "aux-sample-size",
>> };
>>
>> +const char * get_config_term(int term_type)
>> +{
>> + if (term_type >= __PARSE_EVENTS__TERM_TYPE_NR)
>> + return "";
>> + return config_term_names[term_type];
>> +}
>> +
>> static bool config_term_shrinked;
>>
>> static bool
>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>> index bf6e41aa9b6a..0f658732535f 100644
>> --- a/tools/perf/util/parse-events.h
>> +++ b/tools/perf/util/parse-events.h
>> @@ -90,6 +90,8 @@ enum {
>> __PARSE_EVENTS__TERM_TYPE_NR,
>> };
>>
>> +const char *get_config_term(int term_type);
>> +
>> struct parse_events_array {
>> size_t nr_ranges;
>> struct {
>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>> index 9321bd0e2f76..8d6d3fae226d 100644
>> --- a/tools/perf/util/parse-events.y
>> +++ b/tools/perf/util/parse-events.y
>> @@ -832,8 +832,9 @@ PE_TERM '=' PE_NAME
>> PE_TERM '=' PE_VALUE
>> {
>> struct parse_events_term *term;
>> + char *config = strdup(get_config_term($1));
>>
>> - ABORT_ON(parse_events_term__num(&term, (int)$1, NULL, $3, false, &@1,
>> &@3));
>> + ABORT_ON(parse_events_term__num(&term, (int)$1, config, $3, false,
>> &@1, &@3));
>> $$ = term;
>> }
>>
>>
>>> I think it'd be better to fix this up in the
>>> output. For example:
>>>
>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>> index 6cdbee8a12e7..c77c42275efa 100644
>>> --- a/tools/perf/util/pmu.c
>>> +++ b/tools/perf/util/pmu.c
>>> @@ -365,15 +365,21 @@ static int __perf_pmu__new_alias(struct
>>> list_head *list, char *dir, char *name,
>>> memset(newval, 0, sizeof(newval));
>>> ret = 0;
>>> list_for_each_entry(term, &alias->terms, list) {
>>> + const char * config = term->config;
>>> +
>>> if (ret)
>>> ret += scnprintf(newval + ret, sizeof(newval) - ret,
>>> ",");
>>> + if (!config) {
>>> + /* Note: config_term_names in parse_events.c
>>> isn't accessible */
>>> + config = config_term_names[term->type_term];
>> We could just expose some parse-events.c API, like above.
>>
>>> + }
>>> if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
>>> ret += scnprintf(newval + ret, sizeof(newval) - ret,
>>> - "%s=%#x", term->config, term->val.num);
>>> + "%s=%#x", config, term->val.num);
>>> else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
>>> ret += scnprintf(newval + ret, sizeof(newval) - ret,
>>> - "%s=%s", term->config, term->val.str);
>>> + "%s=%s", config, term->val.str);
>>> }
>>>
>>> alias->name = strdup(name);
>> But how about this alternative simple one:
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index e5eae23cfceb..9e5df934d22d 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -3084,7 +3084,7 @@ int parse_events_term__num(struct
>> parse_events_term **term,
>> struct parse_events_term temp = {
>> .type_val = PARSE_EVENTS__TERM_TYPE_NUM,
>> .type_term = type_term,
>> - .config = config,
>> + .config = config ? : strdup(config_term_names[type_term]),
>> .no_value = no_value,
>> .err_term = loc_term ? loc_term->first_column : 0,
>> .err_val = loc_val ? loc_val->first_column : 0,
>> --
> If you have this change isn't the change to parse_events_term__num()
> unnecessary?
eh, this is parse_events_term__num().
Anyway, yes, this 1-line change in parse_events_term__num() only does
look to fix it. More testing required, though.
This 1 liner fix looks good to me.
Cheers,
John
Powered by blists - more mailing lists