| Message ID | 5720a23e-6ac0-9a6d-3d05-4e0f34bc0589@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bug in unused.cocci? | expand |
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
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
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)