[PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[Adrian Hunter]

Hi

Here is a patch (V3) to the event parser for breakpoint events.
I am not that familiar with flex / bison, but it seemed to
need trailing context to stop the mem event colon and slash
delimiters from getting mixed up with delimiters for config
terms or event modifiers.  Please look closely at that.


Changes in V3:

      Add Ian's Reviewed-by
      Re-base

Changes in V2:

      Add comments to tools/perf/util/parse-events.l
      Add a test for 2 mem events back to back with config terms


Adrian Hunter (1):
      perf tools: Allow config terms with breakpoints

 tools/perf/tests/parse-events.c | 157 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/parse-events.c  |  23 +++++-
 tools/perf/util/parse-events.h  |   6 +-
 tools/perf/util/parse-events.l  |  23 +++++-
 tools/perf/util/parse-events.y  |  42 ++++++-----
 5 files changed, 224 insertions(+), 27 deletions(-)


Regards
Adrian

[PATCH V3 1/1] perf tools: Allow config terms with breakpoints

[Adrian Hunter]

Add config terms to the parsing of breakpoint events. Extend "Test event
parsing" to also cover using a confg term.

This makes breakpoint events consistent with other events which already
support config terms.

Example:

  $ cat dr_test.c
  #include <unistd.h>
  #include <stdio.h>

  void func0(void)
  {
  }

  int main()
  {
          printf("func0 %p\n", &func0);
          while (1) {
                  func0();
                  usleep(100000);
          }
          return 0;
  }
  $ gcc -g -O0 -o dr_test dr_test.c
  $ ./dr_test &
  [2] 19646
  func0 0x55feb98dd169
  $ perf record -e mem:0x55feb98dd169:x/name=breakpoint/ -p 19646 -- sleep 0.5
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.017 MB perf.data (5 samples) ]
  $ perf script
      dr_test 19646  5632.956628:          1 breakpoint:      55feb98dd169 func0+0x0 (/home/ahunter/git/work/dr_test)
      dr_test 19646  5633.056866:          1 breakpoint:      55feb98dd169 func0+0x0 (/home/ahunter/git/work/dr_test)
      dr_test 19646  5633.157084:          1 breakpoint:      55feb98dd169 func0+0x0 (/home/ahunter/git/work/dr_test)
      dr_test 19646  5633.257309:          1 breakpoint:      55feb98dd169 func0+0x0 (/home/ahunter/git/work/dr_test)
      dr_test 19646  5633.357532:          1 breakpoint:      55feb98dd169 func0+0x0 (/home/ahunter/git/work/dr_test)
  $ sudo perf test "Test event parsing"
    6: Parse event definition strings                                  :
    6.1: Test event parsing                                            : Ok
  $ sudo perf test -v "Test event parsing" |& grep mem
  running test 8 'mem:0'
  running test 9 'mem:0:x'
  running test 10 'mem:0:r'
  running test 11 'mem:0:w'
  running test 19 'mem:0:u'
  running test 20 'mem:0:x:k'
  running test 21 'mem:0:r:hp'
  running test 22 'mem:0:w:up'
  running test 26 'mem:0:rw'
  running test 27 'mem:0:rw:kp'
  running test 42 'mem:0/1'
  running test 43 'mem:0/2:w'
  running test 44 'mem:0/4:rw:u'
  running test 58 'mem:0/name=breakpoint/'
  running test 59 'mem:0:x/name=breakpoint/'
  running test 60 'mem:0:r/name=breakpoint/'
  running test 61 'mem:0:w/name=breakpoint/'
  running test 62 'mem:0/name=breakpoint/u'
  running test 63 'mem:0:x/name=breakpoint/k'
  running test 64 'mem:0:r/name=breakpoint/hp'
  running test 65 'mem:0:w/name=breakpoint/up'
  running test 66 'mem:0:rw/name=breakpoint/'
  running test 67 'mem:0:rw/name=breakpoint/kp'
  running test 68 'mem:0/1/name=breakpoint/'
  running test 69 'mem:0/2:w/name=breakpoint/'
  running test 70 'mem:0/4:rw/name=breakpoint/u'
  running test 71 'mem:0/1/name=breakpoint1/,mem:0/4:rw/name=breakpoint2/'

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V3:

      Add Ian's Reviewed-by
      Re-base

Changes in V2:

      Add comments to tools/perf/util/parse-events.l
      Add a test for 2 mem events back to back with config terms


 tools/perf/tests/parse-events.c | 157 ++++++++++++++++++++++++++++++++
 tools/perf/util/parse-events.c  |  23 ++++-
 tools/perf/util/parse-events.h  |   6 +-
 tools/perf/util/parse-events.l  |  23 ++++-
 tools/perf/util/parse-events.y  |  42 +++++----
 5 files changed, 224 insertions(+), 27 deletions(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 72a10bed84fd..399f1bc12b0c 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -482,6 +482,93 @@ static int test__checkevent_breakpoint_rw_modifier(struct evlist *evlist)
 	return test__checkevent_breakpoint_rw(evlist);
 }
 
