Re: [PATCH] sparse/pre-process: introduce "dissect_mode" option to fix dissect/semind

Hi Chris,

On 01/16, Chris Li wrote:
>
> On Wed, Dec 17, 2025 at 7:26 AM Oleg Nesterov <[email protected]> wrote:
> >
> > I don't quite understand why does expand() -> collect_arg() path
> > update ->pos for each token in the input *list, but this breaks
> > dissect and thus semind.
>
> That is a good question, I don't understand why it did that either. I
> did some digging, inside macro argument list expansion, the "#include
> " is not allowed. It is not possible to switch streams here. The
> "pos.pos" is for human consumption anyway, it has no effect on the IR
> generation. The only visible effect as far as I can tell is related to
> the preprocessor "-E" in lib.c:
>
>         if (preprocess_only) {
>                 while (!eof_token(token)) {
>                         int prec = 1;
>                         struct token *next = token->next;
>                         const char *separator = "";
>                         if (next->pos.whitespace)
>                                 separator = " ";
>                         if (next->pos.newline) {
>                                 separator = "\n\t\t\t\t\t";
>                                 prec = next->pos.pos; <--- use pos as
> indentation level.
>                                 if (prec > 4)
>                                         prec = 4;
>                         }
>                         printf("%s%.*s", show_token(token), prec, separator);
>                         token = next;
>
> The "-E" output has some indentation enhancement to turn space into
> tab level indentation. This "pos" assignment tries to align the
> indentation context of the input arguments to the same level of the
> expanding macro name.

Yes, exactly! Initially I tried to simply remove these next->pos.* updates
in collect_arg(), but this causes a lot of failures in validation/preprocessor
(due to extra indentations) and I failed to find a simple fix for the
"if (preprocess_only)" code above. Plus I wasn't comfortable because
I don't understand the intent...

> >  void dissect(struct reporter *rep, struct string_list *filelist)
> >  {
> > +       dissect_mode = 1;
>
> I don't think we need dissect_mode. I am leaning towards enabling it
> all the time, maybe except for the preprocessor only mode.

...

> > +               if (!dissect_mode) {
> > +                       next->pos.stream = pos->stream;
> > +                       next->pos.line = pos->line;
> > +                       next->pos.pos = pos->pos;
> > +               }
>
> Maybe change it to "if (preprocess_only)", and fix all the validation
> error output of the checker. What do you say?

Agreed! This was my plan B ;)

With this change

	-               if (!dissect_mode) {
	+               if (preprocess_only) {

make check reports 2 failures

	-parsing/attr-cleanup.c:10:17: error: argument is not an identifier
	+parsing/attr-cleanup.c:10:27: error: argument is not an identifier

	-sizeof-void.c:20:14: warning: expression using sizeof(void)
	+sizeof-void.c:20:27: warning: expression using sizeof(void)

but the new positions look more correct.

However. I didn't dare to send this patch because other warnings from
sizeof-void.c still blame the column 14, this looks inconsistent...
But perhaps we don't really care?

So. I am going to update the changelog and send the trivial V2 below.

Will you agree?

Oleg.
---

diff --git a/pre-process.c b/pre-process.c
index 3fb25082..a4bb6cb6 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -294,9 +294,11 @@ static struct token *collect_arg(struct token *prev, int vararg, struct position
 		} else if (match_op(next, ',') && !nesting && !vararg) {
 			break;
 		}
-		next->pos.stream = pos->stream;
-		next->pos.line = pos->line;
-		next->pos.pos = pos->pos;
+		if (preprocess_only) {
+			next->pos.stream = pos->stream;
+			next->pos.line = pos->line;
+			next->pos.pos = pos->pos;
+		}
 		next->pos.newline = 0;
 		p = &next->next;
 	}
diff --git a/validation/parsing/attr-cleanup.c b/validation/parsing/attr-cleanup.c
index ac64649c..fa3cb1ca 100644
--- a/validation/parsing/attr-cleanup.c
+++ b/validation/parsing/attr-cleanup.c
@@ -24,7 +24,7 @@ int test(int n)
  * check-command: sparse -Wunknown-attribute $file
  *
  * check-error-start
-parsing/attr-cleanup.c:10:17: error: argument is not an identifier
+parsing/attr-cleanup.c:10:27: error: argument is not an identifier
 parsing/attr-cleanup.c:11:39: error: an argument is expected for attribute 'cleanup'
 parsing/attr-cleanup.c:12:40: error: an argument is expected for attribute 'cleanup'
 parsing/attr-cleanup.c:13:43: error: Expected ) after attribute's argument'
diff --git a/validation/sizeof-void.c b/validation/sizeof-void.c
index 0fd917a2..6792ff02 100644
--- a/validation/sizeof-void.c
+++ b/validation/sizeof-void.c
@@ -36,7 +36,7 @@ sizeof-void.c:16:14: warning: expression using sizeof(void)
 sizeof-void.c:17:14: warning: expression using sizeof(void)
 sizeof-void.c:18:14: warning: expression using sizeof(void)
 sizeof-void.c:19:14: warning: expression using sizeof(void)
-sizeof-void.c:20:14: warning: expression using sizeof(void)
+sizeof-void.c:20:27: warning: expression using sizeof(void)
 sizeof-void.c:21:14: warning: expression using sizeof(void)
 sizeof-void.c:22:14: warning: expression using sizeof(void)
 sizeof-void.c:23:14: warning: expression using sizeof(void)