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
