perf: Enable more compiler warnings

Related to a shadowed variable bug fix Valdis Kletnieks noticed
that perf does not get built with -Wshadow, which could have
helped us avoid the bug.

So enable -Wshadow and also enable the following warnings on
perf builds, in addition to the already enabled -Wall -Wextra
-std=gnu99 warnings:

 -Wcast-align
 -Wformat=2
 -Wshadow
 -Winit-self
 -Wpacked
 -Wredundant-decls
 -Wstack-protector
 -Wstrict-aliasing=3
 -Wswitch-default
 -Wswitch-enum
 -Wno-system-headers
 -Wundef
 -Wvolatile-register-var
 -Wwrite-strings
 -Wbad-function-cast
 -Wmissing-declarations
 -Wmissing-prototypes
 -Wnested-externs
 -Wold-style-definition
 -Wstrict-prototypes
 -Wdeclaration-after-statement

And change/fix the perf code to build cleanly under GCC 4.3.2.

The list of warnings enablement is rather arbitrary: it's based
on my (quick) reading of the GCC manpages and trying them on
perf.

I categorized the warnings based on individually enabling them
and looking whether they trigger something in the perf build.
If i liked those warnings (i.e. if they trigger for something
that arguably could be improved) i enabled the warning.

If the warnings seemed to come from language laywers spamming
the build with tons of nuisance warnings i generally kept them
off. Most of the sign conversion related warnings were in
this category. (A second patch enabling some of the sign
warnings might be welcome - sign bugs can be nasty.)

I also kept warnings that seem to make sense from their manpage
description and which produced no actual warnings on our code
base. These warnings might still be turned off if they end up
being a nuisance.

I also left out a few warnings that are not supported in older
compilers.

[ Note that these changes might break the build on older
  compilers i did not test, or on non-x86 architectures that
  produce different warnings, so more testing would be welcome. ]

Reported-by: Valdis.Kletnieks@vt.edu
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <new-submission>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

tools/perf/Makefile

@@ -166,7 +166,12 @@ endif
 
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
-CFLAGS = $(M64) -ggdb3 -Wall -Wextra -Wstrict-prototypes -Wmissing-declarations -Wmissing-prototypes -std=gnu99 -Wdeclaration-after-statement -Werror -O6
+#
+# Include saner warnings here, which can catch bugs:
+#
+EXTRA_WARNINGS = -Wcast-align -Wformat=2 -Wshadow -Winit-self -Wpacked -Wredundant-decls -Wstack-protector -Wstrict-aliasing=3 -Wswitch-default -Wswitch-enum -Wno-system-headers -Wundef -Wvolatile-register-var -Wwrite-strings -Wbad-function-cast -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wstrict-prototypes -Wdeclaration-after-statement
+
+CFLAGS = $(M64) -ggdb3 -Wall -Wextra -std=gnu99 -Werror -O6 $(EXTRA_WARNINGS)
 LDFLAGS = -lpthread -lrt -lelf -lm
 ALL_CFLAGS = $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)

tools/perf/builtin-annotate.c

