threads / bug / 59729

Bug in unused.cocci?

Subject: Bug in unused.cocci?

## tl;dr

3 messages between May 11, 2023 and May 12, 2023.

replies: 2people: 2as markdown or json

Phillip Wood· May 11, 2023, 09:16 UTC · lore

The CI static analysis job for my unit test framework patch[1] fails[2] with

Coccinelle suggests the following changes in 'contrib/coccinelle/ALL.cocci.patch': diff -u -p a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c

--- a/t/unit-tests/t-strbuf.c
+++ b/t/unit-tests/t-strbuf.c
@@ -27,13 +27,9 @@ static void t_static_init(void)

  static void t_dynamic_init(void)
  {
-	struct strbuf buf;
-
-	strbuf_init(&buf, 1024);
  	check_uint(buf.len, ==, 0);
  	check_uint(buf.alloc, >=, 1024);
  	check_char(buf.buf[0], ==, '\0');
-	strbuf_release(&buf);
  }

  static void t_addch(struct strbuf *buf, void *data)
error: Coccinelle suggested some changes

I think this is due to a bug in unused.cocci. I'm not sure what is going 
wrong and admittedly we're unlikely to see code where an strbuf is 
initialized and then used it without calling any of the strbuf_* 
functions within our main codebase but it would be nice if the rule 
could handle this.

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/
[2] 
https://github.com/phillipwood/git/actions/runs/4938207776/jobs/8827751328

Taylor Blau· May 11, 2023, 14:55 UTC · re: Phillip Wood · lore

Re: Bug in unused.cocci?

On Thu, May 11, 2023 at 10:16:21AM +0100, Phillip Wood wrote:

> I think this is due to a bug in unused.cocci. I'm not sure what is going
> wrong and admittedly we're unlikely to see code where an strbuf is
> initialized and then used it without calling any of the strbuf_* functions
> within our main codebase but it would be nice if the rule could handle this.

I don't think that this is a bug in unused.cocci, but rather a bug in spatch not being able to read t/unit-tests/test-lib.h.

    $ spatch --verbose-parsing --debug --all-includes \
        --sp-file contrib/coccinelle/unused.old.cocci \
        t/unit-tests/t-strbuf.c | grep '^bad'
    init_defs_builtins: /usr/lib/coccinelle/standard.h
    -----------------------------------------------------------------------
    processing semantic patch file: contrib/coccinelle/unused.old.cocci
    with isos from: /usr/lib/coccinelle/standard.iso
    -----------------------------------------------------------------------

    HANDLING: t/unit-tests/t-strbuf.c
    parse error
     = error in t/unit-tests/test-lib.h; set verbose_parsing for more info
    badcount: 3
    bad: int test_done(void);
    bad:
    bad: /* Skip the current test. */
    BAD:!!!!! __attribute__((format (printf, 1, 2)))

From my understanding, spatch happily ignores macros that it doesn't understand (like check_uint() and check_char()), so to it this code looks like:

    struct strbuf buf;

    strbuf_init(&buf, 1024);
    strbuf_release(&buf);

which it marks as unused and applies the patch. Strangely, if you force it to pre-process with the appropriate macro file by passing it explicitly, it works as expected:

    $ spatch --macro-file t/unit-tests/test-lib.h \
        --sp-file contrib/coccinelle/unused.old.cocci \
        t/unit-tests/t-strbuf.c
    init_defs_builtins: /usr/lib/coccinelle/standard.h
    init_defs: t/unit-tests/test-lib.h
    HANDLING: t/unit-tests/t-strbuf.c

I am puzzled by spatch's behavior here.

Thanks, Taylor

Phillip Wood· May 12, 2023, 15:07 UTC · re: Taylor Blau · lore

Re: Bug in unused.cocci?

Hi Taylor

On 11/05/2023 15:55, Taylor Blau wrote:

Show 48 quoted lines

> On Thu, May 11, 2023 at 10:16:21AM +0100, Phillip Wood wrote:
>> I think this is due to a bug in unused.cocci. I'm not sure what is going
>> wrong and admittedly we're unlikely to see code where an strbuf is
>> initialized and then used it without calling any of the strbuf_* functions
>> within our main codebase but it would be nice if the rule could handle this.
> 
> I don't think that this is a bug in unused.cocci, but rather a bug in
> spatch not being able to read t/unit-tests/test-lib.h.
> 
>      $ spatch --verbose-parsing --debug --all-includes \
>          --sp-file contrib/coccinelle/unused.old.cocci \
>          t/unit-tests/t-strbuf.c | grep '^bad'
>      init_defs_builtins: /usr/lib/coccinelle/standard.h
>      -----------------------------------------------------------------------
>      processing semantic patch file: contrib/coccinelle/unused.old.cocci
>      with isos from: /usr/lib/coccinelle/standard.iso
>      -----------------------------------------------------------------------
> 
>      HANDLING: t/unit-tests/t-strbuf.c
>      parse error
>       = error in t/unit-tests/test-lib.h; set verbose_parsing for more info
>      badcount: 3
>      bad: int test_done(void);
>      bad:
>      bad: /* Skip the current test. */
>      BAD:!!!!! __attribute__((format (printf, 1, 2)))
> 
>  From my understanding, spatch happily ignores macros that it doesn't
> understand (like check_uint() and check_char()), so to it this code
> looks like:
> 
>      struct strbuf buf;
> 
>      strbuf_init(&buf, 1024);
>      strbuf_release(&buf);
> 
> which it marks as unused and applies the patch. Strangely, if you force
> it to pre-process with the appropriate macro file by passing it
> explicitly, it works as expected:
> 
>      $ spatch --macro-file t/unit-tests/test-lib.h \
>          --sp-file contrib/coccinelle/unused.old.cocci \
>          t/unit-tests/t-strbuf.c
>      init_defs_builtins: /usr/lib/coccinelle/standard.h
>      init_defs: t/unit-tests/test-lib.h
>      HANDLING: t/unit-tests/t-strbuf.c
> 
> I am puzzled by spatch's behavior here.

Thanks for looking at this. It's good that unused.cocci is not buggy but I agree spatch's behavior is confusing. There is a similar test for STRBUF_INIT which looks like

	static void t_static_init(void)
	{
		struct strbuf buf = STRBUF_INIT;

		check_uint(buf.len, ==, 0);
		check_uint(buf.alloc, ==, 0);
		if (check(buf.buf == strbuf_slopbuf))
			return; /* avoid de-referencing buf.buf */
		check_char(buf.buf[0], ==, '\0');
	}

Which does not trigger this issue. Presumably the "if" statement is stopping it from ignoring the check() macro even though it does not understand it. If I change t_dynamic_init() to do

	if (!check(buf.buf != NULL))
		check_char(buf.buf[0], ==, '\0');

Then the static analysis job passes but I don't think that is really fixing the problem.

Thanks

Phillip