[PATCHSET 0/7] perf tools: Check error during collapsing hist entries

[PATCHSET 0/7] perf tools: Check error during collapsing hist entries

[Namhyung Kim]

Hi,

This patchset checks error case during the process of collapsing hist
entries.  It's a preparation of upcoming hierarchy patchset which adds
more work in the collapsing path.  If there's an error during this
stage, it'll stop processing and show warning to user.

Thanks,
Namhyung


Namhyung Kim (7):
  perf callchain: Check return value of add_child()
  perf callchain: Check return value of fill_node()
  perf callchain: Add enum match_result for match_chain()
  perf callchain: Check return value of split_add_child()
  perf callchain: Check return value of append_chain_children()
  perf hists: Return error from hists__collapse_resort()
  perf report: Check error during report__collapse_hists()

 tools/perf/builtin-report.c | 14 +++++--
 tools/perf/util/callchain.c | 94 +++++++++++++++++++++++++++++++++------------
 tools/perf/util/hist.c      | 27 ++++++++-----
 tools/perf/util/hist.h      |  4 +-
 4 files changed, 100 insertions(+), 39 deletions(-)

-- 
2.6.4

[PATCH 1/7] perf callchain: Check return value of add_child()

[Namhyung Kim]

The create_child() in add_child() can return NULL in case of memory
allocation failure.  So check the return value and bail out.  The proper
error handling will be added later.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 53c43eb9489e..134d88b33fc1 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -453,6 +453,9 @@ add_child(struct callchain_node *parent,
 	struct callchain_node *new;
 
 	new = create_child(parent, false);
+	if (new == NULL)
+		return NULL;
+
 	fill_node(new, cursor);
 
 	new->children_hit = 0;
@@ -524,6 +527,8 @@ split_add_child(struct callchain_node *parent,
 
 		node = callchain_cursor_current(cursor);
 		new = add_child(parent, cursor, period);
+		if (new == NULL)
+			return;
 
 		/*
 		 * This is second child since we moved parent's children
@@ -585,6 +590,9 @@ append_chain_children(struct callchain_node *root,
 	}
 	/* nothing in children, add to the current node */
 	rnode = add_child(root, cursor, period);
+	if (rnode == NULL)
+		return;
+
 	rb_link_node(&rnode->rb_node_in, parent, p);
 	rb_insert_color(&rnode->rb_node_in, &root->rb_root_in);
 
-- 
2.6.4

[PATCH 2/7] perf callchain: Check return value of fill_node()

[Namhyung Kim]

Memory allocation in the fill_node() can fail so change its return type
to int and check it in add_child() too.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 134d88b33fc1..a82ea6f6fc0f 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -416,7 +416,7 @@ create_child(struct callchain_node *parent, bool inherit_children)
 /*
  * Fill the node with callchain values
  */
-static void
+static int
 fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 {
 	struct callchain_cursor_node *cursor_node;
@@ -433,7 +433,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 		call = zalloc(sizeof(*call));
 		if (!call) {
 			perror("not enough memory for the code path tree");
-			return;
+			return -1;
 		}
 		call->ip = cursor_node->ip;
 		call->ms.sym = cursor_node->sym;
@@ -443,6 +443,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 		callchain_cursor_advance(cursor);
 		cursor_node = callchain_cursor_current(cursor);
 	}
+	return 0;
 }
 
 static struct callchain_node *
@@ -456,7 +457,16 @@ add_child(struct callchain_node *parent,
 	if (new == NULL)
 		return NULL;
 
-	fill_node(new, cursor);
+	if (fill_node(new, cursor) < 0) {
+		struct callchain_list *call, *tmp;
+
+		list_for_each_entry_safe(call, tmp, &new->val, list) {
+			list_del(&call->list);
+			free(call);
+		}
+		free(new);
+		return NULL;
+	}
 
 	new->children_hit = 0;
 	new->hit = period;
-- 
2.6.4

[PATCH 3/7] perf callchain: Add enum match_result for match_chain()

[Namhyung Kim]

The append_chain() might return either result of match_chain() or
other (error) code.  But match_chain() can return any value in s64 type
so it's hard to check the error case.  Add new enum match_result and
make match_chain() return non-negative values only so that we can check
the error cases.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index a82ea6f6fc0f..7139d438ee6d 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -475,16 +475,32 @@ add_child(struct callchain_node *parent,
 	return new;
 }
 
+enum match_result {
+	MATCH_ERROR  = -1,
+	MATCH_EQ,
+	MATCH_LT,
+	MATCH_GT,
+};
+
 static s64 match_chain(struct callchain_cursor_node *node,
 		      struct callchain_list *cnode)
 {
 	struct symbol *sym = node->sym;
+	u64 left, right;
 
 	if (cnode->ms.sym && sym &&
-	    callchain_param.key == CCKEY_FUNCTION)
-		return cnode->ms.sym->start - sym->start;
-	else
-		return cnode->ip - node->ip;
+	    callchain_param.key == CCKEY_FUNCTION) {
+		left = cnode->ms.sym->start;
+		right = sym->start;
+	} else {
+		left = cnode->ip;
+		right = node->ip;
+	}
+
+	if (left == right)
+		return MATCH_EQ;
+
+	return left > right ? MATCH_GT : MATCH_LT;
 }
 
 /*
@@ -562,7 +578,7 @@ split_add_child(struct callchain_node *parent,
 	}
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period);
@@ -583,17 +599,17 @@ append_chain_children(struct callchain_node *root,
 
 	/* lookup in childrens */
 	while (*p) {
-		s64 ret;
+		enum match_result ret;
 
 		parent = *p;
 		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
 
 		/* If at least first entry matches, rely to children */
 		ret = append_chain(rnode, cursor, period);
-		if (ret == 0)
+		if (ret == MATCH_EQ)
 			goto inc_children_hit;
 
-		if (ret < 0)
+		if (ret == MATCH_LT)
 			p = &parent->rb_left;
 		else
 			p = &parent->rb_right;
@@ -611,7 +627,7 @@ inc_children_hit:
 	root->children_count++;
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period)
@@ -620,7 +636,7 @@ append_chain(struct callchain_node *root,
 	u64 start = cursor->pos;
 	bool found = false;
 	u64 matches;
-	int cmp = 0;
+	enum match_result cmp = MATCH_ERROR;
 
 	/*
 	 * Lookup in the current node
@@ -646,7 +662,7 @@ append_chain(struct callchain_node *root,
 
 	/* matches not, relay no the parent */
 	if (!found) {
-		WARN_ONCE(!cmp, "Chain comparison error\n");
+		WARN_ONCE(cmp == MATCH_ERROR, "Chain comparison error\n");
 		return cmp;
 	}
 
@@ -655,20 +671,20 @@ append_chain(struct callchain_node *root,
 	/* we match only a part of the node. Split it and add the new chain */
 	if (matches < root->val_nr) {
 		split_add_child(root, cursor, cnode, start, matches, period);
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* we match 100% of the path, increment the hit */
 	if (matches == root->val_nr && cursor->pos == cursor->nr) {
 		root->hit += period;
 		root->count++;
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* We match the node and still have a part remaining */
 	append_chain_children(root, cursor, period);
 
-	return 0;
+	return MATCH_EQ;
 }
 
 int callchain_append(struct callchain_root *root,
-- 
2.6.4

[PATCH 4/7] perf callchain: Check return value of split_add_child()

[Namhyung Kim]

Now create_child() and add_child() return errors so check and pass it
to the caller.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 7139d438ee6d..fc980323539e 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -508,7 +508,7 @@ static s64 match_chain(struct callchain_cursor_node *node,
  * give a part of its callchain to the created child.
  * Then create another child to host the given callchain of new branch
  */
-static void
+static int
 split_add_child(struct callchain_node *parent,
 		struct callchain_cursor *cursor,
 		struct callchain_list *to_split,
@@ -520,6 +520,8 @@ split_add_child(struct callchain_node *parent,
 
 	/* split */
 	new = create_child(parent, true);
+	if (new == NULL)
+		return -1;
 
 	/* split the callchain and move a part to the new child */
 	old_tail = parent->val.prev;
@@ -554,7 +556,7 @@ split_add_child(struct callchain_node *parent,
 		node = callchain_cursor_current(cursor);
 		new = add_child(parent, cursor, period);
 		if (new == NULL)
-			return;
+			return -1;
 
 		/*
 		 * This is second child since we moved parent's children
@@ -576,6 +578,7 @@ split_add_child(struct callchain_node *parent,
 		parent->hit = period;
 		parent->count = 1;
 	}
+	return 0;
 }
 
 static enum match_result
@@ -670,7 +673,10 @@ append_chain(struct callchain_node *root,
 
 	/* we match only a part of the node. Split it and add the new chain */
 	if (matches < root->val_nr) {
-		split_add_child(root, cursor, cnode, start, matches, period);
+		if (split_add_child(root, cursor, cnode, start, matches,
+				    period) < 0)
+			return MATCH_ERROR;
+
 		return MATCH_EQ;
 	}
 
-- 
2.6.4

[PATCH 5/7] perf callchain: Check return value of append_chain_children()

[Namhyung Kim]

Now it can check the error case, so check and pass it to the caller.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index fc980323539e..1d97ea6d5d30 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -586,7 +586,7 @@ append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period);
 
-static void
+static int
 append_chain_children(struct callchain_node *root,
 		      struct callchain_cursor *cursor,
 		      u64 period)
@@ -598,7 +598,7 @@ append_chain_children(struct callchain_node *root,
 
 	node = callchain_cursor_current(cursor);
 	if (!node)
-		return;
+		return -1;
 
 	/* lookup in childrens */
 	while (*p) {
@@ -611,6 +611,8 @@ append_chain_children(struct callchain_node *root,
 		ret = append_chain(rnode, cursor, period);
 		if (ret == MATCH_EQ)
 			goto inc_children_hit;
+		if (ret == MATCH_ERROR)
+			return -1;
 
 		if (ret == MATCH_LT)
 			p = &parent->rb_left;
@@ -620,7 +622,7 @@ append_chain_children(struct callchain_node *root,
 	/* nothing in children, add to the current node */
 	rnode = add_child(root, cursor, period);
 	if (rnode == NULL)
-		return;
+		return -1;
 
 	rb_link_node(&rnode->rb_node_in, parent, p);
 	rb_insert_color(&rnode->rb_node_in, &root->rb_root_in);
@@ -628,6 +630,7 @@ append_chain_children(struct callchain_node *root,
 inc_children_hit:
 	root->children_hit += period;
 	root->children_count++;
+	return 0;
 }
 
 static enum match_result
@@ -688,7 +691,8 @@ append_chain(struct callchain_node *root,
 	}
 
 	/* We match the node and still have a part remaining */
-	append_chain_children(root, cursor, period);
+	if (append_chain_children(root, cursor, period) < 0)
+		return MATCH_ERROR;
 
 	return MATCH_EQ;
 }
@@ -702,7 +706,8 @@ int callchain_append(struct callchain_root *root,
 
 	callchain_cursor_commit(cursor);
 
-	append_chain_children(&root->node, cursor, period);
+	if (append_chain_children(&root->node, cursor, period) < 0)
+		return -1;
 
 	if (cursor->nr > root->max_depth)
 		root->max_depth = cursor->nr;
@@ -730,7 +735,8 @@ merge_chain_branch(struct callchain_cursor *cursor,
 
 	if (src->hit) {
 		callchain_cursor_commit(cursor);
-		append_chain_children(dst, cursor, src->hit);
+		if (append_chain_children(dst, cursor, src->hit) < 0)
+			return -1;
 	}
 
 	n = rb_first(&src->rb_root_in);
-- 
2.6.4

[PATCH 6/7] perf hists: Return error from hists__collapse_resort()

[Namhyung Kim]

Currently hists__collapse_resort() and hists__collapse_insert_entry()
don't return error code. Now callchain_merge() can check error case,
abort and pass the error to the user.  Later patch can add more work
which can be failed too.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 27 +++++++++++++++++----------
 tools/perf/util/hist.h |  4 ++--
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 81ce0aff69d1..89fde7feb88b 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1009,8 +1009,8 @@ void hist_entry__delete(struct hist_entry *he)
  * collapse the histogram
  */
 
-bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
-				  struct rb_root *root, struct hist_entry *he)
+int hists__collapse_insert_entry(struct hists *hists, struct rb_root *root,
+				 struct hist_entry *he)
 {
 	struct rb_node **p = &root->rb_node;
 	struct rb_node *parent = NULL;
@@ -1030,12 +1030,13 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 
 			if (symbol_conf.use_callchain) {
 				callchain_cursor_reset(&callchain_cursor);
-				callchain_merge(&callchain_cursor,
-						iter->callchain,
-						he->callchain);
+				if (callchain_merge(&callchain_cursor,
+						    iter->callchain,
+						    he->callchain) < 0)
+					return -1;
 			}
 			hist_entry__delete(he);
-			return false;
+			return 0;
 		}
 
 		if (cmp < 0)
@@ -1047,7 +1048,7 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 
 	rb_link_node(&he->rb_node_in, parent, p);
 	rb_insert_color(&he->rb_node_in, root);
-	return true;
+	return 1;
 }
 
 struct rb_root *hists__get_rotate_entries_in(struct hists *hists)
@@ -1073,14 +1074,15 @@ static void hists__apply_filters(struct hists *hists, struct hist_entry *he)
 	hists__filter_entry_by_socket(hists, he);
 }
 
-void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
+int hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
 {
 	struct rb_root *root;
 	struct rb_node *next;
 	struct hist_entry *n;
+	int ret;
 
 	if (!sort__need_collapse)
-		return;
+		return 0;
 
 	hists->nr_entries = 0;
 
@@ -1095,7 +1097,11 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
 		next = rb_next(&n->rb_node_in);
 
 		rb_erase(&n->rb_node_in, root);
-		if (hists__collapse_insert_entry(hists, &hists->entries_collapsed, n)) {
+		ret = hists__collapse_insert_entry(hists, &hists->entries_collapsed, n);
+		if (ret < 0)
+			return -1;
+
+		if (ret) {
 			/*
 			 * If it wasn't combined with one of the entries already
 			 * collapsed, we need to apply the filters that may have
@@ -1106,6 +1112,7 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
 		if (prog)
 			ui_progress__update(prog, 1);
 	}
+	return 0;
 }
 
 static int hist_entry__sort(struct hist_entry *a, struct hist_entry *b)
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index d4ec4822a103..9c72f8331d1c 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -129,7 +129,7 @@ int hist_entry__sort_snprintf(struct hist_entry *he, char *bf, size_t size,
 void hist_entry__delete(struct hist_entry *he);
 
 void hists__output_resort(struct hists *hists, struct ui_progress *prog);
-void hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
+int hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
 
 void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);
 void hists__delete_entries(struct hists *hists);
@@ -188,7 +188,7 @@ int hists__init(void);
 int __hists__init(struct hists *hists);
 
 struct rb_root *hists__get_rotate_entries_in(struct hists *hists);
-bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
+int hists__collapse_insert_entry(struct hists *hists,
 				  struct rb_root *root, struct hist_entry *he);
 
 struct perf_hpp {
-- 
2.6.4

[PATCH 7/7] perf report: Check error during report__collapse_hists()

[Namhyung Kim]

If it returns an error, warn user and bail out instead of silently
ignoring.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2bf537f190a0..7b933a9cf84f 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -466,10 +466,11 @@ static int report__browse_hists(struct report *rep)
 	return ret;
 }
 
-static void report__collapse_hists(struct report *rep)
+static int report__collapse_hists(struct report *rep)
 {
 	struct ui_progress prog;
 	struct perf_evsel *pos;
+	int ret = 0;
 
 	ui_progress__init(&prog, rep->nr_entries, "Merging related events...");
 
@@ -481,7 +482,9 @@ static void report__collapse_hists(struct report *rep)
 
 		hists->socket_filter = rep->socket_filter;
 
-		hists__collapse_resort(hists, &prog);
+		ret = hists__collapse_resort(hists, &prog);
+		if (ret < 0)
+			break;
 
 		/* Non-group events are considered as leader */
 		if (symbol_conf.event_group &&
@@ -494,6 +497,7 @@ static void report__collapse_hists(struct report *rep)
 	}
 
 	ui_progress__finish();
+	return ret;
 }
 
 static void report__output_resort(struct report *rep)
@@ -561,7 +565,11 @@ static int __cmd_report(struct report *rep)
 		}
 	}
 
-	report__collapse_hists(rep);
+	ret = report__collapse_hists(rep);
+	if (ret) {
+		ui__error("failed to process hist entry\n");
+		return ret;
+	}
 
 	if (session_done())
 		return 0;
-- 
2.6.4

Re: [PATCH 3/7] perf callchain: Add enum match_result for match_chain()

[Jiri Olsa]

On Fri, Jan 22, 2016 at 10:41:36PM +0900, Namhyung Kim wrote:

SNIP

>  	/* lookup in childrens */
>  	while (*p) {
> -		s64 ret;
> +		enum match_result ret;
>  
>  		parent = *p;
>  		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
>  
>  		/* If at least first entry matches, rely to children */
>  		ret = append_chain(rnode, cursor, period);
> -		if (ret == 0)
> +		if (ret == MATCH_EQ)
>  			goto inc_children_hit;
>  
> -		if (ret < 0)
> +		if (ret == MATCH_LT)
>  			p = &parent->rb_left;
>  		else
>  			p = &parent->rb_right;

so if we want to use the return values like that you
probably missed 2 other places

thanks,
jirka


---
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 7139d438ee6d..dc08e76aa8d9 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -565,7 +565,7 @@ split_add_child(struct callchain_node *parent,
 		cnode = list_first_entry(&first->val, struct callchain_list,
 					 list);
 
-		if (match_chain(node, cnode) < 0)
+		if (match_chain(node, cnode) == MATCH_LT)
 			pp = &p->rb_left;
 		else
 			pp = &p->rb_right;
@@ -652,7 +652,7 @@ append_chain(struct callchain_node *root,
 			break;
 
 		cmp = match_chain(node, cnode);
-		if (cmp)
+		if (cmp != MATCH_EQ)
 			break;
 
 		found = true;

Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist entries

[Jiri Olsa]

On Fri, Jan 22, 2016 at 10:41:33PM +0900, Namhyung Kim wrote:
> Hi,
> 
> This patchset checks error case during the process of collapsing hist
> entries.  It's a preparation of upcoming hierarchy patchset which adds
> more work in the collapsing path.  If there's an error during this
> stage, it'll stop processing and show warning to user.
> 
> Thanks,
> Namhyung
> 
> 
> Namhyung Kim (7):
>   perf callchain: Check return value of add_child()
>   perf callchain: Check return value of fill_node()
>   perf callchain: Add enum match_result for match_chain()
>   perf callchain: Check return value of split_add_child()
>   perf callchain: Check return value of append_chain_children()
>   perf hists: Return error from hists__collapse_resort()
>   perf report: Check error during report__collapse_hists()

I saw 2 other functions allocating memory and not checked:
  callchain_cursor_append
  callchain_node__make_parent_list 

thanks,
jirka

Re: [PATCH 3/7] perf callchain: Add enum match_result for match_chain()

[Namhyung Kim]

Hi Jiri,

On Sat, Jan 23, 2016 at 06:01:10PM +0100, Jiri Olsa wrote:
> On Fri, Jan 22, 2016 at 10:41:36PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> >  	/* lookup in childrens */
> >  	while (*p) {
> > -		s64 ret;
> > +		enum match_result ret;
> >  
> >  		parent = *p;
> >  		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
> >  
> >  		/* If at least first entry matches, rely to children */
> >  		ret = append_chain(rnode, cursor, period);
> > -		if (ret == 0)
> > +		if (ret == MATCH_EQ)
> >  			goto inc_children_hit;
> >  
> > -		if (ret < 0)
> > +		if (ret == MATCH_LT)
> >  			p = &parent->rb_left;
> >  		else
> >  			p = &parent->rb_right;
> 
> so if we want to use the return values like that you
> probably missed 2 other places

Right!

> 
> 
> ---
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 7139d438ee6d..dc08e76aa8d9 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -565,7 +565,7 @@ split_add_child(struct callchain_node *parent,
>  		cnode = list_first_entry(&first->val, struct callchain_list,
>  					 list);
>  
> -		if (match_chain(node, cnode) < 0)
> +		if (match_chain(node, cnode) == MATCH_LT)
>  			pp = &p->rb_left;
>  		else
>  			pp = &p->rb_right;
> @@ -652,7 +652,7 @@ append_chain(struct callchain_node *root,
>  			break;
>  
>  		cmp = match_chain(node, cnode);
> -		if (cmp)
> +		if (cmp != MATCH_EQ)

This has same effect since I chose 0 for MATCH_EQ intentionally.  But
yes, it'd be better making it explicit.  Will change.

Thanks,
Namhyung


>  			break;
>  
>  		found = true;

Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist entries

[Namhyung Kim]

On Sat, Jan 23, 2016 at 06:01:21PM +0100, Jiri Olsa wrote:
> On Fri, Jan 22, 2016 at 10:41:33PM +0900, Namhyung Kim wrote:
> > Hi,
> > 
> > This patchset checks error case during the process of collapsing hist
> > entries.  It's a preparation of upcoming hierarchy patchset which adds
> > more work in the collapsing path.  If there's an error during this
> > stage, it'll stop processing and show warning to user.
> > 
> > Thanks,
> > Namhyung
> > 
> > 
> > Namhyung Kim (7):
> >   perf callchain: Check return value of add_child()
> >   perf callchain: Check return value of fill_node()
> >   perf callchain: Add enum match_result for match_chain()
> >   perf callchain: Check return value of split_add_child()
> >   perf callchain: Check return value of append_chain_children()
> >   perf hists: Return error from hists__collapse_resort()
> >   perf report: Check error during report__collapse_hists()
> 
> I saw 2 other functions allocating memory and not checked:
>   callchain_cursor_append

Ok, but this function is basically for the 'addition' path.  Well it's
also used by the 'collapsing' path but it never allocates new node
since it reuses the existing ones.  I'll prepare a different patchset
for the 'addition' path later..


>   callchain_node__make_parent_list

It seems not called in the collapsing path.  It should be handled by
a separate patchset.

Thanks,
Namhyung

[PATCH v2 3/7] perf callchain: Add enum match_result for match_chain()

[Namhyung Kim]

The append_chain() might return either result of match_chain() or
other (error) code.  But match_chain() can return any value in s64 type
so it's hard to check the error case.  Add new enum match_result and
make match_chain() return non-negative values only so that we can check
the error cases.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 52 +++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index a82ea6f6fc0f..dab2c1f1e86b 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -475,16 +475,32 @@ add_child(struct callchain_node *parent,
 	return new;
 }
 
-static s64 match_chain(struct callchain_cursor_node *node,
-		      struct callchain_list *cnode)
+enum match_result {
+	MATCH_ERROR  = -1,
+	MATCH_EQ,
+	MATCH_LT,
+	MATCH_GT,
+};
+
+static enum match_result match_chain(struct callchain_cursor_node *node,
+				     struct callchain_list *cnode)
 {
 	struct symbol *sym = node->sym;
+	u64 left, right;
 
 	if (cnode->ms.sym && sym &&
-	    callchain_param.key == CCKEY_FUNCTION)
-		return cnode->ms.sym->start - sym->start;
-	else
-		return cnode->ip - node->ip;
+	    callchain_param.key == CCKEY_FUNCTION) {
+		left = cnode->ms.sym->start;
+		right = sym->start;
+	} else {
+		left = cnode->ip;
+		right = node->ip;
+	}
+
+	if (left == right)
+		return MATCH_EQ;
+
+	return left > right ? MATCH_GT : MATCH_LT;
 }
 
 /*
@@ -549,7 +565,7 @@ split_add_child(struct callchain_node *parent,
 		cnode = list_first_entry(&first->val, struct callchain_list,
 					 list);
 
-		if (match_chain(node, cnode) < 0)
+		if (match_chain(node, cnode) == MATCH_LT)
 			pp = &p->rb_left;
 		else
 			pp = &p->rb_right;
@@ -562,7 +578,7 @@ split_add_child(struct callchain_node *parent,
 	}
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period);
@@ -583,17 +599,17 @@ append_chain_children(struct callchain_node *root,
 
 	/* lookup in childrens */
 	while (*p) {
-		s64 ret;
+		enum match_result ret;
 
 		parent = *p;
 		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
 
 		/* If at least first entry matches, rely to children */
 		ret = append_chain(rnode, cursor, period);
-		if (ret == 0)
+		if (ret == MATCH_EQ)
 			goto inc_children_hit;
 
-		if (ret < 0)
+		if (ret == MATCH_LT)
 			p = &parent->rb_left;
 		else
 			p = &parent->rb_right;
@@ -611,7 +627,7 @@ inc_children_hit:
 	root->children_count++;
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period)
@@ -620,7 +636,7 @@ append_chain(struct callchain_node *root,
 	u64 start = cursor->pos;
 	bool found = false;
 	u64 matches;
-	int cmp = 0;
+	enum match_result cmp = MATCH_ERROR;
 
 	/*
 	 * Lookup in the current node
@@ -636,7 +652,7 @@ append_chain(struct callchain_node *root,
 			break;
 
 		cmp = match_chain(node, cnode);
-		if (cmp)
+		if (cmp != MATCH_EQ)
 			break;
 
 		found = true;
@@ -646,7 +662,7 @@ append_chain(struct callchain_node *root,
 
 	/* matches not, relay no the parent */
 	if (!found) {
-		WARN_ONCE(!cmp, "Chain comparison error\n");
+		WARN_ONCE(cmp == MATCH_ERROR, "Chain comparison error\n");
 		return cmp;
 	}
 
@@ -655,20 +671,20 @@ append_chain(struct callchain_node *root,
 	/* we match only a part of the node. Split it and add the new chain */
 	if (matches < root->val_nr) {
 		split_add_child(root, cursor, cnode, start, matches, period);
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* we match 100% of the path, increment the hit */
 	if (matches == root->val_nr && cursor->pos == cursor->nr) {
 		root->hit += period;
 		root->count++;
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* We match the node and still have a part remaining */
 	append_chain_children(root, cursor, period);
 
-	return 0;
+	return MATCH_EQ;
 }
 
 int callchain_append(struct callchain_root *root,
-- 
2.6.4

Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist entries

[Jiri Olsa]

On Sun, Jan 24, 2016 at 01:37:55PM +0900, Namhyung Kim wrote:
> On Sat, Jan 23, 2016 at 06:01:21PM +0100, Jiri Olsa wrote:
> > On Fri, Jan 22, 2016 at 10:41:33PM +0900, Namhyung Kim wrote:
> > > Hi,
> > > 
> > > This patchset checks error case during the process of collapsing hist
> > > entries.  It's a preparation of upcoming hierarchy patchset which adds
> > > more work in the collapsing path.  If there's an error during this
> > > stage, it'll stop processing and show warning to user.
> > > 
> > > Thanks,
> > > Namhyung
> > > 
> > > 
> > > Namhyung Kim (7):
> > >   perf callchain: Check return value of add_child()
> > >   perf callchain: Check return value of fill_node()
> > >   perf callchain: Add enum match_result for match_chain()
> > >   perf callchain: Check return value of split_add_child()
> > >   perf callchain: Check return value of append_chain_children()
> > >   perf hists: Return error from hists__collapse_resort()
> > >   perf report: Check error during report__collapse_hists()
> > 
> > I saw 2 other functions allocating memory and not checked:
> >   callchain_cursor_append
> 
> Ok, but this function is basically for the 'addition' path.  Well it's
> also used by the 'collapsing' path but it never allocates new node
> since it reuses the existing ones.  I'll prepare a different patchset
> for the 'addition' path later..
> 
> 
> >   callchain_node__make_parent_list
> 
> It seems not called in the collapsing path.  It should be handled by
> a separate patchset.

ok, with the v2 for patch 3 

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka