Skip to contentSkip to conversation

perf: bug fixes, cleanup of parse events memory allocations

PATCH Applied David Ahern 4 participants 17 messages Jul 2, 2013 – Jul 19, 2013 perf events

lore
Share & tools
lore

David Ahern

author
#0 lore raw

cover perf: bug fixes, cleanup of parse events memory allocations

David Ahern (6):
perf evsel: fix count parameter to read call in event_format__new perf evlist: fix use of uninitialized variable
perf event: initialize allocated memory for synthesized events
perf: don't free list head in parse_events__free_terms perf test: make terms a stack variable in test_term perf parse events: demystify memory allocations

 tools/perf/tests/parse-events.c |   14 ++++-----
 tools/perf/util/event.c         |    8 ++---
 tools/perf/util/evlist.c        |    2 +-
 tools/perf/util/evsel.c         |    2 +-
 tools/perf/util/parse-events.c  |   54 ++++++++++------------------------
 tools/perf/util/parse-events.h  |   10 +++----
 tools/perf/util/parse-events.y  |   62 +++++++++++++++++++++++++--------------
 7 files changed, 72 insertions(+), 80 deletions(-)
Signature · 1 line
1.7.10.1
author
#3 lore raw

PATCH 2/6 perf evlist: fix use of uninitialized variable

Fixes valgrind complaint:
==1870== Syscall param write(buf) points to uninitialised byte(s)

==1870==    at 0x4E3F5B0: __write_nocancel (in /lib64/libpthread-2.14.90.so)
==1870==    by 0x449D7C: perf_evlist__start_workload (evlist.c:846)
==1870==    by 0x427BC1: cmd_record (builtin-record.c:561)
==1870==    by 0x419D72: run_builtin (perf.c:319)
==1870==    by 0x4195F2: main (perf.c:376)

==1870== Address 0x7feffcdd7 is on thread 1's stack

Signed-off-by David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
---
 tools/perf/util/evlist.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/evlist.c +1 -1
