RE: [PATCH 3/4] perf annotate: add powerpc support

[David Laight]

From: Ravi Bangoria
> Sent: 28 June 2016 12:37
> 
> Powerpc has long list of branch instructions and hardcoding them in table
> appears to be error-prone. So, add new function to find instruction
> instead of creating table.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx>
> ---
>  tools/perf/util/annotate.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 36a5825..96c6610 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -476,6 +476,68 @@ static int ins__cmp(const void *a, const void *b)
>  	return strcmp(ia->name, ib->name);
>  }
> 
> +static struct ins *ins__find_powerpc(const char *name)

It would be better if the function name include 'branch'.

> +{
> +	int i;
> +	struct ins *ins;
> +
> +	ins = zalloc(sizeof(struct ins));
> +	if (!ins)
> +		return NULL;
> +
> +	ins->name = strdup(name);
> +	if (!ins->name)
> +		return NULL;

	You leak 'ins' here.

> +
> +	if (name[0] == 'b') {
> +		/* branch instructions */
> +		ins->ops = &jump_ops;
> +
> +		/*
> +		 * - Few start with 'b', but aren't branch instructions.
> +		 * - Let's also ignore instructions involving 'ctr' and
> +		 *   'tar' since target branch addresses for those can't
> +		 *   be determined statically.
> +		 */
> +		if (!strncmp(name, "bcd", 3)   ||
> +		    !strncmp(name, "brinc", 5) ||
> +		    !strncmp(name, "bper", 4)  ||
> +		    strstr(name, "ctr")        ||
> +		    strstr(name, "tar"))
> +			return NULL;

	More importantly you leak 'ins' and 'ins->name' here.
	And on other paths below.

...

	David