[PATCH 0/1] remap-log: fix map generator

[PATCH 0/1] remap-log: fix map generator

[Artem Bityutskiy]

Hi Al,

sorry for re-sending, but I've decided to CC LKML this time because there
may be other people interested in this.

We have found your remap-log tool [1] and started using it - it is very
useful and thank you for sharing it.

I have found a bug in this tool though. I am providing two patches which fix
the bug, but differently. I prefer the first one though.

The code of remap-log is a bit tricky and I am not 100% sure my fixes are
correct. Could you please take a look?

Should we try to make remap-log be part of the kernel? I think linux-next
could use it to compare build logs and bug people about new warnings.

Or should we setup a separate git tree for this tool?

[1] http://thread.gmane.org/gmane.linux.kernel/409379

--
Best Regards,
Artem Bityutskiy

[PATCH 1/1 (variant 1)] remap-log: fix map generator

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This patch fixes a bug in the 'parse_diff()' function. This function iterates
over every changed file and every hunk in this file and at each iteration it
prints the hunk's map for the _previous_ iteration. This is taken into account
when it switches to the next file - just before starting processing hunks in
this file, it prints the map for the last hunk of the _previous_ file.

However, if we are at the very last file or we have a diff which changes only
one single file, and the last hunk is processed - we do not print the map of
this last hunk and just exit. The result is a buggy map - because the map of
the very last hunk is not printed.

Here is a short example to demonstrate the bug. The diff is:

|||diff --git a/sound/soc/codecs/sn95031.c b/sound/soc/codecs/sn95031.c
|||index 920e7bb..8c43fdd 100644
|||--- a/sound/soc/codecs/sn95031.c
|||+++ b/sound/soc/codecs/sn95031.c
|||@@ -811,0 +812 @@ static int sn95031_pcm_hs_mute(struct snd_soc_dai *dai, int mute)
|||+       int tmp = !mute;
|||@@ -813 +814 @@ static int sn95031_pcm_hs_mute(struct snd_soc_dai *dai, int mute)
|||-                       SN95031_HSLVOLCTRL, BIT(7), (!mute << 7));
|||+                       SN95031_HSLVOLCTRL, BIT(7), tmp << 7);
|||@@ -815 +816,3 @@ static int sn95031_pcm_hs_mute(struct snd_soc_dai *dai, int mute)
|||-                       SN95031_HSRVOLCTRL, BIT(7), (!mute << 7));
|||+                       SN95031_HSRVOLCTRL, BIT(7), tmp << 7);
|||+       snd_soc_update_bits(dai->codec,
|||+                       SN95031_DRIVEREN, BIT(0) | BIT(1), tmp | tmp << 1);

And remap-log generates an incorrect map:

M sound/soc/codecs/sn95031.c sound/soc/codecs/sn95031.c
1 1
812 813
813 0
814 815
815 0

Notice that the last map entry basically says that all lines in the old file
starting from line 815 have been deleted, which is not true - only one line
number 815 has been deleted.

The correct map should look like this:

M sound/soc/codecs/sn95031.c sound/soc/codecs/sn95031.c
1 1
812 813
813 0
814 815
815 0
816 819

