[PATCH] warnings: ensure the source filename is shown at least once

[PATCH] warnings: ensure the source filename is shown at least once

[Luc Van Oostenryck]

When checking a series of files, if some warnings or errors
are issued but only coming from some includes, it's not possible
to identify which source file is responsible since its filename
is not displayed.

So, if the first warning is from a file other than the source
file, display first a note coming from the source file itself.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c                         | 13 +++++++++++++
 validation/cast-kinds-check.c |  1 +
 2 files changed, 14 insertions(+)

diff --git a/lib.c b/lib.c
index 4e8d7b451747..b94c2c83c1c5 100644
--- a/lib.c
+++ b/lib.c
@@ -50,6 +50,18 @@
 #include "bits.h"
 
 
+static void show_top_filename(const char *name)
+{
+	static const char *last;
+
+	if (name == base_filename)
+		return;
+	if (name == last)
+		return;
+	fprintf(stderr, "%s: note: in included file:\n", base_filename);
+	last = name;
+}
+
 static void do_warn(const char *type, struct position pos, const char * fmt, va_list args)
 {
 	static char buffer[512];
@@ -63,6 +75,7 @@ static void do_warn(const char *type, struct position pos, const char * fmt, va_
 	name = stream_name(pos.stream);
 		
 	fflush(stdout);
+	show_top_filename(name);
 	fprintf(stderr, "%s:%d:%d: %s%s%s\n",
 		name, pos.line, pos.pos, diag_prefix, type, buffer);
 }
diff --git a/validation/cast-kinds-check.c b/validation/cast-kinds-check.c
index 0c0cd67368a3..32a106d413da 100644
--- a/validation/cast-kinds-check.c
+++ b/validation/cast-kinds-check.c
@@ -6,6 +6,7 @@
  * check-assert: sizeof(long) == 8
  *
  * check-error-start
+cast-kinds-check.c: note: in included file:
 optim/cast-kinds.c:5:45: warning: cast drops bits
 optim/cast-kinds.c:6:47: warning: cast drops bits
 optim/cast-kinds.c:7:46: warning: cast drops bits
-- 
2.27.0

Re: [PATCH] warnings: ensure the source filename is shown at least once

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

On Sat, Jul 18, 2020 at 11:40 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> When checking a series of files, if some warnings or errors
> are issued but only coming from some includes, it's not possible
> to identify which source file is responsible since its filename
> is not displayed.
>
> So, if the first warning is from a file other than the source
> file, display first a note coming from the source file itself.

This really isn't enough when the include chain is deeper and more complex.

How about something a bit more complex? This is only lightly tested,
but I don't have time for anything more right now..

It results in things like

    kernel/exit.c: note: in included file (through
include/linux/sched/signal.h, include/linux/rcuwait.h,
include/linux/percpu-rwsem.h, include/linux/fs.h,
include/linux/huge_mm.h, include/linux/mm.h):
    ./include/linux/sched/task.h:115:34: warning: context imbalance in
'wait_task_zombie' - unexpected unlock
    ./include/linux/sched/task.h:115:34: warning: context imbalance in
'wait_task_stopped' - unexpected unlock
    ./include/linux/sched/task.h:115:34: warning: context imbalance in
'wait_task_continued' - unexpected unlock

to give an example of a warning happening in a header file that got
included through a deeper chain.

That stream chaining information might perhaps be useful for other cases too?

Would it be better to save the whole 'pos' for the chain, so that
you'd get line numbers etc for the chain? Probably. As mentioned, this
is a quick "how about something like this".

If you extend it and do more testing, you can have my sign-off, or
just take ownership of the patch entirely with my ack..

Now off for more kernel stuff after a quick sparse excursion...

                   Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 8070 bytes --]

 lib.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 pre-process.c | 12 ++++-----
 symbol.c      |  2 +-
 token.h       |  7 ++---
 tokenize.c    | 14 +++++++---
 5 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/lib.c b/lib.c
index 4e8d7b45..dcbbb5b3 100644
--- a/lib.c
+++ b/lib.c
@@ -49,22 +49,98 @@
 #include "version.h"
 #include "bits.h"
 
+static int prettify(const char **fnamep)
+{
+	const char *name = *fnamep;
+	int len = strlen(name);
+
+	if (len > 2 && !memcmp(name, "./", 2)) {
+		name += 2;
+		len -= 2;
+	}
+
+	*fnamep = name;
+	return len;
+}
+
+static const char *show_include_chain(int stream, const char *base)
+{
+	static char buffer[200];
+	int len = 0;
+
+	while ((stream = stream_prev(stream)) >= 0) {
+		const char *p = stream_name(stream);
+		int pretty_len;
+
+		if (p == base)
+			break;
+
+		pretty_len = prettify(&p);
+		if (pretty_len <= 0)
+			break;
+
+		/*
+		 * At worst, we'll need " (through %s, ...)" in addition to the
+		 * new filename
+		 */
+		if (pretty_len + len + 20 > sizeof(buffer)) {
+			if (!len)
+				return "";
+			memcpy(buffer+len, ", ...", 5);
+			len += 5;
+			break;
+		}
+
+		if (!len) {
+			memcpy(buffer, " (through ", 10);
+			len = 10;
+		} else {
+			buffer[len++] = ',';
+			buffer[len++] = ' ';
+		}
+
+		memcpy(buffer+len, p, pretty_len);
+		len += pretty_len;
+	}
+	if (!len)
+		return "";
+
+	buffer[len] = ')';
+	buffer[len+1] = 0;
+	return buffer;
+}
+
+static const char *show_stream_name(struct position pos)
+{
+	const char *name = stream_name(pos.stream);
+	static const char *last;
+
+	if (name == base_filename)
+		return name;
+	if (name == last)
+		return name;
+	last = name;
+
+	fprintf(stderr, "%s: note: in included file%s:\n",
+		base_filename,
+		show_include_chain(pos.stream, base_filename));
+	return name;
+}
 
 static void do_warn(const char *type, struct position pos, const char * fmt, va_list args)
 {
 	static char buffer[512];
-	const char *name;
 
 	/* Shut up warnings if position is bad_token.pos */
 	if (pos.type == TOKEN_BAD)
 		return;
 
 	vsprintf(buffer, fmt, args);	
-	name = stream_name(pos.stream);
-		
+
 	fflush(stdout);
 	fprintf(stderr, "%s:%d:%d: %s%s%s\n",
-		name, pos.line, pos.pos, diag_prefix, type, buffer);
+		show_stream_name(pos), pos.line, pos.pos,
+		diag_prefix, type, buffer);
 }
 
 static int show_info = 1;
@@ -275,7 +351,7 @@ static struct symbol_list *sparse_file(const char *filename)
 	base_filename = filename;
 
 	// Tokenize the input stream
-	token = tokenize(filename, fd, NULL, includepath);
+	token = tokenize(filename, fd, -1, NULL, includepath);
 	close(fd);
 
 	return sparse_tokenstream(token);
diff --git a/pre-process.c b/pre-process.c
index 403e3507..ab974b22 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -894,7 +894,7 @@ static void set_stream_include_path(struct stream *stream)
 #define PATH_MAX 4096	// for Hurd where it's not defined
 #endif
 
