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

On Sat, Jan 17, 2026 at 6:19 AM Oleg Nesterov <[email protected]> wrote:
>
> 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...

As far as I can tell, the new position report is more useful to the
reader. I haven't heard any objections yet. Let's put it on the
sparse-dev for a bit then merge into sparse. In the worst case there
is some burning reason to use the old behavior, we can always change
the behavior back. It is only  software and it is easy to fix.

>
> > >  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?

Well, we care in the sense that we don't want unnecessary check
failures. But we can update the expected output of the validation
check to silence the error.

>
> So. I am going to update the changelog and send the trivial V2 below.
>
> Will you agree?

Agree. As far as I can tell. The new position is more desirable. Let's
switch to the new position.

Chris

>
> 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)
>