To fix the bug this patch re-works the 'parse_diff()' function a little and
makes it print the map of the current hunk at each iteration, not the previous
hunk.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 tools/remap-log.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/tools/remap-log.c b/tools/remap-log.c
index 327b870..011a7cb 100644
--- a/tools/remap-log.c
+++ b/tools/remap-log.c
@@ -331,7 +331,6 @@ void parse_diff(void)
 {
 	int skipping = -1, suppress = 1;
 	char *name1 = NULL, *name2 = NULL;
-	int from = 1, to = 1;
 	int l1, l2, n1, n2;
 	enum cmd {
 		Diff, Hunk, New, Del, Copy, Rename, Junk
@@ -367,21 +366,15 @@ void parse_diff(void)
 					printf("M %s %s\n", name1, name2);
 				if (!parse_hunk(&l1, &l2, &n1, &n2))
 					goto Ediff;
-				if (l1 > from)
-					printf("%d %d\n", from, to);
+				if (!skipping && l1 > 1)
+					printf("1 1\n");
 				if (n1)
 					printf("%d 0\n", l1);
-				from = l1 + n1;
-				to = l2 + n2;
+				printf("%d %d\n", l1 + n1, l2 + n2);
 			}
 			skipping = 1;
 			break;
 		case Diff:
-			if (!suppress) {
-				if (!skipping)
-					printf("M %s %s\n", name1, name2);
-				printf("%d %d\n", from, to);
-			}
 			free(name1);
 			free(name2);
 			name2 = strrchr(line, ' ');
@@ -401,7 +394,6 @@ void parse_diff(void)
 				goto Ediff;
 			skipping = 0;
 			suppress = 0;
-			from = to = 1;
 			break;
 		case New:
 			if (skipping)
-- 
1.7.7.1

[PATCH 1/1 (variant 2)] remap-log: fix map generator

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

This patch fixes a bug in the 'parse_diff()' function. This function iterates
over every changed file and every hunk in this file and at each iteration it
prints the hunk's map for the _previous_ iteration. This is taken into account
when it switches to the next file - just before starting processing hunks in
this file, it prints the map for the last hunk of the _previous_ file.

However, if we are at the very last file or we have a diff which changes only
one single file, and the last hunk is processed - we do not print the map of
this last hunk and just exit. The result is a buggy map - because the map of
the very last hunk is not printed.

Here is a short example to demonstrate the bug. The diff is:

|||diff --git a/sound/soc/codecs/sn95031.c b/sound/soc/codecs/sn95031.c
|||index 920e7bb..8c43fdd 100644
|||--- a/sound/soc/codecs/sn95031.c
|||+++ b/sound/soc/codecs/sn95031.c
|||@@ -811,0 +812 @@ static int sn95031_pcm_hs_mute(struct snd_soc_dai *dai, int mute)
|||+       int tmp = !mute;
|||@@ -813 +814 @@ static int sn95031_pcm_hs_mute(struct snd_soc_dai *dai, int mute)
|||-                       SN95031_HSLVOLCTRL, BIT(7), (!mute << 7));
|||+                       SN95031_HSLVOLCTRL, BIT(7), tmp << 7);
|||@@ -815 +816,3 @@ static int sn95031_pcm_hs_mute(struct snd_soc_dai *dai, int mute)
|||-                       SN95031_HSRVOLCTRL, BIT(7), (!mute << 7));
|||+                       SN95031_HSRVOLCTRL, BIT(7), tmp << 7);
|||+       snd_soc_update_bits(dai->codec,
|||+                       SN95031_DRIVEREN, BIT(0) | BIT(1), tmp | tmp << 1);

And remap-log generates an incorrect map:

M sound/soc/codecs/sn95031.c sound/soc/codecs/sn95031.c
1 1
812 813
813 0
814 815
815 0

Notice that the last map entry basically says that all lines in the old file
starting from line 815 have been deleted, which is not true - only one line
number 815 has been deleted.

The correct map should look like this:

M sound/soc/codecs/sn95031.c sound/soc/codecs/sn95031.c
1 1
812 813
813 0
814 815
815 0
816 819

To fix the bug this patch prints the map of the last hung just before
'parse_diff()' exits.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 tools/remap-log.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/tools/remap-log.c b/tools/remap-log.c
index 327b870..ee79ed9 100644
--- a/tools/remap-log.c
+++ b/tools/remap-log.c
@@ -424,6 +424,9 @@ void parse_diff(void)
 			break;
 		}
 	}
+
+	if (!suppress && skipping && from + to != 2)
+		printf("%d %d\n", from, to);
 	return;
 Ediff:
 	die("odd diff");
-- 
1.7.7.1

Re: [PATCH 0/1] remap-log: fix map generator

[Artem Bityutskiy]