index 4a901be..d8f34e0 100644
@@ -838,7 +838,7 @@ out_close_ready_pipe:
 int perf_evlist__start_workload(struct perf_evlist *evlist)
 {
 	if (evlist->workload.cork_fd > 0) {
-		char bf;
+		char bf = 0;
 		int ret;
 		/*
 		 * Remove the cork, let it rip!
Signature · 1 line
1.7.10.1
author
#9 lore raw

PATCH 5/6 perf test: make terms a stack variable in test_term

In reply to David Ahern

No need to malloc the memory for it.

Signed-off-by David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
---
 tools/perf/tests/parse-events.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
tools/perf/tests/parse-events.c +5 -9
index ad950f5..344c844 100644
@@ -1246,24 +1246,20 @@ static int test_events(struct evlist_test *events, unsigned cnt)
 
 static int test_term(struct terms_test *t)
 {
-	struct list_head *terms;
+	struct list_head terms;
 	int ret;
 
-	terms = malloc(sizeof(*terms));
-	if (!terms)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(terms);
+	INIT_LIST_HEAD(&terms);
 
-	ret = parse_events_terms(terms, t->str);
+	ret = parse_events_terms(&terms, t->str);
 	if (ret) {
 		pr_debug("failed to parse terms '%s', err %d\n",
 			 t->str , ret);
 		return ret;
 	}
 
-	ret = t->check(terms);
-	parse_events__free_terms(terms);
+	ret = t->check(&terms);
+	parse_events__free_terms(&terms);
 
 	return ret;
 }
Signature · 1 line
1.7.10.1
author
#11 lore raw

PATCH 6/6 perf parse events: demystify memory allocations

In reply to David Ahern

List heads are currently allocated way down the function chain in __add_event and add_tracepoint and then freed when the scanner code calls parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With this patch the list_head is allocated in the scanner code and freed when the scanner code calls parse_events_update_lists.

Signed-off-by David Ahern <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
---
 tools/perf/util/parse-events.c |   52 +++++++++++----------------------
 tools/perf/util/parse-events.h |   10 +++----
 tools/perf/util/parse-events.y |   62 ++++++++++++++++++++++++++--------------
 3 files changed, 61 insertions(+), 63 deletions(-)
tools/perf/util/parse-events.c +16 -36
index 3a34f7b..8e3e270 100644
@@ -264,40 +264,29 @@ const char *event_type(int type)
 
 
 
-static int __add_event(struct list_head **_list, int *idx,
+static int __add_event(struct list_head *list, int *idx,
 		       struct perf_event_attr *attr,
 		       char *name, struct cpu_map *cpus)
 {
 	struct perf_evsel *evsel;
-	struct list_head *list = *_list;
-
-	if (!list) {
-		list = malloc(sizeof(*list));
-		if (!list)
-			return -ENOMEM;
-		INIT_LIST_HEAD(list);
-	}
 
 	event_attr_init(attr);
 
 	evsel = perf_evsel__new(attr, (*idx)++);
-	if (!evsel) {
-		free(list);
+	if (!evsel)
 		return -ENOMEM;
-	}
 
 	evsel->cpus = cpus;
 	if (name)
 		evsel->name = strdup(name);
 	list_add_tail(&evsel->node, list);
-	*_list = list;
 	return 0;
 }
 
-static int add_event(struct list_head **_list, int *idx,
+static int add_event(struct list_head *list, int *idx,
 		     struct perf_event_attr *attr, char *name)
 {
-	return __add_event(_list, idx, attr, name, NULL);
+	return __add_event(list, idx, attr, name, NULL);
 }
 
 static int parse_aliases(char *str, const char *names[][PERF_EVSEL__MAX_ALIASES], int size)

@@ -318,7 +307,7 @@ static int parse_aliases(char *str, const char *names[][PERF_EVSEL__MAX_ALIASES]
 	return -1;
 }
 
-int parse_events_add_cache(struct list_head **list, int *idx,
+int parse_events_add_cache(struct list_head *list, int *idx,
 			   char *type, char *op_result1, char *op_result2)
 {
 	struct perf_event_attr attr;

@@ -379,31 +368,21 @@ int parse_events_add_cache(struct list_head **list, int *idx,
 	return add_event(list, idx, &attr, name);
 }
 
-static int add_tracepoint(struct list_head **listp, int *idx,
+static int add_tracepoint(struct list_head *list, int *idx,
 			  char *sys_name, char *evt_name)
 {
 	struct perf_evsel *evsel;
-	struct list_head *list = *listp;
-
-	if (!list) {
-		list = malloc(sizeof(*list));
-		if (!list)
-			return -ENOMEM;
-		INIT_LIST_HEAD(list);
-	}
 
 	evsel = perf_evsel__newtp(sys_name, evt_name, (*idx)++);
-	if (!evsel) {
-		free(list);
+	if (!evsel)
 		return -ENOMEM;
-	}
 
 	list_add_tail(&evsel->node, list);
-	*listp = list;
+
 	return 0;
 }
 
-static int add_tracepoint_multi_event(struct list_head **list, int *idx,
+static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 				      char *sys_name, char *evt_name)
 {
 	char evt_path[MAXPATHLEN];

@@ -435,7 +414,7 @@ static int add_tracepoint_multi_event(struct list_head **list, int *idx,
 	return ret;
 }
 
-static int add_tracepoint_event(struct list_head **list, int *idx,
+static int add_tracepoint_event(struct list_head *list, int *idx,
 				char *sys_name, char *evt_name)
 {
 	return strpbrk(evt_name, "*?") ?

@@ -443,7 +422,7 @@ static int add_tracepoint_event(struct list_head **list, int *idx,
 	       add_tracepoint(list, idx, sys_name, evt_name);
 }
 
-static int add_tracepoint_multi_sys(struct list_head **list, int *idx,
+static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 				    char *sys_name, char *evt_name)
 {
 	struct dirent *events_ent;

@@ -475,7 +454,7 @@ static int add_tracepoint_multi_sys(struct list_head **list, int *idx,
 	return ret;
 }
 
-int parse_events_add_tracepoint(struct list_head **list, int *idx,
+int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				char *sys, char *event)
 {
 	int ret;

@@ -530,7 +509,7 @@ do {					\
 	return 0;
 }
 
-int parse_events_add_breakpoint(struct list_head **list, int *idx,
+int parse_events_add_breakpoint(struct list_head *list, int *idx,
 				void *ptr, char *type)
 {
 	struct perf_event_attr attr;

@@ -611,7 +590,7 @@ static int config_attr(struct perf_event_attr *attr,
 	return 0;
 }
 
-int parse_events_add_numeric(struct list_head **list, int *idx,
+int parse_events_add_numeric(struct list_head *list, int *idx,
 			     u32 type, u64 config,
 			     struct list_head *head_config)
 {

@@ -644,7 +623,7 @@ static char *pmu_event_name(struct list_head *head_terms)
 	return NULL;
 }
 
-int parse_events_add_pmu(struct list_head **list, int *idx,
+int parse_events_add_pmu(struct list_head *list, int *idx,
 			 char *name, struct list_head *head_config)
 {
 	struct perf_event_attr attr;

@@ -687,6 +666,7 @@ void parse_events__set_leader(char *name, struct list_head *list)
 	leader->group_name = name ? strdup(name) : NULL;
 }
 
+/* list_event is assumed to point to malloc'ed memory */
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all)
 {
tools/perf/util/parse-events.h +5 -5
index 080f7cf..f1cb4c4 100644
@@ -85,16 +85,16 @@ void parse_events__free_terms(struct list_head *terms);
 int parse_events__modifier_event(struct list_head *list, char *str, bool add);
 int parse_events__modifier_group(struct list_head *list, char *event_mod);
 int parse_events_name(struct list_head *list, char *name);
-int parse_events_add_tracepoint(struct list_head **list, int *idx,
+int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				char *sys, char *event);
-int parse_events_add_numeric(struct list_head **list, int *idx,
+int parse_events_add_numeric(struct list_head *list, int *idx,
 			     u32 type, u64 config,
 			     struct list_head *head_config);
-int parse_events_add_cache(struct list_head **list, int *idx,
+int parse_events_add_cache(struct list_head *list, int *idx,
 			   char *type, char *op_result1, char *op_result2);
-int parse_events_add_breakpoint(struct list_head **list, int *idx,
+int parse_events_add_breakpoint(struct list_head *list, int *idx,
 				void *ptr, char *type);
-int parse_events_add_pmu(struct list_head **list, int *idx,
+int parse_events_add_pmu(struct list_head *list, int *idx,
 			 char *pmu , struct list_head *head_config);
 void parse_events__set_leader(char *name, struct list_head *list);
 void parse_events_update_lists(struct list_head *list_event,
tools/perf/util/parse-events.y +40 -22

149 lines not on this page · raw message

Signature · 1 line
1.7.10.1
author
#7 lore raw

PATCH 4/6 perf: don't free list head in parse_events__free_terms

In reply to David Ahern

Function should only be freeing the entries in the list, not the list_head itself.

Signed-off-by David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
---
 tools/perf/util/parse-events.c |    2 --
 1 file changed, 2 deletions(-)
tools/perf/util/parse-events.c +0 -2
index 1ae166c..3a34f7b 100644
@@ -1258,6 +1258,4 @@ void parse_events__free_terms(struct list_head *terms)
 
 	list_for_each_entry_safe(term, h, terms, list)
 		free(term);
-
-	free(terms);
 }
Signature · 1 line
1.7.10.1
author
#5 lore raw

PATCH 3/6 perf event: initialize allocated memory for synthesized events

In reply to David Ahern

Fixes valgrind complaint:

=3118== Syscall param write(buf) points to uninitialised byte(s)

==3118==    at 0x4E3F5B0: __write_nocancel (in /lib64/libpthread-2.14.90.so)
==3118==    by 0x42F0EB: process_synthesized_event (builtin-record.c:89)
==3118==    by 0x44E81C: perf_event__synthesize_mmap_events (event.c:230)
==3118==    by 0x44F27C: perf_event__synthesize_threads (event.c:307)
==3118==    by 0x42FEEA: cmd_record (builtin-record.c:530)
==3118==    by 0x419D22: run_builtin (perf.c:319)
==3118==    by 0x4195A2: main (perf.c:376)
Signed-off-by David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
---
 tools/perf/util/event.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
tools/perf/util/event.c +4 -4
index 5cd13d7..556b999 100644
@@ -316,11 +316,11 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
 	union perf_event *comm_event, *mmap_event;
 	int err = -1, thread, j;
 
-	comm_event = malloc(sizeof(comm_event->comm) + machine->id_hdr_size);
+	comm_event = zalloc(sizeof(comm_event->comm) + machine->id_hdr_size);
 	if (comm_event == NULL)
 		goto out;
 
-	mmap_event = malloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
+	mmap_event = zalloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
 	if (mmap_event == NULL)
 		goto out_free_comm;
 

@@ -375,11 +375,11 @@ int perf_event__synthesize_threads(struct perf_tool *tool,
 	union perf_event *comm_event, *mmap_event;
 	int err = -1;
 
-	comm_event = malloc(sizeof(comm_event->comm) + machine->id_hdr_size);
+	comm_event = zalloc(sizeof(comm_event->comm) + machine->id_hdr_size);
 	if (comm_event == NULL)
 		goto out;
 
-	mmap_event = malloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
+	mmap_event = zalloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
 	if (mmap_event == NULL)
 		goto out_free_comm;
Signature · 1 line
1.7.10.1
author
#1 lore raw

PATCH 1/6 perf evsel: fix count parameter to read call in event_format__new

In reply to David Ahern

per realloc above the length of the buffer is alloc_size, not BUFSIZ. Adjust length per size as done for buf start.

Addresses some valgrind complaints:

==1870== Syscall param read(buf) points to unaddressable byte(s)

==1870==    at 0x4E3F610: __read_nocancel (in /lib64/libpthread-2.14.90.so)
==1870==    by 0x44AEE1: event_format__new (unistd.h:45)
==1870==    by 0x44B025: perf_evsel__newtp (evsel.c:158)
==1870==    by 0x451919: add_tracepoint_event (parse-events.c:395)
==1870==    by 0x479815: parse_events_parse (parse-events.y:292)
==1870==    by 0x45463A: parse_events_option (parse-events.c:861)
==1870==    by 0x44FEE4: get_value (parse-options.c:113)
==1870==    by 0x450767: parse_options_step (parse-options.c:192)
==1870==    by 0x450C40: parse_options (parse-options.c:422)
==1870==    by 0x42735F: cmd_record (builtin-record.c:918)
==1870==    by 0x419D72: run_builtin (perf.c:319)
==1870==    by 0x4195F2: main (perf.c:376)
==1870==  Address 0xcffebf0 is 0 bytes after a block of size 8,192 alloc'd
==1870==    at 0x4C2A62F: malloc (vg_replace_malloc.c:270)
==1870==    by 0x4C2A7A3: realloc (vg_replace_malloc.c:662)
==1870==    by 0x44AF07: event_format__new (evsel.c:121)
==1870==    by 0x44B025: perf_evsel__newtp (evsel.c:158)
==1870==    by 0x451919: add_tracepoint_event (parse-events.c:395)
==1870==    by 0x479815: parse_events_parse (parse-events.y:292)
==1870==    by 0x45463A: parse_events_option (parse-events.c:861)
==1870==    by 0x44FEE4: get_value (parse-options.c:113)
==1870==    by 0x450767: parse_options_step (parse-options.c:192)
==1870==    by 0x450C40: parse_options (parse-options.c:422)
==1870==    by 0x42735F: cmd_record (builtin-record.c:918)
==1870==    by 0x419D72: run_builtin (perf.c:319)
Signed-off-by David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
---
 tools/perf/util/evsel.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/evsel.c +1 -1
index dea0684..01fdfc6 100644
@@ -124,7 +124,7 @@ struct event_format *event_format__new(const char *sys, const char *name)
 			bf = nbf;
 		}
 
-		n = read(fd, bf + size, BUFSIZ);
+		n = read(fd, bf + size, alloc_size - size);
 		if (n < 0)
 			goto out_free_bf;
 		size += n;
Signature · 1 line
1.7.10.1
Applied
#16 lore raw
In reply to David Ahern

Em Tue, Jul 02, 2013 at 01:27:19PM -0600, David Ahern escreveu:

Quoted 7 lines

David Ahern (6):
perf evsel: fix count parameter to read call in event_format__new perf evlist: fix use of uninitialized variable
perf event: initialize allocated memory for synthesized events
perf: don't free list head in parse_events__free_terms perf test: make terms a stack variable in test_term perf parse events: demystify memory allocations

Thanks, applied,

- Arnaldo

#12 lore raw

Re: [PATCH 6/6] perf parse events: demystify memory allocations

In reply to David Ahern
Quoted 8 lines · David Ahern

List heads are currently allocated way down the function chain in __add_event and add_tracepoint and then freed when the scanner code calls parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With this patch the list_head is allocated in the scanner code and freed when the scanner code calls parse_events_update_lists.

SNIP

Quoted 14 lines
tools/perf/util/parse-events.y +3 -2
@@ -266,9 +279,10 @@ event_legacy_mem:
 PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
 {
 	struct parse_events_evlist *data = _data;
-	struct list_head *list = NULL;
+	struct list_head *list;
 
-	ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
 					     (void *) $2, $4));
 	$$ = list;
 }

tools/perf/util/parse-events.y PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc · line 297

Quoted 10 lines
tools/perf/util/parse-events.y +3 -2
@@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
 PE_PREFIX_MEM PE_VALUE sep_dc
 {
 	struct parse_events_evlist *data = _data;
-	struct list_head *list = NULL;
+	struct list_head *list;
 
-	ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
 					     (void *) $2, NULL));

so who now frees the list if there's an error
in parse_events_add_breakpoint?

ditto for other ABORT_ON cases

jirka

author
#13 lore raw

Re: [PATCH 6/6] perf parse events: demystify memory allocations

Quoted 32 lines · Jiri Olsa
Quoted 8 lines · David Ahern

List heads are currently allocated way down the function chain in __add_event and add_tracepoint and then freed when the scanner code calls parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With this patch the list_head is allocated in the scanner code and freed when the scanner code calls parse_events_update_lists.

SNIP

Quoted 20 lines
tools/perf/util/parse-events.y +4 -3
@@ -266,9 +279,10 @@ event_legacy_mem:
  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  {
  	struct parse_events_evlist *data = _data;
-	struct list_head *list = NULL;
+	struct list_head *list;
 
-	ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
  					     (void *) $2, $4));
  	$$ = list;
  }

@@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  PE_PREFIX_MEM PE_VALUE sep_dc
  {
  	struct parse_events_evlist *data = _data;
-	struct list_head *list = NULL;
+	struct list_head *list;
Quoted 7 lines

tools/perf/util/parse-events.y PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc · line 297

Quoted 4 lines
tools/perf/util/parse-events.y +2 -1
-	ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
  					     (void *) $2, NULL));