@@ -81,7 +81,7 @@ struct hist_entry {
 struct sort_entry {
 	struct list_head list;
 
-	char *header;
+	const char *header;
 
 	int64_t (*cmp)(struct hist_entry *, struct hist_entry *);
 	int64_t (*collapse)(struct hist_entry *, struct hist_entry *);
@@ -225,7 +225,7 @@ static struct sort_entry sort_sym = {
 static int sort__need_collapse = 0;
 
 struct sort_dimension {
-	char			*name;
+	const char		*name;
 	struct sort_entry	*entry;
 	int			taken;
 };
@@ -723,7 +723,7 @@ parse_line(FILE *file, struct symbol *sym, u64 start, u64 len)
 		const char *path = NULL;
 		unsigned int hits = 0;
 		double percent = 0.0;
-		char *color;
+		const char *color;
 		struct sym_ext *sym_ext = sym->priv;
 
 		offset = line_ip - start;
@@ -805,7 +805,7 @@ static void free_source_line(struct symbol *sym, int len)
 
 /* Get the filename:line for the colored entries */
 static void
-get_source_line(struct symbol *sym, u64 start, int len, char *filename)
+get_source_line(struct symbol *sym, u64 start, int len, const char *filename)
 {
 	int i;
 	char cmd[PATH_MAX * 2];
@@ -851,7 +851,7 @@ get_source_line(struct symbol *sym, u64 start, int len, char *filename)
 	}
 }
 
-static void print_summary(char *filename)
+static void print_summary(const char *filename)
 {
 	struct sym_ext *sym_ext;
 	struct rb_node *node;
@@ -867,7 +867,7 @@ static void print_summary(char *filename)
 	node = rb_first(&root_sym_ext);
 	while (node) {
 		double percent;
-		char *color;
+		const char *color;
 		char *path;
 
 		sym_ext = rb_entry(node, struct sym_ext, node);
@@ -882,7 +882,7 @@ static void print_summary(char *filename)
 
 static void annotate_sym(struct dso *dso, struct symbol *sym)
 {
-	char *filename = dso->name, *d_filename;
+	const char *filename = dso->name, *d_filename;
 	u64 start, end, len;
 	char command[PATH_MAX*2];
 	FILE *file;
@@ -892,7 +892,7 @@ static void annotate_sym(struct dso *dso, struct symbol *sym)
 	if (sym->module)
 		filename = sym->module->path;
 	else if (dso == kernel_dso)
-		filename = vmlinux;
+		filename = vmlinux_name;
 
 	start = sym->obj_start;
 	if (!start)
@@ -964,7 +964,7 @@ static int __cmd_annotate(void)
 	int ret, rc = EXIT_FAILURE;
 	unsigned long offset = 0;
 	unsigned long head = 0;
-	struct stat stat;
+	struct stat input_stat;
 	event_t *event;
 	uint32_t size;
 	char *buf;
@@ -977,13 +977,13 @@ static int __cmd_annotate(void)
 		exit(-1);
 	}
 
-	ret = fstat(input, &stat);
+	ret = fstat(input, &input_stat);
 	if (ret < 0) {
 		perror("failed to stat file");
 		exit(-1);
 	}
 
-	if (!stat.st_size) {
+	if (!input_stat.st_size) {
 		fprintf(stderr, "zero-sized file, nothing to do!\n");
 		exit(0);
 	}
@@ -1010,10 +1010,10 @@ static int __cmd_annotate(void)
 
 	if (head + event->header.size >= page_size * mmap_window) {
 		unsigned long shift = page_size * (head / page_size);
-		int ret;
+		int munmap_ret;
 
-		ret = munmap(buf, page_size * mmap_window);
-		assert(ret == 0);
+		munmap_ret = munmap(buf, page_size * mmap_window);
+		assert(munmap_ret == 0);
 
 		offset += shift;
 		head -= shift;
@@ -1049,7 +1049,7 @@ static int __cmd_annotate(void)
 
 	head += size;
 
-	if (offset + head < (unsigned long)stat.st_size)
+	if (offset + head < (unsigned long)input_stat.st_size)
 		goto more;
 
 	rc = EXIT_SUCCESS;
@@ -1092,7 +1092,7 @@ static const struct option options[] = {
 		    "be more verbose (show symbol address, etc)"),
 	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
 		    "dump raw trace in ASCII"),
-	OPT_STRING('k', "vmlinux", &vmlinux, "file", "vmlinux pathname"),
+	OPT_STRING('k', "vmlinux", &vmlinux_name, "file", "vmlinux pathname"),
 	OPT_BOOLEAN('m', "modules", &modules,
 		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
 	OPT_BOOLEAN('l', "print-line", &print_line,

tools/perf/builtin-help.c

@@ -456,6 +456,7 @@ int cmd_help(int argc, const char **argv, const char *prefix __used)
 		break;
 	case HELP_FORMAT_WEB:
 		show_html_page(argv[0]);
+	default:
 		break;
 	}
 

tools/perf/builtin-report.c

@@ -97,6 +97,7 @@ static int repsep_fprintf(FILE *fp, const char *fmt, ...)
 		n = vasprintf(&bf, fmt, ap);
 		if (n > 0) {
 			char *sep = bf;
+
 			while (1) {
 				sep = strchr(sep, *field_sep);
 				if (sep == NULL)
@@ -144,7 +145,7 @@ struct hist_entry {
 struct sort_entry {
 	struct list_head list;
 
-	char *header;
+	const char *header;
 
 	int64_t (*cmp)(struct hist_entry *, struct hist_entry *);
 	int64_t (*collapse)(struct hist_entry *, struct hist_entry *);
@@ -328,7 +329,7 @@ static int sort__need_collapse = 0;
 static int sort__has_parent = 0;
 
 struct sort_dimension {
-	char			*name;
+	const char		*name;
 	struct sort_entry	*entry;
 	int			taken;
 };
@@ -343,7 +344,7 @@ static struct sort_dimension sort_dimensions[] = {
 
 static LIST_HEAD(hist_entry__sort_list);
 
-static int sort_dimension__add(char *tok)
+static int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
 
@@ -602,6 +603,7 @@ hist_entry_callchain__fprintf(FILE *fp, struct hist_entry *self,
 		case CHAIN_GRAPH_REL:
 			ret += callchain__fprintf_graph(fp, chain,
 							total_samples, 1, 1);
+		case CHAIN_NONE:
 		default:
 			break;
 		}
@@ -1290,7 +1292,7 @@ process_lost_event(event_t *event, unsigned long offset, unsigned long head)
 static void trace_event(event_t *event)
 {
 	unsigned char *raw_event = (void *)event;
-	char *color = PERF_COLOR_BLUE;
+	const char *color = PERF_COLOR_BLUE;
 	int i, j;
 
 	if (!dump_trace)
@@ -1348,7 +1350,7 @@ process_read_event(event_t *event, unsigned long offset, unsigned long head)
 	struct perf_counter_attr *attr = perf_header__find_attr(event->read.id);
 
 	if (show_threads) {
-		char *name = attr ? __event_name(attr->type, attr->config)
+		const char *name = attr ? __event_name(attr->type, attr->config)
 				   : "unknown";
 		perf_read_values_add_value(&show_threads_values,
 					   event->read.pid, event->read.tid,
@@ -1411,27 +1413,27 @@ process_event(event_t *event, unsigned long offset, unsigned long head)
 
 static u64 perf_header__sample_type(void)
 {
-	u64 sample_type = 0;
+	u64 type = 0;
 	int i;
 
 	for (i = 0; i < header->attrs; i++) {
 		struct perf_header_attr *attr = header->attr[i];
 
-		if (!sample_type)
-			sample_type = attr->attr.sample_type;
-		else if (sample_type != attr->attr.sample_type)
+		if (!type)
+			type = attr->attr.sample_type;
+		else if (type != attr->attr.sample_type)
 			die("non matching sample_type");
 	}
 
-	return sample_type;
+	return type;
 }
 
 static int __cmd_report(void)
 {
 	int ret, rc = EXIT_FAILURE;
 	unsigned long offset = 0;
 	unsigned long head, shift;
-	struct stat stat;
+	struct stat input_stat;
 	event_t *event;
 	uint32_t size;
 	char *buf;
@@ -1450,13 +1452,13 @@ static int __cmd_report(void)
 		exit(-1);
 	}
 
-	ret = fstat(input, &stat);
+	ret = fstat(input, &input_stat);
 	if (ret < 0) {
 		perror("failed to stat file");
 		exit(-1);
 	}
 
-	if (!stat.st_size) {
+	if (!input_stat.st_size) {
 		fprintf(stderr, "zero-sized file, nothing to do!\n");
 		exit(0);
 	}
@@ -1524,12 +1526,12 @@ static int __cmd_report(void)
 		size = 8;
 
 	if (head + event->header.size >= page_size * mmap_window) {
-		int ret;
+		int munmap_ret;
 
 		shift = page_size * (head / page_size);
 
-		ret = munmap(buf, page_size * mmap_window);
-		assert(ret == 0);
+		munmap_ret = munmap(buf, page_size * mmap_window);
+		assert(munmap_ret == 0);
 
 		offset += shift;
 		head -= shift;
@@ -1568,7 +1570,7 @@ static int __cmd_report(void)
 	if (offset + head >= header->data_offset + header->data_size)
 		goto done;
 
-	if (offset + head < (unsigned long)stat.st_size)
+	if (offset + head < (unsigned long)input_stat.st_size)
 		goto more;
 
 done:
@@ -1666,7 +1668,7 @@ static const struct option options[] = {
 		    "be more verbose (show symbol address, etc)"),
 	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
 		    "dump raw trace in ASCII"),
-	OPT_STRING('k', "vmlinux", &vmlinux, "file", "vmlinux pathname"),
+	OPT_STRING('k', "vmlinux", &vmlinux_name, "file", "vmlinux pathname"),
 	OPT_BOOLEAN('m', "modules", &modules,
 		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
 	OPT_BOOLEAN('n', "show-nr-samples", &show_nr_samples,

tools/perf/builtin-top.c

@@ -120,7 +120,8 @@ static void parse_source(struct sym_entry *syme)
 	struct module *module;
 	struct section *section = NULL;
 	FILE *file;
-	char command[PATH_MAX*2], *path = vmlinux;
+	char command[PATH_MAX*2];
+	const char *path = vmlinux_name;
 	u64 start, end, len;
 
 	if (!syme)
@@ -487,10 +488,12 @@ static void print_sym_table(void)
 	);
 
 	for (nd = rb_first(&tmp); nd; nd = rb_next(nd)) {
-		struct sym_entry *syme = rb_entry(nd, struct sym_entry, rb_node);
-		struct symbol *sym = (struct symbol *)(syme + 1);
+		struct symbol *sym;
 		double pcnt;
 
+		syme = rb_entry(nd, struct sym_entry, rb_node);
+		sym = (struct symbol *)(syme + 1);
+
 		if (++printed > print_entries || (int)syme->snap_count < count_filter)
 			continue;
 
@@ -609,7 +612,7 @@ static void print_mapped_keys(void)
 
 	fprintf(stdout, "\t[f]     profile display filter (count).    \t(%d)\n", count_filter);
 
-	if (vmlinux) {
+	if (vmlinux_name) {
 		fprintf(stdout, "\t[F]     annotate display filter (percent). \t(%d%%)\n", sym_pcnt_filter);
 		fprintf(stdout, "\t[s]     annotate symbol.                   \t(%s)\n", name?: "NULL");
 		fprintf(stdout, "\t[S]     stop annotation.\n");
@@ -638,7 +641,9 @@ static int key_mapped(int c)
 		case 'F':
 		case 's':
 		case 'S':
-			return vmlinux ? 1 : 0;
+			return vmlinux_name ? 1 : 0;
+		default:
+			break;
 	}
 
 	return 0;
@@ -724,6 +729,8 @@ static void handle_keypress(int c)
 		case 'z':
 			zero = ~zero;
 			break;
+		default:
+			break;
 	}
 }
 
@@ -812,13 +819,13 @@ static int parse_symbols(void)
 {
 	struct rb_node *node;
 	struct symbol  *sym;
-	int modules = vmlinux ? 1 : 0;
+	int use_modules = vmlinux_name ? 1 : 0;
 
 	kernel_dso = dso__new("[kernel]", sizeof(struct sym_entry));
 	if (kernel_dso == NULL)
 		return -1;
 
-	if (dso__load_kernel(kernel_dso, vmlinux, symbol_filter, verbose, modules) <= 0)
+	if (dso__load_kernel(kernel_dso, vmlinux_name, symbol_filter, verbose, use_modules) <= 0)
 		goto out_delete_dso;
 
 	node = rb_first(&kernel_dso->syms);
@@ -1114,7 +1121,7 @@ static const struct option options[] = {
 			    "system-wide collection from all CPUs"),
 	OPT_INTEGER('C', "CPU", &profile_cpu,
 		    "CPU to profile on"),
-	OPT_STRING('k', "vmlinux", &vmlinux, "file", "vmlinux pathname"),
+	OPT_STRING('k', "vmlinux", &vmlinux_name, "file", "vmlinux pathname"),
 	OPT_INTEGER('m', "mmap-pages", &mmap_pages,
 		    "number of mmap data pages"),
 	OPT_INTEGER('r', "realtime", &realtime_prio,

tools/perf/util/abspath.c

@@ -50,7 +50,8 @@ const char *make_absolute_path(const char *path)
 			die ("Could not get current working directory");
 
 		if (last_elem) {
-			int len = strlen(buf);
+			len = strlen(buf);
+
 			if (len + strlen(last_elem) + 2 > PATH_MAX)
 				die ("Too long path name: '%s/%s'",
 						buf, last_elem);

tools/perf/util/cache.h

@@ -52,7 +52,6 @@ extern const char *perf_mailmap_file;
 extern void maybe_flush_or_die(FILE *, const char *);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
-extern ssize_t read_in_full(int fd, void *buf, size_t count);
 extern ssize_t write_in_full(int fd, const void *buf, size_t count);
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);