[-- Attachment #1.1: Type: text/plain, Size: 227 bytes --]

On Thu, 2011-11-03 at 16:01 +0200, Artem Bityutskiy wrote:
> [1] http://thread.gmane.org/gmane.linux.kernel/409379

For reference, here is the sources of remap-log that we are using.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #1.2: remap-log.c --]
[-- Type: text/x-csrc, Size: 10361 bytes --]

/*
 * Copyright (c) 2006, Al Viro.  All rights reserved.
 * 
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 *
 * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
 * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
 * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
 * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
 * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
 * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
 * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
 * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
 * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 * SUCH DAMAGE.
 */

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <limits.h>

char *prefix1 = "a/", *prefix2 = "b/";
char *from_prefix = "", *old_prefix = "O:", *new_prefix = "";
size_t from_len = 0;

char *line;
size_t size;

void die(char *s)
{
	fprintf(stderr, "remap: %s\n", s);
	exit(1);
}

void Enomem(void)
{
	die("out of memory");
}

void Eio(void)
{
	die("IO error");
}

int _getline(FILE *f)
{
	char *s;
	if (!fgets(line, size, f)) {
		if (!feof(f))
			Eio();
		return 0;
	}
	for (s = line + strlen(line); s[-1] != '\n'; s = s + strlen(s)) {
		if (s == line + size - 1) {
			line = realloc(line, 2 * size);
			if (!line)
				Enomem();
			s = line + size - 1;
			size *= 2;
		}
		if (!fgets(s, size - (s - line), f)) {
			if (!feof(f))
				Eio();
			return 1;
		}
	}
	s[-1] = '\0';
	return 1;
}

/* to == 0 -> deletion */
struct range_map {
	int from, to;
};

struct file_map {
	char *name;
	struct file_map *next;
	char *new_name;
	int count;
	int allocated;
	int last;
	struct range_map ranges[];
};

struct file_map *alloc_map(char *name)
{
	struct file_map *map;

	map = malloc(sizeof(struct file_map) + 16 * sizeof(struct range_map));
	if (!map)
		Enomem();
	map->name = map->new_name = strdup(name);
	if (!map->name)
		Enomem();
	map->count = 0;
	map->allocated = 16;
	map->next = NULL;
	map->last = 0;
	return map;
}

/* this is 32bit FNV1 */
uint32_t FNV_hash(char *name)
{
	uint32_t n = 0x811c9dc5;
	while (*name) {
		unsigned char c = *name++;
		n *= 0x01000193;
		n ^= c;
	}
	return n;
}

struct file_map *hash[1024];

int hash_map(struct file_map *map)
{
	int n = FNV_hash(map->name) % 1024;
	struct file_map **p = &hash[n];

	while (*p) {
		if (!strcmp((*p)->name, map->name))
			return 0;
		p = &(*p)->next;
	}
	*p = map;
	if (map->new_name && !map->count)
		return 0;
	if (map->new_name && map->ranges[0].from != 1)
		return 0;
	return 1;
}

struct file_map *find_map(char *name)
{
	static struct file_map *last = NULL;
	int n = FNV_hash(name) % 1024;
	struct file_map *p;

	if (last && !strcmp(last->name, name))
		return last;

	for (p = hash[n]; p && strcmp(p->name, name); p = p->next)
		;
	if (p)
		last = p;
	return p;
}

void parse_map(char *name)
{
	struct file_map *map = NULL;
	struct range_map *range;
	char *s;
	FILE *f;

	f = fopen(name, "r");
	if (!f)
		die("can't open map");
	while (_getline(f)) {
		if (line[0] == 'D') {
			if (map && !hash_map(map))
				goto Ebadmap;
			if (line[1] != ' ')
				goto Ebadmap;
			if (strchr(line + 2, ' '))
				goto Ebadmap;
			map = alloc_map(line + 2);
			map->new_name = NULL;
			continue;
		}
		if (line[0] == 'M') {
			if (map && !hash_map(map))
				goto Ebadmap;
			if (line[1] != ' ')
				goto Ebadmap;
			s = strchr(line + 2, ' ');
			if (!s)
				goto Ebadmap;
			*s++ = '\0';
			if (strchr(s, ' '))
				goto Ebadmap;
			map = alloc_map(line + 2);
			if (strcmp(line + 2, s)) {
				map->new_name = strdup(s);
				if (!map->new_name)
					Enomem();
			}
			continue;
		}
		if (!map || !map->new_name)
			goto Ebadmap;
		if (map->count == map->allocated) {
			int n = 2 * map->allocated;
			map = realloc(map, sizeof(struct file_map) +
					   n * sizeof(struct range_map));
			if (!map)
				Enomem();
			map->allocated = n;
		}
		range = &map->ranges[map->count++];
		if (sscanf(line, "%d %d%*c", &range->from, &range->to) != 2)
			goto Ebadmap;
		if (range > map->ranges && range->from <= range[-1].from)
			goto Ebadmap;
	}
	if (map && !hash_map(map))
		goto Ebadmap;
	fclose(f);
	return;
Ebadmap:
	die("bad map");
}

struct range_map *find_range(struct file_map *map, int l)
{
	struct range_map *range = &map->ranges[map->last];
	struct range_map *p;

	if (range->from <= l) {
		p = &map->ranges[map->count - 1];
		if (p->from > l) {
			for (p = range; p->from <= l; p++)
				;
			p--;
		}
	} else {
		for (p = map->ranges; p->from <= l; p++)
			;
		p--;
	}
	map->last = p - map->ranges;
	return p;
}

void mapline(void)
{
	struct file_map *map;
	struct range_map *range;
	unsigned long l;
	char *s1, *s2;
	char *name;

	if (strncmp(line, from_prefix, from_len))
		goto noise;
	s1 = strchr(line + from_len, ':');
	if (!s1)
		goto noise;
	s2 = strchr(line + from_len, ' ');
	if (s2 && s2 < s1)
		goto noise;
	l = strtoul(s1 + 1, &s2, 10);
	if (s2 == s1 + 1 || *s2 != ':' || !l || l > INT_MAX)
		goto noise;
	*s1++ = *s2++ = '\0';
	name = line + from_len;
	map = find_map(name);
	if (!map)
		goto new;
	if (!map->new_name)
		goto old;
	name = map->new_name;
	range = find_range(map, l);
	if (!range->to)
		goto old;
	l += range->to - range->from;
new:
	printf("%s%s:%lu:%s\n", new_prefix, name, l, s2);
	return;
old:
	s1[-1] = s2[-1] = ':';
	printf("%s%s\n", old_prefix, line + from_len);
	return;
noise:
	printf("%s\n", line);
}

int parse_hunk(int *l1, int *l2, int *n1, int *n2)
{
	unsigned long n;
	char *s, *p;
	if (line[3] != '-')
		return 0;
	n = strtoul(line + 4, &s, 10);
	if (s == line + 4 || n > INT_MAX)
		return 0;
	*l1 = n;
	if (*s == ',') {
		n = strtoul(s + 1, &p, 10);
		if (p == s + 1 || n > INT_MAX)
			return 0;
		*n1 = n;
		if (!n)
			(*l1)++;
	} else {
		p = s;
		*n1 = 1;
	}
	if (*p != ' ' || p[1] != '+')
		return 0;
	n = strtoul(p + 2, &s, 10);
	if (s == p + 2 || n > INT_MAX)
		return 0;
	*l2 = n;
	if (*s == ',') {
		n = strtoul(s + 1, &p, 10);
		if (p == s + 1 || n > INT_MAX)
			return 0;
		*n2 = n;
		if (!n)
			(*l2)++;
	} else {
		p = s;
		*n2 = 1;
	}
	return 1;
}

void parse_diff(void)
{
	int skipping = -1, suppress = 1;
	char *name1 = NULL, *name2 = NULL;
	int from = 1, to = 1;
	int l1, l2, n1, n2;
	enum cmd {
		Diff, Hunk, New, Del, Copy, Rename, Junk
	} cmd;
	static struct { const char *s; size_t len; } pref[] = {
		[Hunk] = {"@@ ", 3},
		[Diff] = {"diff ", 5},
		[New] = {"new file ", 9},
		[Del] = {"deleted file ", 12},
		[Copy] = {"copy from ", 10},
		[Rename] = {"rename from ", 11},
		[Junk] = {"", 0},
	};
	size_t len1 = strlen(prefix1), len2 = strlen(prefix2);

	while (_getline(stdin)) {
		if (skipping > 0) {
			switch (line[0]) {
			case '+':
			case '-':
			case '\\':
				continue;
			}
		}
		for (cmd = 0; strncmp(line, pref[cmd].s, pref[cmd].len); cmd++)
			;
		switch (cmd) {
		case Hunk:
			if (skipping < 0)
				goto Ediff;
			if (!suppress) {
				if (!skipping)
					printf("M %s %s\n", name1, name2);
				if (!parse_hunk(&l1, &l2, &n1, &n2))
					goto Ediff;
				if (l1 > from)
					printf("%d %d\n", from, to);
				if (n1)
					printf("%d 0\n", l1);
				from = l1 + n1;
				to = l2 + n2;
			}
			skipping = 1;
			break;
		case Diff:
			if (!suppress) {
				if (!skipping)
					printf("M %s %s\n", name1, name2);
				printf("%d %d\n", from, to);
			}
			free(name1);
			free(name2);
			name2 = strrchr(line, ' ');
			if (!name2)
				goto Ediff;
			*name2 = '\0';
			name1 = strrchr(line, ' ');
			if (!name1)
				goto Ediff;
			if (strncmp(name1 + 1, prefix1, len1))
				goto Ediff;
			if (strncmp(name2 + 1, prefix2, len2))
				goto Ediff;
			name1 = strdup(name1 + len1 + 1);
			name2 = strdup(name2 + len2 + 1);
			if (!name1 || !name2)
				goto Ediff;
			skipping = 0;
			suppress = 0;
			from = to = 1;
			break;
		case New:
			if (skipping)
				goto Ediff;
			suppress = 1;
			break;
		case Del:
		case Copy:
			if (skipping)
				goto Ediff;
			printf("D %s\n", name2);
			suppress = 1;
			break;
		case Rename:
			if (skipping)
				goto Ediff;
			printf("D %s\n", name2);
			break;
		default:
			break;
		}
	}
	return;
Ediff:
	die("odd diff");
}

int main(int argc, char **argv)
{
	char *map_name = NULL;
	char opt;
	char *arg;
	size_t len;
	size = 256;
	line = malloc(size);
	if (!line)
		Enomem();
	for (argc--, argv++; argc; argc--, argv++) {
		if (argv[0][0] != '-') {
			map_name = argv[0];
			continue;
		}
		opt = argv[0][1];
		if (!opt)
			goto Eargs;
		arg = argv[0] + 2;
		if (!*arg) {
			if (!--argc)
				goto Eargs;
			arg = *++argv;
		}
		len = strlen(arg);
		switch (opt) {
		case 'O':
			prefix1 = malloc(len + 2);
			if (!prefix1)
				Enomem();
			memcpy(prefix1, arg, len);
			prefix1[len] = '/';
			prefix1[len + 1] = '\0';
			break;
		case 'N':
			prefix2 = malloc(len + 2);
			if (!prefix2)
				Enomem();
			memcpy(prefix2, arg, len);
			prefix2[len] = '/';
			prefix2[len + 1] = '\0';
			break;
		case 'p':
			from_prefix = arg;
			from_len = len;
			break;
		case 'o':
			old_prefix = arg;
			break;
		case 'n':
			new_prefix = arg;
			break;
		default:
		Eargs:
			die("bad arguments");
		}
	}

	if (!map_name) {
		parse_diff();
	} else {
		parse_map(map_name);
		while (_getline(stdin))
			mapline();
	}
	return 0;

}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]