-static int try_include(const char *path, const char *filename, int flen, struct token **where, const char **next_path)
+static int try_include(struct position pos, const char *path, const char *filename, int flen, struct token **where, const char **next_path)
 {
 	int fd;
 	int plen = strlen(path);
@@ -910,8 +910,8 @@ static int try_include(const char *path, const char *filename, int flen, struct
 		return 1;
 	fd = open(fullname, O_RDONLY);
 	if (fd >= 0) {
-		char *streamname = xmemdup(fullname, plen + flen);
-		*where = tokenize(streamname, fd, *where, next_path);
+		char *streamname = streamname = xmemdup(fullname, plen + flen);
+		*where = tokenize(streamname, fd, pos.stream, *where, next_path);
 		close(fd);
 		return 1;
 	}
@@ -923,7 +923,7 @@ static int do_include_path(const char **pptr, struct token **list, struct token
 	const char *path;
 
 	while ((path = *pptr++) != NULL) {
-		if (!try_include(path, filename, flen, list, pptr))
+		if (!try_include(token->pos, path, filename, flen, list, pptr))
 			continue;
 		return 1;
 	}
@@ -966,7 +966,7 @@ static int handle_include_path(struct stream *stream, struct token **list, struc
 
 	/* Absolute path? */
 	if (filename[0] == '/') {
-		if (try_include("", filename, flen, list, includepath))
+		if (try_include(token->pos, "", filename, flen, list, includepath))
 			return 0;
 		goto out;
 	}
@@ -2091,7 +2091,7 @@ static void create_arglist(struct symbol *sym, int count)
 static void init_preprocessor(void)
 {
 	int i;
-	int stream = init_stream("preprocessor", -1, includepath);
+	int stream = init_stream("preprocessor", -1, includepath, -1);
 	static struct {
 		const char *name;
 		int (*handler)(struct stream *, struct token **, struct token *);
diff --git a/symbol.c b/symbol.c
index c0ca79e4..2cc9f82d 100644
--- a/symbol.c
+++ b/symbol.c
@@ -776,7 +776,7 @@ struct symbol	zero_int;
 
 void init_symbols(void)
 {
-	int stream = init_stream("builtin", -1, includepath);
+	int stream = init_stream("builtin", -1, includepath, -1);
 
 #define __IDENT(n,str,res) \
 	hash_ident(&n)
diff --git a/token.h b/token.h
index c5fdf3d0..6d2b0b65 100644
--- a/token.h
+++ b/token.h
@@ -49,7 +49,7 @@ enum constantfile {
 extern const char *includepath[];
 
 struct stream {
-	int fd;
+	int fd, prev;
 	const char *name;
 	const char *path;    // input-file path - see set_stream_include_path()
 	const char **next_path;
@@ -214,7 +214,8 @@ static inline struct token *containing_token(struct token **p)
 extern struct token eof_token_entry;
 #define eof_token(x) ((x) == &eof_token_entry)
 
-extern int init_stream(const char *, int fd, const char **next_path);
+extern int init_stream(const char *, int fd, const char **next_path, int prev_stream);
+extern int stream_prev(int stream);
 extern const char *stream_name(int stream);
 extern struct ident *hash_ident(struct ident *);
 extern struct ident *built_in_ident(const char *);
@@ -224,7 +225,7 @@ extern const char *show_ident(const struct ident *);
 extern const char *show_string(const struct string *string);
 extern const char *show_token(const struct token *);
 extern const char *quote_token(const struct token *);
-extern struct token * tokenize(const char *, int, struct token *, const char **next_path);
+extern struct token * tokenize(const char *, int, int, struct token *, const char **next_path);
 extern struct token * tokenize_buffer(void *, unsigned long, struct token **);
 
 extern void show_identifier_stats(void);
diff --git a/tokenize.c b/tokenize.c
index d3371e1e..c5ba6e6b 100644
--- a/tokenize.c
+++ b/tokenize.c
@@ -62,6 +62,13 @@ const char *stream_name(int stream)
 	return input_streams[stream].name;
 }
 
+int stream_prev(int stream)
+{
+	if (stream < 0 || stream > input_stream_nr)
+		return -1;
+	return input_streams[stream].prev;
+}
+
 static struct position stream_pos(stream_t *stream)
 {
 	struct position pos;
@@ -300,7 +307,7 @@ int *hash_stream(const char *name)
 	return input_stream_hashes + hash;
 }
 
-int init_stream(const char *name, int fd, const char **next_path)
+int init_stream(const char *name, int fd, const char **next_path, int prev_stream)
 {
 	int stream = input_stream_nr, *hash;
 	struct stream *current;
@@ -319,6 +326,7 @@ int init_stream(const char *name, int fd, const char **next_path)
 	current->next_path = next_path;
 	current->path = NULL;
 	current->constant = CONSTANT_FILE_MAYBE;
+	current->prev = prev_stream;
 	input_stream_nr = stream+1;
 	hash = hash_stream(name);
 	current->next_stream = *hash;
@@ -1006,14 +1014,14 @@ struct token * tokenize_buffer(void *buffer, unsigned long size, struct token **
 	return begin;
 }
 
-struct token * tokenize(const char *name, int fd, struct token *endtoken, const char **next_path)
+struct token * tokenize(const char *name, int fd, int prev_stream, struct token *endtoken, const char **next_path)
 {
 	struct token *begin, *end;
 	stream_t stream;
 	unsigned char buffer[BUFSIZE];
 	int idx;
 
-	idx = init_stream(name, fd, next_path);
+	idx = init_stream(name, fd, next_path, prev_stream);
 	if (idx < 0) {
 		// info(endtoken->pos, "File %s is const", name);
 		return endtoken;

Re: [PATCH] warnings: ensure the source filename is shown at least once

[Luc Van Oostenryck]

On Sat, Jul 18, 2020 at 01:21:35PM -0700, Linus Torvalds wrote:
> On Sat, Jul 18, 2020 at 11:40 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > When checking a series of files, if some warnings or errors
> > are issued but only coming from some includes, it's not possible
> > to identify which source file is responsible since its filename
> > is not displayed.
> >
> > So, if the first warning is from a file other than the source
> > file, display first a note coming from the source file itself.
> 
> This really isn't enough when the include chain is deeper and more complex.

Yes, my use case was much more limited.
 
> How about something a bit more complex? This is only lightly tested,
> but I don't have time for anything more right now..
> 
> It results in things like

I like it a lot and it works nicely.
 
> That stream chaining information might perhaps be useful for other cases too?

For sparse itself, I don't know, but for some other tools that would
analyze the code/#include structure, yes surely.
 
> Would it be better to save the whole 'pos' for the chain, so that
> you'd get line numbers etc for the chain? Probably. As mentioned, this
> is a quick "how about something like this".

I added this in a following patch.
 
> If you extend it and do more testing, you can have my sign-off, or
> just take ownership of the patch entirely with my ack..

I only removed a repetition in an initialization, so I added your s-o-b.

> Now off for more kernel stuff after a quick sparse excursion...

Yes, sure!
Thanks a lot because it's, I think, a really nice improvement.
-- Luc