so who now frees the list if there's an error
in parse_events_add_breakpoint?

According to valgrind that memory is not freed prior to this patch, so this one does not introduce new leaks.

Quoted 2 lines

ditto for other ABORT_ON cases

I will whip up a patch to free memory on failure paths.

David

#14 lore raw

Re: [PATCH 6/6] perf parse events: demystify memory allocations

Quoted 41 lines · David Ahern
Quoted 32 lines · Jiri Olsa
Quoted 8 lines · David Ahern

List heads are currently allocated way down the function chain in __add_event and add_tracepoint and then freed when the scanner code calls parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With this patch the list_head is allocated in the scanner code and freed when the scanner code calls parse_events_update_lists.

SNIP

Quoted 20 lines
tools/perf/util/parse-events.y +4 -3
@@ -266,9 +279,10 @@ event_legacy_mem:
 PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
 {
 	struct parse_events_evlist *data = _data;
-	struct list_head *list = NULL;
+	struct list_head *list;
 
-	ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
 					     (void *) $2, $4));
 	$$ = list;
 }

@@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
 PE_PREFIX_MEM PE_VALUE sep_dc
 {
 	struct parse_events_evlist *data = _data;
-	struct list_head *list = NULL;
+	struct list_head *list;
Quoted 7 lines

tools/perf/util/parse-events.y PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc · line 297

Quoted 4 lines
tools/perf/util/parse-events.y +2 -1
-	ABORT_ON(parse_events_add_breakpoint(&list, &data->idx,
+	ALLOC_LIST(list);
+	ABORT_ON(parse_events_add_breakpoint(list, &data->idx,
 					     (void *) $2, NULL));

so who now frees the list if there's an error
in parse_events_add_breakpoint?

Quoted 2 lines

According to valgrind that memory is not freed prior to this patch, so this one does not introduce new leaks.

I thought this hunk did:

        evsel = perf_evsel__new(attr, (*idx)++);

- if (!evsel) {
- free(list);

but I might have missed other cases..

jirka

author
#6 lore raw

Re: [PATCH 3/6] perf event: initialize allocated memory for synthesized events

In reply to David Ahern

Arnaldo:

Don't see this one in your perf/core branch.

Quoted 54 lines · David Ahern

Fixes valgrind complaint:

=3118== Syscall param write(buf) points to uninitialised byte(s)

==3118==    at 0x4E3F5B0: __write_nocancel (in /lib64/libpthread-2.14.90.so)
==3118==    by 0x42F0EB: process_synthesized_event (builtin-record.c:89)
==3118==    by 0x44E81C: perf_event__synthesize_mmap_events (event.c:230)
==3118==    by 0x44F27C: perf_event__synthesize_threads (event.c:307)
==3118==    by 0x42FEEA: cmd_record (builtin-record.c:530)
==3118==    by 0x419D22: run_builtin (perf.c:319)
==3118==    by 0x4195A2: main (perf.c:376)
Signed-off-by David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
---
  tools/perf/util/event.c |    8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
tools/perf/util/event.c +4 -4
index 5cd13d7..556b999 100644
@@ -316,11 +316,11 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
  	union perf_event *comm_event, *mmap_event;
  	int err = -1, thread, j;
 
-	comm_event = malloc(sizeof(comm_event->comm) + machine->id_hdr_size);
+	comm_event = zalloc(sizeof(comm_event->comm) + machine->id_hdr_size);
  	if (comm_event == NULL)
  		goto out;
 
-	mmap_event = malloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
+	mmap_event = zalloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
  	if (mmap_event == NULL)
  		goto out_free_comm;
 

@@ -375,11 +375,11 @@ int perf_event__synthesize_threads(struct perf_tool *tool,
  	union perf_event *comm_event, *mmap_event;
  	int err = -1;
 
-	comm_event = malloc(sizeof(comm_event->comm) + machine->id_hdr_size);
+	comm_event = zalloc(sizeof(comm_event->comm) + machine->id_hdr_size);
  	if (comm_event == NULL)
  		goto out;
 
-	mmap_event = malloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
+	mmap_event = zalloc(sizeof(mmap_event->mmap) + machine->id_hdr_size);
  	if (mmap_event == NULL)
  		goto out_free_comm;
merge bot
#2 lore raw

[tip:perf/urgent] perf evsel: Fix count parameter to read call in event_format__new

In reply to David Ahern
perf evsel: Fix count parameter to read call in event_format__new
Commit-ID:  7cab84e8975cfb8a59ce3e79ce75e5eedd384484
Gitweb:     http://git.kernel.org/tip/7cab84e8975cfb8a59ce3e79ce75e5eedd384484
Author:     David Ahern <[email protected]>
AuthorDate: Tue, 2 Jul 2013 13:27:20 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 8 Jul 2013 17:40:39 -0300

perf evsel: Fix count parameter to read call in event_format__new

per realloc above the length of the buffer is alloc_size, not BUFSIZ. Adjust length per size as done for buf start.

Addresses some valgrind complaints:

==1870== Syscall param read(buf) points to unaddressable byte(s)

==1870==    at 0x4E3F610: __read_nocancel (in /lib64/libpthread-2.14.90.so)
==1870==    by 0x44AEE1: event_format__new (unistd.h:45)
==1870==    by 0x44B025: perf_evsel__newtp (evsel.c:158)
==1870==    by 0x451919: add_tracepoint_event (parse-events.c:395)
==1870==    by 0x479815: parse_events_parse (parse-events.y:292)
==1870==    by 0x45463A: parse_events_option (parse-events.c:861)
==1870==    by 0x44FEE4: get_value (parse-options.c:113)
==1870==    by 0x450767: parse_options_step (parse-options.c:192)
==1870==    by 0x450C40: parse_options (parse-options.c:422)
==1870==    by 0x42735F: cmd_record (builtin-record.c:918)
==1870==    by 0x419D72: run_builtin (perf.c:319)
==1870==    by 0x4195F2: main (perf.c:376)
==1870==  Address 0xcffebf0 is 0 bytes after a block of size 8,192 alloc'd
==1870==    at 0x4C2A62F: malloc (vg_replace_malloc.c:270)
==1870==    by 0x4C2A7A3: realloc (vg_replace_malloc.c:662)
==1870==    by 0x44AF07: event_format__new (evsel.c:121)
==1870==    by 0x44B025: perf_evsel__newtp (evsel.c:158)
==1870==    by 0x451919: add_tracepoint_event (parse-events.c:395)
==1870==    by 0x479815: parse_events_parse (parse-events.y:292)
==1870==    by 0x45463A: parse_events_option (parse-events.c:861)
==1870==    by 0x44FEE4: get_value (parse-options.c:113)
==1870==    by 0x450767: parse_options_step (parse-options.c:192)
==1870==    by 0x450C40: parse_options (parse-options.c:422)
==1870==    by 0x42735F: cmd_record (builtin-record.c:918)
==1870==    by 0x419D72: run_builtin (perf.c:319)
Signed-off-by David Ahern <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/evsel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/evsel.c +1 -1
index 63b6f8c..df99ebe 100644
@@ -124,7 +124,7 @@ struct event_format *event_format__new(const char *sys, const char *name)
 			bf = nbf;
 		}
 
-		n = read(fd, bf + size, BUFSIZ);
+		n = read(fd, bf + size, alloc_size - size);
 		if (n < 0)
 			goto out_free_bf;
 		size += n;
merge bot
#4 lore raw

[tip:perf/core] perf evlist: Fix use of uninitialized variable

In reply to David Ahern
perf evlist: Fix use of uninitialized variable
Commit-ID:  b3824404ebbd489858f3a7097c715120118fa5cd
Gitweb:     http://git.kernel.org/tip/b3824404ebbd489858f3a7097c715120118fa5cd
Author:     David Ahern <[email protected]>
AuthorDate: Tue, 2 Jul 2013 13:27:21 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 12 Jul 2013 13:46:12 -0300

perf evlist: Fix use of uninitialized variable

Fixes valgrind complaint:
==1870== Syscall param write(buf) points to uninitialised byte(s)

==1870==    at 0x4E3F5B0: __write_nocancel (in /lib64/libpthread-2.14.90.so)
==1870==    by 0x449D7C: perf_evlist__start_workload (evlist.c:846)
==1870==    by 0x427BC1: cmd_record (builtin-record.c:561)
==1870==    by 0x419D72: run_builtin (perf.c:319)
==1870==    by 0x4195F2: main (perf.c:376)

==1870== Address 0x7feffcdd7 is on thread 1's stack

Signed-off-by David Ahern <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/evlist.c +1 -1
index 4a901be..d8f34e0 100644
@@ -838,7 +838,7 @@ out_close_ready_pipe:
 int perf_evlist__start_workload(struct perf_evlist *evlist)
 {
 	if (evlist->workload.cork_fd > 0) {
-		char bf;
+		char bf = 0;
 		int ret;
 		/*
 		 * Remove the cork, let it rip!
merge bot
#8 lore raw

[tip:perf/core] perf tools: Don' t free list head in parse_events__free_terms

In reply to David Ahern
perf tools: Don't free list head in parse_events__free_terms
Commit-ID:  0142dab01cd690f6e376f1fb4d4462beb054dfaf
Gitweb:     http://git.kernel.org/tip/0142dab01cd690f6e376f1fb4d4462beb054dfaf
Author:     David Ahern <[email protected]>
AuthorDate: Tue, 2 Jul 2013 13:27:23 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 12 Jul 2013 13:46:14 -0300

perf tools: Don't free list head in parse_events__free_terms

Function should only be freeing the entries in the list in case of failure, as those were allocated there, not the list_head itself.

Signed-off-by David Ahern <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/parse-events.c | 2 --
 1 file changed, 2 deletions(-)
tools/perf/util/parse-events.c +0 -2
index ef72e98..bcf83ee 100644
@@ -1260,6 +1260,4 @@ void parse_events__free_terms(struct list_head *terms)
 
 	list_for_each_entry_safe(term, h, terms, list)
 		free(term);
-
-	free(terms);
 }
merge bot
#10 lore raw

[tip:perf/core] perf tests: Make terms a stack variable in test_term

In reply to David Ahern
perf tests: Make terms a stack variable in test_term
Commit-ID:  c549aca50134640537bf0fbae43c08fd5ff91932
Gitweb:     http://git.kernel.org/tip/c549aca50134640537bf0fbae43c08fd5ff91932
Author:     David Ahern <[email protected]>
AuthorDate: Tue, 2 Jul 2013 13:27:24 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 12 Jul 2013 13:46:17 -0300

perf tests: Make terms a stack variable in test_term

No need to malloc the memory for it.

Signed-off-by David Ahern <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/tests/parse-events.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
tools/perf/tests/parse-events.c +5 -9
index ad950f5..344c844 100644
@@ -1246,24 +1246,20 @@ static int test_events(struct evlist_test *events, unsigned cnt)
 
 static int test_term(struct terms_test *t)
 {
-	struct list_head *terms;
+	struct list_head terms;
 	int ret;
 
-	terms = malloc(sizeof(*terms));
-	if (!terms)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(terms);
+	INIT_LIST_HEAD(&terms);
 
-	ret = parse_events_terms(terms, t->str);
+	ret = parse_events_terms(&terms, t->str);
 	if (ret) {
 		pr_debug("failed to parse terms '%s', err %d\n",
 			 t->str , ret);
 		return ret;
 	}
 
-	ret = t->check(terms);
-	parse_events__free_terms(terms);
+	ret = t->check(&terms);
+	parse_events__free_terms(&terms);
 
 	return ret;
 }
merge bot
#15 lore raw

[tip:perf/core] perf parse events: Demystify memory allocations

In reply to David Ahern
perf parse events: Demystify memory allocations
Commit-ID:  c5cd8ac07e7ad5f21b1930b23b2e1bb231958430
Gitweb:     http://git.kernel.org/tip/c5cd8ac07e7ad5f21b1930b23b2e1bb231958430
Author:     David Ahern <[email protected]>
AuthorDate: Tue, 2 Jul 2013 13:27:25 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 12 Jul 2013 13:52:05 -0300

perf parse events: Demystify memory allocations

List heads are currently allocated way down the function chain in __add_event and add_tracepoint and then freed when the scanner code calls parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With this patch the list_head is allocated in the scanner code and freed when the scanner code calls parse_events_update_lists.

Signed-off-by David Ahern <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/parse-events.c | 52 +++++++++++------------------------
 tools/perf/util/parse-events.h | 10 +++----
 tools/perf/util/parse-events.y | 62 +++++++++++++++++++++++++++---------------
 3 files changed, 61 insertions(+), 63 deletions(-)
tools/perf/util/parse-events.c +16 -36
index bcf83ee..e853769 100644
@@ -264,40 +264,29 @@ const char *event_type(int type)
 
 
 
-static int __add_event(struct list_head **_list, int *idx,
+static int __add_event(struct list_head *list, int *idx,
 		       struct perf_event_attr *attr,
 		       char *name, struct cpu_map *cpus)
 {
 	struct perf_evsel *evsel;
-	struct list_head *list = *_list;
-
-	if (!list) {
-		list = malloc(sizeof(*list));
-		if (!list)
-			return -ENOMEM;
-		INIT_LIST_HEAD(list);
-	}
 
 	event_attr_init(attr);
 
 	evsel = perf_evsel__new(attr, (*idx)++);
-	if (!evsel) {
-		free(list);
+	if (!evsel)
 		return -ENOMEM;
-	}
 
 	evsel->cpus = cpus;
 	if (name)
 		evsel->name = strdup(name);
 	list_add_tail(&evsel->node, list);
-	*_list = list;
 	return 0;
 }
 
-static int add_event(struct list_head **_list, int *idx,
+static int add_event(struct list_head *list, int *idx,
 		     struct perf_event_attr *attr, char *name)
 {
-	return __add_event(_list, idx, attr, name, NULL);
+	return __add_event(list, idx, attr, name, NULL);
 }
 
 static int parse_aliases(char *str, const char *names[][PERF_EVSEL__MAX_ALIASES], int size)

@@ -318,7 +307,7 @@ static int parse_aliases(char *str, const char *names[][PERF_EVSEL__MAX_ALIASES]
 	return -1;
 }
 
-int parse_events_add_cache(struct list_head **list, int *idx,
+int parse_events_add_cache(struct list_head *list, int *idx,
 			   char *type, char *op_result1, char *op_result2)
 {
 	struct perf_event_attr attr;

@@ -379,31 +368,21 @@ int parse_events_add_cache(struct list_head **list, int *idx,
 	return add_event(list, idx, &attr, name);
 }
 
-static int add_tracepoint(struct list_head **listp, int *idx,
+static int add_tracepoint(struct list_head *list, int *idx,
 			  char *sys_name, char *evt_name)
 {
 	struct perf_evsel *evsel;
-	struct list_head *list = *listp;
-
-	if (!list) {
-		list = malloc(sizeof(*list));
-		if (!list)
-			return -ENOMEM;
-		INIT_LIST_HEAD(list);
-	}
 
 	evsel = perf_evsel__newtp(sys_name, evt_name, (*idx)++);
-	if (!evsel) {
-		free(list);
+	if (!evsel)
 		return -ENOMEM;
-	}
 
 	list_add_tail(&evsel->node, list);
-	*listp = list;
+
 	return 0;
 }
 
-static int add_tracepoint_multi_event(struct list_head **list, int *idx,
+static int add_tracepoint_multi_event(struct list_head *list, int *idx,
 				      char *sys_name, char *evt_name)
 {
 	char evt_path[MAXPATHLEN];

@@ -435,7 +414,7 @@ static int add_tracepoint_multi_event(struct list_head **list, int *idx,
 	return ret;
 }
 
-static int add_tracepoint_event(struct list_head **list, int *idx,
+static int add_tracepoint_event(struct list_head *list, int *idx,
 				char *sys_name, char *evt_name)
 {
 	return strpbrk(evt_name, "*?") ?

@@ -443,7 +422,7 @@ static int add_tracepoint_event(struct list_head **list, int *idx,
 	       add_tracepoint(list, idx, sys_name, evt_name);
 }
 
-static int add_tracepoint_multi_sys(struct list_head **list, int *idx,
+static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
 				    char *sys_name, char *evt_name)
 {
 	struct dirent *events_ent;

@@ -475,7 +454,7 @@ static int add_tracepoint_multi_sys(struct list_head **list, int *idx,
 	return ret;
 }
 
-int parse_events_add_tracepoint(struct list_head **list, int *idx,
+int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				char *sys, char *event)
 {
 	int ret;

@@ -530,7 +509,7 @@ do {					\
 	return 0;
 }
 
-int parse_events_add_breakpoint(struct list_head **list, int *idx,
+int parse_events_add_breakpoint(struct list_head *list, int *idx,
 				void *ptr, char *type)
 {
 	struct perf_event_attr attr;

@@ -611,7 +590,7 @@ static int config_attr(struct perf_event_attr *attr,
 	return 0;
 }
 
-int parse_events_add_numeric(struct list_head **list, int *idx,
+int parse_events_add_numeric(struct list_head *list, int *idx,
 			     u32 type, u64 config,
 			     struct list_head *head_config)
 {

@@ -644,7 +623,7 @@ static char *pmu_event_name(struct list_head *head_terms)
 	return NULL;
 }
 
-int parse_events_add_pmu(struct list_head **list, int *idx,
+int parse_events_add_pmu(struct list_head *list, int *idx,
 			 char *name, struct list_head *head_config)
 {
 	struct perf_event_attr attr;

@@ -687,6 +666,7 @@ void parse_events__set_leader(char *name, struct list_head *list)
 	leader->group_name = name ? strdup(name) : NULL;
 }
 
+/* list_event is assumed to point to malloc'ed memory */
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all)
 {
tools/perf/util/parse-events.h +5 -5
index 080f7cf..f1cb4c4 100644
@@ -85,16 +85,16 @@ void parse_events__free_terms(struct list_head *terms);
 int parse_events__modifier_event(struct list_head *list, char *str, bool add);
 int parse_events__modifier_group(struct list_head *list, char *event_mod);
 int parse_events_name(struct list_head *list, char *name);
-int parse_events_add_tracepoint(struct list_head **list, int *idx,
+int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				char *sys, char *event);
-int parse_events_add_numeric(struct list_head **list, int *idx,
+int parse_events_add_numeric(struct list_head *list, int *idx,
 			     u32 type, u64 config,
 			     struct list_head *head_config);
-int parse_events_add_cache(struct list_head **list, int *idx,
+int parse_events_add_cache(struct list_head *list, int *idx,
 			   char *type, char *op_result1, char *op_result2);
-int parse_events_add_breakpoint(struct list_head **list, int *idx,
+int parse_events_add_breakpoint(struct list_head *list, int *idx,
 				void *ptr, char *type);
-int parse_events_add_pmu(struct list_head **list, int *idx,
+int parse_events_add_pmu(struct list_head *list, int *idx,
 			 char *pmu , struct list_head *head_config);
 void parse_events__set_leader(char *name, struct list_head *list);
 void parse_events_update_lists(struct list_head *list_event,
tools/perf/util/parse-events.y +40 -22

149 lines not on this page · raw message