Bug in unused.cocci?

Bug in unused.cocci?

[Phillip Wood]

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

Re: Bug in unused.cocci?

[Taylor Blau]

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

Re: Bug in unused.cocci?

[Phillip Wood]

Hi Taylor

On 11/05/2023 15:55, Taylor Blau wrote:
> 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