+static int test__checkevent_breakpoint_modifier_name(struct evlist *evlist)
+{
+	struct evsel *evsel = evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
+	TEST_ASSERT_VAL("wrong name",
+			!strcmp(evsel__name(evsel), "breakpoint"));
+
+	return test__checkevent_breakpoint(evlist);
+}
+
+static int test__checkevent_breakpoint_x_modifier_name(struct evlist *evlist)
+{
+	struct evsel *evsel = evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong exclude_user", evsel->core.attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
+	TEST_ASSERT_VAL("wrong name",
+			!strcmp(evsel__name(evsel), "breakpoint"));
+
+	return test__checkevent_breakpoint_x(evlist);
+}
+
+static int test__checkevent_breakpoint_r_modifier_name(struct evlist *evlist)
+{
+	struct evsel *evsel = evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong exclude_user", evsel->core.attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", !evsel->core.attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
+	TEST_ASSERT_VAL("wrong name",
+			!strcmp(evsel__name(evsel), "breakpoint"));
+
+	return test__checkevent_breakpoint_r(evlist);
+}
+
+static int test__checkevent_breakpoint_w_modifier_name(struct evlist *evlist)
+{
+	struct evsel *evsel = evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
+	TEST_ASSERT_VAL("wrong name",
+			!strcmp(evsel__name(evsel), "breakpoint"));
+
+	return test__checkevent_breakpoint_w(evlist);
+}
+
+static int test__checkevent_breakpoint_rw_modifier_name(struct evlist *evlist)
+{
+	struct evsel *evsel = evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong exclude_user", evsel->core.attr.exclude_user);
+	TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
+	TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
+	TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
+	TEST_ASSERT_VAL("wrong name",
+			!strcmp(evsel__name(evsel), "breakpoint"));
+
+	return test__checkevent_breakpoint_rw(evlist);
+}
+
+static int test__checkevent_breakpoint_2_events(struct evlist *evlist)
+{
+	struct evsel *evsel = evlist__first(evlist);
+
+	TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
+
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->core.attr.type);
+	TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "breakpoint1"));
+
+	evsel = evsel__next(evsel);
+
+	TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->core.attr.type);
+	TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "breakpoint2"));
+
+	return TEST_OK;
+}
+
 static int test__checkevent_pmu(struct evlist *evlist)
 {
 
@@ -1901,6 +1988,76 @@ static const struct evlist_test test__events[] = {
 		.check = test__term_equal_legacy,
 		/* 9 */
 	},
+	{
+		.name  = "mem:0/name=breakpoint/",
+		.check = test__checkevent_breakpoint,
+		/* 0 */
+	},
+	{
+		.name  = "mem:0:x/name=breakpoint/",
+		.check = test__checkevent_breakpoint_x,
+		/* 1 */
+	},
+	{
+		.name  = "mem:0:r/name=breakpoint/",
+		.check = test__checkevent_breakpoint_r,
+		/* 2 */
+	},
+	{
+		.name  = "mem:0:w/name=breakpoint/",
+		.check = test__checkevent_breakpoint_w,
+		/* 3 */
+	},
+	{
+		.name  = "mem:0/name=breakpoint/u",
+		.check = test__checkevent_breakpoint_modifier_name,
+		/* 4 */
+	},
+	{
+		.name  = "mem:0:x/name=breakpoint/k",
+		.check = test__checkevent_breakpoint_x_modifier_name,
+		/* 5 */
+	},
+	{
+		.name  = "mem:0:r/name=breakpoint/hp",
+		.check = test__checkevent_breakpoint_r_modifier_name,
+		/* 6 */
+	},
+	{
+		.name  = "mem:0:w/name=breakpoint/up",
+		.check = test__checkevent_breakpoint_w_modifier_name,
+		/* 7 */
+	},
+	{
+		.name  = "mem:0:rw/name=breakpoint/",
+		.check = test__checkevent_breakpoint_rw,
+		/* 8 */
+	},
+	{
+		.name  = "mem:0:rw/name=breakpoint/kp",
+		.check = test__checkevent_breakpoint_rw_modifier_name,
+		/* 9 */
+	},
+	{
+		.name  = "mem:0/1/name=breakpoint/",
+		.check = test__checkevent_breakpoint_len,
+		/* 0 */
+	},
+	{
+		.name  = "mem:0/2:w/name=breakpoint/",
+		.check = test__checkevent_breakpoint_len_w,
+		/* 1 */
+	},
+	{
+		.name  = "mem:0/4:rw/name=breakpoint/u",
+		.check = test__checkevent_breakpoint_len_rw_modifier,
+		/* 2 */
+	},
+	{
+		.name  = "mem:0/1/name=breakpoint1/,mem:0/4:rw/name=breakpoint2/",
+		.check = test__checkevent_breakpoint_2_events,
+		/* 3 */
+	},
 };
 
 static const struct evlist_test test__events_pmu[] = {
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index b93264f8a37c..cfa25441a94d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -939,10 +939,14 @@ do {					\
 	return 0;
 }
 
-int parse_events_add_breakpoint(struct list_head *list, int *idx,
-				u64 addr, char *type, u64 len)
+int parse_events_add_breakpoint(struct parse_events_state *parse_state,
+				struct list_head *list,
+				u64 addr, char *type, u64 len,
+				struct list_head *head_config __maybe_unused)
 {
 	struct perf_event_attr attr;
+	LIST_HEAD(config_terms);
+	const char *name;
 
 	memset(&attr, 0, sizeof(attr));
 	attr.bp_addr = addr;
@@ -963,8 +967,19 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
 	attr.type = PERF_TYPE_BREAKPOINT;
 	attr.sample_period = 1;
 
-	return add_event(list, idx, &attr, /*name=*/NULL, /*mertic_id=*/NULL,
-			 /*config_terms=*/NULL);
+	if (head_config) {
+		if (config_attr(&attr, head_config, parse_state->error,
+				config_term_common))
+			return -EINVAL;
+
+		if (get_config_terms(head_config, &config_terms))
+			return -ENOMEM;
+	}
+
+	name = get_config_name(head_config);
+
+	return add_event(list, &parse_state->idx, &attr, name, /*mertic_id=*/NULL,
+			 &config_terms);
 }
 
 static int check_type_val(struct parse_events_term *term,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 2021fe145410..b0eb95f93e9c 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -185,8 +185,10 @@ int parse_events_add_cache(struct list_head *list, int *idx, const char *name,
 			   struct parse_events_state *parse_state,
 			   struct list_head *head_config);
 int parse_events__decode_legacy_cache(const char *name, int pmu_type, __u64 *config);
-int parse_events_add_breakpoint(struct list_head *list, int *idx,
-				u64 addr, char *type, u64 len);
+int parse_events_add_breakpoint(struct parse_events_state *parse_state,
+				struct list_head *list,
+				u64 addr, char *type, u64 len,
+				struct list_head *head_config);
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, char *name,
 			 struct list_head *head_config,
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 6deb70c25984..7629af3d5c7c 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -190,11 +190,16 @@ name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!\-]*
 name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
 name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
 drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
-/* If you add a modifier you need to update check_modifier() */
+/*
+ * If you add a modifier you need to update check_modifier().
+ * Also, the letters in modifier_event must not be in modifier_bp.
+ */
 modifier_event	[ukhpPGHSDIWeb]+
 modifier_bp	[rwx]{1,3}
 lc_type 	(L1-dcache|l1-d|l1d|L1-data|L1-icache|l1-i|l1i|L1-instruction|LLC|L2|dTLB|d-tlb|Data-TLB|iTLB|i-tlb|Instruction-TLB|branch|branches|bpu|btb|bpc|node)
 lc_op_result	(load|loads|read|store|stores|write|prefetch|prefetches|speculative-read|speculative-load|refs|Reference|ops|access|misses|miss)
+digit		[0-9]
+non_digit	[^0-9]
 
 %%
 
@@ -304,8 +309,20 @@ r0x{num_raw_hex}	{ return str(yyscanner, PE_RAW); }
 
 <mem>{
 {modifier_bp}		{ return str(yyscanner, PE_MODIFIER_BP); }
-:			{ return ':'; }
-"/"			{ return '/'; }
+	/*
+	 * The colon before memory access modifiers can get mixed up with the
+	 * colon before event modifiers. Fortunately none of the option letters
+	 * are the same, so trailing context can be used disambiguate the two
+	 * cases.
+	 */
+":"/{modifier_bp}	{ return str(yyscanner, PE_BP_COLON); }
+	/*
+	 * The slash before memory length can get mixed up with the slash before
+	 * config terms. Fortunately config terms do not start with a numeric
+	 * digit, so trailing context can be used disambiguate the two cases.
+	 */
+"/"/{digit}		{ return str(yyscanner, PE_BP_SLASH); }
+"/"/{non_digit}		{ BEGIN(config); return '/'; }
 {num_dec}		{ return value(yyscanner, 10); }
 {num_hex}		{ return value(yyscanner, 16); }
 	/*
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 4e1f5de35be8..51944dd06115 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -58,7 +58,7 @@ static void free_list_evsel(struct list_head* list_evsel)
 %token PE_EVENT_NAME
 %token PE_RAW PE_NAME
 %token PE_BPF_OBJECT PE_BPF_SOURCE
-%token PE_MODIFIER_EVENT PE_MODIFIER_BP
+%token PE_MODIFIER_EVENT PE_MODIFIER_BP PE_BP_COLON PE_BP_SLASH
 %token PE_LEGACY_CACHE
 %token PE_PREFIX_MEM PE_PREFIX_RAW PE_PREFIX_GROUP
 %token PE_ERROR
@@ -79,6 +79,8 @@ static void free_list_evsel(struct list_head* list_evsel)
 %type <str> PE_LEGACY_CACHE
 %type <str> PE_MODIFIER_EVENT
 %type <str> PE_MODIFIER_BP
+%type <str> PE_BP_COLON
+%type <str> PE_BP_SLASH
 %type <str> PE_EVENT_NAME
 %type <str> PE_KERNEL_PMU_EVENT PE_PMU_EVENT_FAKE
 %type <str> PE_DRV_CFG_TERM
@@ -274,7 +276,7 @@ event_def
 event_def: event_pmu |
 	   event_legacy_symbol |
 	   event_legacy_cache sep_dc |
-	   event_legacy_mem |
+	   event_legacy_mem sep_dc |
 	   event_legacy_tracepoint sep_dc |
 	   event_legacy_numeric sep_dc |
 	   event_legacy_raw sep_dc |
@@ -502,16 +504,16 @@ PE_LEGACY_CACHE opt_event_config
 }
 
 event_legacy_mem:
-PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
+PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
 {
-	struct parse_events_state *parse_state = _parse_state;
 	struct list_head *list;
 	int err;
 
 	list = alloc_list();
 	ABORT_ON(!list);
-	err = parse_events_add_breakpoint(list, &parse_state->idx,
-					  $2, $6, $4);
+	err = parse_events_add_breakpoint(_parse_state, list,
+					  $2, $6, $4, $7);
+	parse_events_terms__delete($7);
 	free($6);
 	if (err) {
 		free(list);
@@ -520,31 +522,33 @@ PE_PREFIX_MEM PE_VALUE '/' PE_VALUE ':' PE_MODIFIER_BP sep_dc
 	$$ = list;
 }
 |
-PE_PREFIX_MEM PE_VALUE '/' PE_VALUE sep_dc
+PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
 {
-	struct parse_events_state *parse_state = _parse_state;
 	struct list_head *list;
+	int err;
 
 	list = alloc_list();
 	ABORT_ON(!list);
-	if (parse_events_add_breakpoint(list, &parse_state->idx,
-					$2, NULL, $4)) {
+	err = parse_events_add_breakpoint(_parse_state, list,
+					  $2, NULL, $4, $5);
+	parse_events_terms__delete($5);
+	if (err) {
 		free(list);
 		YYABORT;
 	}
 	$$ = list;
 }
 |
-PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
+PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
 {
-	struct parse_events_state *parse_state = _parse_state;
 	struct list_head *list;
 	int err;
 
 	list = alloc_list();
 	ABORT_ON(!list);
-	err = parse_events_add_breakpoint(list, &parse_state->idx,
-					  $2, $4, 0);
+	err = parse_events_add_breakpoint(_parse_state, list,
+					  $2, $4, 0, $5);
+	parse_events_terms__delete($5);
 	free($4);
 	if (err) {
 		free(list);
@@ -553,15 +557,17 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
 	$$ = list;
 }
 |
-PE_PREFIX_MEM PE_VALUE sep_dc
+PE_PREFIX_MEM PE_VALUE opt_event_config
 {
-	struct parse_events_state *parse_state = _parse_state;
 	struct list_head *list;
+	int err;
 
 	list = alloc_list();
 	ABORT_ON(!list);
-	if (parse_events_add_breakpoint(list, &parse_state->idx,
-					$2, NULL, 0)) {
+	err = parse_events_add_breakpoint(_parse_state, list,
+					  $2, NULL, 0, $3);
+	parse_events_terms__delete($3);
+	if (err) {
 		free(list);
 		YYABORT;
 	}
-- 
2.34.1

Re: [PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[Adrian Hunter]

On 25/05/23 11:29, Adrian Hunter wrote:
> Hi
> 
> Here is a patch (V3) to the event parser for breakpoint events.
> I am not that familiar with flex / bison, but it seemed to
> need trailing context to stop the mem event colon and slash
> delimiters from getting mixed up with delimiters for config
> terms or event modifiers.  Please look closely at that.
> 
> 
> Changes in V3:
> 
>       Add Ian's Reviewed-by
>       Re-base

Still applies.  Any more comments?

> 
> Changes in V2:
> 
>       Add comments to tools/perf/util/parse-events.l
>       Add a test for 2 mem events back to back with config terms
> 
> 
> Adrian Hunter (1):
>       perf tools: Allow config terms with breakpoints
> 
>  tools/perf/tests/parse-events.c | 157 ++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/parse-events.c  |  23 +++++-
>  tools/perf/util/parse-events.h  |   6 +-
>  tools/perf/util/parse-events.l  |  23 +++++-
>  tools/perf/util/parse-events.y  |  42 ++++++-----
>  5 files changed, 224 insertions(+), 27 deletions(-)
> 
> 
> Regards
> Adrian

Re: [PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[Arnaldo Carvalho de Melo]

Em Tue, Jun 06, 2023 at 08:00:33AM +0300, Adrian Hunter escreveu:
> On 25/05/23 11:29, Adrian Hunter wrote:
> > Hi
> > 
> > Here is a patch (V3) to the event parser for breakpoint events.
> > I am not that familiar with flex / bison, but it seemed to
> > need trailing context to stop the mem event colon and slash
> > delimiters from getting mixed up with delimiters for config
> > terms or event modifiers.  Please look closely at that.
> > 
> > 
> > Changes in V3:
> > 
> >       Add Ian's Reviewed-by
> >       Re-base
> 
> Still applies.  Any more comments?

Tried it now, twice, once after removing the O= build dir:

  CC      /tmp/build/perf-tools-next/tests/event-times.o
  CC      /tmp/build/perf-tools-next/tests/expr.o
  BISON   /tmp/build/perf-tools-next/util/parse-events-bison.c
util/parse-events.y:508.24-34: warning: unused value: $3 [-Wother]
  508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
      |                        ^~~~~~~~~~~
util/parse-events.y:508.45-55: warning: unused value: $5 [-Wother]
  508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
      |                                             ^~~~~~~~~~~
util/parse-events.y:526.24-34: warning: unused value: $3 [-Wother]
  526 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
      |                        ^~~~~~~~~~~
util/parse-events.y:543.24-34: warning: unused value: $3 [-Wother]
  543 | PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
      |                        ^~~~~~~~~~~
  CC      /tmp/build/perf-tools-next/tests/backward-ring-buffer.o


⬢[acme@toolbox perf-tools-next]$ cat /etc/redhat-release
Fedora release 36 (Thirty Six)

⬢[acme@toolbox perf-tools-next]$ rpm -q bison
bison-3.8.2-2.fc36.x86_64
⬢[acme@toolbox perf-tools-next]$ rpm -q flex
flex-2.6.4-10.fc36.x86_64
⬢[acme@toolbox perf-tools-next]$


- Arnaldo
 
> > 
> > Changes in V2:
> > 
> >       Add comments to tools/perf/util/parse-events.l
> >       Add a test for 2 mem events back to back with config terms
> > 
> > 
> > Adrian Hunter (1):
> >       perf tools: Allow config terms with breakpoints
> > 
> >  tools/perf/tests/parse-events.c | 157 ++++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/parse-events.c  |  23 +++++-
> >  tools/perf/util/parse-events.h  |   6 +-
> >  tools/perf/util/parse-events.l  |  23 +++++-
> >  tools/perf/util/parse-events.y  |  42 ++++++-----
> >  5 files changed, 224 insertions(+), 27 deletions(-)
> > 
> > 
> > Regards
> > Adrian
> 

-- 

- Arnaldo

Re: [PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[Arnaldo Carvalho de Melo]

Em Tue, Jun 06, 2023 at 03:15:59PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jun 06, 2023 at 08:00:33AM +0300, Adrian Hunter escreveu:
> > On 25/05/23 11:29, Adrian Hunter wrote:
> > > Hi
> > > 
> > > Here is a patch (V3) to the event parser for breakpoint events.
> > > I am not that familiar with flex / bison, but it seemed to
> > > need trailing context to stop the mem event colon and slash
> > > delimiters from getting mixed up with delimiters for config
> > > terms or event modifiers.  Please look closely at that.
> > > 
> > > 
> > > Changes in V3:
> > > 
> > >       Add Ian's Reviewed-by
> > >       Re-base
> > 
> > Still applies.  Any more comments?
> 
> Tried it now, twice, once after removing the O= build dir:
> 
>   CC      /tmp/build/perf-tools-next/tests/event-times.o
>   CC      /tmp/build/perf-tools-next/tests/expr.o
>   BISON   /tmp/build/perf-tools-next/util/parse-events-bison.c
> util/parse-events.y:508.24-34: warning: unused value: $3 [-Wother]
>   508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>       |                        ^~~~~~~~~~~
> util/parse-events.y:508.45-55: warning: unused value: $5 [-Wother]
>   508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>       |                                             ^~~~~~~~~~~
> util/parse-events.y:526.24-34: warning: unused value: $3 [-Wother]
>   526 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
>       |                        ^~~~~~~~~~~
> util/parse-events.y:543.24-34: warning: unused value: $3 [-Wother]
>   543 | PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>       |                        ^~~~~~~~~~~
>   CC      /tmp/build/perf-tools-next/tests/backward-ring-buffer.o

But it doesn't _break_ the build, just warns thiis when generating the
.c file, the next build it notices it is already generated, no warnings.

- Arnaldo
 
> 
> ⬢[acme@toolbox perf-tools-next]$ cat /etc/redhat-release
> Fedora release 36 (Thirty Six)
> 
> ⬢[acme@toolbox perf-tools-next]$ rpm -q bison
> bison-3.8.2-2.fc36.x86_64
> ⬢[acme@toolbox perf-tools-next]$ rpm -q flex
> flex-2.6.4-10.fc36.x86_64
> ⬢[acme@toolbox perf-tools-next]$
> 
> 
> - Arnaldo
>  
> > > 
> > > Changes in V2:
> > > 
> > >       Add comments to tools/perf/util/parse-events.l
> > >       Add a test for 2 mem events back to back with config terms
> > > 
> > > 
> > > Adrian Hunter (1):
> > >       perf tools: Allow config terms with breakpoints
> > > 
> > >  tools/perf/tests/parse-events.c | 157 ++++++++++++++++++++++++++++++++++++++++
> > >  tools/perf/util/parse-events.c  |  23 +++++-
> > >  tools/perf/util/parse-events.h  |   6 +-
> > >  tools/perf/util/parse-events.l  |  23 +++++-
> > >  tools/perf/util/parse-events.y  |  42 ++++++-----
> > >  5 files changed, 224 insertions(+), 27 deletions(-)
> > > 
> > > 
> > > Regards
> > > Adrian
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

Re: [PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[Adrian Hunter]

On 6/06/23 22:22, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jun 06, 2023 at 03:15:59PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Jun 06, 2023 at 08:00:33AM +0300, Adrian Hunter escreveu:
>>> On 25/05/23 11:29, Adrian Hunter wrote:
>>>> Hi
>>>>
>>>> Here is a patch (V3) to the event parser for breakpoint events.
>>>> I am not that familiar with flex / bison, but it seemed to
>>>> need trailing context to stop the mem event colon and slash
>>>> delimiters from getting mixed up with delimiters for config
>>>> terms or event modifiers.  Please look closely at that.
>>>>
>>>>
>>>> Changes in V3:
>>>>
>>>>       Add Ian's Reviewed-by
>>>>       Re-base
>>>
>>> Still applies.  Any more comments?
>>
>> Tried it now, twice, once after removing the O= build dir:
>>
>>   CC      /tmp/build/perf-tools-next/tests/event-times.o
>>   CC      /tmp/build/perf-tools-next/tests/expr.o
>>   BISON   /tmp/build/perf-tools-next/util/parse-events-bison.c
>> util/parse-events.y:508.24-34: warning: unused value: $3 [-Wother]
>>   508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>>       |                        ^~~~~~~~~~~
>> util/parse-events.y:508.45-55: warning: unused value: $5 [-Wother]
>>   508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>>       |                                             ^~~~~~~~~~~
>> util/parse-events.y:526.24-34: warning: unused value: $3 [-Wother]
>>   526 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
>>       |                        ^~~~~~~~~~~
>> util/parse-events.y:543.24-34: warning: unused value: $3 [-Wother]
>>   543 | PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>>       |                        ^~~~~~~~~~~
>>   CC      /tmp/build/perf-tools-next/tests/backward-ring-buffer.o
> 
> But it doesn't _break_ the build, just warns thiis when generating the
> .c file, the next build it notices it is already generated, no warnings.

The build script I was using has a flaw.  It doesn't show warnings
unless the build fails, assuming warnings are errors.  But these
are not.

The first hack that came to mind, and seems to work, is:

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index bbfb8110947c..dd36be3832b6 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -510,6 +510,8 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event
 	struct list_head *list;
 	int err;
 
+	$3 = $3;
+	$5 = $5;
 	list = alloc_list();
 	ABORT_ON(!list);
 	err = parse_events_add_breakpoint(_parse_state, list,
@@ -528,6 +530,7 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
 	struct list_head *list;
 	int err;
 
+	$3 = $3;
 	list = alloc_list();
 	ABORT_ON(!list);
 	err = parse_events_add_breakpoint(_parse_state, list,
@@ -545,6 +548,7 @@ PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
 	struct list_head *list;
 	int err;
 
+	$3 = $3;
 	list = alloc_list();
 	ABORT_ON(!list);
 	err = parse_events_add_breakpoint(_parse_state, list,


> 
> - Arnaldo
>  
>>
>> ⬢[acme@toolbox perf-tools-next]$ cat /etc/redhat-release
>> Fedora release 36 (Thirty Six)
>>
>> ⬢[acme@toolbox perf-tools-next]$ rpm -q bison
>> bison-3.8.2-2.fc36.x86_64
>> ⬢[acme@toolbox perf-tools-next]$ rpm -q flex
>> flex-2.6.4-10.fc36.x86_64
>> ⬢[acme@toolbox perf-tools-next]$
>>
>>
>> - Arnaldo
>>  
>>>>
>>>> Changes in V2:
>>>>
>>>>       Add comments to tools/perf/util/parse-events.l
>>>>       Add a test for 2 mem events back to back with config terms
>>>>
>>>>
>>>> Adrian Hunter (1):
>>>>       perf tools: Allow config terms with breakpoints
>>>>
>>>>  tools/perf/tests/parse-events.c | 157 ++++++++++++++++++++++++++++++++++++++++
>>>>  tools/perf/util/parse-events.c  |  23 +++++-
>>>>  tools/perf/util/parse-events.h  |   6 +-
>>>>  tools/perf/util/parse-events.l  |  23 +++++-
>>>>  tools/perf/util/parse-events.y  |  42 ++++++-----
>>>>  5 files changed, 224 insertions(+), 27 deletions(-)
>>>>
>>>>
>>>> Regards
>>>> Adrian
>>>
>>
>> -- 
>>
>> - Arnaldo
> 

Re: [PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[Adrian Hunter]

On 6/06/23 22:37, Adrian Hunter wrote:
> On 6/06/23 22:22, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Jun 06, 2023 at 03:15:59PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> Em Tue, Jun 06, 2023 at 08:00:33AM +0300, Adrian Hunter escreveu:
>>>> On 25/05/23 11:29, Adrian Hunter wrote:
>>>>> Hi
>>>>>
>>>>> Here is a patch (V3) to the event parser for breakpoint events.
>>>>> I am not that familiar with flex / bison, but it seemed to
>>>>> need trailing context to stop the mem event colon and slash
>>>>> delimiters from getting mixed up with delimiters for config
>>>>> terms or event modifiers.  Please look closely at that.
>>>>>
>>>>>
>>>>> Changes in V3:
>>>>>
>>>>>       Add Ian's Reviewed-by
>>>>>       Re-base
>>>>
>>>> Still applies.  Any more comments?
>>>
>>> Tried it now, twice, once after removing the O= build dir:
>>>
>>>   CC      /tmp/build/perf-tools-next/tests/event-times.o
>>>   CC      /tmp/build/perf-tools-next/tests/expr.o
>>>   BISON   /tmp/build/perf-tools-next/util/parse-events-bison.c
>>> util/parse-events.y:508.24-34: warning: unused value: $3 [-Wother]
>>>   508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>>>       |                        ^~~~~~~~~~~
>>> util/parse-events.y:508.45-55: warning: unused value: $5 [-Wother]
>>>   508 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>>>       |                                             ^~~~~~~~~~~
>>> util/parse-events.y:526.24-34: warning: unused value: $3 [-Wother]
>>>   526 | PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
>>>       |                        ^~~~~~~~~~~
>>> util/parse-events.y:543.24-34: warning: unused value: $3 [-Wother]
>>>   543 | PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>>>       |                        ^~~~~~~~~~~
>>>   CC      /tmp/build/perf-tools-next/tests/backward-ring-buffer.o
>>
>> But it doesn't _break_ the build, just warns thiis when generating the
>> .c file, the next build it notices it is already generated, no warnings.
> 
> The build script I was using has a flaw.  It doesn't show warnings
> unless the build fails, assuming warnings are errors.  But these
> are not.
> 
> The first hack that came to mind, and seems to work, is:
> 
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index bbfb8110947c..dd36be3832b6 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -510,6 +510,8 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event
>  	struct list_head *list;
>  	int err;
>  
> +	$3 = $3;
> +	$5 = $5;
>  	list = alloc_list();
>  	ABORT_ON(!list);
>  	err = parse_events_add_breakpoint(_parse_state, list,
> @@ -528,6 +530,7 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
>  	struct list_head *list;
>  	int err;
>  
> +	$3 = $3;
>  	list = alloc_list();
>  	ABORT_ON(!list);
>  	err = parse_events_add_breakpoint(_parse_state, list,
> @@ -545,6 +548,7 @@ PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>  	struct list_head *list;
>  	int err;
>  
> +	$3 = $3;
>  	list = alloc_list();
>  	ABORT_ON(!list);
>  	err = parse_events_add_breakpoint(_parse_state, list,
> 
> 

Maybe this is cleaner:


From: Adrian Hunter <adrian.hunter@intel.com>
Date: Wed, 7 Jun 2023 08:12:29 +0300
Subject: [PATCH] perf tools: Suppress bison unused value warnings

Patch "perf tools: Allow config terms with breakpoints" introduced
parse tokens for colons and slashes within breakpoint parsing to
prevent mix up with colons and slashes related to config terms.

The token values are not needed but introduce bison "unused value"
warnings.

Suppress those warnings.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/parse-events.h | 4 ++++
 tools/perf/util/parse-events.y | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index b0eb95f93e9c..5fdc1f33f57e 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -228,6 +228,10 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
 void parse_events_error__print(struct parse_events_error *err,
 			       const char *event);
 
+static inline void parse_events_unused_value(const void *x __maybe_unused)
+{
+}
+
 #ifdef HAVE_LIBELF_SUPPORT
 /*
  * If the probe point starts with '%',
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index bbfb8110947c..0c3d086cc22a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -510,6 +510,9 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event
 	struct list_head *list;
 	int err;
 
+	parse_events_unused_value(&$3);
+	parse_events_unused_value(&$5);
+
 	list = alloc_list();
 	ABORT_ON(!list);
 	err = parse_events_add_breakpoint(_parse_state, list,
@@ -528,6 +531,8 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
 	struct list_head *list;
 	int err;
 
+	parse_events_unused_value(&$3);
+
 	list = alloc_list();
 	ABORT_ON(!list);
 	err = parse_events_add_breakpoint(_parse_state, list,
@@ -545,6 +550,8 @@ PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
 	struct list_head *list;
 	int err;
 
+	parse_events_unused_value(&$3);
+
 	list = alloc_list();
 	ABORT_ON(!list);
 	err = parse_events_add_breakpoint(_parse_state, list,
-- 
2.34.1

Re: [PATCH V3 0/1] perf tools: Allow config terms with breakpoints

[Arnaldo Carvalho de Melo]

Em Wed, Jun 07, 2023 at 08:33:07AM +0300, Adrian Hunter escreveu:
> Maybe this is cleaner:

It is, I folded it into your original patch, thanks!

- Arnaldo
 
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Wed, 7 Jun 2023 08:12:29 +0300
> Subject: [PATCH] perf tools: Suppress bison unused value warnings
> 
> Patch "perf tools: Allow config terms with breakpoints" introduced
> parse tokens for colons and slashes within breakpoint parsing to
> prevent mix up with colons and slashes related to config terms.
> 
> The token values are not needed but introduce bison "unused value"
> warnings.
> 
> Suppress those warnings.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/util/parse-events.h | 4 ++++
>  tools/perf/util/parse-events.y | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index b0eb95f93e9c..5fdc1f33f57e 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -228,6 +228,10 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
>  void parse_events_error__print(struct parse_events_error *err,
>  			       const char *event);
>  
> +static inline void parse_events_unused_value(const void *x __maybe_unused)
> +{
> +}
> +
>  #ifdef HAVE_LIBELF_SUPPORT
>  /*
>   * If the probe point starts with '%',
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index bbfb8110947c..0c3d086cc22a 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -510,6 +510,9 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event
>  	struct list_head *list;
>  	int err;
>  
> +	parse_events_unused_value(&$3);
> +	parse_events_unused_value(&$5);
> +
>  	list = alloc_list();
>  	ABORT_ON(!list);
>  	err = parse_events_add_breakpoint(_parse_state, list,
> @@ -528,6 +531,8 @@ PE_PREFIX_MEM PE_VALUE PE_BP_SLASH PE_VALUE opt_event_config
>  	struct list_head *list;
>  	int err;
>  
> +	parse_events_unused_value(&$3);
> +
>  	list = alloc_list();
>  	ABORT_ON(!list);
>  	err = parse_events_add_breakpoint(_parse_state, list,
> @@ -545,6 +550,8 @@ PE_PREFIX_MEM PE_VALUE PE_BP_COLON PE_MODIFIER_BP opt_event_config
>  	struct list_head *list;
>  	int err;
>  
> +	parse_events_unused_value(&$3);
> +
>  	list = alloc_list();
>  	ABORT_ON(!list);
>  	err = parse_events_add_breakpoint(_parse_state, list,
> -- 
> 2.34.1
> 
> 

-- 

- Arnaldo